Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(276)

Issue 2611183007: PlzNavigate: Fix for the http/tests/inspector/resource-har-conversion.html layout test failure. (Closed)

Created:
3 years, 11 months ago by ananta
Modified:
3 years, 11 months ago
Reviewers:
Nate Chapin, jam, pfeldman
CC:
chromium-reviews, mlamouri+watch-content_chromium.org, caseq+blink_chromium.org, creis+watch_chromium.org, nasko+codewatch_chromium.org, lushnikov+blink_chromium.org, pfeldman+blink_chromium.org, dglazkov+blink, darin-cc_chromium.org, devtools-reviews_chromium.org, blink-reviews, apavlov+blink_chromium.org, kinuko+watch, blink-reviews-api_chromium.org, kozyatinskiy+blink_chromium.org
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

PlzNavigate: Fix for the http/tests/inspector/resource-har-conversion.html layout test failure. This test turns off caching for the reload request initiated via the DevTools network agent. Without PlzNavigate, this goes through the normal resource load path, where in RenderFrameImpl::decidePolicyForNavigation is invoked after which the cache policy is set in the WebURLRequest to disabled correctly. With PlzNavigate, the reload request is sent out to the browser via the BeginNavigate IPC in RenderFrameImpl::decidePolicyForNavigation() and we indicate to blink that the client handled the request. As a result the cache policy does not get set to disabled. Fix is to add query the dev tools agent if caching is disabled. To achieve this following changes were needed. 1. Add a virtual function cacheDisabled() to the WebDevToolsAgent class. 2. This function calls to the newly added cacheDisabled() function in the InspectorNetworkAgent class. which checks the state variable for the same. BUG=673745 Review-Url: https://codereview.chromium.org/2611183007 Cr-Commit-Position: refs/heads/master@{#442773} Committed: https://chromium.googlesource.com/chromium/src/+/9c22f660541434f9666391a38895d4d20d487d03

Patch Set 1 #

Total comments: 5

Patch Set 2 : Add a flag isCacheDisabled to the NavigationPolicyInfo structure #

Patch Set 3 : Force PlzNavigate to true for try runs #

Patch Set 4 : Reverting force PlzNavigate as this patch only affects layout tests #

Patch Set 5 : Rebaseline enable-browser-side-navigation #

Total comments: 3

Patch Set 6 : Fix line endings #

Unified diffs Side-by-side diffs Delta from patch set Stats (+42 lines, -5 lines) Patch
M content/renderer/render_frame_impl.cc View 1 2 3 2 chunks +12 lines, -2 lines 0 comments Download
M third_party/WebKit/LayoutTests/FlagExpectations/enable-browser-side-navigation View 1 2 3 4 1 chunk +0 lines, -2 lines 0 comments Download
M third_party/WebKit/Source/core/inspector/InspectorNetworkAgent.h View 1 chunk +1 line, -0 lines 0 comments Download
M third_party/WebKit/Source/core/inspector/InspectorNetworkAgent.cpp View 1 2 3 4 5 2 chunks +9 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/web/FrameLoaderClientImpl.cpp View 1 2 3 4 5 1 chunk +6 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/web/WebDevToolsAgentImpl.h View 3 1 chunk +1 line, -0 lines 0 comments Download
M third_party/WebKit/Source/web/WebDevToolsAgentImpl.cpp View 1 chunk +6 lines, -0 lines 0 comments Download
M third_party/WebKit/public/web/WebDevToolsAgent.h View 1 chunk +4 lines, -0 lines 0 comments Download
M third_party/WebKit/public/web/WebFrameClient.h View 1 2 chunks +3 lines, -1 line 0 comments Download

Depends on Patchset:

Messages

Total messages: 35 (24 generated)
ananta
3 years, 11 months ago (2017-01-07 03:30:38 UTC) #2
jam
content/ lgtm
3 years, 11 months ago (2017-01-09 15:59:22 UTC) #7
pfeldman
https://codereview.chromium.org/2611183007/diff/1/content/renderer/render_frame_impl.cc File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/2611183007/diff/1/content/renderer/render_frame_impl.cc#newcode6211 content/renderer/render_frame_impl.cc:6211: load_flags = net::LOAD_BYPASS_CACHE; You just overrode non-cache-related bits set ...
3 years, 11 months ago (2017-01-09 16:40:41 UTC) #9
jam
https://codereview.chromium.org/2611183007/diff/1/content/renderer/render_frame_impl.cc File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/2611183007/diff/1/content/renderer/render_frame_impl.cc#newcode6211 content/renderer/render_frame_impl.cc:6211: load_flags = net::LOAD_BYPASS_CACHE; On 2017/01/09 16:40:41, pfeldman wrote: > ...
3 years, 11 months ago (2017-01-09 16:46:24 UTC) #10
ananta
https://codereview.chromium.org/2611183007/diff/1/content/renderer/render_frame_impl.cc File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/2611183007/diff/1/content/renderer/render_frame_impl.cc#newcode6211 content/renderer/render_frame_impl.cc:6211: load_flags = net::LOAD_BYPASS_CACHE; On 2017/01/09 16:40:41, pfeldman wrote: > ...
3 years, 11 months ago (2017-01-10 01:20:09 UTC) #11
jam
lgtm
3 years, 11 months ago (2017-01-10 20:46:22 UTC) #20
pfeldman
https://codereview.chromium.org/2611183007/diff/80001/third_party/WebKit/Source/web/FrameLoaderClientImpl.cpp File third_party/WebKit/Source/web/FrameLoaderClientImpl.cpp (right): https://codereview.chromium.org/2611183007/diff/80001/third_party/WebKit/Source/web/FrameLoaderClientImpl.cpp#newcode557 third_party/WebKit/Source/web/FrameLoaderClientImpl.cpp:557: devToolsAgent() ? devToolsAgent()->cacheDisabled() : false; Request already carries cache ...
3 years, 11 months ago (2017-01-10 23:10:05 UTC) #27
pfeldman
patch #5 module comment lgtm https://codereview.chromium.org/2611183007/diff/80001/third_party/WebKit/Source/core/inspector/InspectorNetworkAgent.cpp File third_party/WebKit/Source/core/inspector/InspectorNetworkAgent.cpp (right): https://codereview.chromium.org/2611183007/diff/80001/third_party/WebKit/Source/core/inspector/InspectorNetworkAgent.cpp#newcode1516 third_party/WebKit/Source/core/inspector/InspectorNetworkAgent.cpp:1516: return m_state->booleanProperty(NetworkAgentState::cacheDisabled, false); m_state->booleanProperty(NetworkAgentState::networkAgentEnabled, ...
3 years, 11 months ago (2017-01-11 00:21:52 UTC) #28
ananta
https://codereview.chromium.org/2611183007/diff/80001/third_party/WebKit/Source/core/inspector/InspectorNetworkAgent.cpp File third_party/WebKit/Source/core/inspector/InspectorNetworkAgent.cpp (right): https://codereview.chromium.org/2611183007/diff/80001/third_party/WebKit/Source/core/inspector/InspectorNetworkAgent.cpp#newcode1516 third_party/WebKit/Source/core/inspector/InspectorNetworkAgent.cpp:1516: return m_state->booleanProperty(NetworkAgentState::cacheDisabled, false); On 2017/01/11 00:21:52, pfeldman wrote: > ...
3 years, 11 months ago (2017-01-11 00:47:29 UTC) #29
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2611183007/100001
3 years, 11 months ago (2017-01-11 00:52:55 UTC) #32
commit-bot: I haz the power
3 years, 11 months ago (2017-01-11 02:29:10 UTC) #35
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/9c22f660541434f9666391a38895...

Powered by Google App Engine
This is Rietveld 408576698