Remove DeferredByClient

Remove DeferredByClient

It's used for font loads and image document loads. Move
ImageDocuments to a loading system akin to precaching, where
ImageLoader just creates a dummy ImageReosurce and lets
ImageDocument pipe the bytes from the main resource into the
ImageResource. Font loads can be deferred by checking the
Resource::Type directly in ResourceFetcher::resourceNeedsLoad.

BUG=

Committed: https://crrev.com/2aefbfcdd9cc5fe1a533db44b220e6418519df46
Cr-Commit-Position: refs/heads/master@{#390146}

[Nate Chapin, 2016-04-19 19:41:11]

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

[Nate Chapin, 2016-04-19 19:41:12]

What do you think of this?

I think we should either use DeferredByClient more, or not at all. I'm inclined
to say not at all.

[yhirano, 2016-04-20 08:42:42]

https://codereview.chromium.org/1879793003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/ImageLoader.cpp (left):

https://codereview.chromium.org/1879793003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/ImageLoader.cpp:154: ,
m_loadingImageDocument(false)
Can you tell me why you deleted this statement?

https://codereview.chromium.org/1879793003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/ImageLoader.cpp (right):

https://codereview.chromium.org/1879793003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/ImageLoader.cpp:369:
setImage(ImageResource::create(imageSourceToKURL(m_element->imageSourceURL())));
ImageResource::create(const ResourceRequest&) is exposed for testing, according
to the comment. Is it OK to use it here?

[hiroshige, 2016-04-21 07:52:50]

This CL moves a part of delayed loading control from FetchRequest::DeferOption
to ResourceFetcher::resourceNeedsLoad(). Are you going to centralize delayed
loading control to ResourceFetcher::resourceNeedsLoad(), or somewhere else?

[Nate Chapin, 2016-04-21 18:49:33]

On 2016/04/21 07:52:50, hiroshige wrote:
> This CL moves a part of delayed loading control from FetchRequest::DeferOption
> to ResourceFetcher::resourceNeedsLoad(). Are you going to centralize delayed
> loading control to ResourceFetcher::resourceNeedsLoad(), or somewhere else?

resourceNeedsLoad() should now be the only place where we have logic related to
deferring a load before the creation of a ResourceLoader.

[Nate Chapin, 2016-04-21 18:52:46]

https://codereview.chromium.org/1879793003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/ImageLoader.cpp (left):

https://codereview.chromium.org/1879793003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/ImageLoader.cpp:154: ,
m_loadingImageDocument(false)
On 2016/04/20 08:42:41, yhirano wrote:
> Can you tell me why you deleted this statement?

It's a mistake. My first pass deleted this variable, and I missed re-adding it
here.

https://codereview.chromium.org/1879793003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/ImageLoader.cpp (right):

https://codereview.chromium.org/1879793003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/ImageLoader.cpp:369:
setImage(ImageResource::create(imageSourceToKURL(m_element->imageSourceURL())));
On 2016/04/20 08:42:41, yhirano wrote:
> ImageResource::create(const ResourceRequest&) is exposed for testing,
according
> to the comment. Is it OK to use it here?

It's safe to use, it's mostly a question of whether other non-testing callers
will misuse it. I think that's OK? I've removed the "Exposed for testing"
comment to match.

[yhirano, 2016-04-22 04:12:13]

https://codereview.chromium.org/1879793003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/ImageLoader.cpp (right):

https://codereview.chromium.org/1879793003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/ImageLoader.cpp:42: #include
"core/html/ImageDocument.h"
Do we need to include this header?

https://codereview.chromium.org/1879793003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/ImageLoader.cpp:370:
setImage(ImageResource::create(imageSourceToKURL(m_element->imageSourceURL())));
imageResource->setStatus(Pending) needed?

[Nate Chapin, 2016-04-25 22:13:03]

https://codereview.chromium.org/1879793003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/ImageLoader.cpp (right):

https://codereview.chromium.org/1879793003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/ImageLoader.cpp:42: #include
"core/html/ImageDocument.h"
On 2016/04/22 04:12:13, yhirano wrote:
> Do we need to include this header?

No, removed.

https://codereview.chromium.org/1879793003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/ImageLoader.cpp:370:
setImage(ImageResource::create(imageSourceToKURL(m_element->imageSourceURL())));
On 2016/04/22 04:12:13, yhirano wrote:
> imageResource->setStatus(Pending) needed?

"Need", not as the code currently works. But it's certainly more consistent to
set as Pending, so I added it :)

[Nate Chapin, 2016-04-25 22:13:09]

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

[commit-bot: I haz the power, 2016-04-25 22:13:53]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1879793003/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1879793003/80001

[commit-bot: I haz the power, 2016-04-26 00:35:24]

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

[commit-bot: I haz the power, 2016-04-26 00:35:25]

Dry run: This issue passed the CQ dry run.

[yhirano, 2016-04-26 11:23:28]

lgtm

[hiroshige, 2016-04-26 12:22:12]

> resourceNeedsLoad() should now be the only place where we have logic related
to
> deferring a load before the creation of a ResourceLoader.

Thanks for clarification. Could you add this kind of description to the CL
description?

https://codereview.chromium.org/1879793003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/ImageLoader.cpp (left):

https://codereview.chromium.org/1879793003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/ImageLoader.cpp:287:
m_loadingImageDocument = false;
This block doesn't have corresponding lines of code in the new code.
Should we handle this case (e.g. set |m_loadingImageDocument = false| just
before Line 363 in the new code)?

[Nate Chapin, 2016-04-26 23:25:52]

Description was changed from

==========
Remove DeferredByClient

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

to

==========
Remove DeferredByClient

It's used for font loads and image document loads. Move
ImageDocuments to a loading system akin to precaching, where
ImageLoader just creates a dummy ImageReosurce and lets
ImageDocument pipe the bytes from the main resource into the
ImageResource. Font loads can be deferred by checking the
Resource::Type directly in ResourceFetcher::resourceNeedsLoad.

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

[Nate Chapin, 2016-04-26 23:27:40]

https://codereview.chromium.org/1879793003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/ImageLoader.cpp (left):

https://codereview.chromium.org/1879793003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/ImageLoader.cpp:287:
m_loadingImageDocument = false;
On 2016/04/26 12:22:12, hiroshige wrote:
> This block doesn't have corresponding lines of code in the new code.
> Should we handle this case (e.g. set |m_loadingImageDocument = false| just
> before Line 363 in the new code)?

I think it's sufficient to just skip the m_loadingImageDocument block in
updateFromElement(). That's added. PTAL.

[Nate Chapin, 2016-04-26 23:27:46]

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

[commit-bot: I haz the power, 2016-04-26 23:28:00]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1879793003/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1879793003/100001

[commit-bot: I haz the power, 2016-04-27 00:52:05]

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

[commit-bot: I haz the power, 2016-04-27 00:52:06]

Dry run: This issue passed the CQ dry run.

[hiroshige, 2016-04-27 04:19:00]

lgtm

[Nate Chapin, 2016-04-27 19:08:23]

The CQ bit was checked by japhet@chromium.org

[Nate Chapin, 2016-04-27 19:08:23]

The patchset sent to the CQ was uploaded after l-g-t-m from yhirano@chromium.org
Link to the patchset: https://codereview.chromium.org/1879793003/#ps100001
(title: "Add UpdateForcedReload check for the m_loadingImageDocument special
case")

[commit-bot: I haz the power, 2016-04-27 19:08:50]

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

[commit-bot: I haz the power, 2016-04-27 19:13:48]

Description was changed from

==========
Remove DeferredByClient

It's used for font loads and image document loads. Move
ImageDocuments to a loading system akin to precaching, where
ImageLoader just creates a dummy ImageReosurce and lets
ImageDocument pipe the bytes from the main resource into the
ImageResource. Font loads can be deferred by checking the
Resource::Type directly in ResourceFetcher::resourceNeedsLoad.

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

to

==========
Remove DeferredByClient

It's used for font loads and image document loads. Move
ImageDocuments to a loading system akin to precaching, where
ImageLoader just creates a dummy ImageReosurce and lets
ImageDocument pipe the bytes from the main resource into the
ImageResource. Font loads can be deferred by checking the
Resource::Type directly in ResourceFetcher::resourceNeedsLoad.

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

[commit-bot: I haz the power, 2016-04-27 19:13:49]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-04-30 17:11:51]

Patchset 6 (id:??) landed as
https://crrev.com/2aefbfcdd9cc5fe1a533db44b220e6418519df46
Cr-Commit-Position: refs/heads/master@{#390146}

[commit-bot: I haz the power, 2016-04-30 17:51:13]

Description was changed from

==========
Remove DeferredByClient

It's used for font loads and image document loads. Move
ImageDocuments to a loading system akin to precaching, where
ImageLoader just creates a dummy ImageReosurce and lets
ImageDocument pipe the bytes from the main resource into the
ImageResource. Font loads can be deferred by checking the
Resource::Type directly in ResourceFetcher::resourceNeedsLoad.

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

to

==========
Remove DeferredByClient

It's used for font loads and image document loads. Move
ImageDocuments to a loading system akin to precaching, where
ImageLoader just creates a dummy ImageReosurce and lets
ImageDocument pipe the bytes from the main resource into the
ImageResource. Font loads can be deferred by checking the
Resource::Type directly in ResourceFetcher::resourceNeedsLoad.

BUG=

Committed: https://crrev.com/2aefbfcdd9cc5fe1a533db44b220e6418519df46
Cr-Commit-Position: refs/heads/master@{#390146}
==========