Bug 52452 - Web Inspector: [Extensions API] webInspector.resources.onFinished is not fired for redirected resources
Summary: Web Inspector: [Extensions API] webInspector.resources.onFinished is not fire...
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-01-14 08:54 PST by Andrey Kosyakov
Modified: 2011-11-30 09:42 PST (History)
9 users (show)

See Also:


Attachments
patch (13.93 KB, patch)
2011-01-14 09:17 PST, Andrey Kosyakov
no flags Details | Formatted Diff | Diff
patch to land (15.42 KB, patch)
2011-01-17 09:15 PST, Andrey Kosyakov
no flags 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-01-14 08:54:30 PST
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.
Comment 1 Andrey Kosyakov 2011-01-14 09:17:45 PST
Created attachment 78948 [details]
patch
Comment 2 Pavel Feldman 2011-01-17 01:59:32 PST
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
Comment 3 Andrey Kosyakov 2011-01-17 09:15:45 PST
Created attachment 79180 [details]
patch to land
Comment 4 Andrey Kosyakov 2011-01-17 09:19:43 PST
(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.
Comment 5 Eric Seidel (no email) 2011-01-31 16:07:09 PST
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.
Comment 6 Andrey Kosyakov 2011-11-30 09:42:16 PST
Manually committed r75950: http://trac.webkit.org/changeset/75950