Web Inspector: [Extensions API] webInspector.resources.onFinished is not fired for redirected resources

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

[Andrey Kosyakov]

webInspector.resources.onFinished is not fired for resources that were redirected.

In general, the logic of starting/finishing is often inconsistent in NetworkManager: sometimes, not all consumers are notified on resource being finished, sometimes resources are not cleaned up upon finishing from NetworkManager._resourcesById.

[Andrey Kosyakov]

Created attachment 78948 [details]
patch

[Pavel Feldman]

Comment on attachment 78948 [details]
patch

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

> Source/WebCore/inspector/front-end/NetworkManager.js:183
> +        this._finishResource(resource, time);

We should not do two refreshes per operation.

> Source/WebCore/inspector/front-end/NetworkManager.js:221
> +        this._startResource(resource);

Audits will break on web sockets.

> Source/WebCore/inspector/front-end/NetworkManager.js:258
> +        this._finishResource(resource, time);

ditto

[Andrey Kosyakov]

Created attachment 79180 [details]
patch to land

[Andrey Kosyakov]

(In reply to comment #2)
> (From update of attachment 78948 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=78948&action=review
> 
> > Source/WebCore/inspector/front-end/NetworkManager.js:183
> > +        this._finishResource(resource, time);
> 
> We should not do two refreshes per operation.

As discussed, I would rather leave this as is: NetworkPanel.refreshResources() is cheap, as it postpones actual DOM refresh to an async timer event (so two refreshes will still be one). OTOH, I'm reluctant to put any skipRefresh-like flags into the interface, especially in view of planned transition to events.

> > Source/WebCore/inspector/front-end/NetworkManager.js:221
> > +        this._startResource(resource);
> 
> Audits will break on web sockets.
> 
> > Source/WebCore/inspector/front-end/NetworkManager.js:258
> > +        this._finishResource(resource, time);
> 
> ditto

We just use these events there for updating the progress indicator, so it won'r really break. Thanks, though, fixed this.

[Eric Seidel (no email)]

Comment on attachment 78948 [details]
patch

Cleared Pavel Feldman's review+ from obsolete attachment 78948 [details] so that this bug does not appear in http://webkit.org/pending-commit.

[Andrey Kosyakov]

Manually committed r75950: http://trac.webkit.org/changeset/75950