Bug 164892

Summary: Web Inspector: Should be able to see where Resources came from (Memory Cache, Disk Cache)
Product: WebKit Reporter: Joseph Pecoraro <joepeck>
Component: Web InspectorAssignee: Joseph Pecoraro <joepeck>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, bburg, buildbot, cdumez, commit-queue, dbates, inspector-bugzilla-changes, japhet, joepeck, keith_miller, koivisto, mark.lam, msaboff, rniwa, saam, webkit-bug-importer, youennf
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
Attachments:
Description Flags
[PATCH] Proposed Patch
buildbot: commit-queue-
[IMAGE] Network Tab
none
Archive of layout-test-results from ews102 for mac-elcapitan
none
[PATCH] Proposed Patch
none
[PATCH] Proposed Patch
joepeck: review-
[PATCH] Proposed Fix bburg: review+, bburg: commit-queue-

Description Joseph Pecoraro 2016-11-17 14:50:04 PST
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").
Comment 1 Radar WebKit Bug Importer 2016-11-17 14:50:40 PST
<rdar://problem/29320562>
Comment 2 Joseph Pecoraro 2017-03-06 22:25:54 PST
Created attachment 303626 [details]
[PATCH] Proposed Patch
Comment 3 Joseph Pecoraro 2017-03-06 22:26:18 PST
Created attachment 303627 [details]
[IMAGE] Network Tab
Comment 4 WebKit Commit Bot 2017-03-06 22:28:05 PST
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`)
Comment 5 WebKit Commit Bot 2017-03-06 22:28:19 PST
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.
Comment 6 Build Bot 2017-03-06 23:23:47 PST
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
Comment 7 Build Bot 2017-03-06 23:23:54 PST
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
Comment 8 Joseph Pecoraro 2017-03-06 23:34:12 PST
Does the disk-cache not work on El Capitan?
Comment 9 Joseph Pecoraro 2017-03-06 23:35:31 PST
(In reply to comment #8)
> Does the disk-cache not work on El Capitan?

Oh WebKit 1. Duh.
Comment 10 Joseph Pecoraro 2017-03-06 23:42:58 PST
Created attachment 303631 [details]
[PATCH] Proposed Patch
Comment 11 WebKit Commit Bot 2017-03-06 23:45:14 PST
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.
Comment 12 Joseph Pecoraro 2017-03-06 23:52:40 PST
Created attachment 303632 [details]
[PATCH] Proposed Patch
Comment 13 Devin Rousso 2017-03-07 01:42:20 PST
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?
Comment 14 Antti Koivisto 2017-03-07 06:31:14 PST
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?
Comment 15 Joseph Pecoraro 2017-03-07 11:54:48 PST
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.
Comment 16 Joseph Pecoraro 2017-03-07 12:53:31 PST
Created attachment 303704 [details]
[PATCH] Proposed Fix
Comment 17 BJ Burg 2017-03-08 16:42:42 PST
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
Comment 18 Joseph Pecoraro 2017-03-08 19:42:24 PST
<https://trac.webkit.org/changeset/213621>