Make default console messages line numbers consistent.

• Component: Web Inspector (Deprecated)
• Status: RESOLVED FIXED

[Vsevolod Vlasov]

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).

[Vsevolod Vlasov]

Created attachment 118364 [details]
Patch

[WebKit Review Bot]

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]

Created attachment 118374 [details]
Patch

[Alexey Proskuryakov]

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]

(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]

Can someone confirm what the intended behavior is?

[Kent Tamura]

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]

> 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]

(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]

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]

Created attachment 122003 [details]
Patch

[Vsevolod Vlasov]

Created attachment 122007 [details]
Patch

[Gustavo Noronha (kov)]

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 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 on attachment 122007 [details]
Patch

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

[Vsevolod Vlasov]

Created attachment 122012 [details]
Patch

[Early Warning System Bot]

Comment on attachment 122012 [details]
Patch

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

[Vsevolod Vlasov]

Created attachment 122015 [details]
Patch

[Gustavo Noronha (kov)]

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 on attachment 122015 [details]
Patch

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

[Gyuyoung Kim]

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 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 on attachment 122012 [details]
Patch

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

[Gyuyoung Kim]

Comment on attachment 122012 [details]
Patch

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

[Vsevolod Vlasov]

Created attachment 122036 [details]
Patch

[Early Warning System Bot]

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 on attachment 122036 [details]
Patch

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

[Gyuyoung Kim]

Comment on attachment 122036 [details]
Patch

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

[Vsevolod Vlasov]

Created attachment 122046 [details]
Patch

[Gyuyoung Kim]

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 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]

(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]

Created attachment 122063 [details]
Patch

[Vsevolod Vlasov]

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

[Balazs Kelemen]

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]

(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]

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

[Csaba Osztrogonác]

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 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]

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

[Vsevolod Vlasov]

Bots look hapy now, closing as fixed.