Bug 171385 – Web Inspector: webkit reload policy should match default behavior

• Component: Web Inspector
• Status: RESOLVED FIXED

[Alexander Romanovich]

The reload policy used when the web inspector is both open and foregrounded (i.e. all subresources revalidated) does not match the (new) reload policy used when the web inspector is open and backgrounded, or not open at all (i.e. only expired subresources revalidated).

This could be confusing to developers who would expect the web inspector to reflect what's actually happening under normal conditions. It would be preferable if the foregrounded web inspector used the default reload policy. If the developer needs to force revalidation, they can do so by reloading from origin via shift-refresh.

See original context here: https://bugs.webkit.org/show_bug.cgi?id=171264#c16

[Blaze Burg]

(In reply to Alexander Romanovich from comment #0)
> The reload policy used when the web inspector is both open and foregrounded
> (i.e. all subresources revalidated) does not match the (new) reload policy
> used when the web inspector is open and backgrounded, or not open at all
> (i.e. only expired subresources revalidated).

It doesn't have to do with foreground vs background. It's whether you are sending Cmd-R to Safari or Web Inspector, which use different APIs. Currently, Web Inspector does this in the backend:

    OptionSet<ReloadOption> reloadOptions;
    if (optionalIgnoreCache && *optionalIgnoreCache)
        reloadOptions |= ReloadOption::FromOrigin;
    m_page.mainFrame().loader().reload(reloadOptions);

So I believe we just need to use ReloadOption::ExpiredOnly in the normal case.

> This could be confusing to developers who would expect the web inspector to
> reflect what's actually happening under normal conditions. It would be
> preferable if the foregrounded web inspector used the default reload policy.
> If the developer needs to force revalidation, they can do so by reloading
> from origin via shift-refresh.
> 
> See original context here: https://bugs.webkit.org/show_bug.cgi?id=171264#c16

[Radar WebKit Bug Importer]

<rdar://problem/31871515>

[Alexander Romanovich]

Makes sense.

Another impact of the difference in API between Web Inspector and Safari results in the fact that you can force a reload-from-origin with cmd-shift-r in Web Inspector but not in Safari. It would be helpful to allow cmd-shift-r in Safari as well.

Aside from clearing up the above-mentioned confusion that can arise from a divergence in behavior, allowing cmd-shift-r in Safari, and not just in Web Inspector, would contribute to solving this long standing issue: Safari does not currently support 304s for main resources like all other major browsers do (https://bugs.webkit.org/show_bug.cgi?id=114738).

This is because a cmd-r in Safari currently does a reload-from-origin for the main resource, whereas it should perform revalidation. Resolving that would be a huge performance win, especially on large pages like blogs, and my hope would be that solving this would assist in bringing traction on that very important issue as well.

[Blaze Burg]

(In reply to Alexander Romanovich from comment #3)
> Makes sense.
> 
> Another impact of the difference in API between Web Inspector and Safari
> results in the fact that you can force a reload-from-origin with cmd-shift-r
> in Web Inspector but not in Safari. It would be helpful to allow cmd-shift-r
> in Safari as well.

This was introduced in Safari Technology Preview 28, as Cmd-Option-R. I might add a secondary key binding in Web Inspector to match that one.

[Blaze Burg]

Created attachment 309233 [details]
Patch

[Build Bot]

Comment on attachment 309233 [details]
Patch

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

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

[Build Bot]

Created attachment 309245 [details]
Archive of layout-test-results from ews100 for mac-elcapitan

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

[Build Bot]

Comment on attachment 309233 [details]
Patch

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

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

[Build Bot]

Created attachment 309247 [details]
Archive of layout-test-results from ews104 for mac-elcapitan-wk2

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

[Build Bot]

Comment on attachment 309233 [details]
Patch

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

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

[Build Bot]

Created attachment 309255 [details]
Archive of layout-test-results from ews117 for mac-elcapitan

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

[Antti Koivisto]

Comment on attachment 309233 [details]
Patch

r=me, with test/test results updated

[Blaze Burg]

(In reply to Build Bot from comment #10)
> Comment on attachment 309233 [details]
> Patch
> 
> Attachment 309233 [details] did not pass mac-debug-ews (mac):
> Output: http://webkit-queues.webkit.org/results/3682352
> 
> New failing tests:
> http/tests/inspector/network/resource-response-source-memory-cache.html

It seems that this test no longer does what it's supposed to. It's not clear to me why the new ReloadOption would cause this to return HTTP 200 instead of still using the memory cache (HTTP 304). Antti, is this expected behavior?

[Antti Koivisto]

The failure is

 PASS: Resource should be exist.
-PASS: statusCode should be 304
+FAIL: statusCode should be 304
+    Expected: 304
+    Actual: 200
 PASS: responseSource should be Symbol(memory-cache)

That is, the response is coming from the memory cache and has not been revalidated from the server (original 200 instead of 304). That is exactly what is expected from this change.

If you still want to test 304 case you should modify (or expend) the test to use an expired resource (Cache-Control: max-age=0 or similar).

[Blaze Burg]

(In reply to Antti Koivisto from comment #14)
> The failure is
> 
>  PASS: Resource should be exist.
> -PASS: statusCode should be 304
> +FAIL: statusCode should be 304
> +    Expected: 304
> +    Actual: 200
>  PASS: responseSource should be Symbol(memory-cache)
> 
> That is, the response is coming from the memory cache and has not been
> revalidated from the server (original 200 instead of 304). That is exactly
> what is expected from this change.
> 
> If you still want to test 304 case you should modify (or expend) the test to
> use an expired resource (Cache-Control: max-age=0 or similar).

I tried pretty much every combination of cache control response headers (using cache-simulator.cgi), and none of them caused this to return an HTTP 304. I'll just re baseline this to expect HTTP 200.

[Joseph Pecoraro]

(In reply to Brian Burg from comment #15)
> (In reply to Antti Koivisto from comment #14)
> > The failure is
> > 
> >  PASS: Resource should be exist.
> > -PASS: statusCode should be 304
> > +FAIL: statusCode should be 304
> > +    Expected: 304
> > +    Actual: 200
> >  PASS: responseSource should be Symbol(memory-cache)
> > 
> > That is, the response is coming from the memory cache and has not been
> > revalidated from the server (original 200 instead of 304). That is exactly
> > what is expected from this change.
> > 
> > If you still want to test 304 case you should modify (or expend) the test to
> > use an expired resource (Cache-Control: max-age=0 or similar).
> 
> I tried pretty much every combination of cache control response headers
> (using cache-simulator.cgi), and none of them caused this to return an HTTP
> 304. I'll just re baseline this to expect HTTP 200.

Is there any way to get a Memory Cache revalidation response now that we can test?

[Blaze Burg]

Created attachment 309628 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 309628 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Base/Main.js:1914
> -    PageAgent.reload(window.event ? window.event.shiftKey : false);
> +    PageAgent.reload.invoke({shouldIgnoreCache: window.event ? window.event.shiftKey : false});

So there is currently no way for users to trigger this new path?

> Source/WebInspectorUI/UserInterface/Test/FrontendTestHarness.js:130
> +        return PageAgent.reload.invoke({shouldIgnoreCache: !!shouldIgnoreCache, revalidateUnexpiredResources: true})

And tests test a different behavior than what users would test.

---

There should be a test for both the behavior users will see and this special reload behavior we have only for tests.

[Blaze Burg]

Created attachment 310208 [details]
Proposed Fix v2

[Build Bot]

Comment on attachment 310208 [details]
Proposed Fix v2

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

New failing tests:
http/tests/inspector/network/resource-response-source-memory-cache.html
http/tests/inspector/network/resource-response-source-memory-cache-revalidate-expired-only.html

[Build Bot]

Created attachment 310215 [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

[Build Bot]

Comment on attachment 310208 [details]
Proposed Fix v2

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

New failing tests:
http/tests/inspector/network/resource-response-source-memory-cache-revalidate-expired-only.html
http/tests/inspector/network/resource-response-source-memory-cache.html
http/tests/inspector/network/resource-sizes-memory-cache.html

[Build Bot]

Created attachment 310217 [details]
Archive of layout-test-results from ews106 for mac-elcapitan-wk2

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

[Build Bot]

Comment on attachment 310208 [details]
Proposed Fix v2

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

New failing tests:
http/tests/inspector/network/resource-response-source-memory-cache.html
http/tests/inspector/network/resource-response-source-memory-cache-revalidate-expired-only.html

[Build Bot]

Created attachment 310218 [details]
Archive of layout-test-results from ews112 for mac-elcapitan

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

[Joseph Pecoraro]

Comment on attachment 310208 [details]
Proposed Fix v2

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

Patch looks good to me, just get the tests passing properly.

> Source/JavaScriptCore/inspector/protocol/Page.json:116
> +                { "name": "revalidateUnexpiredResources", "type": "boolean", "optional": true, "description": "If true, unexpired cached subresources will be revalidated when the main resource loads. Otherwise, only expired cached subresources will be revalidated (the default behavior for most WebKit clients)." },

Maybe we should name this revalidateAllResources or just revalidateResources?

> LayoutTests/http/tests/inspector/network/resource-response-source-memory-cache-revalidate-expired-only-expected.txt:9
> +FAIL: statusCode should be 200
> +    Expected: 200
> +    Actual: 304

This seems unexpected.

[Blaze Burg]

(In reply to Joseph Pecoraro from comment #26)
> Comment on attachment 310208 [details]
> Proposed Fix v2
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=310208&action=review
> 
> Patch looks good to me, just get the tests passing properly.
> 
> > Source/JavaScriptCore/inspector/protocol/Page.json:116
> > +                { "name": "revalidateUnexpiredResources", "type": "boolean", "optional": true, "description": "If true, unexpired cached subresources will be revalidated when the main resource loads. Otherwise, only expired cached subresources will be revalidated (the default behavior for most WebKit clients)." },
> 
> Maybe we should name this revalidateAllResources or just revalidateResources? 

Ok. I prefer revalidateAllResources.

> > LayoutTests/http/tests/inspector/network/resource-response-source-memory-cache-revalidate-expired-only-expected.txt:9
> > +FAIL: statusCode should be 200
> > +    Expected: 200
> > +    Actual: 304
> 
> This seems unexpected.

Looks like I forgot to rebaseline a final time.

[Blaze Burg]

Committed r217248: <http://trac.webkit.org/changeset/217248>