[Devtools] Fixed bug with devtools blocking requests incorrectly

[Devtools] Fixed bug with devtools blocking requests incorrectly

Fixes a regression where devtools was announcing the block of requests
when it should have not been chaning the cache policy instead.

R=dgozman
CC=jkarlin,shaochuan
BUG=660438

[allada, 2016-11-04 00:19:58]

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

[commit-bot: I haz the power, 2016-11-04 00:20:40]

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

[allada, 2016-11-04 00:22:48]

PTL, This is really hard to write a test for since it requires devtools to
reload. I could write a browser test maybe?

[allada, 2016-11-04 01:39:48]

allada@chromium.org changed reviewers:
+ jkarlin@chromium.org, shaochuan@google.com

[commit-bot: I haz the power, 2016-11-04 03:59:11]

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

[commit-bot: I haz the power, 2016-11-04 03:59:12]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[jkarlin, 2016-11-04 12:27:25]

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

https://codereview.chromium.org/2479713003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/inspector/InspectorNetworkAgent.cpp:631:
request.getCachePolicy() != WebCachePolicy::ReturnCacheDataDontLoad &&
If the cache is disabled in devtools and the policy is ReturnCacheDataDontLoad
then we need to abort the request. Otherwise we'll just return something from
what is supposed to be a disabled cache.

[dgozman, 2016-11-04 18:31:43]

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

https://codereview.chromium.org/2479713003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/inspector/InspectorNetworkAgent.cpp:631:
request.getCachePolicy() != WebCachePolicy::ReturnCacheDataDontLoad &&
On 2016/11/04 12:27:25, jkarlin wrote:
> If the cache is disabled in devtools and the policy is ReturnCacheDataDontLoad
> then we need to abort the request. Otherwise we'll just return something from
> what is supposed to be a disabled cache.

Thanks for explanation. I guess we at least need better UI exposure for this, as
right now everyone thinks it's blocked by mistake.

[paulirish, 2016-11-07 06:43:24]

Just spitballing here, but.. 

How about a separate InspectorNetworkAgent::shouldCachePolicyBlockRequest method
where we can do these policy checks and check on cacheDisabled. Then use that
over in
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/loader/Fr...
and return a something like ResourceRequestBlockedReasonCacheDisabled.

[dgozman, 2016-11-07 06:52:33]

On 2016/11/07 06:43:24, paulirish wrote:
> Just spitballing here, but.. 
> 
> How about a separate InspectorNetworkAgent::shouldCachePolicyBlockRequest
method
> where we can do these policy checks and check on cacheDisabled. Then use that
> over in
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/loader/Fr...
> and return a something like ResourceRequestBlockedReasonCacheDisabled.

Yeah, something along these lines.

[jkarlin, 2016-11-07 12:41:31]

On 2016/11/07 06:52:33, dgozman wrote:
> On 2016/11/07 06:43:24, paulirish wrote:
> > Just spitballing here, but.. 
> > 
> > How about a separate InspectorNetworkAgent::shouldCachePolicyBlockRequest
> method
> > where we can do these policy checks and check on cacheDisabled. Then use
that
> > over in
> >
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/loader/Fr...
> > and return a something like ResourceRequestBlockedReasonCacheDisabled.
> 
> Yeah, something along these lines.

sgtm

[allada, 2016-11-08 01:23:21]

Description was changed from

==========
[Devtools] Fixed bug with devtools blocking requests incorrectly

Fixes a regression where devtools was announcing the block of requests
when it should have not been chaning the cache policy instead.

R=dgozman
CC=jkarlin,shaochuan
BUG=660438
==========

to

==========
[Devtools] Fixed bug with devtools blocking requests incorrectly

Fixes a regression where devtools was announcing the block of requests
when it should have not been chaning the cache policy instead.

R=dgozman
CC=jkarlin,shaochuan
BUG=660438
==========