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
: WebKit
Web Inspector (Deprecated)
: 528+ (Nightly build)
: All All
: P2 Normal
Assigned To:
:
:
:
:
  Show dependency treegraph
 
Reported: 2011-08-17 06:28 PST by
Modified: 2012-12-11 08:11 PST (History)


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


Note

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


Description From 2011-08-17 06:28:08 PST
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 From 2011-08-17 08:48:22 PST -------
Created an attachment (id=104176) [details]
[PATCH] For ews.
------- Comment #2 From 2011-08-17 08:51:29 PST -------
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 From 2011-08-29 12:33:55 PST -------
(From update of attachment 104176 [details])
Clearing r? for now.
------- Comment #4 From 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 From 2012-11-27 17:31:38 PST -------
I don't think anyone is doing it.
------- Comment #6 From 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 From 2012-12-04 08:43:34 PST -------
Created an attachment (id=177496) [details]
Patch
------- Comment #8 From 2012-12-05 01:18:21 PST -------
Created an attachment (id=177694) [details]
Patch
------- Comment #9 From 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 From 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 From 2012-12-05 01:26:17 PST -------
(From update of attachment 177694 [details])
Attachment 177694 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/15138541
------- Comment #12 From 2012-12-05 01:27:20 PST -------
(From update of attachment 177694 [details])
Attachment 177694 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/15138542
------- Comment #13 From 2012-12-05 01:47:20 PST -------
(From update of attachment 177694 [details])
Attachment 177694 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/15157267
------- Comment #14 From 2012-12-05 02:05:29 PST -------
(From update of attachment 177694 [details])
Attachment 177694 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/15158215
------- Comment #15 From 2012-12-05 02:23:57 PST -------
(From update of attachment 177694 [details])
Attachment 177694 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/15144436
------- Comment #16 From 2012-12-05 06:46:18 PST -------
Created an attachment (id=177739) [details]
Patch
------- Comment #17 From 2012-12-05 06:47:43 PST -------
(In reply to comment #16)
> Created an attachment (id=177739) [details] [details]
> Patch

This one might actually compile. Note to self: don't upload patches that depend on other patches. :)
------- Comment #18 From 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 From 2012-12-06 11:15:29 PST -------
(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?
------- Comment #20 From 2012-12-06 11:25:17 PST -------
(In reply to comment #19)
> (From update of attachment 177739 [details] [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 From 2012-12-10 05:02:43 PST -------
(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.

> 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 From 2012-12-10 12:57:40 PST -------
(In reply to comment #21)
> (From update of attachment 177739 [details] [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 From 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 From 2012-12-11 01:53:50 PST -------
Created an attachment (id=178751) [details]
Patch
------- Comment #25 From 2012-12-11 02:25:56 PST -------
Created an attachment (id=178756) [details]
Patch
------- Comment #26 From 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 From 2012-12-11 02:44:45 PST -------
(From update of attachment 178756 [details])
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 From 2012-12-11 04:46:17 PST -------
(In reply to comment #27)
> (From update of attachment 178756 [details] [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 From 2012-12-11 07:22:05 PST -------
Created an attachment (id=178800) [details]
Patch for landing
------- Comment #30 From 2012-12-11 08:11:07 PST -------
(From update of attachment 178800 [details])
Clearing flags on attachment: 178800

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