Bug 36949 - Web Inspector: remove inspector methods from ScriptExecutionContext and derived classes
: Web Inspector: remove inspector methods from ScriptExecutionContext and deriv...
Status: RESOLVED FIXED
: WebKit
Web Inspector (Deprecated)
: 528+ (Nightly build)
: All All
: P2 Normal
Assigned To:
:
:
:
:
  Show dependency treegraph
 
Reported: 2010-04-01 04:48 PST by
Modified: 2010-04-07 09:18 PST (History)


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 PST, Andrey Kosyakov
no flags Review Patch | 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 PST, Andrey Kosyakov
abarth: review-
Review Patch | 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 PST, Andrey Kosyakov
no flags Review Patch | 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 PST, Andrey Kosyakov
no flags Review Patch | 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 PST, Andrey Kosyakov
yurys: review+
ap: commit‑queue-
Review Patch | 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 PST, Andrey Kosyakov
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 2010-04-01 04:48:41 PST
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 From 2010-04-02 05:54:01 PST -------
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 From 2010-04-02 05:55:36 PST -------
Created an attachment (id=52410) [details]
Remove inspector methods from ScriptExecutionContext & dervied classes. Remove MessageDestination parameter from console message calls. Remove logging upon successful XHR & importScripts.
------- Comment #3 From 2010-04-02 06:04:19 PST -------
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 From 2010-04-02 06:39:27 PST -------
Created an attachment (id=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 From 2010-04-02 08:09:13 PST -------
(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.
------- Comment #6 From 2010-04-02 08:17:58 PST -------
(In reply to comment #5)
> (From update of attachment 52411 [details] [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 From 2010-04-02 08:29:33 PST -------
I don't think that refactoring code and drastically changing observable behavior should be done in one patch.
------- Comment #8 From 2010-04-02 08:31:44 PST -------
(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 From 2010-04-02 08:38:15 PST -------
Yes. Why do this in the same patch with refactoring?
------- Comment #10 From 2010-04-02 08:40:20 PST -------
(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.
------- Comment #11 From 2010-04-02 08:50:15 PST -------
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 From 2010-04-02 09:00:50 PST -------
(In reply to comment #10)
> (From update of attachment 52411 [details] [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 From 2010-04-02 09:15:51 PST -------
> 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 From 2010-04-06 07:20:34 PST -------
Created an attachment (id=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 From 2010-04-06 07:27:18 PST -------
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 From 2010-04-06 07:51:04 PST -------
Created an attachment (id=52638) [details]
Remove inspector methods from ScriptExecutionContext & dervied classes. Remove MessageDestination parameter from console message calls (style fixed)
------- Comment #17 From 2010-04-06 09:39:44 PST -------
(From update of attachment 52638 [details])
Please file a bug for the code cleanup and put it into the TODO comments. Otherwise looks good.
------- Comment #18 From 2010-04-06 09:46:46 PST -------
Created an attachment (id=52647) [details]
Remove inspector methods from ScriptExecutionContext & dervied classes. Remove MessageDestination parameter from console message calls (added bug reference for TODO)
------- Comment #19 From 2010-04-06 10:45:31 PST -------
+    // 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 From 2010-04-06 10:51:26 PST -------
+        Removed redundant FrameLoaderClient::dispatchDidLoadResourceFromMemoryCache()

I don't think that's true.
------- Comment #21 From 2010-04-06 10:56:48 PST -------
+    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 From 2010-04-06 23:43:13 PST -------
(From update of attachment 52638 [details])
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 From 2010-04-07 02:41:19 PST -------
Created an attachment (id=52719) [details]
Remove inspector methods from ScriptExecutionContext & dervied classes. Remove MessageDestination parameter from console message calls (style fixed, ChangeLog entries fixed)
------- Comment #24 From 2010-04-07 03:09:01 PST -------
(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 From 2010-04-07 09:18:46 PST -------
(From update of attachment 52719 [details])
Clearing flags on attachment: 52719

Committed r57210: <http://trac.webkit.org/changeset/57210>
------- Comment #26 From 2010-04-07 09:18:54 PST -------
All reviewed patches have been landed.  Closing bug.