We used to have several inspector methods exposed via delegating methods in ScriptExecutionContext and its derived classes (Document & WorkerContext). Since we now expose InspectorController from ScriptExecutionContext, removing those methods allows us to simplify things a bit. This also allows to remove extra parameter to Document::addMessage that used to select between two possible message destinations.
We also decided to remove logging of XHR and script import as part of this change, as these seem to have little value (covered by resource tracking) and clutter console logs.
Created attachment 52410 [details] Remove inspector methods from ScriptExecutionContext & dervied classes. Remove MessageDestination parameter from console message calls. Remove logging upon successful XHR & importScripts.
Attachment 52410 [details] did not pass style-queue: Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1 WebCore/workers/WorkerContext.cpp:208: One line control clauses should not use braces. [whitespace/braces] [4] Total errors found: 1 in 49 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 52411 [details] Remove inspector methods from ScriptExecutionContext & dervied classes. Remove MessageDestination parameter from console message calls. Remove logging upon successful XHR & importScripts (style fixed)
Comment on attachment 52411 [details] Remove inspector methods from ScriptExecutionContext & dervied classes. Remove MessageDestination parameter from console message calls. Remove logging upon successful XHR & importScripts (style fixed) > > - scriptExecutionContext()->scriptImported(scriptLoader.identifier(), scriptLoader.script()); > #if ENABLE(INSPECTOR) > - scriptExecutionContext()->addMessage(InspectorControllerDestination, JSMessageSource, LogMessageType, LogMessageLevel, "Worker script imported: \"" + *it + "\".", callerLine, callerURL); > + if (InspectorController* inspector = scriptExecutionContext()->inspectorController()) > + inspector->scriptImported(scriptLoader.identifier(), scriptLoader.script()); > #endif Looks like after this change script imports won't be reported to the user because InspectorController will be 0.
(In reply to comment #5) > (From update of attachment 52411 [details]) > > > > - scriptExecutionContext()->scriptImported(scriptLoader.identifier(), scriptLoader.script()); > > #if ENABLE(INSPECTOR) > > - scriptExecutionContext()->addMessage(InspectorControllerDestination, JSMessageSource, LogMessageType, LogMessageLevel, "Worker script imported: \"" + *it + "\".", callerLine, callerURL); > > + if (InspectorController* inspector = scriptExecutionContext()->inspectorController()) > > + inspector->scriptImported(scriptLoader.identifier(), scriptLoader.script()); > > #endif > Looks like after this change script imports won't be reported to the user > because InspectorController will be 0. addMessage() was dropped intentionally (we don't want this reported to console), and scriptImported() didn't work before (used to be notImplemented() before the patch) -- this will only be fixed once we implement worker debugging.
I don't think that refactoring code and drastically changing observable behavior should be done in one patch.
(In reply to comment #7) > I don't think that refactoring code and drastically changing observable > behavior should be done in one patch. What do you mean by "drastically changing observable behavior"? Afaik all clients were not implementing the callback that is getting removed + the only visible change is that we are stopping polluting inspector's console (not to be confused with Console) by the XHR-related records.
Yes. Why do this in the same patch with refactoring?
Comment on attachment 52411 [details] Remove inspector methods from ScriptExecutionContext & dervied classes. Remove MessageDestination parameter from console message calls. Remove logging upon successful XHR & importScripts (style fixed) This patch looks great. I'm really happy we're going to stop spamming the console with XHR finished messages. I'm surprised this didn't change the expected results of any LayoutTests. I'd imagine that any test that used XHR would have its expected output changed. Are you sure you ran all the tests? Please separate this change into two patches: 1) Removing the console spam. 2) Removing all the dead code. Thanks for the patch.
Removing console spam should be done in a separate bug whose title says so. We might already have one, I didn't search.
(In reply to comment #10) > (From update of attachment 52411 [details]) > This patch looks great. I'm really happy we're going to stop spamming the > console with XHR finished messages. > > I'm surprised this didn't change the expected results of any LayoutTests. I'd > imagine that any test that used XHR would have its expected output changed. > Are you sure you ran all the tests? > > Please separate this change into two patches: > 1) Removing the console spam. > 2) Removing all the dead code. > > Thanks for the patch. Thanks -- I'll split the patch. The tests are fine, 'cause we removed messages that were only sent to inspector console (not the Console) -- there don't seem to be tests that cover this.
> Thanks -- I'll split the patch. The tests are fine, 'cause we removed > messages that were only sent to inspector console (not the Console) -- there > don't seem to be tests that cover this. Ah, I see. Well, it's too bad we don't have coverage of this then. :)
Created attachment 52636 [details] Remove inspector methods from ScriptExecutionContext & dervied classes. Remove MessageDestination parameter from console message calls (split XHR/importScripts logging into separate patch)
Attachment 52636 [details] did not pass style-queue: Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1 WebKit/chromium/public/WebCommonWorkerClient.h:52: Should have a space between // and comment [whitespace/comments] [4] WebCore/workers/WorkerContext.cpp:207: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] WebCore/workers/WorkerContext.cpp:210: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] WebKit/chromium/src/WebWorkerClientImpl.h:81: Should have a space between // and comment [whitespace/comments] [4] Total errors found: 4 in 45 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 52638 [details] Remove inspector methods from ScriptExecutionContext & dervied classes. Remove MessageDestination parameter from console message calls (style fixed)
Comment on attachment 52638 [details] Remove inspector methods from ScriptExecutionContext & dervied classes. Remove MessageDestination parameter from console message calls (style fixed) Please file a bug for the code cleanup and put it into the TODO comments. Otherwise looks good.
Created attachment 52647 [details] Remove inspector methods from ScriptExecutionContext & dervied classes. Remove MessageDestination parameter from console message calls (added bug reference for TODO)
+ // TODO(37155): the below is for compatibility only and + // should be removed once Chromium is updated to remove + // message destination parameter Please don't use TODO, use FIXME. And please give a full bug URL - there are several bug tracking systems in use for the project.
+ Removed redundant FrameLoaderClient::dispatchDidLoadResourceFromMemoryCache() I don't think that's true.
+ virtual void postConsoleMessageToWorkerObject( + int sourceIdentifier, + int messageType, + int messageLevel, + const WebString& message, + int lineNumber, + const WebString& sourceURL) This isn't common style in WebKit. We don't wrap lines to make them fit on an iPhone screen. dispatchTaskToMainThread(createCallbackTask(&postConsoleMessageTask, this, - static_cast<int>(destination), static_cast<int>(source), static_cast<int>(type), static_cast<int>(level), I'm not sure if these type casts are necessary. -__ZN20WebFrameLoaderClient39dispatchDidLoadResourceByXMLHttpRequestEmRKN7WebCore12ScriptStringE There is no need to touch WebCore.order - Apple updates it as necessary, and it's not used by other vendors.
Comment on attachment 52638 [details] Remove inspector methods from ScriptExecutionContext & dervied classes. Remove MessageDestination parameter from console message calls (style fixed) Cleared Yury Semikhatsky's review+ from obsolete attachment 52638 [details] so that this bug does not appear in http://webkit.org/pending-commit.
Created attachment 52719 [details] Remove inspector methods from ScriptExecutionContext & dervied classes. Remove MessageDestination parameter from console message calls (style fixed, ChangeLog entries fixed)
(In reply to comment #21) > + virtual void postConsoleMessageToWorkerObject( > + int sourceIdentifier, > + int messageType, > + int messageLevel, > + const WebString& message, > + int lineNumber, > + const WebString& sourceURL) > > This isn't common style in WebKit. We don't wrap lines to make them fit on an > iPhone screen. Considering style guide does not state this explicitly, I thought it might be good idea to preserve existing wrapping style for the file. Fixed per your request, though. > > dispatchTaskToMainThread(createCallbackTask(&postConsoleMessageTask, this, > - static_cast<int>(destination), > static_cast<int>(source), > static_cast<int>(type), > static_cast<int>(level), > > I'm not sure if these type casts are necessary. They're not indeed (fixed this as well). As you can see, it wasn't me who added them, just tried to minimize patch footprint. > -__ZN20WebFrameLoaderClient39dispatchDidLoadResourceByXMLHttpRequestEmRKN7WebCore12ScriptStringE > > There is no need to touch WebCore.order - Apple updates it as necessary, and > it's not used by other vendors. Thanks, fixed. >+ Removed redundant > FrameLoaderClient::dispatchDidLoadResourceFromMemoryCache() > I don't think that's true. Indeed, I accidently put a wrong method name there. Fixed.
Comment on attachment 52719 [details] Remove inspector methods from ScriptExecutionContext & dervied classes. Remove MessageDestination parameter from console message calls (style fixed, ChangeLog entries fixed) Clearing flags on attachment: 52719 Committed r57210: <http://trac.webkit.org/changeset/57210>
All reviewed patches have been landed. Closing bug.