Bug 74075 - Make default console messages line numbers consistent.
Summary: Make default console messages line numbers consistent.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated) (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Vsevolod Vlasov
URL:
Keywords:
Depends on: 76167
Blocks:
  Show dependency treegraph
 
Reported: 2011-12-08 04:41 PST by Vsevolod Vlasov
Modified: 2012-01-13 08:31 PST (History)
17 users (show)

See Also:


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+
gns: 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

Note You need to log in before you can comment on or make changes to this bug.
Description Vsevolod Vlasov 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).
Comment 1 Vsevolod Vlasov 2011-12-08 05:01:45 PST
Created attachment 118364 [details]
Patch
Comment 2 WebKit Review Bot 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
Comment 3 Vsevolod Vlasov 2011-12-08 05:51:51 PST
Created attachment 118374 [details]
Patch
Comment 4 Alexey Proskuryakov 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.
Comment 5 Adam Barth 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.
Comment 6 Alexey Proskuryakov 2011-12-08 15:28:00 PST
Can someone confirm what the intended behavior is?
Comment 7 Kent Tamura 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.
Comment 8 Adam Barth 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).
Comment 9 Vsevolod Vlasov 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
Comment 10 Alexey Proskuryakov 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.
Comment 11 Vsevolod Vlasov 2012-01-11 04:21:33 PST
Created attachment 122003 [details]
Patch
Comment 12 Vsevolod Vlasov 2012-01-11 04:58:25 PST
Created attachment 122007 [details]
Patch
Comment 13 Gustavo Noronha (kov) 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
Comment 14 Gyuyoung Kim 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
Comment 15 Early Warning System Bot 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
Comment 16 Vsevolod Vlasov 2012-01-11 05:39:32 PST
Created attachment 122012 [details]
Patch
Comment 17 Early Warning System Bot 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
Comment 18 Vsevolod Vlasov 2012-01-11 06:27:23 PST
Created attachment 122015 [details]
Patch
Comment 19 Gustavo Noronha (kov) 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
Comment 20 Early Warning System Bot 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
Comment 21 Gyuyoung Kim 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
Comment 22 Pavel Feldman 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.
Comment 23 Collabora GTK+ EWS bot 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
Comment 24 Gyuyoung Kim 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
Comment 25 Vsevolod Vlasov 2012-01-11 08:51:38 PST
Created attachment 122036 [details]
Patch
Comment 26 Early Warning System Bot 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
Comment 27 Gustavo Noronha (kov) 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
Comment 28 Gyuyoung Kim 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
Comment 29 Vsevolod Vlasov 2012-01-11 10:16:01 PST
Created attachment 122046 [details]
Patch
Comment 30 Gyuyoung Kim 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
Comment 31 Raphael Kubo da Costa (:rakuco) 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.
Comment 32 Vsevolod Vlasov 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.
Comment 33 Vsevolod Vlasov 2012-01-11 11:51:30 PST
Created attachment 122063 [details]
Patch
Comment 34 Vsevolod Vlasov 2012-01-12 02:57:42 PST
Committed r104803: <http://trac.webkit.org/changeset/104803>
Comment 35 Balazs Kelemen 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.
Comment 37 Nikolas Zimmermann 2012-01-12 03:53:36 PST
Broke the mac build, I landed a fix in r104807, which saves the rollout.
Comment 38 Csaba Osztrogonác 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.
Comment 39 Csaba Osztrogonác 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.
Comment 40 Csaba Osztrogonác 2012-01-12 06:47:24 PST
Mac is fixed by http://trac.webkit.org/changeset/104820
Comment 41 Vsevolod Vlasov 2012-01-13 08:31:39 PST
Bots look hapy now, closing as fixed.