Bug 164892 – Web Inspector: Should be able to see where Resources came from (Memory Cache, Disk Cache)

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Joseph Pecoraro]

Summary:
Should be able to see where Resources came from (Memory Cache, Disk Cache)

ResourceResponse has the following:

>    // This is primarily for testing support. It is not necessarily accurate in all scenarios.
>    enum class Source { Unknown, Network, DiskCache, DiskCacheAfterValidation, MemoryCache, MemoryCacheAfterValidation };
>    Source source() const;

It sounds like this would be useful to get working more generally and not just for testing, then exposing it in Web Inspector.

Notes:
- If we allow the inspector to intercept and return its own data, that would be another potential Source ("Inspector").

[Radar WebKit Bug Importer]

<rdar://problem/29320562>

[Joseph Pecoraro]

Created attachment 303626 [details]
[PATCH] Proposed Patch

[Joseph Pecoraro]

Created attachment 303627 [details]
[IMAGE] Network Tab

[WebKit Commit Bot]

This patch modifies the inspector protocol generator. Please ensure that you have rebaselined any generator test results (i.e., by running `Tools/Scripts/run-inspector-generator-tests --reset-results`)

[WebKit Commit Bot]

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


ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generator.py:76:  multiple statements on one line (semicolon)  [pep8/E702] [5]
Total errors found: 1 in 32 files


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

[Build Bot]

Comment on attachment 303626 [details]
[PATCH] Proposed Patch

Attachment 303626 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/3257136

New failing tests:
http/tests/inspector/network/resource-response-source-disk-cache.html

[Build Bot]

Created attachment 303628 [details]
Archive of layout-test-results from ews102 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Joseph Pecoraro]

Does the disk-cache not work on El Capitan?

[Joseph Pecoraro]

(In reply to comment #8)
> Does the disk-cache not work on El Capitan?

Oh WebKit 1. Duh.

[Joseph Pecoraro]

Created attachment 303631 [details]
[PATCH] Proposed Patch

[WebKit Commit Bot]

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


WARNING: Not running on native Windows.
ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generator.py:76:  multiple statements on one line (semicolon)  [pep8/E702] [5]
Total errors found: 1 in 34 files


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

[Joseph Pecoraro]

Created attachment 303632 [details]
[PATCH] Proposed Patch

[Devin Rousso]

Comment on attachment 303632 [details]
[PATCH] Proposed Patch

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

> LayoutTests/http/tests/inspector/network/resource-response-source-disk-cache.html:35
> +                ]).then(([resourceWasAddedEvent, responseReceivedEvent, loadCompleteEvent]) => {

Style: I think our style now is preferring to put any `.then` on new lines with even indentation as the previous line.

])
.then(([resourceWasAddedEvent, responseReceivedEvent, loadCompleteEvent]) => {

> Source/WebCore/ChangeLog:9
> +        FIXME: Needs tests.

Maybe add a bug for this and put it here too?

> Source/WebCore/ChangeLog:27
> +

NIT: extra newline

> Source/WebCore/inspector/InspectorNetworkAgent.cpp:214
> +static Inspector::Protocol::Network::Response::Source responseSource(ResourceResponse::Source source)

I think this function could be named a bit better.  Maybe something like `determineProtocolResponseSource`?  The usage below seems awkward to read and doesn't immediately indicate what it does (to me, although I do have very limited working experience with WebCore style).

> Source/WebCore/inspector/InspectorNetworkAgent.cpp:217
> +    case ResourceResponse::Source::Unknown:

Would it make sense to also have the default case be Unknown?

> Source/WebInspectorUI/UserInterface/Controllers/FrameResourceManager.js:232
> +        const elapsedTime = WebInspector.timelineManager.computeElapsedTime(timestamp);

So what exactly is our policy for using `const`?  My understanding was that we preferred using it when the value didn't change between executions (such as function arguments).  I am totally fine with this, I'd just like to understand what the current preference is.

> Source/WebInspectorUI/UserInterface/Models/Resource.js:59
> +        this._statusCode = undefined;

I think you could set this to `null` and do an `isNaN` check instead of using undefined.

> Source/WebInspectorUI/UserInterface/Models/Resource.js:151
> +            console.error("Unknown response source type: ", source);

Aren't we using `WebInspector.reportInternalError` now instead of `console.error`, or are they two different usages?

> Source/WebInspectorUI/UserInterface/Models/Resource.js:455
> +    hasResponse()

Is there a reason that this isn't a getter?

[Antti Koivisto]

Comment on attachment 303632 [details]
[PATCH] Proposed Patch

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

> Source/WebCore/platform/network/ResourceResponseBase.h:138
> +    // Source is mutable so it can be set on a const response.
>      enum class Source { Unknown, Network, DiskCache, DiskCacheAfterValidation, MemoryCache, MemoryCacheAfterValidation };
>      WEBCORE_EXPORT Source source() const;
> -    WEBCORE_EXPORT void setSource(Source);
> +    void setSource(Source source) const { m_source = source; }

I don't like turning source mutable. How about just making a copy of the response in the one case (I think) you need this?

[Joseph Pecoraro]

Comment on attachment 303632 [details]
[PATCH] Proposed Patch

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

>> LayoutTests/http/tests/inspector/network/resource-response-source-disk-cache.html:35
>> +                ]).then(([resourceWasAddedEvent, responseReceivedEvent, loadCompleteEvent]) => {
> 
> Style: I think our style now is preferring to put any `.then` on new lines with even indentation as the previous line.
> 
> ])
> .then(([resourceWasAddedEvent, responseReceivedEvent, loadCompleteEvent]) => {

Yeah, I've seen that style, it doesn't make sense to me. This is much clearer to me, so I'd like to push back on this.

>> Source/WebCore/ChangeLog:9
>> +        FIXME: Needs tests.
> 
> Maybe add a bug for this and put it here too?

Oops, I was supposed to update this with the tests I added above!

>> Source/WebCore/inspector/InspectorNetworkAgent.cpp:217
>> +    case ResourceResponse::Source::Unknown:
> 
> Would it make sense to also have the default case be Unknown?

In C++ we avoid default so that the compiler will tell us that we've handled all possible enum values.

>> Source/WebCore/platform/network/ResourceResponseBase.h:138
>> +    void setSource(Source source) const { m_source = source; }
> 
> I don't like turning source mutable. How about just making a copy of the response in the one case (I think) you need this?

Okay, follow-up coming.

>> Source/WebInspectorUI/UserInterface/Controllers/FrameResourceManager.js:232
>> +        const elapsedTime = WebInspector.timelineManager.computeElapsedTime(timestamp);
> 
> So what exactly is our policy for using `const`?  My understanding was that we preferred using it when the value didn't change between executions (such as function arguments).  I am totally fine with this, I'd just like to understand what the current preference is.

Yeah, I wanted to use const just for responseSource below, and carried it through to all of the things above. Your description more clearly matches our style then my use, I'll change.

>> Source/WebInspectorUI/UserInterface/Models/Resource.js:151
>> +            console.error("Unknown response source type: ", source);
> 
> Aren't we using `WebInspector.reportInternalError` now instead of `console.error`, or are they two different usages?

Not sure we've settled on anything here.

>> Source/WebInspectorUI/UserInterface/Models/Resource.js:455
>> +    hasResponse()
> 
> Is there a reason that this isn't a getter?

Most of our "hasFoo()" names are functions not getters.

[Joseph Pecoraro]

Created attachment 303704 [details]
[PATCH] Proposed Fix

[Blaze Burg]

Comment on attachment 303704 [details]
[PATCH] Proposed Fix

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

r=me

> LayoutTests/http/tests/inspector/network/resource-response-source-disk-cache.html:49
> +        name: "ClearMemoryCache",

Please put this in the 'setup' function for the first test case.

> LayoutTests/http/tests/inspector/network/resource-response-source-memory-cache.html:21
> +                    for (let x of WebInspector.frameResourceManager.mainFrame.resourceCollection.items) {

'item'?

> Source/JavaScriptCore/inspector/scripts/codegen/generator.py:149
> +        if not len(fields):

Nit: this does the same thing and is more pythonic:

    if not fields:

> Source/WebInspectorUI/UserInterface/Controllers/FrameResourceManager.js:243
> +        console.assert(resource.cached);

Please log `resource` and maybe an error message so we know what design condition is violated.

> Source/WebInspectorUI/UserInterface/Views/ResourceTimelineDataGridNode.js:229
> +            console.assert(this._resource.cached);

Ditto

[Joseph Pecoraro]

<https://trac.webkit.org/changeset/213621>