Bug 66371 - Web Inspector: ConsoleTypes should not expose MessageType - it should be private to inspector.
: Web Inspector: ConsoleTypes should not expose MessageType - it should be priv...
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated)
: 528+ (Nightly build)
: All All
: P2 Normal
Assigned To: Mike West
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-08-17 06:28 PDT by Pavel Feldman
Modified: 2012-12-11 08:11 PST (History)
41 users (show)

See Also:


Attachments
[PATCH] For ews. (110.96 KB, patch)
2011-08-17 08:48 PDT, Pavel Feldman
no flags Details | Formatted Diff | Diff
Patch (126.86 KB, patch)
2012-12-04 08:43 PST, Mike West
no flags Details | Formatted Diff | Diff
Patch (127.08 KB, patch)
2012-12-05 01:18 PST, Mike West
no flags Details | Formatted Diff | Diff
Patch (134.04 KB, patch)
2012-12-05 06:46 PST, Mike West
no flags Details | Formatted Diff | Diff
Patch (136.35 KB, patch)
2012-12-11 01:53 PST, Mike West
no flags Details | Formatted Diff | Diff
Patch (136.16 KB, patch)
2012-12-11 02:25 PST, Mike West
no flags Details | Formatted Diff | Diff
Patch for landing (136.20 KB, patch)
2012-12-11 07:22 PST, Mike West
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Pavel Feldman 2011-08-17 06:28:08 PDT
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.
Comment 1 Pavel Feldman 2011-08-17 08:48:22 PDT
Created attachment 104176 [details]
[PATCH] For ews.
Comment 2 WebKit Review Bot 2011-08-17 08:51:29 PDT
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.
Comment 3 Pavel Feldman 2011-08-29 12:33:55 PDT
Comment on attachment 104176 [details]
[PATCH] For ews.

Clearing r? for now.
Comment 4 Mike West 2012-11-27 06:46:55 PST
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...
Comment 5 Pavel Feldman 2012-11-27 17:31:38 PST
I don't think anyone is doing it.
Comment 6 Mike West 2012-12-04 07:57:19 PST
(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?
Comment 7 Mike West 2012-12-04 08:43:34 PST
Created attachment 177496 [details]
Patch
Comment 8 Mike West 2012-12-05 01:18:21 PST
Created attachment 177694 [details]
Patch
Comment 9 WebKit Review Bot 2012-12-05 01:20:36 PST
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
Comment 10 WebKit Review Bot 2012-12-05 01:21:25 PST
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.
Comment 11 Early Warning System Bot 2012-12-05 01:26:17 PST
Comment on attachment 177694 [details]
Patch

Attachment 177694 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/15138541
Comment 12 Early Warning System Bot 2012-12-05 01:27:20 PST
Comment on attachment 177694 [details]
Patch

Attachment 177694 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/15138542
Comment 13 Build Bot 2012-12-05 01:47:20 PST
Comment on attachment 177694 [details]
Patch

Attachment 177694 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/15157267
Comment 14 Build Bot 2012-12-05 02:05:29 PST
Comment on attachment 177694 [details]
Patch

Attachment 177694 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/15158215
Comment 15 WebKit Review Bot 2012-12-05 02:23:57 PST
Comment on attachment 177694 [details]
Patch

Attachment 177694 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/15144436
Comment 16 Mike West 2012-12-05 06:46:18 PST
Created attachment 177739 [details]
Patch
Comment 17 Mike West 2012-12-05 06:47:43 PST
(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. :)
Comment 18 WebKit Review Bot 2012-12-05 06:48:44 PST
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.
Comment 19 Darin Fisher (:fishd, Google) 2012-12-06 11:15:29 PST
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?
Comment 20 Mike West 2012-12-06 11:25:17 PST
(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.
Comment 21 Pavel Feldman 2012-12-10 05:02:43 PST
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?
Comment 22 Mike West 2012-12-10 12:57:40 PST
(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.
Comment 23 Timothy Hatcher 2012-12-10 18:25:55 PST
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.
Comment 24 Mike West 2012-12-11 01:53:50 PST
Created attachment 178751 [details]
Patch
Comment 25 Mike West 2012-12-11 02:25:56 PST
Created attachment 178756 [details]
Patch
Comment 26 WebKit Review Bot 2012-12-11 02:28:27 PST
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.
Comment 27 Kent Tamura 2012-12-11 02:44:45 PST
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?
Comment 28 Mike West 2012-12-11 04:46:17 PST
(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.
Comment 29 Mike West 2012-12-11 07:22:05 PST
Created attachment 178800 [details]
Patch for landing
Comment 30 WebKit Review Bot 2012-12-11 08:11:07 PST
Comment on attachment 178800 [details]
Patch for landing

Clearing flags on attachment: 178800

Committed r137318: <http://trac.webkit.org/changeset/137318>
Comment 31 WebKit Review Bot 2012-12-11 08:11:16 PST
All reviewed patches have been landed.  Closing bug.