Bug 59193 – Web Inspector: Use different SourceFrame instances for ResourcesPanel and NetworkPanel

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector (Deprecated)

[Vsevolod Vlasov]

Use different SourceFrame instances for ResourcesPanel and NetworkPanel.

Network panel SourceFrame instance should not be editable. It should not be updated when Resource is edited in Resources/Scripts panels.

[Vsevolod Vlasov]

Created attachment 90696 [details]
Patch

[Pavel Feldman]

Comment on attachment 90696 [details]
Patch

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

> Source/WebCore/inspector/front-end/NetworkItemView.js:114
> +WebInspector.NetworkItemView.sourceViewForResource = function(resource)

I don't see where this caching is used. Also, should be private.

> Source/WebCore/inspector/front-end/NetworkItemView.js:123
> +WebInspector.NetworkItemView.contentViewForResource = function(resource)

Should be private.

> Source/WebCore/inspector/front-end/NetworkItemView.js:130
> +        return WebInspector.NetworkItemView.sourceViewForResource(resource);

(do we really cache anything?)

> Source/WebCore/inspector/front-end/ResourcesPanel.js:86
> +    case WebInspector.resourceCategories.documents:

I've seen it earlier. We should not dupe logic like this.

> Source/WebCore/inspector/front-end/ResourcesPanel.js:102
> +    if (!resource._resourcesPanelSourceView)

Can we stop caching views in the resource?

> Source/WebCore/inspector/front-end/ResourcesPanel.js:114
> +    if (!revision._view) {

ditto

[Vsevolod Vlasov]

Created attachment 93677 [details]
Patch with fixes

[Vsevolod Vlasov]

Created attachment 93678 [details]
Patch with fixes

Oops, forgot to update the ChangeLog

[Vsevolod Vlasov]

Created attachment 93679 [details]
Patch with fixes

[Vsevolod Vlasov]

Comment on attachment 93679 [details]
Patch with fixes

Found a problem with ResourceTreeModel, dropping flags.

[WebKit Review Bot]

Comment on attachment 93679 [details]
Patch with fixes

Attachment 93679 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8704152

New failing tests:
http/tests/inspector/resource-tree/resource-tree-frame-add.html
inspector/console/console-log-document-proto.html
inspector/profiler/cpu-profiler-profiling.html
http/tests/inspector/console-xhr-logging.html
inspector/console/console-log-toString-object.html
inspector/console/console-command-clear.html
inspector/console/alert-toString-exception.html
inspector/console/console-format-collections.html
inspector/console/console-nested-group.html
inspector/console/console-dir-global.html
inspector/debugger/debugger-autocontinue-on-syntax-error.html
http/tests/inspector/change-iframe-src.html
inspector/console/console-assert.html
http/tests/appcache/interrupted-update.html
inspector/console/console-dir.html
inspector/console/console-format.html
http/tests/inspector/console-resource-errors.html
inspector/console/console-dirxml.html
http/tests/inspector/resource-parameters.html
inspector/console/console-log-syntax-error.html

[WebKit Review Bot]

Created attachment 93717 [details]
Archive of layout-test-results from ec2-cr-linux-02

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-02  Port: Chromium  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Vsevolod Vlasov]

Created attachment 93975 [details]
Patch with fixes

[WebKit Review Bot]

Comment on attachment 93975 [details]
Patch with fixes

Attachment 93975 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8710648

New failing tests:
inspector/debugger/source-frame.html

[WebKit Review Bot]

Created attachment 94001 [details]
Archive of layout-test-results from ec2-cr-linux-02

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-02  Port: Chromium  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Vsevolod Vlasov]

Created attachment 94054 [details]
Patch with failing test fix

Fixed failing test

[Pavel Podivilov]

Comment on attachment 94054 [details]
Patch with failing test fix

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

> Source/WebCore/inspector/front-end/ResourceView.js:46
> +WebInspector.ResourceView.isSourceViewableForResource = function(resource)

Maybe hasTextContent?

> Source/WebCore/inspector/front-end/ResourceView.js:73
> +    WebInspector.SourceFrameDelegate.call(this);

SourceFrameDelegate is a scripts panel specific delegate used to invert SourceFrame -> ScriptsPanel dependency. It's strange that you derive ResourceSourceFrameDelegate from it. Could you implement TextViewerDelegate interface instead?

[Vsevolod Vlasov]

Created attachment 94609 [details]
Patch with fixes after review

(In reply to comment #13)
> (From update of attachment 94054 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=94054&action=review
> 
> > Source/WebCore/inspector/front-end/ResourceView.js:46
> > +WebInspector.ResourceView.isSourceViewableForResource = function(resource)
> 
> Maybe hasTextContent?
Done.

> > Source/WebCore/inspector/front-end/ResourceView.js:73
> > +    WebInspector.SourceFrameDelegate.call(this);
> 
> SourceFrameDelegate is a scripts panel specific delegate used to invert SourceFrame -> ScriptsPanel dependency. It's strange that you derive ResourceSourceFrameDelegate from it. Could you implement TextViewerDelegate interface instead?
As discussed, I have overridden these methods in SourceFrame descendants.

[Vsevolod Vlasov]

Created attachment 94610 [details]
Indent fix

[Yury Semikhatsky]

Comment on attachment 94610 [details]
Indent fix

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

> Source/WebCore/inspector/front-end/ResourceView.js:95
> +    _contentLoaded: function (callback, text)

Why not make it a local function in requestContent?

> Source/WebCore/inspector/front-end/ResourceView.js:-220
> -        function contentLoaded(text)

What's the reason for moving this out of the requestContent?

> Source/WebCore/inspector/front-end/ResourcesPanel.js:407
> +        return treeElement.sourceView();

Can treeElement be undefined?

> Source/WebCore/inspector/front-end/ResourcesPanel.js:1189
> +    this._resource.addEventListener("errors-warnings-cleared", this._errorsWarningsCleared, this);

You can listen for ConsoleCleared for consistency, otherwise some console-related events come from Resources while others from the Console directly.

> Source/WebCore/inspector/front-end/ResourcesPanel.js:-1289
> -        if (this._storagePanel.currentQuery)

Should this check be preserved?

> Source/WebCore/inspector/front-end/ResourcesPanel.js:1344
> +        if (this._storagePanel.currentQuery)

Remove this.

> Source/WebCore/inspector/front-end/ResourcesPanel.js:1411
> +        delete oldView;

This doesn't make sense, please remove.

[Vsevolod Vlasov]

Created attachment 94781 [details]
Patch with fixes

(In reply to comment #16)
> (From update of attachment 94610 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=94610&action=review
> 
> > Source/WebCore/inspector/front-end/ResourceView.js:95
> > +    _contentLoaded: function (callback, text)
> 
> Why not make it a local function in requestContent?
Done.

> > Source/WebCore/inspector/front-end/ResourceView.js:-220
> > -        function contentLoaded(text)
> 
> What's the reason for moving this out of the requestContent?
None :) Moved back in.

> > Source/WebCore/inspector/front-end/ResourcesPanel.js:407
> > +        return treeElement.sourceView();
> 
> Can treeElement be undefined?
No, revision treeElement always already exists when showRevisionView is called.

> > Source/WebCore/inspector/front-end/ResourcesPanel.js:1189
> > +    this._resource.addEventListener("errors-warnings-cleared", this._errorsWarningsCleared, this);
> 
> You can listen for ConsoleCleared for consistency, otherwise some console-related events come from Resources while others from the Console directly.
Now listening to both events from resource only to keep logic for resource selection at one place.

> > Source/WebCore/inspector/front-end/ResourcesPanel.js:-1289
> > -        if (this._storagePanel.currentQuery)
> 
> Should this check be preserved?
Done.

> > Source/WebCore/inspector/front-end/ResourcesPanel.js:1344
> > +        if (this._storagePanel.currentQuery)
> 
> Remove this.
Done.

> > Source/WebCore/inspector/front-end/ResourcesPanel.js:1411
> > +        delete oldView;
> 
> This doesn't make sense, please remove.
Done.

[Yury Semikhatsky]

Comment on attachment 94781 [details]
Patch with fixes

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

> Source/WebCore/inspector/front-end/ResourceView.js:152
> +            this.resource.setContent(this._textModel.text, majorChange, function() {});

Please remove last parameter since it's optional. In similar calls above it's just omitted.

[Yury Semikhatsky]

Comment on attachment 94781 [details]
Patch with fixes

Clearing flags on attachment: 94781

Committed r87383: <http://trac.webkit.org/changeset/87383>

[Yury Semikhatsky]

All reviewed patches have been landed.  Closing bug.