PlzNavigate: Fix for the http/tests/inspector/resource-har-conversion.html layout test failure.

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

[ananta, 2017-01-07 03:30:37]

ananta@chromium.org changed reviewers:
+ jam@chromium.org, japhet@chromium.org

[ananta, 2017-01-07 03:30:48]

The CQ bit was checked by ananta@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-07 03:30:58]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-07 05:12:27]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-07 05:12:28]

Dry run: This issue passed the CQ dry run.

[jam, 2017-01-09 15:59:22]

content/ lgtm

[pfeldman, 2017-01-09 16:40:41]

pfeldman@chromium.org changed reviewers:
+ pfeldman@chromium.org

[pfeldman, 2017-01-09 16:40:41]

https://codereview.chromium.org/2611183007/diff/1/content/renderer/render_fra...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/2611183007/diff/1/content/renderer/render_fra...
content/renderer/render_frame_impl.cc:6211: load_flags = net::LOAD_BYPASS_CACHE;
You just overrode non-cache-related bits set in GetLoadFlagsForWebURLRequest.

https://codereview.chromium.org/2611183007/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/inspector/InspectorNetworkAgent.h (right):

https://codereview.chromium.org/2611183007/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/inspector/InspectorNetworkAgent.h:230: bool
cacheDisabled();
Any reason for exposing it here instead of adding a bit into
WebFrameClient::NavigationPolicyInfo in decidePolicyForNavigation? That would
look more consistent with the rest of the plumbing.

[jam, 2017-01-09 16:46:24]

https://codereview.chromium.org/2611183007/diff/1/content/renderer/render_fra...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/2611183007/diff/1/content/renderer/render_fra...
content/renderer/render_frame_impl.cc:6211: load_flags = net::LOAD_BYPASS_CACHE;
On 2017/01/09 16:40:41, pfeldman wrote:
> You just overrode non-cache-related bits set in GetLoadFlagsForWebURLRequest.

whops, thanks for catching that.

Given that this code is only for non plznavigate, I've found it helpful to send
a tryrun with plznavigate enabled. That way we can check that it doesn't
introduce any regressions compared to a cl with just plznavigate enabled (i run
those about daily at https://codereview.chromium.org/2385413004/)

[ananta, 2017-01-10 01:20:09]

https://codereview.chromium.org/2611183007/diff/1/content/renderer/render_fra...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/2611183007/diff/1/content/renderer/render_fra...
content/renderer/render_frame_impl.cc:6211: load_flags = net::LOAD_BYPASS_CACHE;
On 2017/01/09 16:40:41, pfeldman wrote:
> You just overrode non-cache-related bits set in GetLoadFlagsForWebURLRequest.

Thanks for catching this. Fixed

https://codereview.chromium.org/2611183007/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/inspector/InspectorNetworkAgent.h (right):

https://codereview.chromium.org/2611183007/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/inspector/InspectorNetworkAgent.h:230: bool
cacheDisabled();
On 2017/01/09 16:40:41, pfeldman wrote:
> Any reason for exposing it here instead of adding a bit into
> WebFrameClient::NavigationPolicyInfo in decidePolicyForNavigation? That would
> look more consistent with the rest of the plumbing.

The setCacheDisabled function in the InspectorNetworkAgent class is called via
an IPC message DevToolsAgentMsg_DispatchOnInspectorBackend which is sent from
the browser
https://cs.chromium.org/chromium/src/content/browser/devtools/worker_devtools...

That function sets a state variable internally. We need the getter function to
query this state.

I left the function as is. I added a flag isCacheDisabled to the
NavigationPolicyInfo structure. This is set in
FrameLoaderClientImpl::decidePolicyForNavigation. We use this flag to determine
whether to turn off caching for the request.

Thanks

[ananta, 2017-01-10 01:20:22]

The CQ bit was checked by ananta@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-10 01:21:40]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-10 03:24:15]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-10 03:24:16]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)

[ananta, 2017-01-10 20:36:41]

The CQ bit was checked by ananta@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-10 20:37:29]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-10 20:41:05]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-10 20:41:06]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[jam, 2017-01-10 20:46:22]

lgtm

[ananta, 2017-01-10 20:57:34]

The CQ bit was checked by ananta@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-10 20:58:08]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[ananta, 2017-01-10 21:04:21]

The CQ bit was checked by ananta@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-10 21:04:41]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-10 22:33:28]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-10 22:33:29]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[pfeldman, 2017-01-10 23:10:05]

https://codereview.chromium.org/2611183007/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/FrameLoaderClientImpl.cpp (right):

https://codereview.chromium.org/2611183007/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/FrameLoaderClientImpl.cpp:557: devToolsAgent() ?
devToolsAgent()->cacheDisabled() : false;
Request already carries cache policy that inspector set to BypassingCache, can
you reuse it? If not, we should instroduce a bit on ResourceRequest for
consistency with the rest of the plumbing. Devtools' cache disabled state should
not be exposed outside network agent.

[pfeldman, 2017-01-11 00:21:52]

patch #5 module comment lgtm

https://codereview.chromium.org/2611183007/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/inspector/InspectorNetworkAgent.cpp (right):

https://codereview.chromium.org/2611183007/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/InspectorNetworkAgent.cpp:1516: return
m_state->booleanProperty(NetworkAgentState::cacheDisabled, false);
m_state->booleanProperty(NetworkAgentState::networkAgentEnabled, false) &&
m_state->booleanProperty(NetworkAgentState::cacheDisabled, false);

[ananta, 2017-01-11 00:47:29]

https://codereview.chromium.org/2611183007/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/inspector/InspectorNetworkAgent.cpp (right):

https://codereview.chromium.org/2611183007/diff/80001/third_party/WebKit/Sour...
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:
> m_state->booleanProperty(NetworkAgentState::networkAgentEnabled, false) &&
> m_state->booleanProperty(NetworkAgentState::cacheDisabled, false);

Done.

[ananta, 2017-01-11 00:52:04]

The CQ bit was checked by ananta@chromium.org

[ananta, 2017-01-11 00:52:04]

The patchset sent to the CQ was uploaded after l-g-t-m from jam@chromium.org,
pfeldman@chromium.org
Link to the patchset: https://codereview.chromium.org/2611183007/#ps100001
(title: "Fix line endings")

[commit-bot: I haz the power, 2017-01-11 00:52:55]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-11 02:28:34]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1484095924512220,
"parent_rev": "b88b62647053821810cf652a35d6ebd1c8b643ac", "commit_rev":
"9c22f660541434f9666391a38895d4d20d487d03"}

[commit-bot: I haz the power, 2017-01-11 02:29:09]

Description was changed from

==========
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
==========

to

==========
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/+/9c22f660541434f9666391a38895...
==========

[commit-bot: I haz the power, 2017-01-11 02:29:10]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/9c22f660541434f9666391a38895...