Bug 36949 - Web Inspector: remove inspector methods from ScriptExecutionContext and derived classes
Summary: Web Inspector: remove inspector methods from ScriptExecutionContext and deriv...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated) (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-04-01 04:48 PDT by Andrey Kosyakov
Modified: 2010-04-07 09:18 PDT (History)
13 users (show)

See Also:


Attachments
Remove inspector methods from ScriptExecutionContext & dervied classes. Remove MessageDestination parameter from console message calls. Remove logging upon successful XHR & importScripts. (64.72 KB, patch)
2010-04-02 05:55 PDT, Andrey Kosyakov
no flags Details | Formatted Diff | Diff
Remove inspector methods from ScriptExecutionContext & dervied classes. Remove MessageDestination parameter from console message calls. Remove logging upon successful XHR & importScripts (style fixed) (64.70 KB, patch)
2010-04-02 06:39 PDT, Andrey Kosyakov
abarth: review-
Details | Formatted Diff | Diff
Remove inspector methods from ScriptExecutionContext & dervied classes. Remove MessageDestination parameter from console message calls (split XHR/importScripts logging into separate patch) (61.02 KB, patch)
2010-04-06 07:20 PDT, Andrey Kosyakov
no flags Details | Formatted Diff | Diff
Remove inspector methods from ScriptExecutionContext & dervied classes. Remove MessageDestination parameter from console message calls (style fixed) (61.02 KB, patch)
2010-04-06 07:51 PDT, Andrey Kosyakov
no flags Details | Formatted Diff | Diff
Remove inspector methods from ScriptExecutionContext & dervied classes. Remove MessageDestination parameter from console message calls (added bug reference for TODO) (61.02 KB, patch)
2010-04-06 09:46 PDT, Andrey Kosyakov
yurys: review+
ap: commit-queue-
Details | Formatted Diff | Diff
Remove inspector methods from ScriptExecutionContext & dervied classes. Remove MessageDestination parameter from console message calls (style fixed, ChangeLog entries fixed) (60.88 KB, patch)
2010-04-07 02:41 PDT, Andrey Kosyakov
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andrey Kosyakov 2010-04-01 04:48:41 PDT
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.
Comment 1 Andrey Kosyakov 2010-04-02 05:54:01 PDT
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.
Comment 2 Andrey Kosyakov 2010-04-02 05:55:36 PDT
Created attachment 52410 [details]
Remove inspector methods from ScriptExecutionContext & dervied classes. Remove MessageDestination parameter from console message calls. Remove logging upon successful XHR & importScripts.
Comment 3 WebKit Review Bot 2010-04-02 06:04:19 PDT
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.
Comment 4 Andrey Kosyakov 2010-04-02 06:39:27 PDT
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 5 Yury Semikhatsky 2010-04-02 08:09:13 PDT
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.
Comment 6 Andrey Kosyakov 2010-04-02 08:17:58 PDT
(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.
Comment 7 Alexey Proskuryakov 2010-04-02 08:29:33 PDT
I don't think that refactoring code and drastically changing observable behavior should be done in one patch.
Comment 8 Pavel Feldman 2010-04-02 08:31:44 PDT
(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.
Comment 9 Alexey Proskuryakov 2010-04-02 08:38:15 PDT
Yes. Why do this in the same patch with refactoring?
Comment 10 Adam Barth 2010-04-02 08:40:20 PDT
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.
Comment 11 Alexey Proskuryakov 2010-04-02 08:50:15 PDT
Removing console spam should be done in a separate bug whose title says so. We might already have one, I didn't search.
Comment 12 Andrey Kosyakov 2010-04-02 09:00:50 PDT
(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.
Comment 13 Adam Barth 2010-04-02 09:15:51 PDT
> 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.  :)
Comment 14 Andrey Kosyakov 2010-04-06 07:20:34 PDT
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)
Comment 15 WebKit Review Bot 2010-04-06 07:27:18 PDT
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.
Comment 16 Andrey Kosyakov 2010-04-06 07:51:04 PDT
Created attachment 52638 [details]
Remove inspector methods from ScriptExecutionContext & dervied classes. Remove MessageDestination parameter from console message calls (style fixed)
Comment 17 Yury Semikhatsky 2010-04-06 09:39:44 PDT
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.
Comment 18 Andrey Kosyakov 2010-04-06 09:46:46 PDT
Created attachment 52647 [details]
Remove inspector methods from ScriptExecutionContext & dervied classes. Remove MessageDestination parameter from console message calls (added bug reference for TODO)
Comment 19 Alexey Proskuryakov 2010-04-06 10:45:31 PDT
+    // 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.
Comment 20 Alexey Proskuryakov 2010-04-06 10:51:26 PDT
+        Removed redundant FrameLoaderClient::dispatchDidLoadResourceFromMemoryCache()

I don't think that's true.
Comment 21 Alexey Proskuryakov 2010-04-06 10:56:48 PDT
+    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 22 Eric Seidel (no email) 2010-04-06 23:43:13 PDT
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.
Comment 23 Andrey Kosyakov 2010-04-07 02:41:19 PDT
Created attachment 52719 [details]
Remove inspector methods from ScriptExecutionContext & dervied classes. Remove MessageDestination parameter from console message calls (style fixed, ChangeLog entries fixed)
Comment 24 Andrey Kosyakov 2010-04-07 03:09:01 PDT
(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 25 WebKit Commit Bot 2010-04-07 09:18:46 PDT
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>
Comment 26 WebKit Commit Bot 2010-04-07 09:18:54 PDT
All reviewed patches have been landed.  Closing bug.