Unify Resource loading status tracking

Unify Resource loading status tracking

Resource keeps an m_loading bit as well as an m_status enum. m_loading mostly corresponds to m_status == Pending, so unify them.

FontResource also keeps a separate state enum. Merge most of that into Resource::Status.

Centralize image deferral logic in ResourceFetcher, rather than having ImageResource::load() and ImageResource::fetch() participate.

BUG=

Committed: https://crrev.com/32360d4f64daee1daa4bb4d40d66324da0e782ba
Cr-Commit-Position: refs/heads/master@{#383326}

[Nate Chapin, 2016-03-18 21:23:58]

Description was changed from

==========
Unify Resource loading status tracking

BUG=
==========

to

==========
Unify Resource loading status tracking

Resource keeps an m_loading bit as well as an m_status enum. m_loading mostly
corresponds to m_status == Pending, so unify them.

FontResource also keeps a separate state enum. Merge most of that into
Resource::Status.

Centralize image deferral logic in ResourceFetcher, rather than having
ImageResource::load() and ImageResource::fetch() participate.

BUG=
==========

[Nate Chapin, 2016-03-18 21:24:05]

japhet@chromium.org changed reviewers:
+ hiroshige@chromium.org, mkwst@chromium.org

[Nate Chapin, 2016-03-18 21:24:06]

https://codereview.chromium.org/1802123002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/fetch/Resource.cpp (left):

https://codereview.chromium.org/1802123002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/Resource.cpp:676: if (m_status != Cached)
This clause is weird. If we cancel *while finishing*, m_status used to be
Pending, but with this change, it's Cached. That means that we used to remove
resources from MemoryCache if they were being cancelled during completion. The
m_loader null check above ensures we won't reach this point if the load is
really, truly complete, so dropping this if() keeps the logic pretty close to
the same.

Leaving the if() in place breaks a layout test:
http/tests/inspector/network/network-xhr-same-url-as-main-resource.html

and 3 browser_tests:
ExtensionRequestLimitingThrottleBrowserTest.DoNotThrottleCachedResponse_Redirect
ExtensionRequestLimitingThrottleBrowserTest.DoNotThrottleCachedResponse
ExtensionRequestLimitingThrottleBrowserTest.DoNotThrottleCachedResponse_NonRedirectCached

The layout test breaks because the inspector tries to pre-cache the url in
question when it opens, then immediately drops its reference to it. The test
depends on dropping the reference removing the resource from MemoryCache, as it
then requests that url and breaks if it is a cache. The browser tests have a
similar (but not identical) root cause, but I didn't dig down to find the exact
details.

https://codereview.chromium.org/1802123002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp (right):

https://codereview.chromium.org/1802123002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:670: if
(existingResource->isImage() && !context().allowImage(m_imagesEnabled,
existingResource->url()))
shouldDeferImageLoad(), if used here, blocks SVGImages from loading inline data:
images. It'd be nice make it shouldDeferImageLoad() here, too, for consistency.

[hiroshige, 2016-03-18 23:21:20]

https://codereview.chromium.org/1802123002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/fetch/CachingCorrectnessTest.cpp (right):

https://codereview.chromium.org/1802123002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/CachingCorrectnessTest.cpp:94:
resource->setStatus(Resource::Cached);
If setStatus(Cached) and finish() at Line 397 etc. are used for the same purpose
of make Resource loaded, can we call finish() here for consistency?

https://codereview.chromium.org/1802123002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/CachingCorrectnessTest.cpp:107:
resource->setStatus(Resource::Cached);
ditto.

https://codereview.chromium.org/1802123002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/fetch/RawResourceTest.cpp (right):

https://codereview.chromium.org/1802123002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/RawResourceTest.cpp:183:
raw->setResponse(response);
How about calling raw->finish() for consistency?

https://codereview.chromium.org/1802123002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/fetch/Resource.cpp (left):

https://codereview.chromium.org/1802123002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/Resource.cpp:676: if (m_status != Cached)
> cancel *while finishing*
This is the case in XHR and might cause the test failures. Removing the |if|
looks good for keeping XHR's behavior.

The fact that |m_loader| is still non null and we can cancel Resource's loading
and prevent it from being cached in notifyFinished() looks awkward -- it means
Resource is not completely loaded in that sense -- but it's the current
behavior.

https://codereview.chromium.org/1802123002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp (right):

https://codereview.chromium.org/1802123002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:670: if
(existingResource->isImage() && !context().allowImage(m_imagesEnabled,
existingResource->url()))
On 2016/03/18 21:24:06, Nate Chapin wrote:
> shouldDeferImageLoad(), if used here, blocks SVGImages from loading inline
data:
> images. It'd be nice make it shouldDeferImageLoad() here, too, for
consistency.

Offline discussion memo: can we |m_autoLoadImages| to true only for
ResourceFetcher for SVGs and use shouldDeferImageLoad() here?

https://codereview.chromium.org/1802123002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/FrameSerializer.cpp (right):

https://codereview.chromium.org/1802123002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/FrameSerializer.cpp:415: if (!font ||
font->isLoading() || !font->resourceBuffer() || !shouldAddURL(font->url()))
What is the intention to this change?
I feel |!font->isLoaded()| looks good as-is.

[Nate Chapin, 2016-03-21 22:04:01]

https://codereview.chromium.org/1802123002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/fetch/CachingCorrectnessTest.cpp (right):

https://codereview.chromium.org/1802123002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/CachingCorrectnessTest.cpp:94:
resource->setStatus(Resource::Cached);
On 2016/03/18 23:21:20, hiroshige wrote:
> If setStatus(Cached) and finish() at Line 397 etc. are used for the same
purpose
> of make Resource loaded, can we call finish() here for consistency?

Added finish() call, but leaving setStatus() call in for type == Image with
explanatory comment.

https://codereview.chromium.org/1802123002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/CachingCorrectnessTest.cpp:107:
resource->setStatus(Resource::Cached);
On 2016/03/18 23:21:20, hiroshige wrote:
> ditto.

Done.

https://codereview.chromium.org/1802123002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/fetch/RawResourceTest.cpp (right):

https://codereview.chromium.org/1802123002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/RawResourceTest.cpp:183:
raw->setResponse(response);
On 2016/03/18 23:21:20, hiroshige wrote:
> How about calling raw->finish() for consistency?

Done.

https://codereview.chromium.org/1802123002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/fetch/Resource.cpp (left):

https://codereview.chromium.org/1802123002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/Resource.cpp:676: if (m_status != Cached)
On 2016/03/18 23:21:20, hiroshige wrote:
> > cancel *while finishing*
> This is the case in XHR and might cause the test failures. Removing the |if|
> looks good for keeping XHR's behavior.
> 
> The fact that |m_loader| is still non null and we can cancel Resource's
loading
> and prevent it from being cached in notifyFinished() looks awkward -- it means
> Resource is not completely loaded in that sense -- but it's the current
> behavior.

Yeah, it seems like a good opportunity for cleanup if the weak reference
MemoryCache doesn't work out.

https://codereview.chromium.org/1802123002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp (right):

https://codereview.chromium.org/1802123002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:670: if
(existingResource->isImage() && !context().allowImage(m_imagesEnabled,
existingResource->url()))
On 2016/03/18 23:21:20, hiroshige wrote:
> On 2016/03/18 21:24:06, Nate Chapin wrote:
> > shouldDeferImageLoad(), if used here, blocks SVGImages from loading inline
> data:
> > images. It'd be nice make it shouldDeferImageLoad() here, too, for
> consistency.
> 
> Offline discussion memo: can we |m_autoLoadImages| to true only for
> ResourceFetcher for SVGs and use shouldDeferImageLoad() here?

https://chromium.googlesource.com/chromium/src/+/master/third_party/WebKit/La...
fails with this change.

It appears that m_imagesEnabled and m_autoLoadImages are intentionally subtly
different. m_imagesEnabled being false blocks *everything*, while
m_autoLoadImages being false blocks network requests only (but not data: urls or
cache hits). It would be nice if we could unify this behavior, but I don't think
this CL is the place to do it.

https://codereview.chromium.org/1802123002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/FrameSerializer.cpp (right):

https://codereview.chromium.org/1802123002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/FrameSerializer.cpp:415: if (!font ||
font->isLoading() || !font->resourceBuffer() || !shouldAddURL(font->url()))
On 2016/03/18 23:21:20, hiroshige wrote:
> What is the intention to this change?
> I feel |!font->isLoaded()| looks good as-is.

This was a failed attempt at fixing some unit test failures, and isn't
necessary. Dropped.

[hiroshige, 2016-03-22 18:21:33]

https://codereview.chromium.org/1802123002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/fetch/CachingCorrectnessTest.cpp (right):

https://codereview.chromium.org/1802123002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/CachingCorrectnessTest.cpp:94:
resource->setStatus(Resource::Cached);
On 2016/03/21 22:04:01, Nate Chapin wrote:
> On 2016/03/18 23:21:20, hiroshige wrote:
> > If setStatus(Cached) and finish() at Line 397 etc. are used for the same
> purpose
> > of make Resource loaded, can we call finish() here for consistency?
> 
> Added finish() call, but leaving setStatus() call in for type == Image with
> explanatory comment.

Oh, OK. Please mention that this is a test-only hack in the comment.

https://codereview.chromium.org/1802123002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp (right):

https://codereview.chromium.org/1802123002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:670: if
(existingResource->isImage() && !context().allowImage(m_imagesEnabled,
existingResource->url()))
On 2016/03/21 22:04:01, Nate Chapin wrote:
> On 2016/03/18 23:21:20, hiroshige wrote:
> > On 2016/03/18 21:24:06, Nate Chapin wrote:
> > > shouldDeferImageLoad(), if used here, blocks SVGImages from loading inline
> > data:
> > > images. It'd be nice make it shouldDeferImageLoad() here, too, for
> > consistency.
> > 
> > Offline discussion memo: can we |m_autoLoadImages| to true only for
> > ResourceFetcher for SVGs and use shouldDeferImageLoad() here?
> 
>
https://chromium.googlesource.com/chromium/src/+/master/third_party/WebKit/La...
> fails with this change.
> 
> It appears that m_imagesEnabled and m_autoLoadImages are intentionally subtly
> different. m_imagesEnabled being false blocks *everything*, while
> m_autoLoadImages being false blocks network requests only (but not data: urls
or
> cache hits). It would be nice if we could unify this behavior, but I don't
think
> this CL is the place to do it.

I see.
So, if allowImage() is false,
(1) we reload Resource here, just to make all Resource to be deferred by
shouldDeferImageLoad() in resourceNeedsLoad(), and
(2) we apply this intentionally to data URLs (right?), and thus this if
statement has higher priority than |if(isStaticData)| at Line 679.
Please add comments here to describe these two points.

[Nate Chapin, 2016-03-22 19:51:23]

https://codereview.chromium.org/1802123002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/fetch/CachingCorrectnessTest.cpp (right):

https://codereview.chromium.org/1802123002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/CachingCorrectnessTest.cpp:94:
resource->setStatus(Resource::Cached);
On 2016/03/22 18:21:33, hiroshige wrote:
> On 2016/03/21 22:04:01, Nate Chapin wrote:
> > On 2016/03/18 23:21:20, hiroshige wrote:
> > > If setStatus(Cached) and finish() at Line 397 etc. are used for the same
> > purpose
> > > of make Resource loaded, can we call finish() here for consistency?
> > 
> > Added finish() call, but leaving setStatus() call in for type == Image with
> > explanatory comment.
> 
> Oh, OK. Please mention that this is a test-only hack in the comment.

Done.

https://codereview.chromium.org/1802123002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp (right):

https://codereview.chromium.org/1802123002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:670: if
(existingResource->isImage() && !context().allowImage(m_imagesEnabled,
existingResource->url()))
On 2016/03/22 18:21:33, hiroshige wrote:
> On 2016/03/21 22:04:01, Nate Chapin wrote:
> > On 2016/03/18 23:21:20, hiroshige wrote:
> > > On 2016/03/18 21:24:06, Nate Chapin wrote:
> > > > shouldDeferImageLoad(), if used here, blocks SVGImages from loading
inline
> > > data:
> > > > images. It'd be nice make it shouldDeferImageLoad() here, too, for
> > > consistency.
> > > 
> > > Offline discussion memo: can we |m_autoLoadImages| to true only for
> > > ResourceFetcher for SVGs and use shouldDeferImageLoad() here?
> > 
> >
>
https://chromium.googlesource.com/chromium/src/+/master/third_party/WebKit/La...
> > fails with this change.
> > 
> > It appears that m_imagesEnabled and m_autoLoadImages are intentionally
subtly
> > different. m_imagesEnabled being false blocks *everything*, while
> > m_autoLoadImages being false blocks network requests only (but not data:
urls
> or
> > cache hits). It would be nice if we could unify this behavior, but I don't
> think
> > this CL is the place to do it.
> 
> I see.
> So, if allowImage() is false,
> (1) we reload Resource here, just to make all Resource to be deferred by
> shouldDeferImageLoad() in resourceNeedsLoad(), and
> (2) we apply this intentionally to data URLs (right?), and thus this if
> statement has higher priority than |if(isStaticData)| at Line 679.
> Please add comments here to describe these two points.

Done.

[Nate Chapin, 2016-03-25 17:11:49]

On 2016/03/22 19:51:23, Nate Chapin wrote:
>
https://codereview.chromium.org/1802123002/diff/80001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/fetch/CachingCorrectnessTest.cpp (right):
> 
>
https://codereview.chromium.org/1802123002/diff/80001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/fetch/CachingCorrectnessTest.cpp:94:
> resource->setStatus(Resource::Cached);
> On 2016/03/22 18:21:33, hiroshige wrote:
> > On 2016/03/21 22:04:01, Nate Chapin wrote:
> > > On 2016/03/18 23:21:20, hiroshige wrote:
> > > > If setStatus(Cached) and finish() at Line 397 etc. are used for the same
> > > purpose
> > > > of make Resource loaded, can we call finish() here for consistency?
> > > 
> > > Added finish() call, but leaving setStatus() call in for type == Image
with
> > > explanatory comment.
> > 
> > Oh, OK. Please mention that this is a test-only hack in the comment.
> 
> Done.
> 
>
https://codereview.chromium.org/1802123002/diff/80001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp (right):
> 
>
https://codereview.chromium.org/1802123002/diff/80001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:670: if
> (existingResource->isImage() && !context().allowImage(m_imagesEnabled,
> existingResource->url()))
> On 2016/03/22 18:21:33, hiroshige wrote:
> > On 2016/03/21 22:04:01, Nate Chapin wrote:
> > > On 2016/03/18 23:21:20, hiroshige wrote:
> > > > On 2016/03/18 21:24:06, Nate Chapin wrote:
> > > > > shouldDeferImageLoad(), if used here, blocks SVGImages from loading
> inline
> > > > data:
> > > > > images. It'd be nice make it shouldDeferImageLoad() here, too, for
> > > > consistency.
> > > > 
> > > > Offline discussion memo: can we |m_autoLoadImages| to true only for
> > > > ResourceFetcher for SVGs and use shouldDeferImageLoad() here?
> > > 
> > >
> >
>
https://chromium.googlesource.com/chromium/src/+/master/third_party/WebKit/La...
> > > fails with this change.
> > > 
> > > It appears that m_imagesEnabled and m_autoLoadImages are intentionally
> subtly
> > > different. m_imagesEnabled being false blocks *everything*, while
> > > m_autoLoadImages being false blocks network requests only (but not data:
> urls
> > or
> > > cache hits). It would be nice if we could unify this behavior, but I don't
> > think
> > > this CL is the place to do it.
> > 
> > I see.
> > So, if allowImage() is false,
> > (1) we reload Resource here, just to make all Resource to be deferred by
> > shouldDeferImageLoad() in resourceNeedsLoad(), and
> > (2) we apply this intentionally to data URLs (right?), and thus this if
> > statement has higher priority than |if(isStaticData)| at Line 679.
> > Please add comments here to describe these two points.
> 
> Done.

Friendly ping :)

[hiroshige, 2016-03-25 17:35:36]

lgtm.

[Nate Chapin, 2016-03-25 17:35:57]

The CQ bit was checked by japhet@chromium.org

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

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1802123002/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1802123002/140001

[hiroshige, 2016-03-25 17:38:22]

https://codereview.chromium.org/1802123002/diff/140001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/permissionclient/image-permissions-expected.txt
(right):

https://codereview.chromium.org/1802123002/diff/140001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/permissionclient/image-permissions-expected.txt:3:
PERMISSION CLIENT: allowImage((file
test):permissionclient/resources/boston.gif): false
Er, one more comment: do the additional lines here indicates more network
request than before?

[Nate Chapin, 2016-03-25 17:40:24]

On 2016/03/25 17:38:22, hiroshige wrote:
>
https://codereview.chromium.org/1802123002/diff/140001/third_party/WebKit/Lay...
> File
> third_party/WebKit/LayoutTests/permissionclient/image-permissions-expected.txt
> (right):
> 
>
https://codereview.chromium.org/1802123002/diff/140001/third_party/WebKit/Lay...
>
third_party/WebKit/LayoutTests/permissionclient/image-permissions-expected.txt:3:
> PERMISSION CLIENT: allowImage((file
> test):permissionclient/resources/boston.gif): false
> Er, one more comment: do the additional lines here indicates more network
> request than before?

No, they just indicate a couple of extra calls of context().allowImage().

[hiroshige, 2016-03-25 17:42:24]

On 2016/03/25 17:40:24, Nate Chapin wrote:
> On 2016/03/25 17:38:22, hiroshige wrote:
> >
>
https://codereview.chromium.org/1802123002/diff/140001/third_party/WebKit/Lay...
> > File
> >
third_party/WebKit/LayoutTests/permissionclient/image-permissions-expected.txt
> > (right):
> > 
> >
>
https://codereview.chromium.org/1802123002/diff/140001/third_party/WebKit/Lay...
> >
>
third_party/WebKit/LayoutTests/permissionclient/image-permissions-expected.txt:3:
> > PERMISSION CLIENT: allowImage((file
> > test):permissionclient/resources/boston.gif): false
> > Er, one more comment: do the additional lines here indicates more network
> > request than before?
> 
> No, they just indicate a couple of extra calls of context().allowImage().

Understood, thanks! still lgtm.

[commit-bot: I haz the power, 2016-03-25 18:58:41]

Description was changed from

==========
Unify Resource loading status tracking

Resource keeps an m_loading bit as well as an m_status enum. m_loading mostly
corresponds to m_status == Pending, so unify them.

FontResource also keeps a separate state enum. Merge most of that into
Resource::Status.

Centralize image deferral logic in ResourceFetcher, rather than having
ImageResource::load() and ImageResource::fetch() participate.

BUG=
==========

to

==========
Unify Resource loading status tracking

Resource keeps an m_loading bit as well as an m_status enum. m_loading mostly
corresponds to m_status == Pending, so unify them.

FontResource also keeps a separate state enum. Merge most of that into
Resource::Status.

Centralize image deferral logic in ResourceFetcher, rather than having
ImageResource::load() and ImageResource::fetch() participate.

BUG=
==========

[commit-bot: I haz the power, 2016-03-25 18:58:42]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-03-25 18:59:56]

Description was changed from

==========
Unify Resource loading status tracking

Resource keeps an m_loading bit as well as an m_status enum. m_loading mostly
corresponds to m_status == Pending, so unify them.

FontResource also keeps a separate state enum. Merge most of that into
Resource::Status.

Centralize image deferral logic in ResourceFetcher, rather than having
ImageResource::load() and ImageResource::fetch() participate.

BUG=
==========

to

==========
Unify Resource loading status tracking

Resource keeps an m_loading bit as well as an m_status enum. m_loading mostly
corresponds to m_status == Pending, so unify them.

FontResource also keeps a separate state enum. Merge most of that into
Resource::Status.

Centralize image deferral logic in ResourceFetcher, rather than having
ImageResource::load() and ImageResource::fetch() participate.

BUG=

Committed: https://crrev.com/32360d4f64daee1daa4bb4d40d66324da0e782ba
Cr-Commit-Position: refs/heads/master@{#383326}
==========

[commit-bot: I haz the power, 2016-03-25 18:59:57]

Patchset 8 (id:??) landed as
https://crrev.com/32360d4f64daee1daa4bb4d40d66324da0e782ba
Cr-Commit-Position: refs/heads/master@{#383326}