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.
Created attachment 104176 [details] [PATCH] For ews.
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 on attachment 104176 [details] [PATCH] For ews. Clearing r? for now.
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...
I don't think anyone is doing it.
(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?
Created attachment 177496 [details] Patch
Created attachment 177694 [details] Patch
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.
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 on attachment 177694 [details] Patch Attachment 177694 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/15138541
Comment on attachment 177694 [details] Patch Attachment 177694 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/15138542
Comment on attachment 177694 [details] Patch Attachment 177694 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/15157267
Comment on attachment 177694 [details] Patch Attachment 177694 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/15158215
Comment on attachment 177694 [details] Patch Attachment 177694 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/15144436
Created attachment 177739 [details] Patch
(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. :)
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 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?
(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 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?
(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.
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.
Created attachment 178751 [details] Patch
Created attachment 178756 [details] Patch
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 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?
(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.
Created attachment 178800 [details] Patch for landing
Comment on attachment 178800 [details] Patch for landing Clearing flags on attachment: 178800 Committed r137318: <http://trac.webkit.org/changeset/137318>
All reviewed patches have been landed. Closing bug.