Bug 74075

Summary:

Make default console messages line numbers consistent.

Product:

WebKit

Reporter:

Vsevolod Vlasov <vsevik>

Component:

Web Inspector (Deprecated)

Assignee:

Vsevolod Vlasov <vsevik>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

abarth, ap, dglazkov, gustavo.noronha, gustavo, japhet, joepeck, kbalazs, ossy, pfeldman, rakuco, timothy, tkent, webkit.review.bot, xan.lopez, yurys, zimmermann

Priority:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

76167

Bug Blocks:

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	none
Patch	none
Patch	gustavo.noronha: commit-queue-
Patch	pfeldman: review+, gustavo: commit-queue-
Patch	webkit-ews: commit-queue-
Patch	gyuyoung.kim: commit-queue-
Patch	none

Vsevolod Vlasov

Reported 2011-12-08 04:41:27 PST

Console messages line number should be 1 by default. When line number is unknown, we should always pass 1 to Console::addMessage(). Currently zeros and ones are mixed all over the code, sometimes we even have different default values in one file (DOMWindow.cpp, FrameLoader.cpp).

Attachments
Patch (90.70 KB, patch) 2011-12-08 05:01 PST, Vsevolod Vlasov	no flags	Details Formatted Diff Diff
Patch (129.40 KB, patch) 2011-12-08 05:51 PST, Vsevolod Vlasov	no flags	Details Formatted Diff Diff
Patch (958.32 KB, patch) 2012-01-11 04:21 PST, Vsevolod Vlasov	no flags	Details Formatted Diff Diff
Patch (958.45 KB, patch) 2012-01-11 04:58 PST, Vsevolod Vlasov	no flags	Details Formatted Diff Diff
Patch (966.16 KB, patch) 2012-01-11 05:39 PST, Vsevolod Vlasov	gustavo.noronha: commit-queue-	Details Formatted Diff Diff
Patch (966.98 KB, patch) 2012-01-11 06:27 PST, Vsevolod Vlasov	pfeldman: review+ gustavo: commit-queue-	Details Formatted Diff Diff
Patch (968.57 KB, patch) 2012-01-11 08:51 PST, Vsevolod Vlasov	webkit-ews: commit-queue-	Details Formatted Diff Diff
Patch (969.51 KB, patch) 2012-01-11 10:16 PST, Vsevolod Vlasov	gyuyoung.kim: commit-queue-	Details Formatted Diff Diff
Patch (969.51 KB, patch) 2012-01-11 11:51 PST, Vsevolod Vlasov	no flags	Details Formatted Diff Diff
Show Obsolete (7) View All Add attachment proposed patch, testcase, etc.

Vsevolod Vlasov

Comment 1 2011-12-08 05:01:45 PST

Created attachment 118364 [details] Patch

WebKit Review Bot

Comment 2 2011-12-08 05:34:55 PST

Comment on attachment 118364 [details] Patch Attachment 118364 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/10786237 New failing tests: http/tests/websocket/tests/hixie76/url-parsing.html http/tests/websocket/tests/hixie76/bad-sub-protocol-control-chars.html

Vsevolod Vlasov

Comment 3 2011-12-08 05:51:51 PST

Created attachment 118374 [details] Patch

Alexey Proskuryakov

Comment 4 2011-12-08 14:43:25 PST

Isn't the idea that 0 would not be shown in UI, being an invalid value for line number? Sending 1 is a direct lie that UI can not hide.

Adam Barth

Comment 5 2011-12-08 15:17:09 PST

(In reply to comment #4) > Isn't the idea that 0 would not be shown in UI, being an invalid value for line number? Sending 1 is a direct lie that UI can not hide. In that case, we should change all the other call sites to use 0. Currently 1 is much more prevalent than 0 in these sorts of call site.

Alexey Proskuryakov

Comment 6 2011-12-08 15:28:00 PST

Can someone confirm what the intended behavior is?

Kent Tamura

Comment 7 2011-12-08 15:33:31 PST

Comment on attachment 118374 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=118374&action=review > Source/WebCore/loader/FrameLoader.cpp:1396 > - frame->domWindow()->console()->addMessage(JSMessageSource, LogMessageType, ErrorMessageLevel, "Not allowed to load local resource: " + url, 0, String()); > + frame->domWindow()->console()->addMessage(JSMessageSource, LogMessageType, ErrorMessageLevel, "Not allowed to load local resource: " + url, 1, String()); We had better introduce an overload of addMessage() without the line number and the source URL argument, or add default argument values to them.

Adam Barth

Comment 8 2011-12-08 15:36:07 PST

> We had better introduce an overload of addMessage() without the line number and the source URL argument, or add default argument values to them. We have one on ScriptExecutionContext (or maybe Document).

Vsevolod Vlasov

Comment 9 2011-12-13 04:15:55 PST

(In reply to comment #5) > (In reply to comment #4) > > Isn't the idea that 0 would not be shown in UI, being an invalid value for line number? Sending 1 is a direct lie that UI can not hide. > > In that case, we should change all the other call sites to use 0. Currently 1 is much more prevalent than 0 in these sorts of call site. I am not sure how to measure which one is more prevalent, but based on test results, both are widespread: 125 results with line number equal to one: http://codesearch.google.com/codesearch#search/&exact_package=chromium&q=%22console%20message:%20line%201:%22&type=cs 285 results with line number equal to zero: http://codesearch.google.com/codesearch#search/&exact_package=chromium&q=%22console%20message:%20line%200:%22&type=cs I suggest we set line number to zero by default and don't add line number to output in this case: "CONSOLE MESSAGE:" (or "CONSOLE MESSAGE: line unknown:"). (In reply to comment #7) > We had better introduce an overload of addMessage() without the line number and the source URL argument, or add default argument values to them. We have it in ScriptExecutionContext but can add one to Console once we decide what the default argument value should be. http://codesearch.google.com/codesearch#OAMlx_jo-ck/src/third_party/WebKit/Source/WebCore/dom/ScriptExecutionContext.cpp&type=cs&l=331

Alexey Proskuryakov

Comment 10 2011-12-22 10:42:27 PST

I'm not a Web Inspector expert, but comment 9 sounds perfectly good to me. I wouldn't print "line unknown", as that's not useful information.

Vsevolod Vlasov

Comment 11 2012-01-11 04:21:33 PST

Created attachment 122003 [details] Patch

Vsevolod Vlasov

Comment 12 2012-01-11 04:58:25 PST

Created attachment 122007 [details] Patch

Gustavo Noronha (kov)

Comment 13 2012-01-11 05:12:46 PST

Comment on attachment 122007 [details] Patch Attachment 122007 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/11195427

Gyuyoung Kim

Comment 14 2012-01-11 05:18:28 PST

Comment on attachment 122007 [details] Patch Attachment 122007 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/11148484

Early Warning System Bot

Comment 15 2012-01-11 05:18:35 PST

Comment on attachment 122007 [details] Patch Attachment 122007 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/11199391

Vsevolod Vlasov

Comment 16 2012-01-11 05:39:32 PST

Created attachment 122012 [details] Patch

Early Warning System Bot

Comment 17 2012-01-11 06:24:06 PST

Comment on attachment 122012 [details] Patch Attachment 122012 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/11121021

Vsevolod Vlasov

Comment 18 2012-01-11 06:27:23 PST

Created attachment 122015 [details] Patch

Gustavo Noronha (kov)

Comment 19 2012-01-11 06:58:03 PST

Comment on attachment 122015 [details] Patch Attachment 122015 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/11211008

Early Warning System Bot

Comment 20 2012-01-11 06:59:00 PST

Comment on attachment 122015 [details] Patch Attachment 122015 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/11169405

Gyuyoung Kim

Comment 21 2012-01-11 07:01:19 PST

Comment on attachment 122015 [details] Patch Attachment 122015 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/11169406

Pavel Feldman

Comment 22 2012-01-11 07:01:55 PST

Comment on attachment 122012 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=122012&action=review > Source/WebCore/dom/ScriptExecutionContext.h:187 > + virtual void logExceptionToConsole(const String& errorMessage, const String& sourceURL, int lineNumber, PassRefPtr<ScriptCallStack>) = 0; Here and in the other interfaces: consider using OrdinalNumber.

Collabora GTK+ EWS bot

Comment 23 2012-01-11 07:05:07 PST

Comment on attachment 122012 [details] Patch Attachment 122012 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/11121031

Gyuyoung Kim

Comment 24 2012-01-11 07:29:16 PST

Comment on attachment 122012 [details] Patch Attachment 122012 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/11121035

Vsevolod Vlasov

Comment 25 2012-01-11 08:51:38 PST

Created attachment 122036 [details] Patch

Early Warning System Bot

Comment 26 2012-01-11 09:22:52 PST

Comment on attachment 122036 [details] Patch Attachment 122036 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/11213040

Gustavo Noronha (kov)

Comment 27 2012-01-11 09:24:27 PST

Comment on attachment 122036 [details] Patch Attachment 122036 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/11118063

Gyuyoung Kim

Comment 28 2012-01-11 09:27:10 PST

Comment on attachment 122036 [details] Patch Attachment 122036 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/11170450

Vsevolod Vlasov

Comment 29 2012-01-11 10:16:01 PST

Created attachment 122046 [details] Patch

Gyuyoung Kim

Comment 30 2012-01-11 10:59:55 PST

Comment on attachment 122046 [details] Patch Attachment 122046 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/11211071

Raphael Kubo da Costa (:rakuco)

Comment 31 2012-01-11 11:32:57 PST

Comment on attachment 122046 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=122046&action=review > Tools/DumpRenderTree/efl/CMakeLists.txt:51 > + ${WEBCORE_DIR}/insppector Typo.

Vsevolod Vlasov

Comment 32 2012-01-11 11:50:33 PST

(In reply to comment #31) > (From update of attachment 122046 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=122046&action=review > > > Tools/DumpRenderTree/efl/CMakeLists.txt:51 > > + ${WEBCORE_DIR}/insppector > > Typo. Thanks a lot, I missed the typo and was confused with the bot failure.

Vsevolod Vlasov

Comment 33 2012-01-11 11:51:30 PST

Created attachment 122063 [details] Patch

Vsevolod Vlasov

Comment 34 2012-01-12 02:57:42 PST

Committed r104803: <http://trac.webkit.org/changeset/104803>

Balazs Kelemen

Comment 35 2012-01-12 03:33:02 PST

The patch broke a bunch of tests on Qt-WK2 (our own bot, connected to a local master): http://build.webkit.sed.hu/results/x86-32%20Linux%20Qt%20Release%20WebKit2/r104803%20(18438)/results.html I believe the same would happen on the master wk2 bots but if those could reach these tests, but as those are red like hell anyway those exit earlier. the Most likely the changes made to DRT should be ported to WTR as well.

Csaba Osztrogonác

Comment 36 2012-01-12 03:37:29 PST

(In reply to comment #34) > Committed r104803: <http://trac.webkit.org/changeset/104803> Could you update zillion platform specific expected results too? 47 fails on Qt: http://build.webkit.org/results/Qt%20Linux%20Release/r104803%20%2842026%29/results.html 304 fails on Qt-WK2: http://build.webkit.sed.hu/results/x86-32%20Linux%20Qt%20Release%20WebKit2/r104803%20%2818438%29/results.html 34 fails on GTK: http://build.webkit.org/results/GTK%20Linux%2064-bit%20Release/r104804%20(16149)/results.html ...

Nikolas Zimmermann

Comment 37 2012-01-12 03:53:36 PST

Broke the mac build, I landed a fix in r104807, which saves the rollout.

Csaba Osztrogonác

Comment 38 2012-01-12 04:33:36 PST

many general and all Qt expected updates landed in http://trac.webkit.org/changeset/104810 GTK updates landed in http://trac.webkit.org/changeset/104813 WebkitTestRunner (wk2) fix landed in http://trac.webkit.org/changeset/104812 Let's wait all bots cycle and close the bug if everything is OK again.

Csaba Osztrogonác

Comment 39 2012-01-12 06:44:32 PST

Comment on attachment 122063 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=122063&action=review > Tools/DumpRenderTree/mac/UIDelegate.mm:72 > - printf ("CONSOLE MESSAGE: line %d: %s\n", [lineNumber intValue], [message UTF8String]); > + printf ("CONSOLE MESSAGE: "); > + if (lineNumber) > + printf ("line %d: ", [lineNumber intValue]); > + printf ("%s\n", [message UTF8String]); It is still broken on the Mac bot. :-/ http://build.webkit.org/results/SnowLeopard%20Intel%20Release%20(Tests)/r104808%20(36265)/results.html I think the problem is that lineNumber's type is (NSNumber *) Maybe if ([lineNumber intValue]) would be correct, but I don't know too much in objective C.

Csaba Osztrogonác

Comment 40 2012-01-12 06:47:24 PST

Mac is fixed by http://trac.webkit.org/changeset/104820

Vsevolod Vlasov

Comment 41 2012-01-13 08:31:39 PST

Bots look hapy now, closing as fixed.

Note You need to log in before you can comment on or make changes to this bug.