Bug 70600 - Web Inspector: [Extensions API] allow extensions to specify script to be injected on reload
Summary: Web Inspector: [Extensions API] allow extensions to specify script to be inje...
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: Andrey Kosyakov
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-10-21 04:43 PDT by Andrey Kosyakov
Modified: 2011-10-25 05:59 PDT (History)
9 users (show)

See Also:


Attachments
patch (17.74 KB, patch)
2011-10-21 04:45 PDT, Andrey Kosyakov
no flags Details | Formatted Diff | Diff
patch (fixed style) (17.74 KB, patch)
2011-10-21 04:49 PDT, Andrey Kosyakov
pfeldman: review-
Details | Formatted Diff | Diff
patch (17.19 KB, patch)
2011-10-24 09:32 PDT, Andrey Kosyakov
pfeldman: review-
Details | Formatted Diff | Diff
patch (16.95 KB, patch)
2011-10-24 09:55 PDT, Andrey Kosyakov
pfeldman: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andrey Kosyakov 2011-10-21 04:43:18 PDT
This changes webInspector.inspectedWindow.reload() to accept an options object that lets caller to specify a script to inject upon reload in addition to user agent string. The old interface is supported, but passing string instead of object causes deprecation warning to be printed.
Comment 1 Andrey Kosyakov 2011-10-21 04:45:27 PDT
Created attachment 111941 [details]
patch
Comment 2 WebKit Review Bot 2011-10-21 04:47:21 PDT
Attachment 111941 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/insp..." exit_code: 1

Source/WebCore/inspector/InspectorPageAgent.h:67:  This { should be at the end of the previous line  [whitespace/braces] [4]
Source/WebCore/inspector/InspectorPageAgent.h:85:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 2 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Andrey Kosyakov 2011-10-21 04:49:24 PDT
Created attachment 111944 [details]
patch (fixed style)
Comment 4 Pavel Feldman 2011-10-21 05:46:54 PDT
Comment on attachment 111944 [details]
patch (fixed style)

View in context: https://bugs.webkit.org/attachment.cgi?id=111944&action=review

> Source/WebCore/inspector/Inspector.json:164
> +                    { "name": "removeOnReload", "type": "boolean", "optional": true }

"persistent" + docs?

> Source/WebCore/inspector/front-end/ExtensionAPI.js:497
> +        else if (typeof optionsOrUserAgent === "string") {

This will never compile.

> Source/WebCore/inspector/front-end/WorkersSidebarPane.js:81
> +        if (!enabled === !this._fakeWorkersScriptIdentifier)

Could you disable checkbox instead?
Comment 5 Andrey Kosyakov 2011-10-24 09:32:38 PDT
Created attachment 112200 [details]
patch
Comment 6 Andrey Kosyakov 2011-10-24 09:35:07 PDT
(In reply to comment #4)

> > Source/WebCore/inspector/Inspector.json:164
> > +                    { "name": "removeOnReload", "type": "boolean", "optional": true }
> 
> "persistent" + docs?

Dropped this altogether -- instead, we expose scriptToEvaluateOnLoad as a parameter in Page.reload().

> > Source/WebCore/inspector/front-end/ExtensionAPI.js:497
> > +        else if (typeof optionsOrUserAgent === "string") {
> 
> This will never compile.

There are quite a few things in ExtensionAPI.js that won't compile. We need this as a compatibility hack, this will be gone in the future.

> > Source/WebCore/inspector/front-end/WorkersSidebarPane.js:81
> > +        if (!enabled === !this._fakeWorkersScriptIdentifier)
> 
> Could you disable checkbox instead?

Done.
Comment 7 Pavel Feldman 2011-10-24 09:43:39 PDT
Comment on attachment 112200 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=112200&action=review

> Source/WebCore/inspector/InspectorPageAgent.cpp:342
> +    m_pendingScriptToEvaluateOnLoadOnce = optionalScriptToEvaluateOnLoad ? *optionalScriptToEvaluateOnLoad : "";

You don't need this pending script.

> Source/WebCore/inspector/InspectorPageAgent.cpp:584
> +    ScriptState* scriptState = 0; // Initialize lazily, as mainWorldScriptState() will create one if missing.

Early return in case of no front-end?
Comment 8 Andrey Kosyakov 2011-10-24 09:55:42 PDT
Created attachment 112204 [details]
patch

Perform script injection only in case there's an active front-end.
Comment 9 Andrey Kosyakov 2011-10-24 09:59:25 PDT
(In reply to comment #7)
> (From update of attachment 112200 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=112200&action=review
> 
> > Source/WebCore/inspector/InspectorPageAgent.cpp:342
> > +    m_pendingScriptToEvaluateOnLoadOnce = optionalScriptToEvaluateOnLoad ? *optionalScriptToEvaluateOnLoad : "";
> 
> You don't need this pending script.

I do need it: the script is only supposed to live during one reload cycle and we're clearing it upion main frame navigation, which happens between reload() and didClearWindowObject(), where the script is used.

> > Source/WebCore/inspector/InspectorPageAgent.cpp:584
> > +    ScriptState* scriptState = 0; // Initialize lazily, as mainWorldScriptState() will create one if missing.
> 
> Early return in case of no front-end?

Fixed.
Comment 10 Pavel Feldman 2011-10-24 10:12:25 PDT
Comment on attachment 112204 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=112204&action=review

> Source/WebCore/inspector/InspectorPageAgent.h:58
> +class ScriptToEvaluateOnLoad;

Please remove.

> Source/WebCore/inspector/InspectorPageAgent.h:66
> +public:

Why this change?
Comment 11 Andrey Kosyakov 2011-10-25 05:59:41 PDT
Manually committed r98328: as http://trac.webkit.org/changeset/98328