Loading: remove dependencies from ResourceFetcher to ImageResource

Loading: remove dependencies from ResourceFetcher to ImageResource

To move ImageResource out of core/fetch, remove unexpected dependencies
from ResourceFetcher to ImageResource.

BUG=655920
Committed: https://crrev.com/31d4e7d45fb313e90ff623b769cf5e06265db7ce
Cr-Commit-Position: refs/heads/master@{#438156}

[Takashi Toyoshima, 2016-11-17 08:32:28]

Description was changed from

==========
Loading: remove dependencies from ResourceFetcher to ImageResource

To move ImageResource out of core/fetch, remove unexpected dependencies
from ResourceFetcher to ImageResource.

BUG=655920
==========

to

==========
Loading: remove dependencies from ResourceFetcher to ImageResource

To move ImageResource out of core/fetch, remove unexpected dependencies
from ResourceFetcher to ImageResource.

BUG=655920
==========

[Takashi Toyoshima, 2016-11-17 08:32:29]

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

[Takashi Toyoshima, 2016-11-17 08:33:46]

PTAL

https://codereview.chromium.org/2510883003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/fetch/Resource.h (right):

https://codereview.chromium.org/2510883003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/Resource.h:338: // Used by ImageResource to
support LoFi image reload.
Shall we have a stronger comment here? e.g., other sub-classes should not use
this interface, or something.

[hiroshige, 2016-11-17 09:00:33]

https://codereview.chromium.org/2510883003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/fetch/ImageResource.h (right):

https://codereview.chromium.org/2510883003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/ImageResource.h:159: bool
shouldReloadBrokenPlaceholder() const {
Now we can make shouldReloadBrokenPlaceholder() private.

https://codereview.chromium.org/2510883003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/fetch/Resource.h (right):

https://codereview.chromium.org/2510883003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/Resource.h:339: virtual void
mayReloadBrokenResource(ResourceFetcher*,
The name "mayReloadBrokenResource" looks unclear to me, especially I prefer a
name that explicitly states this is only for LoFi or Placeholder reloading.

A candidate might be merging mayReloadBrokenResource() logic and
ReloadBrokenResourcePolicy parameter into reloadIfLoFiOrPlaceholder() and
renaming reloadIfLoFiOrPlaceholder() to reloadIfLoFiOrPlaceholderImage() (and
exposing it in Resource), but I'm not so sure.

[Takashi Toyoshima, 2016-11-17 10:15:43]

How about this change?

https://codereview.chromium.org/2510883003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/fetch/ImageResource.h (right):

https://codereview.chromium.org/2510883003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/ImageResource.h:159: bool
shouldReloadBrokenPlaceholder() const {
On 2016/11/17 09:00:33, hiroshige wrote:
> Now we can make shouldReloadBrokenPlaceholder() private.

Done.

https://codereview.chromium.org/2510883003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/fetch/Resource.h (right):

https://codereview.chromium.org/2510883003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/Resource.h:339: virtual void
mayReloadBrokenResource(ResourceFetcher*,
On 2016/11/17 09:00:33, hiroshige wrote:
> The name "mayReloadBrokenResource" looks unclear to me, especially I prefer a
> name that explicitly states this is only for LoFi or Placeholder reloading.
> 
> A candidate might be merging mayReloadBrokenResource() logic and
> ReloadBrokenResourcePolicy parameter into reloadIfLoFiOrPlaceholder() and
> renaming reloadIfLoFiOrPlaceholder() to reloadIfLoFiOrPlaceholderImage() (and
> exposing it in Resource), but I'm not so sure.

Done.

[Takashi Toyoshima, 2016-11-29 08:13:57]

Description was changed from

==========
Loading: remove dependencies from ResourceFetcher to ImageResource

To move ImageResource out of core/fetch, remove unexpected dependencies
from ResourceFetcher to ImageResource.

BUG=655920
==========

to

==========
Loading: remove dependencies from ResourceFetcher to ImageResource

To move ImageResource out of core/fetch, remove unexpected dependencies
from ResourceFetcher to ImageResource.

(decided to wait for hiroshige@'s ImageResource cleanup)

BUG=655920
==========

[hiroshige, 2016-11-29 08:15:14]

On 2016/11/17 10:15:43, toyoshim wrote:
> How about this change?
> 
>
https://codereview.chromium.org/2510883003/diff/20001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/fetch/ImageResource.h (right):
> 
>
https://codereview.chromium.org/2510883003/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/fetch/ImageResource.h:159: bool
> shouldReloadBrokenPlaceholder() const {
> On 2016/11/17 09:00:33, hiroshige wrote:
> > Now we can make shouldReloadBrokenPlaceholder() private.
> 
> Done.
> 
>
https://codereview.chromium.org/2510883003/diff/20001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/fetch/Resource.h (right):
> 
>
https://codereview.chromium.org/2510883003/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/fetch/Resource.h:339: virtual void
> mayReloadBrokenResource(ResourceFetcher*,
> On 2016/11/17 09:00:33, hiroshige wrote:
> > The name "mayReloadBrokenResource" looks unclear to me, especially I prefer
a
> > name that explicitly states this is only for LoFi or Placeholder reloading.
> > 
> > A candidate might be merging mayReloadBrokenResource() logic and
> > ReloadBrokenResourcePolicy parameter into reloadIfLoFiOrPlaceholder() and
> > renaming reloadIfLoFiOrPlaceholder() to reloadIfLoFiOrPlaceholderImage()
(and
> > exposing it in Resource), but I'm not so sure.
> 
> Done.

As discussed offline, my CL https://codereview.chromium.org/2527353002/ will
remove the dependency from ResourceFetcher to ImageResource so moving
ImageResource after that would be easier.
(I expect the CL will take 2~3 weeks to be landed)

[Takashi Toyoshima, 2016-12-07 09:17:05]

Description was changed from

==========
Loading: remove dependencies from ResourceFetcher to ImageResource

To move ImageResource out of core/fetch, remove unexpected dependencies
from ResourceFetcher to ImageResource.

(decided to wait for hiroshige@'s ImageResource cleanup)

BUG=655920
==========

to

==========
Loading: remove dependencies from ResourceFetcher to ImageResource

To move ImageResource out of core/fetch, remove unexpected dependencies
from ResourceFetcher to ImageResource.

BUG=655920
==========

[Takashi Toyoshima, 2016-12-07 09:19:00]

As we discussed again, this patch will follow the hiroshige@'s patch 4.
I rebased the last patch set to have a right dependency to the patch.

[Takashi Toyoshima, 2016-12-09 05:15:38]

yhirano: just in case, this CL is ready for review and will go before the CL you
reviewed at crrev.com/2549093008

[yhirano, 2016-12-12 06:11:58]

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

https://codereview.chromium.org/2510883003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:1441:
resource->reloadIfLoFiOrPlaceholderImage(this, Resource::kReloadAlways);
Is the nullity check needed?

[hiroshige, 2016-12-12 14:32:39]

non-owner lgtm once yhirano@'s comment is addressed.

[Takashi Toyoshima, 2016-12-13 04:49:16]

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

https://codereview.chromium.org/2510883003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:1441:
resource->reloadIfLoFiOrPlaceholderImage(this, Resource::kReloadAlways);
On 2016/12/12 06:11:57, yhirano wrote:
> Is the nullity check needed?

Done.

[Takashi Toyoshima, 2016-12-13 05:04:08]

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

[commit-bot: I haz the power, 2016-12-13 05:04: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, 2016-12-13 06:32:01]

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

[commit-bot: I haz the power, 2016-12-13 06:32:02]

Dry run: This issue passed the CQ dry run.

[Takashi Toyoshima, 2016-12-13 09:13:28]

yhirano: ptal

[yhirano, 2016-12-13 10:57:15]

lgtm

[Takashi Toyoshima, 2016-12-13 11:22:28]

The CQ bit was checked by toyoshim@chromium.org

[Takashi Toyoshima, 2016-12-13 11:22:29]

The patchset sent to the CQ was uploaded after l-g-t-m from
hiroshige@chromium.org
Link to the patchset: https://codereview.chromium.org/2510883003/#ps80001
(title: "rebase + review #11")

[commit-bot: I haz the power, 2016-12-13 11:22:48]

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-12-13 11:29:53]

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

[commit-bot: I haz the power, 2016-12-13 11:29:54]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Takashi Toyoshima, 2016-12-13 12:01:56]

The CQ bit was checked by toyoshim@chromium.org

[Takashi Toyoshima, 2016-12-13 12:01:56]

The patchset sent to the CQ was uploaded after l-g-t-m from
hiroshige@chromium.org, yhirano@chromium.org
Link to the patchset: https://codereview.chromium.org/2510883003/#ps100001
(title: "rebase ResourceFetcher.cpp")

[commit-bot: I haz the power, 2016-12-13 12:02:22]

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-12-13 13:58:48]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1481630516397910,
"parent_rev": "1a7e8be67d171b6111b132cded069f90cc8237a5", "commit_rev":
"4a252987acf5e874393f4cd02835d125c948973e"}

[commit-bot: I haz the power, 2016-12-13 13:59:13]

Description was changed from

==========
Loading: remove dependencies from ResourceFetcher to ImageResource

To move ImageResource out of core/fetch, remove unexpected dependencies
from ResourceFetcher to ImageResource.

BUG=655920
==========

to

==========
Loading: remove dependencies from ResourceFetcher to ImageResource

To move ImageResource out of core/fetch, remove unexpected dependencies
from ResourceFetcher to ImageResource.

BUG=655920

Review-Url: https://codereview.chromium.org/2510883003
==========

[commit-bot: I haz the power, 2016-12-13 13:59:14]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-12-13 14:00:58]

Description was changed from

==========
Loading: remove dependencies from ResourceFetcher to ImageResource

To move ImageResource out of core/fetch, remove unexpected dependencies
from ResourceFetcher to ImageResource.

BUG=655920

Review-Url: https://codereview.chromium.org/2510883003
==========

to

==========
Loading: remove dependencies from ResourceFetcher to ImageResource

To move ImageResource out of core/fetch, remove unexpected dependencies
from ResourceFetcher to ImageResource.

BUG=655920

Committed: https://crrev.com/31d4e7d45fb313e90ff623b769cf5e06265db7ce
Cr-Commit-Position: refs/heads/master@{#438156}
==========

[commit-bot: I haz the power, 2016-12-13 14:00:59]

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