Bug 89868

Summary: Web Inspector: [Extensions API] Resource manipulations should be based on UISourceCode thus extending Sources Panel.
Product: WebKit Reporter: Vsevolod Vlasov <vsevik>
Component: Web Inspector (Deprecated)Assignee: Vsevolod Vlasov <vsevik>
Status: RESOLVED FIXED    
Severity: Normal CC: apavlov, bweinstein, caseq, joepeck, keishi, loislo, pfeldman, pmuellr, rik, timothy, yurys
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on: 89891, 90530    
Bug Blocks:    
Attachments:
Description Flags
Patch pfeldman: review+

Description Vsevolod Vlasov 2012-06-25 04:57:33 PDT
Patch to follow.
Comment 1 Vsevolod Vlasov 2012-06-27 03:35:49 PDT
Created attachment 149717 [details]
Patch
Comment 2 Pavel Feldman 2012-06-29 07:43:16 PDT
Comment on attachment 149717 [details]
Patch

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

> Source/WebCore/inspector/front-end/ExtensionServer.js:486
> +        var contentProvider = WebInspector.panels.scripts.uiSourceCodeForURL(url) || WebInspector.resourceForURL(url);

This is not nice. Could we move composite provider into the WebInspector and pass it into this class?

> Source/WebCore/inspector/front-end/ExtensionServer.js:509
> +            else

We don't use else after return branches
Comment 3 Andrey Kosyakov 2012-07-02 05:43:20 PDT
Comment on attachment 149717 [details]
Patch

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

> Source/WebCore/inspector/front-end/ExtensionServer.js:510
> +                return this._status.E_FAILED("Resource is not editable")

I think E_NOTSUPPORTED() is more appropriate here.

> Source/WebCore/inspector/front-end/ExtensionServer.js:625
> +        var contentProvider = resource.uiSourceCode() || resource;

Can we extract this to something like ExtensionServer.contentProviderForURL()?

> Source/WebCore/inspector/front-end/StylesPanel.js:116
> +    _callOrSetTimeout: function(func)

func => callback? Also, please annotate.

> LayoutTests/http/tests/inspector/extensions-test.js:37
> +    WebInspector.StyleSource.updateTimeout = oldStyleSheetUpdateTimeout;

Why would we need this for all extension tests?

> LayoutTests/http/tests/inspector/extensions-test.js:54
> +    // Extensions tests override WebInspector.StyleSource.updateTimeout because otherwise extensions don't have any control over applying changes to domain specific bindings.
> +    var oldStyleSheetUpdateTimeout = WebInspector.StyleSource.updateTimeout;
> +    WebInspector.StyleSource.updateTimeout = -1;

As above, this should be done in tests that actually update resource content.

> LayoutTests/inspector/extensions/extensions-resources.html:36
> +            const resourceURLsWhiteList = ["abe.png", "audits-style1.css", "extensions-resources.html", "extensions-test.js", "inspector-test.js", "test-script.js"];

This looks a bit fishy -- can you please check we don't hit the case you're working around in test in real life, e.g. on reload or when we do not change renderer during navigation.
Comment 4 Vsevolod Vlasov 2012-07-03 10:51:49 PDT
Committed r121792: <http://trac.webkit.org/changeset/121792>