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
(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
<rdar://problem/31871515>
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.
(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.
Created attachment 309233 [details] Patch
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
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
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
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
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
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
Comment on attachment 309233 [details] Patch r=me, with test/test results updated
(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?
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).
(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.
(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?
Created attachment 309628 [details] Patch
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.
Created attachment 310208 [details] Proposed Fix v2
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
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
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
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
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
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
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.
(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.
Committed r217248: <http://trac.webkit.org/changeset/217248>