DevTools Disable cache should properly handle LoadOnlyFromCache resources

DevTools Disable cache should properly handle LoadOnlyFromCache resources

Currently, checking the "disable cache" button on the networking page in dev
tools causes all requests to run with a bypass cache directive. Even those
resources meant to be fetched exclusively from the cache. This means resources
meant to be served only from the cache wind up being served from the network.

This CL causes such requests to fail early, as would happen if the object were
not in cache.

BUG=634189
Committed: https://crrev.com/938a1f2e20387f1008f4d34536b705da109792ac
Cr-Commit-Position: refs/heads/master@{#425706}

[jkarlin, 2016-08-22 18:23:49]

Description was changed from

==========
[DevTools Network] Disable cache should support resources served from cache only

Currently, checking the "disable cache" button on the networking page in dev
tools causes all requests to run with a bypass cache directive. Even those
resources meant to be fetched exclusively from the cache. This CL fails
requests early if "disable cache" is set and the requests should be served only
from cache.

BUG=634189
==========

to

==========
[DevTools Network] Disable cache should support resources served from cache only

Currently, checking the "disable cache" button on the networking page in dev
tools causes all requests to run with a bypass cache directive. Even those
resources meant to be fetched exclusively from the cache. This CL fails
requests early if "disable cache" is set and the requests should be served only
from cache.

BUG=634189
==========

[jkarlin, 2016-08-22 18:29:00]

Description was changed from

==========
[DevTools Network] Disable cache should support resources served from cache only

Currently, checking the "disable cache" button on the networking page in dev
tools causes all requests to run with a bypass cache directive. Even those
resources meant to be fetched exclusively from the cache. This CL fails
requests early if "disable cache" is set and the requests should be served only
from cache.

BUG=634189
==========

to

==========
DevTools Disable cache should properly handle LoadOnlyFromCache resources

Currently, checking the "disable cache" button on the networking page in dev
tools causes all requests to run with a bypass cache directive. Even those
resources meant to be fetched exclusively from the cache. This means resources 
meant to be served only from the cache wind up being served from the network.

This CL causes such requests to fail early with a net::ERR_CACHE_MISS
ResourceError, as would happen if the object were not in cache.

BUG=634189
==========

[jkarlin, 2016-08-22 18:53:27]

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

[commit-bot: I haz the power, 2016-08-22 18:54:22]

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, 2016-08-22 20:50:13]

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

[commit-bot: I haz the power, 2016-08-22 20:50:13]

Dry run: This issue passed the CQ dry run.

[jkarlin, 2016-08-23 14:26:00]

jkarlin@chromium.org changed reviewers:
+ japhet@chromium.org

[jkarlin, 2016-08-23 14:26:01]

japhet@ PTAL.

I'm new to this area of code so I'd appreciate a close look. Thanks!

[dgozman, 2016-08-23 15:19:43]

+allada FYI

[Nate Chapin, 2016-08-23 16:05:14]

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

https://codereview.chromium.org/2266913002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/InspectorNetworkAgent.cpp:554: if
(request.getCachePolicy() == WebCachePolicy::ReturnCacheDataDontLoad) {
This means we won't ever hit the disk cache for ReturnCacheDataDontLoad when
devtools disables the cache. Would it be sufficient to just ensure the
setCachePolicy() call here never overwrites a ReturnCacheDataDontLoad?

[jkarlin, 2016-08-23 16:29:23]

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

https://codereview.chromium.org/2266913002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/InspectorNetworkAgent.cpp:554: if
(request.getCachePolicy() == WebCachePolicy::ReturnCacheDataDontLoad) {
On 2016/08/23 16:05:14, Nate Chapin wrote:
> This means we won't ever hit the disk cache for ReturnCacheDataDontLoad when
> devtools disables the cache. Would it be sufficient to just ensure the
> setCachePolicy() call here never overwrites a ReturnCacheDataDontLoad?

I believe developers would expect "disable cache" to mean that there effectively
is no cache, or that it's at least empty all the time. So returning cache miss
seems right to me.

[Nate Chapin, 2016-08-23 16:45:54]

On 2016/08/23 16:29:23, jkarlin wrote:
>
https://codereview.chromium.org/2266913002/diff/40001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/inspector/InspectorNetworkAgent.cpp
(right):
> 
>
https://codereview.chromium.org/2266913002/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/inspector/InspectorNetworkAgent.cpp:554: if
> (request.getCachePolicy() == WebCachePolicy::ReturnCacheDataDontLoad) {
> On 2016/08/23 16:05:14, Nate Chapin wrote:
> > This means we won't ever hit the disk cache for ReturnCacheDataDontLoad when
> > devtools disables the cache. Would it be sufficient to just ensure the
> > setCachePolicy() call here never overwrites a ReturnCacheDataDontLoad?
> 
> I believe developers would expect "disable cache" to mean that there
effectively
> is no cache, or that it's at least empty all the time. So returning cache miss
> seems right to me.

"that it's at least empty all the time" isn't strictly true with the current
implementation, but I agree that it's the theory :)

I agree the result should be that we return a cache miss, I just don't see why
the existing plumbing is insufficient for that.

[jkarlin, 2016-08-23 16:50:25]

On 2016/08/23 16:45:54, Nate Chapin wrote:
> On 2016/08/23 16:29:23, jkarlin wrote:
> >
>
https://codereview.chromium.org/2266913002/diff/40001/third_party/WebKit/Sour...
> > File third_party/WebKit/Source/core/inspector/InspectorNetworkAgent.cpp
> (right):
> > 
> >
>
https://codereview.chromium.org/2266913002/diff/40001/third_party/WebKit/Sour...
> > third_party/WebKit/Source/core/inspector/InspectorNetworkAgent.cpp:554: if
> > (request.getCachePolicy() == WebCachePolicy::ReturnCacheDataDontLoad) {
> > On 2016/08/23 16:05:14, Nate Chapin wrote:
> > > This means we won't ever hit the disk cache for ReturnCacheDataDontLoad
when
> > > devtools disables the cache. Would it be sufficient to just ensure the
> > > setCachePolicy() call here never overwrites a ReturnCacheDataDontLoad?
> > 
> > I believe developers would expect "disable cache" to mean that there
> effectively
> > is no cache, or that it's at least empty all the time. So returning cache
miss
> > seems right to me.
> 
> "that it's at least empty all the time" isn't strictly true with the current
> implementation, but I agree that it's the theory :)
> 
> I agree the result should be that we return a cache miss, I just don't see why
> the existing plumbing is insufficient for that.


The existing plumbing implements "disable cache" by changing all requests to
WebCachePolicy::BypassingCache, which turns all requests into network requests.
This isn't right because ReturnCacheDataDontLoad shouldn't go to network.

If we left it as ReturnCacheDataDontLoad then it will check the actual
HttpCache, which might actually have the response, which is counter intuitive
given that the user checked "disable cache".

[Nate Chapin, 2016-08-23 17:08:38]

On 2016/08/23 16:50:25, jkarlin wrote:
> On 2016/08/23 16:45:54, Nate Chapin wrote:
> > On 2016/08/23 16:29:23, jkarlin wrote:
> > >
> >
>
https://codereview.chromium.org/2266913002/diff/40001/third_party/WebKit/Sour...
> > > File third_party/WebKit/Source/core/inspector/InspectorNetworkAgent.cpp
> > (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2266913002/diff/40001/third_party/WebKit/Sour...
> > > third_party/WebKit/Source/core/inspector/InspectorNetworkAgent.cpp:554: if
> > > (request.getCachePolicy() == WebCachePolicy::ReturnCacheDataDontLoad) {
> > > On 2016/08/23 16:05:14, Nate Chapin wrote:
> > > > This means we won't ever hit the disk cache for ReturnCacheDataDontLoad
> when
> > > > devtools disables the cache. Would it be sufficient to just ensure the
> > > > setCachePolicy() call here never overwrites a ReturnCacheDataDontLoad?
> > > 
> > > I believe developers would expect "disable cache" to mean that there
> > effectively
> > > is no cache, or that it's at least empty all the time. So returning cache
> miss
> > > seems right to me.
> > 
> > "that it's at least empty all the time" isn't strictly true with the current
> > implementation, but I agree that it's the theory :)
> > 
> > I agree the result should be that we return a cache miss, I just don't see
why
> > the existing plumbing is insufficient for that.
> 
> 
> The existing plumbing implements "disable cache" by changing all requests to
> WebCachePolicy::BypassingCache, which turns all requests into network
requests.
> This isn't right because ReturnCacheDataDontLoad shouldn't go to network.
> 
> If we left it as ReturnCacheDataDontLoad then it will check the actual
> HttpCache, which might actually have the response, which is counter intuitive
> given that the user checked "disable cache".

Ok, there's probably a cleaner why to do this.
InspectorNetworkAgent::shouldBlockRequest ought to be able to do this, but I
don't think ReturnCacheDataDontLoad is set yet when shouldBlockRequest() is
called. You could try moving the context().canRequest() call in
ResourceFetcher::requestResource() below context().willStartLoadingResource(),
and just blocking via shouldBlockRequest().

That should work, and I don't *think* there will be side effects, but I could
certainly be wrong.

[jkarlin, 2016-08-23 17:12:33]

On 2016/08/23 17:08:38, Nate Chapin wrote:
> On 2016/08/23 16:50:25, jkarlin wrote:
> > On 2016/08/23 16:45:54, Nate Chapin wrote:
> > > On 2016/08/23 16:29:23, jkarlin wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/2266913002/diff/40001/third_party/WebKit/Sour...
> > > > File third_party/WebKit/Source/core/inspector/InspectorNetworkAgent.cpp
> > > (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/2266913002/diff/40001/third_party/WebKit/Sour...
> > > > third_party/WebKit/Source/core/inspector/InspectorNetworkAgent.cpp:554:
if
> > > > (request.getCachePolicy() == WebCachePolicy::ReturnCacheDataDontLoad) {
> > > > On 2016/08/23 16:05:14, Nate Chapin wrote:
> > > > > This means we won't ever hit the disk cache for
ReturnCacheDataDontLoad
> > when
> > > > > devtools disables the cache. Would it be sufficient to just ensure the
> > > > > setCachePolicy() call here never overwrites a ReturnCacheDataDontLoad?
> > > > 
> > > > I believe developers would expect "disable cache" to mean that there
> > > effectively
> > > > is no cache, or that it's at least empty all the time. So returning
cache
> > miss
> > > > seems right to me.
> > > 
> > > "that it's at least empty all the time" isn't strictly true with the
current
> > > implementation, but I agree that it's the theory :)
> > > 
> > > I agree the result should be that we return a cache miss, I just don't see
> why
> > > the existing plumbing is insufficient for that.
> > 
> > 
> > The existing plumbing implements "disable cache" by changing all requests to
> > WebCachePolicy::BypassingCache, which turns all requests into network
> requests.
> > This isn't right because ReturnCacheDataDontLoad shouldn't go to network.
> > 
> > If we left it as ReturnCacheDataDontLoad then it will check the actual
> > HttpCache, which might actually have the response, which is counter
intuitive
> > given that the user checked "disable cache".
> 
> Ok, there's probably a cleaner why to do this.
> InspectorNetworkAgent::shouldBlockRequest ought to be able to do this, but I
> don't think ReturnCacheDataDontLoad is set yet when shouldBlockRequest() is
> called. You could try moving the context().canRequest() call in
> ResourceFetcher::requestResource() below context().willStartLoadingResource(),
> and just blocking via shouldBlockRequest().
> 
> That should work, and I don't *think* there will be side effects, but I could
> certainly be wrong.


I tried that first, see the related bug for more detail. There are two issues:

1) onerror isn't fired
2) it shows up as "dev tools blocked this resource" or some such, which isn't
very intuitive to the user

[Nate Chapin, 2016-08-23 17:35:33]

On 2016/08/23 17:12:33, jkarlin wrote:
> On 2016/08/23 17:08:38, Nate Chapin wrote:
> > On 2016/08/23 16:50:25, jkarlin wrote:
> > > On 2016/08/23 16:45:54, Nate Chapin wrote:
> > > > On 2016/08/23 16:29:23, jkarlin wrote:
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/2266913002/diff/40001/third_party/WebKit/Sour...
> > > > > File
third_party/WebKit/Source/core/inspector/InspectorNetworkAgent.cpp
> > > > (right):
> > > > > 
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/2266913002/diff/40001/third_party/WebKit/Sour...
> > > > >
third_party/WebKit/Source/core/inspector/InspectorNetworkAgent.cpp:554:
> if
> > > > > (request.getCachePolicy() == WebCachePolicy::ReturnCacheDataDontLoad)
{
> > > > > On 2016/08/23 16:05:14, Nate Chapin wrote:
> > > > > > This means we won't ever hit the disk cache for
> ReturnCacheDataDontLoad
> > > when
> > > > > > devtools disables the cache. Would it be sufficient to just ensure
the
> > > > > > setCachePolicy() call here never overwrites a
ReturnCacheDataDontLoad?
> > > > > 
> > > > > I believe developers would expect "disable cache" to mean that there
> > > > effectively
> > > > > is no cache, or that it's at least empty all the time. So returning
> cache
> > > miss
> > > > > seems right to me.
> > > > 
> > > > "that it's at least empty all the time" isn't strictly true with the
> current
> > > > implementation, but I agree that it's the theory :)
> > > > 
> > > > I agree the result should be that we return a cache miss, I just don't
see
> > why
> > > > the existing plumbing is insufficient for that.
> > > 
> > > 
> > > The existing plumbing implements "disable cache" by changing all requests
to
> > > WebCachePolicy::BypassingCache, which turns all requests into network
> > requests.
> > > This isn't right because ReturnCacheDataDontLoad shouldn't go to network.
> > > 
> > > If we left it as ReturnCacheDataDontLoad then it will check the actual
> > > HttpCache, which might actually have the response, which is counter
> intuitive
> > > given that the user checked "disable cache".
> > 
> > Ok, there's probably a cleaner why to do this.
> > InspectorNetworkAgent::shouldBlockRequest ought to be able to do this, but I
> > don't think ReturnCacheDataDontLoad is set yet when shouldBlockRequest() is
> > called. You could try moving the context().canRequest() call in
> > ResourceFetcher::requestResource() below
context().willStartLoadingResource(),
> > and just blocking via shouldBlockRequest().
> > 
> > That should work, and I don't *think* there will be side effects, but I
could
> > certainly be wrong.
> 
> 
> I tried that first, see the related bug for more detail. There are two issues:
> 
> 1) onerror isn't fired

I suspect https://codereview.chromium.org/2231523002/ would fix this.

> 2) it shows up as "dev tools blocked this resource" or some such, which isn't
> very intuitive to the user

This is relatively easy to fix with a new ResourceRequestBlockedReason value.

[jkarlin, 2016-08-23 17:37:38]

On 2016/08/23 17:35:33, Nate Chapin wrote:
> On 2016/08/23 17:12:33, jkarlin wrote:
> > On 2016/08/23 17:08:38, Nate Chapin wrote:
> > > On 2016/08/23 16:50:25, jkarlin wrote:
> > > > On 2016/08/23 16:45:54, Nate Chapin wrote:
> > > > > On 2016/08/23 16:29:23, jkarlin wrote:
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/2266913002/diff/40001/third_party/WebKit/Sour...
> > > > > > File
> third_party/WebKit/Source/core/inspector/InspectorNetworkAgent.cpp
> > > > > (right):
> > > > > > 
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/2266913002/diff/40001/third_party/WebKit/Sour...
> > > > > >
> third_party/WebKit/Source/core/inspector/InspectorNetworkAgent.cpp:554:
> > if
> > > > > > (request.getCachePolicy() ==
WebCachePolicy::ReturnCacheDataDontLoad)
> {
> > > > > > On 2016/08/23 16:05:14, Nate Chapin wrote:
> > > > > > > This means we won't ever hit the disk cache for
> > ReturnCacheDataDontLoad
> > > > when
> > > > > > > devtools disables the cache. Would it be sufficient to just ensure
> the
> > > > > > > setCachePolicy() call here never overwrites a
> ReturnCacheDataDontLoad?
> > > > > > 
> > > > > > I believe developers would expect "disable cache" to mean that there
> > > > > effectively
> > > > > > is no cache, or that it's at least empty all the time. So returning
> > cache
> > > > miss
> > > > > > seems right to me.
> > > > > 
> > > > > "that it's at least empty all the time" isn't strictly true with the
> > current
> > > > > implementation, but I agree that it's the theory :)
> > > > > 
> > > > > I agree the result should be that we return a cache miss, I just don't
> see
> > > why
> > > > > the existing plumbing is insufficient for that.
> > > > 
> > > > 
> > > > The existing plumbing implements "disable cache" by changing all
requests
> to
> > > > WebCachePolicy::BypassingCache, which turns all requests into network
> > > requests.
> > > > This isn't right because ReturnCacheDataDontLoad shouldn't go to
network.
> > > > 
> > > > If we left it as ReturnCacheDataDontLoad then it will check the actual
> > > > HttpCache, which might actually have the response, which is counter
> > intuitive
> > > > given that the user checked "disable cache".
> > > 
> > > Ok, there's probably a cleaner why to do this.
> > > InspectorNetworkAgent::shouldBlockRequest ought to be able to do this, but
I
> > > don't think ReturnCacheDataDontLoad is set yet when shouldBlockRequest()
is
> > > called. You could try moving the context().canRequest() call in
> > > ResourceFetcher::requestResource() below
> context().willStartLoadingResource(),
> > > and just blocking via shouldBlockRequest().
> > > 
> > > That should work, and I don't *think* there will be side effects, but I
> could
> > > certainly be wrong.
> > 
> > 
> > I tried that first, see the related bug for more detail. There are two
issues:
> > 
> > 1) onerror isn't fired
> 
> I suspect https://codereview.chromium.org/2231523002/ would fix this.
> 
> > 2) it shows up as "dev tools blocked this resource" or some such, which
isn't
> > very intuitive to the user
> 
> This is relatively easy to fix with a new ResourceRequestBlockedReason value.

2231523002 looks promising. I'll build a patch on top of that and give it a
whirl.

[jkarlin, 2016-08-23 17:52:48]

On 2016/08/23 17:37:38, jkarlin wrote:
> On 2016/08/23 17:35:33, Nate Chapin wrote:
> > On 2016/08/23 17:12:33, jkarlin wrote:
> > > On 2016/08/23 17:08:38, Nate Chapin wrote:
> > > > On 2016/08/23 16:50:25, jkarlin wrote:
> > > > > On 2016/08/23 16:45:54, Nate Chapin wrote:
> > > > > > On 2016/08/23 16:29:23, jkarlin wrote:
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/2266913002/diff/40001/third_party/WebKit/Sour...
> > > > > > > File
> > third_party/WebKit/Source/core/inspector/InspectorNetworkAgent.cpp
> > > > > > (right):
> > > > > > > 
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/2266913002/diff/40001/third_party/WebKit/Sour...
> > > > > > >
> > third_party/WebKit/Source/core/inspector/InspectorNetworkAgent.cpp:554:
> > > if
> > > > > > > (request.getCachePolicy() ==
> WebCachePolicy::ReturnCacheDataDontLoad)
> > {
> > > > > > > On 2016/08/23 16:05:14, Nate Chapin wrote:
> > > > > > > > This means we won't ever hit the disk cache for
> > > ReturnCacheDataDontLoad
> > > > > when
> > > > > > > > devtools disables the cache. Would it be sufficient to just
ensure
> > the
> > > > > > > > setCachePolicy() call here never overwrites a
> > ReturnCacheDataDontLoad?
> > > > > > > 
> > > > > > > I believe developers would expect "disable cache" to mean that
there
> > > > > > effectively
> > > > > > > is no cache, or that it's at least empty all the time. So
returning
> > > cache
> > > > > miss
> > > > > > > seems right to me.
> > > > > > 
> > > > > > "that it's at least empty all the time" isn't strictly true with the
> > > current
> > > > > > implementation, but I agree that it's the theory :)
> > > > > > 
> > > > > > I agree the result should be that we return a cache miss, I just
don't
> > see
> > > > why
> > > > > > the existing plumbing is insufficient for that.
> > > > > 
> > > > > 
> > > > > The existing plumbing implements "disable cache" by changing all
> requests
> > to
> > > > > WebCachePolicy::BypassingCache, which turns all requests into network
> > > > requests.
> > > > > This isn't right because ReturnCacheDataDontLoad shouldn't go to
> network.
> > > > > 
> > > > > If we left it as ReturnCacheDataDontLoad then it will check the actual
> > > > > HttpCache, which might actually have the response, which is counter
> > > intuitive
> > > > > given that the user checked "disable cache".
> > > > 
> > > > Ok, there's probably a cleaner why to do this.
> > > > InspectorNetworkAgent::shouldBlockRequest ought to be able to do this,
but
> I
> > > > don't think ReturnCacheDataDontLoad is set yet when shouldBlockRequest()
> is
> > > > called. You could try moving the context().canRequest() call in
> > > > ResourceFetcher::requestResource() below
> > context().willStartLoadingResource(),
> > > > and just blocking via shouldBlockRequest().
> > > > 
> > > > That should work, and I don't *think* there will be side effects, but I
> > could
> > > > certainly be wrong.
> > > 
> > > 
> > > I tried that first, see the related bug for more detail. There are two
> issues:
> > > 
> > > 1) onerror isn't fired
> > 
> > I suspect https://codereview.chromium.org/2231523002/ would fix this.
> > 
> > > 2) it shows up as "dev tools blocked this resource" or some such, which
> isn't
> > > very intuitive to the user
> > 
> > This is relatively easy to fix with a new ResourceRequestBlockedReason
value.
> 
> 2231523002 looks promising. I'll build a patch on top of that and give it a
> whirl.

Okay, done. It shows blocked by dev tools but that's not unreasonable (since dev
tools is intervening due to disable cache behavior). It's the onerror event I
was most worried about.

[jkarlin, 2016-08-23 17:52:57]

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

[commit-bot: I haz the power, 2016-08-23 17:53:21]

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

[Nate Chapin, 2016-08-23 17:56:48]

https://codereview.chromium.org/2266913002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp (left):

https://codereview.chromium.org/2266913002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:442: if
(!request.url().isValid())
Should this check move above canRequest() too?

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

https://codereview.chromium.org/2266913002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/InspectorNetworkAgent.cpp:556: if
(m_state->booleanProperty(NetworkAgentState::cacheDisabled, false) &&
request.getCachePolicy() != WebCachePolicy::ReturnCacheDataDontLoad) {
Can this be reached with policy ReturnCacheDataDontLoad now? Should this be a
DCHECK instead?

[jkarlin, 2016-08-23 18:13:42]

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

[commit-bot: I haz the power, 2016-08-23 18:14:24]

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

[jkarlin, 2016-08-23 18:14:28]

PTAL, thanks!

https://codereview.chromium.org/2266913002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp (left):

https://codereview.chromium.org/2266913002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:442: if
(!request.url().isValid())
On 2016/08/23 17:56:48, Nate Chapin wrote:
> Should this check move above canRequest() too?

Done.

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

https://codereview.chromium.org/2266913002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/InspectorNetworkAgent.cpp:556: if
(m_state->booleanProperty(NetworkAgentState::cacheDisabled, false) &&
request.getCachePolicy() != WebCachePolicy::ReturnCacheDataDontLoad) {
On 2016/08/23 17:56:48, Nate Chapin wrote:
> Can this be reached with policy ReturnCacheDataDontLoad now? Should this be a
> DCHECK instead?

Ah, yes, thanks. Done.

[Nate Chapin, 2016-08-23 18:16:07]

lgtm

[jkarlin, 2016-08-23 18:19:37]

On 2016/08/23 18:16:07, Nate Chapin wrote:
> lgtm

Thanks very much!

[commit-bot: I haz the power, 2016-08-23 19:20:18]

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

[commit-bot: I haz the power, 2016-08-23 19:20:19]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[jkarlin, 2016-08-24 11:50:54]

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

[commit-bot: I haz the power, 2016-08-24 11:51:21]

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, 2016-08-24 12:41:04]

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

[commit-bot: I haz the power, 2016-08-24 12:41:05]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[jkarlin, 2016-08-25 17:40:58]

Description was changed from

==========
DevTools Disable cache should properly handle LoadOnlyFromCache resources

Currently, checking the "disable cache" button on the networking page in dev
tools causes all requests to run with a bypass cache directive. Even those
resources meant to be fetched exclusively from the cache. This means resources 
meant to be served only from the cache wind up being served from the network.

This CL causes such requests to fail early with a net::ERR_CACHE_MISS
ResourceError, as would happen if the object were not in cache.

BUG=634189
==========

to

==========
DevTools Disable cache should properly handle LoadOnlyFromCache resources

Currently, checking the "disable cache" button on the networking page in dev
tools causes all requests to run with a bypass cache directive. Even those
resources meant to be fetched exclusively from the cache. This means resources 
meant to be served only from the cache wind up being served from the network.

This CL causes such requests to fail early, as would happen if the object were
not in cache.

BUG=634189
==========

[jkarlin, 2016-10-06 11:14:36]

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

[commit-bot: I haz the power, 2016-10-06 11:14:51]

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, 2016-10-06 12:36:30]

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

[commit-bot: I haz the power, 2016-10-06 12:36:30]

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_...)

[jkarlin, 2016-10-06 12:46:56]

It looks as if the dependent patchset has landed. Unfortunately, it no longer
modifies the resource to need a synchronous cache hit. The synchronous cache hit
bit made onerror fire. 

That means after landing this, if you have devtools open and "disable cache"
checked, then the resource will be blocked but it won't fire onerror and it
won't report ERR_CACHE_MISS like it should.

[Bryan McQuade, 2016-10-06 19:45:38]

On 2016/10/06 at 12:46:56, jkarlin wrote:
> It looks as if the dependent patchset has landed. Unfortunately, it no longer
modifies the resource to need a synchronous cache hit. The synchronous cache hit
bit made onerror fire. 
> 
> That means after landing this, if you have devtools open and "disable cache"
checked, then the resource will be blocked but it won't fire onerror and it
won't report ERR_CACHE_MISS like it should.

It's not ideal but it's better than the current behavior, so let's proceed with
this change. We can consider addressing the remaining issues in a follow up
change.

[pfeldman, 2016-10-06 20:00:06]

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

[pfeldman, 2016-10-06 20:00:07]

https://codereview.chromium.org/2266913002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/inspector/InspectorNetworkAgent.cpp (right):

https://codereview.chromium.org/2266913002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/inspector/InspectorNetworkAgent.cpp:514: if
(m_state->booleanProperty(NetworkAgentState::cacheDisabled, false) &&
This lists request as "blocked by devtools" in the network panel.

[jkarlin, 2016-10-07 11:43:08]

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

[commit-bot: I haz the power, 2016-10-07 11:43:20]

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, 2016-10-07 11:45:26]

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

[commit-bot: I haz the power, 2016-10-07 11:45:27]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[jkarlin, 2016-10-07 12:34:06]

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

[commit-bot: I haz the power, 2016-10-07 12:34:22]

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

[jkarlin, 2016-10-07 12:41:01]

Nate: Could you take a look at PatchSet #3 of this CL again? That was before we
switched to using "blocked by devtools". The nice thing about that method was
that onerror was called and with the correct exception. Do you think we should
switch back to that? 

CL https://codereview.chromium.org/2231523002 no longer makes dev tools failures
synchronous, so it's not firing onerror.

[commit-bot: I haz the power, 2016-10-07 14:06:03]

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

[commit-bot: I haz the power, 2016-10-07 14:06:04]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[jkarlin, 2016-10-12 17:39:17]

On 2016/10/07 12:41:01, jkarlin wrote:
> Nate: Could you take a look at PatchSet #3 of this CL again? That was before
we
> switched to using "blocked by devtools". The nice thing about that method was
> that onerror was called and with the correct exception. Do you think we should
> switch back to that? 
> 
> CL https://codereview.chromium.org/2231523002 no longer makes dev tools
failures
> synchronous, so it's not firing onerror.

Friendly ping Nate. Any thoughts on going back to PatchSet #3?

[Nate Chapin, 2016-10-12 22:10:36]

On 2016/10/12 at 17:39:17, jkarlin wrote:
> On 2016/10/07 12:41:01, jkarlin wrote:
> > Nate: Could you take a look at PatchSet #3 of this CL again? That was before
we
> > switched to using "blocked by devtools". The nice thing about that method
was
> > that onerror was called and with the correct exception. Do you think we
should
> > switch back to that? 
> > 
> > CL https://codereview.chromium.org/2231523002 no longer makes dev tools
failures
> > synchronous, so it's not firing onerror.
> 
> Friendly ping Nate. Any thoughts on going back to PatchSet #3?

I'd rather add the synchronous cache hit flag than go back to PS3, the extra
logic in requestResource() feels like unnecessary complexity to me.

Why does onerror only fire if the cache hit is synchronous? That seems strange.

[jkarlin, 2016-10-14 14:54:19]

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

[commit-bot: I haz the power, 2016-10-14 14:54:33]

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

[jkarlin, 2016-10-14 14:55:37]

On 2016/10/12 22:10:36, Nate Chapin wrote:
> On 2016/10/12 at 17:39:17, jkarlin wrote:
> > On 2016/10/07 12:41:01, jkarlin wrote:
> > > Nate: Could you take a look at PatchSet #3 of this CL again? That was
before
> we
> > > switched to using "blocked by devtools". The nice thing about that method
> was
> > > that onerror was called and with the correct exception. Do you think we
> should
> > > switch back to that? 
> > > 
> > > CL https://codereview.chromium.org/2231523002 no longer makes dev tools
> failures
> > > synchronous, so it's not firing onerror.
> > 
> > Friendly ping Nate. Any thoughts on going back to PatchSet #3?
> 
> I'd rather add the synchronous cache hit flag than go back to PS3, the extra
> logic in requestResource() feels like unnecessary complexity to me.
> 
> Why does onerror only fire if the cache hit is synchronous? That seems
strange.

Looks like it's been fixed actually. Okay, I'm happy with patch as is.

[commit-bot: I haz the power, 2016-10-14 16:19:35]

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

[commit-bot: I haz the power, 2016-10-14 16:19:36]

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_...)

[Nate Chapin, 2016-10-14 23:00:51]

On 2016/10/14 at 14:55:37, jkarlin wrote:
> On 2016/10/12 22:10:36, Nate Chapin wrote:
> > On 2016/10/12 at 17:39:17, jkarlin wrote:
> > > On 2016/10/07 12:41:01, jkarlin wrote:
> > > > Nate: Could you take a look at PatchSet #3 of this CL again? That was
before
> > we
> > > > switched to using "blocked by devtools". The nice thing about that
method
> > was
> > > > that onerror was called and with the correct exception. Do you think we
> > should
> > > > switch back to that? 
> > > > 
> > > > CL https://codereview.chromium.org/2231523002 no longer makes dev tools
> > failures
> > > > synchronous, so it's not firing onerror.
> > > 
> > > Friendly ping Nate. Any thoughts on going back to PatchSet #3?
> > 
> > I'd rather add the synchronous cache hit flag than go back to PS3, the extra
> > logic in requestResource() feels like unnecessary complexity to me.
> > 
> > Why does onerror only fire if the cache hit is synchronous? That seems
strange.
> 
> Looks like it's been fixed actually. Okay, I'm happy with patch as is.

Re-LGTM then :)

[jkarlin, 2016-10-17 15:35:46]

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

[commit-bot: I haz the power, 2016-10-17 15:36:03]

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, 2016-10-17 17:00:06]

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

[commit-bot: I haz the power, 2016-10-17 17:00:10]

Dry run: This issue passed the CQ dry run.

[jkarlin, 2016-10-17 17:04:32]

The CQ bit was checked by jkarlin@chromium.org

[jkarlin, 2016-10-17 17:04:32]

The patchset sent to the CQ was uploaded after l-g-t-m from japhet@chromium.org
Link to the patchset: https://codereview.chromium.org/2266913002/#ps160001
(title: "Fix test")

[jkarlin, 2016-10-17 17:05:04]

The CQ bit was unchecked by jkarlin@chromium.org

[commit-bot: I haz the power, 2016-10-17 17:05:05]

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

[jkarlin, 2016-10-17 17:05:28]

Nate, can you look at one last minor change? Thanks!

[commit-bot: I haz the power, 2016-10-17 17:10:30]

Description was changed from

==========
DevTools Disable cache should properly handle LoadOnlyFromCache resources

Currently, checking the "disable cache" button on the networking page in dev
tools causes all requests to run with a bypass cache directive. Even those
resources meant to be fetched exclusively from the cache. This means resources 
meant to be served only from the cache wind up being served from the network.

This CL causes such requests to fail early, as would happen if the object were
not in cache.

BUG=634189
==========

to

==========
DevTools Disable cache should properly handle LoadOnlyFromCache resources

Currently, checking the "disable cache" button on the networking page in dev
tools causes all requests to run with a bypass cache directive. Even those
resources meant to be fetched exclusively from the cache. This means resources 
meant to be served only from the cache wind up being served from the network.

This CL causes such requests to fail early, as would happen if the object were
not in cache.

BUG=634189
==========

[commit-bot: I haz the power, 2016-10-17 17:10:32]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2016-10-17 17:14:50]

Description was changed from

==========
DevTools Disable cache should properly handle LoadOnlyFromCache resources

Currently, checking the "disable cache" button on the networking page in dev
tools causes all requests to run with a bypass cache directive. Even those
resources meant to be fetched exclusively from the cache. This means resources 
meant to be served only from the cache wind up being served from the network.

This CL causes such requests to fail early, as would happen if the object were
not in cache.

BUG=634189
==========

to

==========
DevTools Disable cache should properly handle LoadOnlyFromCache resources

Currently, checking the "disable cache" button on the networking page in dev
tools causes all requests to run with a bypass cache directive. Even those
resources meant to be fetched exclusively from the cache. This means resources
meant to be served only from the cache wind up being served from the network.

This CL causes such requests to fail early, as would happen if the object were
not in cache.

BUG=634189

Committed: https://crrev.com/938a1f2e20387f1008f4d34536b705da109792ac
Cr-Commit-Position: refs/heads/master@{#425706}
==========

[commit-bot: I haz the power, 2016-10-17 17:14:51]

Patchset 9 (id:??) landed as
https://crrev.com/938a1f2e20387f1008f4d34536b705da109792ac
Cr-Commit-Position: refs/heads/master@{#425706}

[Nate Chapin, 2016-10-17 17:18:44]

On 2016/10/17 17:14:51, commit-bot: I haz the power wrote:
> Patchset 9 (id:??) landed as
> https://crrev.com/938a1f2e20387f1008f4d34536b705da109792ac
> Cr-Commit-Position: refs/heads/master@{#425706}

lgtm