Bug 64581 – Web Inspector: ApplicationCache UI is confusing and incorrect.

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

[Michael Nordman]

Click the "Resources" tab at the top.

The tree view along the left-hand-side has a top level node for Application Cache. When opened, it shows a child element for every domain in the frame hierarchy of the page subject to inspection. When clicking any of those elements, the right-hand-side shows you the details of the appcache associated with the topmost document in the frame hierarchy, or indicates there is no appcache if there is no association to one.

It doesn't make much sense to list domains in the frameset under the appcache node (I'm not sure what those node are trying to represent). And it's plainly incorrect to show info about the topmost document regardless of which node is selected.

The display of the details of an individual appcache (the right-hand-side) is reasonable, but the navigation elements on the left-hand-side to bring you to the details just don't make sense.

[Michael Nordman]

Here's an idea about how to make more sense out of the left-hand-side.

Under the appcache node, show a child for each origin represented in page being inspected that contains appcaches. If an origin doesn't have any appcaches, don't put a node there. Then under each origin node, show a child for each manifest in that origin, and indicate visually those that are in-use by the page being inspected (dull vs bright tree node icon maybe).

Click on any manifest node see the entries for that manifest on the right-hand-side (the same display as we have now).

[Vsevolod Vlasov]

I have started working on that.

My current approach is to extract ApplicationCacheModel that will store manifest urls related to current page. For each manifest url I will store a set of identifiers of the frames containing documents that are associated with it. The word associated should be read in the broader sense than the one used in specification though. We should show manifest URLs both for caches that are not yet loaded and for caches which failed to load for some reason because this information is valuable for developers.

Knowing all manifest urls we can now add list items for each of them in the ResourcesPanel's Application Cache node. I don't think that we need to group these manifests by origin, since the number of different manifests on one page/origin should not be big, should it?

Anyway, extracting ApplicationCacheModel and storing manifests is the main thing here, I think this is a good start to make application cache UI usable at all. We can discuss if we need to show other manifests from the page frames origins and the need for highlighting used manifests on UI later.

[Michael Nordman]

That sounds like it'd be an improvement. The set of nodes on the right-hand-side will definitely not be too big (at most one node per frame in the page).

Do you plan on using the manifestUrl as the label for the nodes in the right-hand-side?

There is a detail that you should be aware of. Different frames broadly associated to the same manifest url, can be narrowly associated to different 'versions' of that manifest. The detailed list of entries and timeStamps/size values of the ApplicationCacheHost::CacheInfo structure will be different even though the manifestUrl is the same. The ApplicationCacheHost::fillResourceList() and applicationCacheInfo() method, reflect what the info of the specific version associated with that specific doc/worker context. 

With that in mind, if two frames in the same 'tab' are using the same manifest url, it might be best to go ahead and have the same 'url' listed multiple times in the list on the rhs. When clicked on, query the specific 'host' for the detailed listing and info to populate the lhs.

[Vsevolod Vlasov]

Created attachment 114340 [details]
Screenshot

[Vsevolod Vlasov]

Created attachment 114343 [details]
Patch

[Vsevolod Vlasov]

I have uploaded a patch and a screenshot.
This patch creates manifest url nodes under application cache node in resources panel. Under each manifest url all frames using the relevant application cache are listed.

Please note that application cache status icon, delete button etc. are still set to "display:none" in ApplicationCacheItemsView.js because they don't always work properly yet.

[WebKit Review Bot]

Attachment 114343 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/http..." exit_code: 1

Source/WebCore/inspector/InspectorApplicationCacheAgent.cpp:86:  The parameter type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]
Source/WebCore/inspector/InspectorApplicationCacheAgent.cpp:128:  The parameter type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]
Source/WebCore/inspector/InspectorApplicationCacheAgent.h:64:  The parameter type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]
Source/WebCore/inspector/InspectorApplicationCacheAgent.h:66:  The parameter type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]
Total errors found: 4 in 14 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Michael Nordman]

Comment on attachment 114343 [details]
Patch

drive-by comment... screenshot looks great and the 'agent' c++ code lgtm too

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

> Source/WebCore/inspector/InspectorApplicationCacheAgent.cpp:125
> +    *manifestURL = info.m_manifest.string();

There's some overalap between getManifestForFrame() and getApplicationCacheForFrame(), maybe define a private helper method to do much of the overlapping parts (ApplicationCacheHost* getApplicationCacheHostFrame(errString, frameId)).

[WebKit Review Bot]

Comment on attachment 114343 [details]
Patch

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

New failing tests:
http/tests/inspector/resource-tree/appcache-iframe-manifests.html

[Pavel Feldman]

Comment on attachment 114343 [details]
Patch

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

A bunch of nits.

> Source/WebCore/inspector/InspectorApplicationCacheAgent.cpp:76
> +    m_frontend->updateApplicationCacheStatus(m_pageAgent->frameId(frame), manifestURL, status);

"applicationCacheStatusUpdated"

> Source/WebCore/inspector/InspectorApplicationCacheAgent.cpp:130
> +    Frame* frame = m_pageAgent->frameForId(frameId);

As Michael suggests, you should introduce:

DocumentLoader* documentLoader = assertFrameWithDocumentLoader(frameId);
if (!documentLoader)
    return;

As in DOM agent.

> Source/WebCore/inspector/front-end/ApplicationCacheModel.js:58
> +        var frameId = event.data.frame.id;

Please use following notation for compilation:

var frame = /** @type {<frame type here>} */ event.data["frame"];

i.e. you should do event.data[] instead of event.data.

> Source/WebCore/inspector/front-end/ApplicationCacheModel.js:70
> +        this._reset();

It took me few seconds to figure out that this._statuses is initialized from constructor. Avoid this indirection and declare variables in the constructor explicitly.

> Source/WebCore/inspector/front-end/ApplicationCacheModel.js:88
> +            console.error(error);

return; ?

> Source/WebCore/inspector/front-end/ApplicationCacheModel.js:96
> +        if (error)

ditto

> Source/WebCore/inspector/front-end/ApplicationCacheModel.js:100
> +            this._frameManifestUpdated(framesWithManifests[i].frameId, framesWithManifests[i].manifestURL, framesWithManifests[i].status);

I would pass framesWithManifests[i] here...

> Source/WebCore/inspector/front-end/ApplicationCacheModel.js:104
> +     * @param {string} frameId

... and declare parameter type as {ApplicationCacheAgent.FramesWithManifest}. It has been generated for you.

[Vsevolod Vlasov]

All done except for this one.

> > Source/WebCore/inspector/front-end/ApplicationCacheModel.js:100
> > +            this._frameManifestUpdated(framesWithManifests[i].frameId, framesWithManifests[i].manifestURL, framesWithManifests[i].status);
> 
> I would pass framesWithManifests[i] here...
> 
> > Source/WebCore/inspector/front-end/ApplicationCacheModel.js:104
> > +     * @param {string} frameId
> 
> ... and declare parameter type as {ApplicationCacheAgent.FramesWithManifest}. It has been generated for you.
This method is also called from applicationCacheStatusUpdated. I added @param {Array.<ApplicationCacheAgent.FrameWithManifest>} to _framesWithManifestsLoaded() though.

[Vsevolod Vlasov]

Created attachment 114479 [details]
Patch

[WebKit Review Bot]

Attachment 114479 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/http..." exit_code: 1

Source/WebCore/inspector/InspectorApplicationCacheAgent.cpp:89:  The parameter type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]
Source/WebCore/inspector/InspectorApplicationCacheAgent.cpp:131:  The parameter type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]
Source/WebCore/inspector/InspectorApplicationCacheAgent.h:66:  The parameter type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]
Source/WebCore/inspector/InspectorApplicationCacheAgent.h:68:  The parameter type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]
Total errors found: 4 in 19 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Comment on attachment 114479 [details]
Patch

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

[Vsevolod Vlasov]

Committed r99878: <http://trac.webkit.org/changeset/99878>