Do not reload in ImageResource::fetch()

Do not reload in ImageResource::fetch()

When there exists an placeholder ImageResource on MemoryCache and
a new non-placeholder request is created,
this CL starts a new non-placeholder loading with a new ImageResource,
instead of reusing the existing placeholder ImageResource and
triggering (non-placeholder) reloading on that.

This simplifies ImageResource::fetch() and removes one enum value for
controlling reload.

This leaves the existing placeholder ImageResource not to be reloaded
in such cases, and thus the unit tests are modified accordingly.

BUG=667641
TEST=ImageResourceTest.FetchAllowPlaceholderThenDisallowPlaceholder
     ImageResourceTest.FetchAllowPlaceholderThenDisallowPlaceholderAfterLoaded

Review-Url: https://codereview.chromium.org/2602793003
Cr-Commit-Position: refs/heads/master@{#455390}
Committed: https://chromium.googlesource.com/chromium/src/+/79c699b99525ab50568821fce704976a9278e3fa

[hiroshige, 2016-12-28 00:51:37]

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

[commit-bot: I haz the power, 2016-12-28 00:51:49]

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

[hiroshige, 2016-12-28 00:52:10]

Description was changed from

==========
Do not reload in ImageResource::fetch()

BUG=667641
==========

to

==========
Do not reload in ImageResource::fetch()

BUG=667641
TEST=ImageResourceTest.FetchAllowPlaceholderThenDisallowPlaceholder
     ImageResourceTest.FetchAllowPlaceholderThenDisallowPlaceholderAfterLoaded
==========

[hiroshige, 2016-12-28 00:56:06]

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

[hiroshige, 2016-12-28 01:06:11]

From a review comment in https://codereview.chromium.org/2527353002/.

There are test failures in the following case:
(1) The previous ImageResource is a placeholder (m_isPlaceholder == true) and
(2) we call fetch() with non-placeholder request
(request.placeholderImageRequestType() != AllowPlaceholder).

Should the previous ImageResource be reloaded and updated with the full image?
- Previously, Yes: fetch() returns the previous ImageResource and it is
reloaded.
- Afer This CL, No: it makes (1) and (2) different ImageResources, and leaves
(1) a placeholder (not reloaded) while (2) gets fully loaded.

[commit-bot: I haz the power, 2016-12-28 02:11:23]

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

[commit-bot: I haz the power, 2016-12-28 02:11:23]

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

[Nate Chapin, 2017-01-18 21:06:18]

lgtm

[hiroshige, 2017-02-11 00:24:07]

Description was changed from

==========
Do not reload in ImageResource::fetch()

BUG=667641
TEST=ImageResourceTest.FetchAllowPlaceholderThenDisallowPlaceholder
     ImageResourceTest.FetchAllowPlaceholderThenDisallowPlaceholderAfterLoaded
==========

to

==========
Do not reload in ImageResource::fetch()

This CL causes behavior changes and thus test failures in the following case:
(1) The previous ImageResource is a placeholder (m_isPlaceholder == true) and
(2) we call fetch() with non-placeholder request
(request.placeholderImageRequestType() != AllowPlaceholder).

Should the previous ImageResource be reloaded and updated with the full image?
- Previously, Yes: fetch() returns the previous ImageResource and it is
reloaded.
- Afer This CL, No: it makes (1) and (2) different ImageResources, and leaves
(1) a placeholder (not reloaded) while (2) gets fully loaded.

BUG=667641
TEST=ImageResourceTest.FetchAllowPlaceholderThenDisallowPlaceholder
     ImageResourceTest.FetchAllowPlaceholderThenDisallowPlaceholderAfterLoaded
==========

[hiroshige, 2017-02-11 00:27:26]

hiroshige@chromium.org changed reviewers:
+ sclittle@chromium.org

[hiroshige, 2017-02-11 00:27:26]

sclittle@, what do you think about the behavior change (see the CL description)?
If we'd like to keep the current behavior, then probably we have to keep
triggering reloading in ImageResource::fetch().

[sclittle, 2017-02-11 01:02:00]

On 2017/02/11 00:27:26, hiroshige wrote:
> sclittle@, what do you think about the behavior change (see the CL
description)?
> If we'd like to keep the current behavior, then probably we have to keep
> triggering reloading in ImageResource::fetch().

I'm not familiar with how canReuse() interacts with the MemoryCache - suppose a
page references an image 3 times, where the first fetch() is marked
AllowPlaceholder and the later two are marked DisallowPlaceholder.

With this change, AIUI the 2nd fetch() would not reuse the ImageResource from
the 1st fetch(), but would the 3rd fetch() reuse the ImageResource from the 2nd
fetch()?

If not, then this might be a problem, especially for images like spritesheets
that could be referenced lots of times on a single page.

[hiroshige, 2017-02-11 01:07:33]

On 2017/02/11 01:02:00, sclittle wrote:
> On 2017/02/11 00:27:26, hiroshige wrote:
> > sclittle@, what do you think about the behavior change (see the CL
> description)?
> > If we'd like to keep the current behavior, then probably we have to keep
> > triggering reloading in ImageResource::fetch().
> 
> I'm not familiar with how canReuse() interacts with the MemoryCache - suppose
a
> page references an image 3 times, where the first fetch() is marked
> AllowPlaceholder and the later two are marked DisallowPlaceholder.
> 
> With this change, AIUI the 2nd fetch() would not reuse the ImageResource from
> the 1st fetch(), but would the 3rd fetch() reuse the ImageResource from the
2nd
> fetch()?
Yes, I expect so: the 1st fetch() remains a placeholder, and the 2nd/3rd
fetch()es point to the same non-placeholder ImageResourceContent.
(TODO for me: add a unit test for such case)

[sclittle, 2017-02-11 01:11:23]

On 2017/02/11 01:07:33, hiroshige wrote:
> On 2017/02/11 01:02:00, sclittle wrote:
> > On 2017/02/11 00:27:26, hiroshige wrote:
> > > sclittle@, what do you think about the behavior change (see the CL
> > description)?
> > > If we'd like to keep the current behavior, then probably we have to keep
> > > triggering reloading in ImageResource::fetch().
> > 
> > I'm not familiar with how canReuse() interacts with the MemoryCache -
suppose
> a
> > page references an image 3 times, where the first fetch() is marked
> > AllowPlaceholder and the later two are marked DisallowPlaceholder.
> > 
> > With this change, AIUI the 2nd fetch() would not reuse the ImageResource
from
> > the 1st fetch(), but would the 3rd fetch() reuse the ImageResource from the
> 2nd
> > fetch()?
> Yes, I expect so: the 1st fetch() remains a placeholder, and the 2nd/3rd
> fetch()es point to the same non-placeholder ImageResourceContent.
> (TODO for me: add a unit test for such case)

OK, that should be fine then, so long as we have that unit test.

[hiroshige, 2017-03-01 20:09:57]

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

[commit-bot: I haz the power, 2017-03-01 20:10:51]

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

[hiroshige, 2017-03-01 20:17:45]

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

[commit-bot: I haz the power, 2017-03-01 20:18:34]

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

[hiroshige, 2017-03-01 20:21:49]

Description was changed from

==========
Do not reload in ImageResource::fetch()

This CL causes behavior changes and thus test failures in the following case:
(1) The previous ImageResource is a placeholder (m_isPlaceholder == true) and
(2) we call fetch() with non-placeholder request
(request.placeholderImageRequestType() != AllowPlaceholder).

Should the previous ImageResource be reloaded and updated with the full image?
- Previously, Yes: fetch() returns the previous ImageResource and it is
reloaded.
- Afer This CL, No: it makes (1) and (2) different ImageResources, and leaves
(1) a placeholder (not reloaded) while (2) gets fully loaded.

BUG=667641
TEST=ImageResourceTest.FetchAllowPlaceholderThenDisallowPlaceholder
     ImageResourceTest.FetchAllowPlaceholderThenDisallowPlaceholderAfterLoaded
==========

to

==========
Do not reload in ImageResource::fetch()

When there exists an placeholder ImageResource on MemoryCache and
a new non-placeholder request is created,
this CL starts a new non-placeholder loading with a new ImageResource,
instead of reusing the existing placeholder ImageResource and
triggering (non-placeholder) reloading on that.

This simplifies ImageResource::fetch() and removes one enum value for
controlling reload.

This leaves the existing placeholder ImageResource not to be reloaded
in such cases, and thus the unit tests are modified accordingly.

BUG=667641
TEST=ImageResourceTest.FetchAllowPlaceholderThenDisallowPlaceholder
     ImageResourceTest.FetchAllowPlaceholderThenDisallowPlaceholderAfterLoaded
==========

[hiroshige, 2017-03-01 20:23:00]

On 2017/02/11 01:11:23, sclittle wrote:
> On 2017/02/11 01:07:33, hiroshige wrote:
> > On 2017/02/11 01:02:00, sclittle wrote:
> > > On 2017/02/11 00:27:26, hiroshige wrote:
> > > > sclittle@, what do you think about the behavior change (see the CL
> > > description)?
> > > > If we'd like to keep the current behavior, then probably we have to keep
> > > > triggering reloading in ImageResource::fetch().
> > > 
> > > I'm not familiar with how canReuse() interacts with the MemoryCache -
> suppose
> > a
> > > page references an image 3 times, where the first fetch() is marked
> > > AllowPlaceholder and the later two are marked DisallowPlaceholder.
> > > 
> > > With this change, AIUI the 2nd fetch() would not reuse the ImageResource
> from
> > > the 1st fetch(), but would the 3rd fetch() reuse the ImageResource from
the
> > 2nd
> > > fetch()?
> > Yes, I expect so: the 1st fetch() remains a placeholder, and the 2nd/3rd
> > fetch()es point to the same non-placeholder ImageResourceContent.
> > (TODO for me: add a unit test for such case)
> 
> OK, that should be fine then, so long as we have that unit test.

Added that case to the unit tests, by adding third requests to the following
tests:
  ImageResourceTest.FetchAllowPlaceholderThenDisallowPlaceholder
  ImageResourceTest.FetchAllowPlaceholderThenDisallowPlaceholderAfterLoaded

PTAL.

[commit-bot: I haz the power, 2017-03-01 20:49:52]

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

[commit-bot: I haz the power, 2017-03-01 20:49:53]

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

[hiroshige, 2017-03-02 02:46:52]

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

[commit-bot: I haz the power, 2017-03-02 02:47:41]

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, 2017-03-02 06:53:03]

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

[commit-bot: I haz the power, 2017-03-02 06:53:04]

Dry run: This issue passed the CQ dry run.

[sclittle, 2017-03-07 22:40:26]

lgtm

[hiroshige, 2017-03-08 01:19:38]

The CQ bit was checked by hiroshige@chromium.org

[hiroshige, 2017-03-08 01:19:39]

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/2602793003/#ps60001
(title: "Rebase fix")

[commit-bot: I haz the power, 2017-03-08 01:20:51]

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, 2017-03-08 06:11:38]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1488935978929270,
"parent_rev": "7a2d9b79961a229fb72dc617928243eff22755c4", "commit_rev":
"79c699b99525ab50568821fce704976a9278e3fa"}

[commit-bot: I haz the power, 2017-03-08 06:13:00]

Description was changed from

==========
Do not reload in ImageResource::fetch()

When there exists an placeholder ImageResource on MemoryCache and
a new non-placeholder request is created,
this CL starts a new non-placeholder loading with a new ImageResource,
instead of reusing the existing placeholder ImageResource and
triggering (non-placeholder) reloading on that.

This simplifies ImageResource::fetch() and removes one enum value for
controlling reload.

This leaves the existing placeholder ImageResource not to be reloaded
in such cases, and thus the unit tests are modified accordingly.

BUG=667641
TEST=ImageResourceTest.FetchAllowPlaceholderThenDisallowPlaceholder
     ImageResourceTest.FetchAllowPlaceholderThenDisallowPlaceholderAfterLoaded
==========

to

==========
Do not reload in ImageResource::fetch()

When there exists an placeholder ImageResource on MemoryCache and
a new non-placeholder request is created,
this CL starts a new non-placeholder loading with a new ImageResource,
instead of reusing the existing placeholder ImageResource and
triggering (non-placeholder) reloading on that.

This simplifies ImageResource::fetch() and removes one enum value for
controlling reload.

This leaves the existing placeholder ImageResource not to be reloaded
in such cases, and thus the unit tests are modified accordingly.

BUG=667641
TEST=ImageResourceTest.FetchAllowPlaceholderThenDisallowPlaceholder
     ImageResourceTest.FetchAllowPlaceholderThenDisallowPlaceholderAfterLoaded

Review-Url: https://codereview.chromium.org/2602793003
Cr-Commit-Position: refs/heads/master@{#455390}
Committed:
https://chromium.googlesource.com/chromium/src/+/79c699b99525ab50568821fce704...
==========

[commit-bot: I haz the power, 2017-03-08 06:13:04]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/79c699b99525ab50568821fce704...