Bug 66371 – Web Inspector: ConsoleTypes should not expose MessageType - it should be private to inspector.

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

[Pavel Feldman]

There is a confusion in the MessageSource / MessageLevel / MessageType trio. First two are supposed to define message origin and its severity, while the third one is for rendering console.dir/log objects properly in the inspector. Fix that via defining MessageType outside ConsoleTypes.

[Pavel Feldman]

Created attachment 104176 [details]
[PATCH] For ews.

[WebKit Review Bot]

Attachment 104176 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebKit/mac/WebCoreSupport/WebChromeClient.h:79:  The parameter name "source" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebKit/mac/WebCoreSupport/WebChromeClient.h:79:  The parameter name "level" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebKit/win/WebCoreSupport/WebChromeClient.h:83:  The parameter name "source" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebKit/win/WebCoreSupport/WebChromeClient.h:83:  The parameter name "level" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/page/Console.h:60:  The parameter name "callStack" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebKit/gtk/WebCoreSupport/ChromeClientGtk.h:82:  The parameter name "source" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 6 in 78 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Pavel Feldman]

Comment on attachment 104176 [details]
[PATCH] For ews.

Clearing r? for now.

[Mike West]

Is this something you're still working on, Pavel? Since it feels like I'm touching all these call sites anyway, I could pick this up once 100650 stabilizes...

[Pavel Feldman]

I don't think anyone is doing it.

[Mike West]

(In reply to comment #5)
> I don't think anyone is doing it.

Fiddled with this a bit today. What do you think we ought to do with ChromeClient implementations? It doesn't look like the Chromium port uses the type, but the Mac port does seem to at least convert it into an internal string representation (see http://trac.webkit.org/browser/trunk/Source/WebKit/mac/WebCoreSupport/WebChromeClient.mm#L406 and http://trac.webkit.org/browser/trunk/Source/WebKit/mac/WebCoreSupport/WebChromeClient.mm#L360). Do you (or anyone on CC? :) ) know if they need that to pipe things up to their implementation of the console?

[Mike West]

Created attachment 177496 [details]
Patch

[Mike West]

Created attachment 177694 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[WebKit Review Bot]

Attachment 177694 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebKit2/WebProcess/WebCoreSupport/WebChromeClient.cpp:270:  Omit int when using unsigned  [runtime/unsigned] [1]
Source/WebKit/chromium/src/WebSharedWorkerImpl.cpp:212:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Source/WebKit/chromium/src/WebSharedWorkerImpl.cpp:219:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit/chromium/src/WebSharedWorkerImpl.cpp:227:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/page/ChromeClient.h:137:  Omit int when using unsigned  [runtime/unsigned] [1]
Source/WebKit/win/WebCoreSupport/WebChromeClient.h:81:  The parameter name "source" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebKit/win/WebCoreSupport/WebChromeClient.h:81:  The parameter name "level" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebKit/wince/WebCoreSupport/ChromeClientWinCE.cpp:187:  Omit int when using unsigned  [runtime/unsigned] [1]
Source/WebKit/chromium/src/WebPagePopupImpl.cpp:88:  Omit int when using unsigned  [runtime/unsigned] [1]
Source/WebKit/wince/WebCoreSupport/ChromeClientWinCE.h:79:  Omit int when using unsigned  [runtime/unsigned] [1]
Source/WebKit/blackberry/WebCoreSupport/ChromeClientBlackBerry.h:64:  Omit int when using unsigned  [runtime/unsigned] [1]
Source/WebKit/gtk/WebCoreSupport/ChromeClientGtk.h:85:  The parameter name "source" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebKit/blackberry/WebCoreSupport/ChromeClientBlackBerry.cpp:92:  Omit int when using unsigned  [runtime/unsigned] [1]
Source/WebCore/workers/WorkerMessagingProxy.cpp:341:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit/gtk/WebCoreSupport/ChromeClientGtk.cpp:330:  Omit int when using unsigned  [runtime/unsigned] [1]
Source/WebKit/mac/WebCoreSupport/WebChromeClient.h:77:  Omit int when using unsigned  [runtime/unsigned] [1]
Source/WebKit2/WebProcess/WebCoreSupport/WebChromeClient.h:93:  Omit int when using unsigned  [runtime/unsigned] [1]
Total errors found: 17 in 92 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Early Warning System Bot]

Comment on attachment 177694 [details]
Patch

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

[Early Warning System Bot]

Comment on attachment 177694 [details]
Patch

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

[Build Bot]

Comment on attachment 177694 [details]
Patch

Attachment 177694 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/15157267

[Build Bot]

Comment on attachment 177694 [details]
Patch

Attachment 177694 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/15158215

[WebKit Review Bot]

Comment on attachment 177694 [details]
Patch

Attachment 177694 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/15144436

[Mike West]

Created attachment 177739 [details]
Patch

[Mike West]

(In reply to comment #16)
> Created an attachment (id=177739) [details]
> Patch

This one might actually compile. Note to self: don't upload patches that depend on other patches. :)

[WebKit Review Bot]

Attachment 177739 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebKit/chromium/src/WebSharedWorkerImpl.cpp:217:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit/wx/WebKitSupport/ChromeClientWx.cpp:232:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 2 in 93 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Darin Fisher (:fishd, Google)]

Comment on attachment 177739 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=177739&action=review

> Source/WebKit/chromium/public/WebSharedWorkerClient.h:62
> +    virtual void postConsoleMessageToWorkerObject(int, int sourceIdentifier, int messageLevel,

It seems like this will break compatibility with the Chromium code base.  Are you planning on landing Chromium-side support for the new method signature first?  Or, how are you managing the dance of changing this interface?

[Mike West]

(In reply to comment #19)
> (From update of attachment 177739 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=177739&action=review
> 
> > Source/WebKit/chromium/public/WebSharedWorkerClient.h:62
> > +    virtual void postConsoleMessageToWorkerObject(int, int sourceIdentifier, int messageLevel,
> 
> It seems like this will break compatibility with the Chromium code base.  Are you planning on landing Chromium-side support for the new method signature first?  Or, how are you managing the dance of changing this interface?

I've uploaded the patch to have something concrete with which to ask Pavel (hi, Pavel!) if changing the clients is desirable. It wouldn't just break Chromium, it'd be all the ports.

If it's what we want to do, I'd probably add methods to the patch with the old signature that thunk down to the new signature. Once that rolls into Chromium, et al., I'd land a Chromium-side patch to stop using the old signature, then once that rolled into WebKit, I'd remove the old signature once I'd gotten all the other ports migrated over.

That seems like it would work.

[Pavel Feldman]

Comment on attachment 177739 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=177739&action=review

Overall looks good. Please make sure xenon is fine with removing messagetype from Apple webkit ports. AFAIR it was only used in toString form there, so nothing should break.

> Source/WebCore/inspector/InspectorConsoleTypes.h:1
> +/*

This one could be called ConsoleAPITypes

> Source/WebCore/page/Console.cpp:232
> +    internalAddMessage(page(), LogMessageType, ErrorMessageLevel, state, arguments, shouldPrintExceptions());

I only see call sites that use page() as a first argument, why did this change?

[Mike West]

(In reply to comment #21)
> (From update of attachment 177739 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=177739&action=review
> 
> Overall looks good. Please make sure xenon is fine with removing messagetype from Apple webkit ports. AFAIR it was only used in toString form there, so nothing should break.

CCing him (Hi!).

> > Source/WebCore/page/Console.cpp:232
> > +    internalAddMessage(page(), LogMessageType, ErrorMessageLevel, state, arguments, shouldPrintExceptions());
> 
> I only see call sites that use page() as a first argument, why did this change?

As we discussed via chat, the alternative would be to make this ::addMessage method a member function, which would expose MessageType on the public API. It seemed like a better idea to turn it into a static method that didn't live on the object. To do that, I need to pass in the Page object.

[Timothy Hatcher]

This was exposed via WebKit API (not just "toString" debugging). It was added to a dictionary sent to a private Ui delegate method. I don't think anyone uses this private API anymore.

[Mike West]

Created attachment 178751 [details]
Patch

[Mike West]

Created attachment 178756 [details]
Patch

[WebKit Review Bot]

Attachment 178756 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebKit/chromium/src/WebSharedWorkerImpl.cpp:217:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit/chromium/public/WebSharedWorkerClient.h:66:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit/chromium/public/WebSharedWorkerClient.h:69:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit/wx/WebKitSupport/ChromeClientWx.cpp:232:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 4 in 94 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Kent Tamura]

Comment on attachment 178756 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=178756&action=review

Chromium WebKit API change LGTM.

> Source/WebKit/chromium/ChangeLog:9
> +        Drops WebCore::MessageLevel from the addMessageToConsole method, and all
> +        the ancilliary places it touches.

Don't you drop MessageType?

[Mike West]

(In reply to comment #27)
> (From update of attachment 178756 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=178756&action=review
> 
> Chromium WebKit API change LGTM.

Thanks!

> > Source/WebKit/chromium/ChangeLog:9
> > +        Drops WebCore::MessageLevel from the addMessageToConsole method, and all
> > +        the ancilliary places it touches.
> 
> Don't you drop MessageType?

Ah. I did. :)

I'll fix that before landing, assuming that the bots are happy.

[Mike West]

Created attachment 178800 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 178800 [details]
Patch for landing

Clearing flags on attachment: 178800

Committed r137318: <http://trac.webkit.org/changeset/137318>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.