Bug 36949 – Web Inspector: remove inspector methods from ScriptExecutionContext and derived classes

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

[Andrey Kosyakov]

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.

[Andrey Kosyakov]

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.

[Andrey Kosyakov]

Created attachment 52410 [details]
Remove inspector methods from ScriptExecutionContext & dervied classes. Remove MessageDestination parameter from console message calls. Remove logging upon successful XHR & importScripts.

[WebKit Review Bot]

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.

[Andrey Kosyakov]

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)

[Yury Semikhatsky]

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.

[Andrey Kosyakov]

(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.

[Alexey Proskuryakov]

I don't think that refactoring code and drastically changing observable behavior should be done in one patch.

[Pavel Feldman]

(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.

[Alexey Proskuryakov]

Yes. Why do this in the same patch with refactoring?

[Adam Barth]

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.

[Alexey Proskuryakov]

Removing console spam should be done in a separate bug whose title says so. We might already have one, I didn't search.

[Andrey Kosyakov]

(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.

[Adam Barth]

> 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.  :)

[Andrey Kosyakov]

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)

[WebKit Review Bot]

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.

[Andrey Kosyakov]

Created attachment 52638 [details]
Remove inspector methods from ScriptExecutionContext & dervied classes. Remove MessageDestination parameter from console message calls (style fixed)

[Yury Semikhatsky]

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.

[Andrey Kosyakov]

Created attachment 52647 [details]
Remove inspector methods from ScriptExecutionContext & dervied classes. Remove MessageDestination parameter from console message calls (added bug reference for TODO)

[Alexey Proskuryakov]

+    // 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.

[Alexey Proskuryakov]

+        Removed redundant FrameLoaderClient::dispatchDidLoadResourceFromMemoryCache()

I don't think that's true.

[Alexey Proskuryakov]

+    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.

[Eric Seidel (no email)]

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.

[Andrey Kosyakov]

Created attachment 52719 [details]
Remove inspector methods from ScriptExecutionContext & dervied classes. Remove MessageDestination parameter from console message calls (style fixed, ChangeLog entries fixed)

[Andrey Kosyakov]

(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.

[WebKit Commit Bot]

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>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.