Bug 89868 - Web Inspector: [Extensions API] Resource manipulations should be based on UISourceCode thus extending Sources Panel.
Summary: Web Inspector: [Extensions API] Resource manipulations should be based on UIS...
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: Vsevolod Vlasov
URL:
Keywords:
Depends on: 89891 90530
Blocks:
  Show dependency treegraph
 
Reported: 2012-06-25 04:57 PDT by Vsevolod Vlasov
Modified: 2012-07-04 01:49 PDT (History)
11 users (show)

See Also:


Attachments
Patch (25.06 KB, patch)
2012-06-27 03:35 PDT, Vsevolod Vlasov
pfeldman: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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>