Phase II Step 3: Reload LoFi/placeholder images via new ImageResource

Phase II Step 3: Reload LoFi/placeholder images via new ImageResource

Design doc:
https://docs.google.com/document/d/1O-fB83mrE0B_V8gzXNqHgmRLCvstTB4MMi3RnVLr8bE/edit#heading=h.1rxkos5samv7

Previously, the same ImageResource is used for reloading a LoFi/placeholder
image, which makes Resource state control more complex and prevents more
flexible reloading.

In this CL, we
1. Creates a new ImageResource for reloading,
2. Detach the ImageResourceContent from the old ImageResource and
   attach it to the new ImageResource, and
3. Start normal resource loading for the new ImageResource.

We can simplify ImageResource state control because we no longer
"reload" ImageResources for LoFi/Placeholder images.
Particularly, this CL removes:
- Logic for suppressing imageNotifyFinished():
-- ImageResource::m_isSchedulingReload
-- ImageResourceInfo::isSchedulingReload()
-- ImageResourceInfo::schedulingReloadOrShouldReloadBrokenPlaceholder()
- Resource state rewinding:
  setStatus() call in reloadIfLoFiOrPlaceholderImage()
- Direct calls to ResourceFetcher::startLoad() and
  ResourceLoader::cancel() in
  ImageResource::reloadIfLoFiOrPlaceholderImage()
- Modifications to |m_resourceRequest| after the initial request:
-- Resource::setCachePolicyBypassingCache()
-- Resource::setPreviewsStateNoTransform()
-- Resource::clearRangeRequestHeader()

Because we use the same ImageResourceContent object, the users of
images that references ImageResourceContent can get the reloaded images
and ImageResourceObservers are notified of reloaded image updates.

Other changes:
- Additionally triggers LoFi reloading in ImageResource::decodeError().
- Added null checks of getContent() to allow detaching the content.
- Added |FetchRequest::m_enforceNewResource| in order to create a new
  ImageResource (that should be attached to the existing
  ImageResourceContent).

BUG=667641, 677188

[hiroshige, 2016-11-25 10:53:05]

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

[commit-bot: I haz the power, 2016-11-25 10:53:17]

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

[hiroshige, 2016-11-25 11:35:14]

Description was changed from

==========
Reload LoFi/placeholder images without rewinding Resource state

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

to

==========
Reload LoFi/placeholder images without rewinding Resource state

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

[commit-bot: I haz the power, 2016-11-25 11:54:33]

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

[commit-bot: I haz the power, 2016-11-25 11:54:34]

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

[hiroshige, 2016-11-27 08:34:39]

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

[commit-bot: I haz the power, 2016-11-27 08:34:49]

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-11-27 09:44:58]

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

[commit-bot: I haz the power, 2016-11-27 09:44:59]

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

[hiroshige, 2016-11-27 10:23:02]

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

[hiroshige, 2016-12-05 09:02:47]

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

[commit-bot: I haz the power, 2016-12-05 09:03:03]

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-05 10:10:46]

Description was changed from

==========
Reload LoFi/placeholder images without rewinding Resource state

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

to

==========
Reload LoFi/placeholder images without rewinding Resource state

Previously, the same ImageResource is used for reloading a LoFi/placeholder
image, which makes Resource state control more complex and prevents more
flexible reloading.

In this CL, we
1. Creates a new ImageResource for reloading,
2. Detach the ImageResourceContent from the old ImageResource and
   attach it to the new ImageResource, and
3. Start normal resource loading for the new ImageResource.

We can simplify ImageResource state control because we no longer "reload"
ImageResources for LoFi/Placeholder images.
For example, |m_isSchedulingReload| and shouldReloadBrokenPlaceholder() checks
in checkNotify()/didAddClient()/etc. are removed because we no longer have
race between finish notifications and initiating of reloads.

Because we use the same ImageResourceContent object, the users of images that
references ImageResourceContent can get the reloaded images and
ImageResourceObservers are notified of image updates due to reloading.

This CL also changes the timing of image updates for reloading, in order to
simplify the code structure.
Previously, the image is cleared and ImageResourceObservers are notified of
change when reloading is started.
After this CL, the image is not cleared when reloading is started, and is
replaced with a new (reloaded) image when ImageResource receives the first
chunk of data of the new image.

Other changes:
- Triggers LoFi reloading in ImageResourceContent::updateImage(), instead of
  in ResourceFetcher.
- Add null checks because getContent() can be null after detaching.

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

[hiroshige, 2016-12-05 10:10:46]

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

[hiroshige, 2016-12-05 10:11:00]

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

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

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-05 10:12:39]

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

[hiroshige, 2016-12-05 10:12:40]

PTAL.

This is the main (and currently the last) CL for refactoring LoFi reloading.

https://codereview.chromium.org/2527353002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/fetch/ImageResourceTest.cpp (left):

https://codereview.chromium.org/2527353002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/ImageResourceTest.cpp:453:
EXPECT_TRUE(client->notifyFinishedCalled());
I removed ResourceClient-related checks after reloading.

https://codereview.chromium.org/2527353002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/fetch/ImageResourceTest.cpp (right):

https://codereview.chromium.org/2527353002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/ImageResourceTest.cpp:440:
EXPECT_TRUE(content->hasImage());
Now we keep the old image until (partial) data of the new image is received.

https://codereview.chromium.org/2527353002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/ImageResourceTest.cpp:458: EXPECT_EQ(3,
client->imageChangedCount());
The image is now replaced with the new one and we get a notification.

https://codereview.chromium.org/2527353002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/ImageResourceTest.cpp:511:
EXPECT_TRUE(content->hasImage());
Now we keep the old image until (partial) data of the new image is received.

https://codereview.chromium.org/2527353002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/ImageResourceTest.cpp:534: EXPECT_EQ(2,
client->imageChangedCount());
The image is now replaced with the new one and we get a notification.

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

https://codereview.chromium.org/2527353002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/ImageResourceTest.cpp:555:
EXPECT_EQ(Resource::LoadError, cachedImage->getStatus());
Tests that the old ImageResource is cancelled after LoFi reloading.

[commit-bot: I haz the power, 2016-12-05 13:27:57]

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

[commit-bot: I haz the power, 2016-12-05 13:27:58]

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

[hiroshige, 2016-12-07 07:17:06]

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

[commit-bot: I haz the power, 2016-12-07 07:17:23]

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-07 07:18:20]

On 2016/12/05 10:12:40, hiroshige wrote:
> PTAL.
> 
> This is the main (and currently the last) CL for refactoring LoFi reloading.
> 
>
https://codereview.chromium.org/2527353002/diff/40001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/fetch/ImageResourceTest.cpp (left):
> 
>
https://codereview.chromium.org/2527353002/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/fetch/ImageResourceTest.cpp:453:
> EXPECT_TRUE(client->notifyFinishedCalled());
> I removed ResourceClient-related checks after reloading.
> 
>
https://codereview.chromium.org/2527353002/diff/40001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/fetch/ImageResourceTest.cpp (right):
> 
>
https://codereview.chromium.org/2527353002/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/fetch/ImageResourceTest.cpp:440:
> EXPECT_TRUE(content->hasImage());
> Now we keep the old image until (partial) data of the new image is received.
> 
>
https://codereview.chromium.org/2527353002/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/fetch/ImageResourceTest.cpp:458: EXPECT_EQ(3,
> client->imageChangedCount());
> The image is now replaced with the new one and we get a notification.
> 
>
https://codereview.chromium.org/2527353002/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/fetch/ImageResourceTest.cpp:511:
> EXPECT_TRUE(content->hasImage());
> Now we keep the old image until (partial) data of the new image is received.
> 
>
https://codereview.chromium.org/2527353002/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/fetch/ImageResourceTest.cpp:534: EXPECT_EQ(2,
> client->imageChangedCount());
> The image is now replaced with the new one and we get a notification.
> 
>
https://codereview.chromium.org/2527353002/diff/60001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/fetch/ImageResourceTest.cpp (right):
> 
>
https://codereview.chromium.org/2527353002/diff/60001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/fetch/ImageResourceTest.cpp:555:
> EXPECT_EQ(Resource::LoadError, cachedImage->getStatus());
> Tests that the old ImageResource is cancelled after LoFi reloading.

I found slight corner cases in this and depending CLs.
I'll refine the CLs next week.

[commit-bot: I haz the power, 2016-12-07 07:46:53]

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

[commit-bot: I haz the power, 2016-12-07 07:46:53]

Dry run: Try jobs failed on following builders:
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)

[hiroshige, 2016-12-09 07:14:07]

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

[commit-bot: I haz the power, 2016-12-09 07:14:32]

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-09 10:29:41]

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

[commit-bot: I haz the power, 2016-12-09 10:29:42]

Dry run: This issue passed the CQ dry run.

[hiroshige, 2016-12-16 19:04:04]

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

[commit-bot: I haz the power, 2016-12-16 19:04:59]

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-16 19:40:14]

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

[commit-bot: I haz the power, 2016-12-16 19:41:10]

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-16 20:17:46]

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

[commit-bot: I haz the power, 2016-12-16 20:17:47]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[hiroshige, 2016-12-16 20:43:03]

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

[commit-bot: I haz the power, 2016-12-16 20:44:09]

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-16 21:31:16]

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

[commit-bot: I haz the power, 2016-12-16 21:33:28]

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-16 23:01:09]

Description was changed from

==========
Reload LoFi/placeholder images without rewinding Resource state

Previously, the same ImageResource is used for reloading a LoFi/placeholder
image, which makes Resource state control more complex and prevents more
flexible reloading.

In this CL, we
1. Creates a new ImageResource for reloading,
2. Detach the ImageResourceContent from the old ImageResource and
   attach it to the new ImageResource, and
3. Start normal resource loading for the new ImageResource.

We can simplify ImageResource state control because we no longer "reload"
ImageResources for LoFi/Placeholder images.
For example, |m_isSchedulingReload| and shouldReloadBrokenPlaceholder() checks
in checkNotify()/didAddClient()/etc. are removed because we no longer have
race between finish notifications and initiating of reloads.

Because we use the same ImageResourceContent object, the users of images that
references ImageResourceContent can get the reloaded images and
ImageResourceObservers are notified of image updates due to reloading.

This CL also changes the timing of image updates for reloading, in order to
simplify the code structure.
Previously, the image is cleared and ImageResourceObservers are notified of
change when reloading is started.
After this CL, the image is not cleared when reloading is started, and is
replaced with a new (reloaded) image when ImageResource receives the first
chunk of data of the new image.

Other changes:
- Triggers LoFi reloading in ImageResourceContent::updateImage(), instead of
  in ResourceFetcher.
- Add null checks because getContent() can be null after detaching.

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

to

==========
Reload LoFi/placeholder images without rewinding Resource state

Previously, the same ImageResource is used for reloading a LoFi/placeholder
image, which makes Resource state control more complex and prevents more
flexible reloading.

In this CL, we
1. Creates a new ImageResource for reloading,
2. Detach the ImageResourceContent from the old ImageResource and
   attach it to the new ImageResource, and
3. Start normal resource loading for the new ImageResource.

We can simplify ImageResource state control because we no longer "reload"
ImageResources for LoFi/Placeholder images.
For example, |m_isSchedulingReload| and shouldReloadBrokenPlaceholder() checks
in checkNotify()/didAddClient()/etc. are removed because we no longer have
race between finish notifications and initiating of reloads.

Because we use the same ImageResourceContent object, the users of images that
references ImageResourceContent can get the reloaded images and
ImageResourceObservers are notified of image updates due to reloading.

This CL also changes the timing of image updates for reloading, in order to
simplify the code structure.
Previously, the image is cleared and ImageResourceObservers are notified of
change when reloading is started.
After this CL, the image is not cleared and ImageResourceObservers are not
notified when reloading is started.
Instead, the image is cleared and updated and the observers are notified
when the ImageResource receives the first chunk of data of the new image.

Other changes:
- Triggers LoFi reloading in ImageResourceContent::updateImage(), instead of
  in ResourceFetcher.
- Added null checks of getContent() to allow detaching the content.
- Added |FetchRequest::m_enforceNewResource| in order to create a new
  ImageResource (that is attached to the existing ImageResourceContent).

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

[hiroshige, 2016-12-16 23:01:48]

Rebasing and fixes are done. PTAL again.

[hiroshige, 2016-12-16 23:02:24]

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

[hiroshige, 2016-12-16 23:02:25]

+japhet@

[commit-bot: I haz the power, 2016-12-16 23:38:33]

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

[commit-bot: I haz the power, 2016-12-16 23:38:34]

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

[Nate Chapin, 2016-12-28 00:14:58]

I'm still trying to get a feel for the new ImageResource/ImageResourceContent
structure, so I apologize if any of these comments are clueless :)

https://codereview.chromium.org/2527353002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/fetch/Resource.h (right):

https://codereview.chromium.org/2527353002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/Resource.h:354: virtual Resource*
reloadIfLoFiOrPlaceholderImage(
Can we delete this from Resource entirely? It seems like it should be
non-virtual and just live on ImageResource.

https://codereview.chromium.org/2527353002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp (left):

https://codereview.chromium.org/2527353002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:1118: void
ResourceFetcher::handleLoadCompletion(Resource* resource) {
This helper seems like it's not useful if it's only the
context().didLoadResource() call.

https://codereview.chromium.org/2527353002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp (right):

https://codereview.chromium.org/2527353002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:785: if
(fetchRequest.enforceNewResource())
Adding yet another clause to this function makes me sad. Could ImageResource
override canReuse() and get the same effect?

https://codereview.chromium.org/2527353002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/loader/resource/ImageResource.cpp (right):

https://codereview.chromium.org/2527353002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/resource/ImageResource.cpp:177:
request.placeholderImageRequestType() != FetchRequest::AllowPlaceholder &&
This also seems like a case where we could have ImageResource override
canReuse() and not need to special case this.

https://codereview.chromium.org/2527353002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/loader/resource/ImageResourceContent.cpp
(right):

https://codereview.chromium.org/2527353002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/resource/ImageResourceContent.cpp:362: if
(m_info->reloadIfLoFiOrPlaceholderIfNeeded(fetcherForReload))
Is it critical that this happen before notifyObservers? Or in
ImageResourceContent, for that matter? It looks like that wasn't the way it used
to be?

I don't like having to plumb the ResourceFetcher all around to enable it to be
called here if there's a good alternative.

https://codereview.chromium.org/2527353002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/loader/resource/ImageResourceContent.h
(right):

https://codereview.chromium.org/2527353002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/resource/ImageResourceContent.h:143: const
ImageResource* resourceForTest() const;
This is a pretty bad layering violation, right? But at least it's test-only? :/

https://codereview.chromium.org/2527353002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/loader/resource/MockImageResourceClient.h
(right):

https://codereview.chromium.org/2527353002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/resource/MockImageResourceClient.h:16: //
in LoFi image reloading.
This seems like it's going to trip someone up at some point in the future...

[hiroshige, 2016-12-28 01:04:20]

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

[hiroshige, 2016-12-28 01:04:21]

Thanks for your comments!

As for test-related code, I'll try to further refactor ImageResourceTest and
remove dirty hacks and inconsistencies.
(Actually I didn't refactor the tests thoroughly in order not to block this CL
due to test-only refactoring, but it seems now I have more time to do that)

I'll handle remaining comments later.

https://codereview.chromium.org/2527353002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/fetch/Resource.h (right):

https://codereview.chromium.org/2527353002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/Resource.h:354: virtual Resource*
reloadIfLoFiOrPlaceholderImage(
On 2016/12/28 00:14:57, Nate Chapin (slow until Jan 3) wrote:
> Can we delete this from Resource entirely? It seems like it should be
> non-virtual and just live on ImageResource.

This virtual method would remain after this CL for
ResourceFetcher::reloadLoFiImages() to prevent direct dependencies from
ResourceFetcher to ImageResource.
(I also would like to remove this but currently have no good idea)

https://codereview.chromium.org/2527353002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp (left):

https://codereview.chromium.org/2527353002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:1118: void
ResourceFetcher::handleLoadCompletion(Resource* resource) {
On 2016/12/28 00:14:57, Nate Chapin (slow until Jan 3) wrote:
> This helper seems like it's not useful if it's only the
> context().didLoadResource() call.

+tyoshino@ (who introduced this method), is it fine with you to revert
handleLoadCompletion() back to didLoadResource()?

https://codereview.chromium.org/2527353002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/loader/resource/ImageResource.cpp (right):

https://codereview.chromium.org/2527353002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/resource/ImageResource.cpp:177:
request.placeholderImageRequestType() != FetchRequest::AllowPlaceholder &&
On 2016/12/28 00:14:57, Nate Chapin (slow until Jan 3) wrote:
> This also seems like a case where we could have ImageResource override
> canReuse() and not need to special case this.

Sounds good, I created a separate CL:
https://codereview.chromium.org/2602793003/ but it seems to cause behavior
changes. Let's discuss there.

[hiroshige, 2016-12-28 01:04:34]

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

[commit-bot: I haz the power, 2016-12-28 01:04:50]

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-28 02:30:47]

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

[commit-bot: I haz the power, 2016-12-28 02:30:48]

Dry run: This issue passed the CQ dry run.

[hiroshige, 2017-01-18 21:09:04]

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

[commit-bot: I haz the power, 2017-01-18 21:10:12]

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-01-18 22:45:12]

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

[commit-bot: I haz the power, 2017-01-18 22:45:13]

Dry run: This issue passed the CQ dry run.

[hiroshige, 2017-01-18 23:16:48]

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

[commit-bot: I haz the power, 2017-01-18 23:17:47]

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-01-19 00:21:27]

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

[commit-bot: I haz the power, 2017-01-19 00:21:28]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[hiroshige, 2017-01-19 02:46:43]

Patchset #15 (id:280001) has been deleted

[hiroshige, 2017-01-19 02:53:41]

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

[commit-bot: I haz the power, 2017-01-19 02:54:01]

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

[hiroshige, 2017-01-19 02:54:07]

Patchset #16 (id:320001) has been deleted

[hiroshige, 2017-01-19 02:54:36]

Patchset #15 (id:300001) has been deleted

[hiroshige, 2017-01-19 02:59:45]

Patchset #15 (id:340001) has been deleted

[hiroshige, 2017-01-19 02:59:51]

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

[commit-bot: I haz the power, 2017-01-19 03:00:18]

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-01-19 03:02:55]

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

[commit-bot: I haz the power, 2017-01-19 03:02:56]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[hiroshige, 2017-01-19 23:33:28]

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

[commit-bot: I haz the power, 2017-01-19 23:34:34]

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-01-20 01:18:26]

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

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

Dry run: This issue passed the CQ dry run.

[hiroshige, 2017-01-21 01:40:20]

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

[commit-bot: I haz the power, 2017-01-21 01:40:50]

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-01-21 02:06:44]

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

[commit-bot: I haz the power, 2017-01-21 02:06:44]

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

[hiroshige, 2017-01-21 03:12:35]

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

[commit-bot: I haz the power, 2017-01-21 03:12:58]

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

[hiroshige, 2017-01-21 03:15:21]

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

[commit-bot: I haz the power, 2017-01-21 03:15:37]

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

[hiroshige, 2017-01-21 03:16:33]

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

[commit-bot: I haz the power, 2017-01-21 03:16:53]

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

[hiroshige, 2017-01-21 03:46:01]

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

[commit-bot: I haz the power, 2017-01-21 03:46: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, 2017-01-21 05:30:31]

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

[commit-bot: I haz the power, 2017-01-21 05:30:32]

Dry run: This issue passed the CQ dry run.

[hiroshige, 2017-01-24 23:48:20]

Description was changed from

==========
Reload LoFi/placeholder images without rewinding Resource state

Previously, the same ImageResource is used for reloading a LoFi/placeholder
image, which makes Resource state control more complex and prevents more
flexible reloading.

In this CL, we
1. Creates a new ImageResource for reloading,
2. Detach the ImageResourceContent from the old ImageResource and
   attach it to the new ImageResource, and
3. Start normal resource loading for the new ImageResource.

We can simplify ImageResource state control because we no longer "reload"
ImageResources for LoFi/Placeholder images.
For example, |m_isSchedulingReload| and shouldReloadBrokenPlaceholder() checks
in checkNotify()/didAddClient()/etc. are removed because we no longer have
race between finish notifications and initiating of reloads.

Because we use the same ImageResourceContent object, the users of images that
references ImageResourceContent can get the reloaded images and
ImageResourceObservers are notified of image updates due to reloading.

This CL also changes the timing of image updates for reloading, in order to
simplify the code structure.
Previously, the image is cleared and ImageResourceObservers are notified of
change when reloading is started.
After this CL, the image is not cleared and ImageResourceObservers are not
notified when reloading is started.
Instead, the image is cleared and updated and the observers are notified
when the ImageResource receives the first chunk of data of the new image.

Other changes:
- Triggers LoFi reloading in ImageResourceContent::updateImage(), instead of
  in ResourceFetcher.
- Added null checks of getContent() to allow detaching the content.
- Added |FetchRequest::m_enforceNewResource| in order to create a new
  ImageResource (that is attached to the existing ImageResourceContent).

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

to

==========
Phase II Step 3: Reload LoFi/placeholder images via new ImageResource

Previously, the same ImageResource is used for reloading a LoFi/placeholder
image, which makes Resource state control more complex and prevents more
flexible reloading.

In this CL, we
1. Creates a new ImageResource for reloading,
2. Detach the ImageResourceContent from the old ImageResource and
   attach it to the new ImageResource, and
3. Start normal resource loading for the new ImageResource.

We can simplify ImageResource state control because we no longer "reload"
ImageResources for LoFi/Placeholder images.
For example, |m_isSchedulingReload| and shouldReloadBrokenPlaceholder() checks
in checkNotify()/didAddClient()/etc. are removed because we no longer have
race between finish notifications and initiating of reloads.

Because we use the same ImageResourceContent object, the users of images that
references ImageResourceContent can get the reloaded images and
ImageResourceObservers are notified of image updates due to reloading.

This CL also changes the timing of image updates for reloading, in order to
simplify the code structure.
Previously, the image is cleared and ImageResourceObservers are notified of
change when reloading is started.
After this CL, the image is not cleared and ImageResourceObservers are not
notified when reloading is started.
Instead, the image is cleared and updated and the observers are notified
when the ImageResource receives the first chunk of data of the new image.

Other changes:
- Triggers LoFi reloading in ImageResourceContent::updateImage(), instead of
  in ResourceFetcher.
- Added null checks of getContent() to allow detaching the content.
- Added |FetchRequest::m_enforceNewResource| in order to create a new
  ImageResource (that is attached to the existing ImageResourceContent).

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

[hiroshige, 2017-01-25 01:45:18]

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

[commit-bot: I haz the power, 2017-01-25 01:46:06]

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

[hiroshige, 2017-01-25 02:00:50]

Description was changed from

==========
Phase II Step 3: Reload LoFi/placeholder images via new ImageResource

Previously, the same ImageResource is used for reloading a LoFi/placeholder
image, which makes Resource state control more complex and prevents more
flexible reloading.

In this CL, we
1. Creates a new ImageResource for reloading,
2. Detach the ImageResourceContent from the old ImageResource and
   attach it to the new ImageResource, and
3. Start normal resource loading for the new ImageResource.

We can simplify ImageResource state control because we no longer "reload"
ImageResources for LoFi/Placeholder images.
For example, |m_isSchedulingReload| and shouldReloadBrokenPlaceholder() checks
in checkNotify()/didAddClient()/etc. are removed because we no longer have
race between finish notifications and initiating of reloads.

Because we use the same ImageResourceContent object, the users of images that
references ImageResourceContent can get the reloaded images and
ImageResourceObservers are notified of image updates due to reloading.

This CL also changes the timing of image updates for reloading, in order to
simplify the code structure.
Previously, the image is cleared and ImageResourceObservers are notified of
change when reloading is started.
After this CL, the image is not cleared and ImageResourceObservers are not
notified when reloading is started.
Instead, the image is cleared and updated and the observers are notified
when the ImageResource receives the first chunk of data of the new image.

Other changes:
- Triggers LoFi reloading in ImageResourceContent::updateImage(), instead of
  in ResourceFetcher.
- Added null checks of getContent() to allow detaching the content.
- Added |FetchRequest::m_enforceNewResource| in order to create a new
  ImageResource (that is attached to the existing ImageResourceContent).

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

to

==========
Phase II Step 3: Reload LoFi/placeholder images via new ImageResource

Previously, the same ImageResource is used for reloading a LoFi/placeholder
image, which makes Resource state control more complex and prevents more
flexible reloading.

In this CL, we
1. Creates a new ImageResource for reloading,
2. Detach the ImageResourceContent from the old ImageResource and
   attach it to the new ImageResource, and
3. Start normal resource loading for the new ImageResource.

We can simplify ImageResource state control because we no longer "reload"
ImageResources for LoFi/Placeholder images.
For example, |m_isSchedulingReload| and shouldReloadBrokenPlaceholder() checks
in checkNotify()/didAddClient()/etc. are removed because we no longer have
race between finish notifications and initiating of reloads.

Because we use the same ImageResourceContent object, the users of images that
references ImageResourceContent can get the reloaded images and
ImageResourceObservers are notified of image updates due to reloading.

This CL also changes the timing of image updates for reloading, in order to
simplify the code structure.
Previously, the image is cleared and ImageResourceObservers are notified of
change when reloading is started.
After this CL, the image is not cleared and ImageResourceObservers are not
notified when reloading is started.
Instead, the image is cleared and updated and the observers are notified
when the ImageResource receives the first chunk of data of the new image.

Other changes:
- Triggers LoFi reloading in ImageResourceContent::updateImage(), instead of
  in ResourceFetcher.
- Added null checks of getContent() to allow detaching the content.
- Added |FetchRequest::m_enforceNewResource| in order to create a new
  ImageResource (that is attached to the existing ImageResourceContent).

Design doc:
https://docs.google.com/document/d/1O-fB83mrE0B_V8gzXNqHgmRLCvstTB4MMi3RnVLr8...

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

[hiroshige, 2017-01-25 02:01:49]

I updated the design doc (Phase II):
https://docs.google.com/document/d/1O-fB83mrE0B_V8gzXNqHgmRLCvstTB4MMi3RnVLr8...
and updated this CL.

https://codereview.chromium.org/2527353002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp (left):

https://codereview.chromium.org/2527353002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:1118: void
ResourceFetcher::handleLoadCompletion(Resource* resource) {
On 2016/12/28 01:04:21, hiroshige wrote:
> On 2016/12/28 00:14:57, Nate Chapin (slow until Jan 3) wrote:
> > This helper seems like it's not useful if it's only the
> > context().didLoadResource() call.
> 
> +tyoshino@ (who introduced this method), is it fine with you to revert
> handleLoadCompletion() back to didLoadResource()?

I reverted this change (while I was removing reload-initiating code from
ImageResourceContent) and thus we no longer have to handle this issue.

https://codereview.chromium.org/2527353002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp (right):

https://codereview.chromium.org/2527353002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:785: if
(fetchRequest.enforceNewResource())
On 2016/12/28 00:14:57, Nate Chapin wrote:
> Adding yet another clause to this function makes me sad. Could ImageResource
> override canReuse() and get the same effect?

ACK, I'll handle this.

https://codereview.chromium.org/2527353002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/loader/resource/ImageResourceContent.cpp
(right):

https://codereview.chromium.org/2527353002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/resource/ImageResourceContent.cpp:362: if
(m_info->reloadIfLoFiOrPlaceholderIfNeeded(fetcherForReload))
On 2016/12/28 00:14:57, Nate Chapin wrote:
> It looks like that wasn't the way it used
> to be?
> Is it critical that this happen before notifyObservers? Or in
> ImageResourceContent, for that matter? 
> 
> I don't like having to plumb the ResourceFetcher all around to enable it to be
> called here if there's a good alternative.

This is to keep the current behavior of
"ImageResourceObserver::imageNotifyFinished() is not called when Placeholder is
reloaded due to DecodeError".

See the design doc
https://docs.google.com/document/d/1O-fB83mrE0B_V8gzXNqHgmRLCvstTB4MMi3RnVLr8...
(Phase II, Step 3, "Cleanup reloading around DecodeError") for more details
about timing dependencies around invoking reloading.

In the latest Patch Set, I moved reloading from here (and thus I removed the
plumbing between ImageResource and ImageResourceContent -- I agree this was not
clean) to ImageResource::decodeError().
Still we have the plumbing ResourceLoader::reloadIfLoFiOrPlaceholderImage() just
to get a reference to ResourceFetcher though.

Any better thoughts for handling this timing dependency are welcome.

https://codereview.chromium.org/2527353002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/loader/resource/ImageResourceContent.h
(right):

https://codereview.chromium.org/2527353002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/resource/ImageResourceContent.h:143: const
ImageResource* resourceForTest() const;
On 2016/12/28 00:14:58, Nate Chapin wrote:
> This is a pretty bad layering violation, right? But at least it's test-only?
:/

Yes, bad violation.
This is currently needed for tests to retrieve the new ImageResource used for
reloading, because reloading can be initiated inside
ResourceLoader::didReveiveData(), and thus tests don't have direct access to
reloading interface.

I removed this by retrieving the new ImageResource for reloading from
ResourceFetcher::cachedResource() in the unit tests.
This is not super clean, but this doesn't affect non-test code.

https://codereview.chromium.org/2527353002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/loader/resource/MockImageResourceClient.h
(right):

https://codereview.chromium.org/2527353002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/resource/MockImageResourceClient.h:16: //
in LoFi image reloading.
On 2016/12/28 00:14:58, Nate Chapin (slow until Jan 3) wrote:
> This seems like it's going to trip someone up at some point in the future...

I replaced MockImageResourceClient with MockImageResourceObserver and makes the
tests observer-based in https://codereview.chromium.org/2607023002, so this
comment no longer applies (and is deleted).

[commit-bot: I haz the power, 2017-01-25 02:06:39]

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

[commit-bot: I haz the power, 2017-01-25 02:06:40]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)

[hiroshige, 2017-01-25 02:40:16]

Description was changed from

==========
Phase II Step 3: Reload LoFi/placeholder images via new ImageResource

Previously, the same ImageResource is used for reloading a LoFi/placeholder
image, which makes Resource state control more complex and prevents more
flexible reloading.

In this CL, we
1. Creates a new ImageResource for reloading,
2. Detach the ImageResourceContent from the old ImageResource and
   attach it to the new ImageResource, and
3. Start normal resource loading for the new ImageResource.

We can simplify ImageResource state control because we no longer "reload"
ImageResources for LoFi/Placeholder images.
For example, |m_isSchedulingReload| and shouldReloadBrokenPlaceholder() checks
in checkNotify()/didAddClient()/etc. are removed because we no longer have
race between finish notifications and initiating of reloads.

Because we use the same ImageResourceContent object, the users of images that
references ImageResourceContent can get the reloaded images and
ImageResourceObservers are notified of image updates due to reloading.

This CL also changes the timing of image updates for reloading, in order to
simplify the code structure.
Previously, the image is cleared and ImageResourceObservers are notified of
change when reloading is started.
After this CL, the image is not cleared and ImageResourceObservers are not
notified when reloading is started.
Instead, the image is cleared and updated and the observers are notified
when the ImageResource receives the first chunk of data of the new image.

Other changes:
- Triggers LoFi reloading in ImageResourceContent::updateImage(), instead of
  in ResourceFetcher.
- Added null checks of getContent() to allow detaching the content.
- Added |FetchRequest::m_enforceNewResource| in order to create a new
  ImageResource (that is attached to the existing ImageResourceContent).

Design doc:
https://docs.google.com/document/d/1O-fB83mrE0B_V8gzXNqHgmRLCvstTB4MMi3RnVLr8...

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

to

==========
Phase II Step 3: Reload LoFi/placeholder images via new ImageResource

Design doc:
https://docs.google.com/document/d/1O-fB83mrE0B_V8gzXNqHgmRLCvstTB4MMi3RnVLr8...

Previously, the same ImageResource is used for reloading a LoFi/placeholder
image, which makes Resource state control more complex and prevents more
flexible reloading.

In this CL, we
1. Creates a new ImageResource for reloading,
2. Detach the ImageResourceContent from the old ImageResource and
   attach it to the new ImageResource, and
3. Start normal resource loading for the new ImageResource.

We can simplify ImageResource state control because we no longer
"reload" ImageResources for LoFi/Placeholder images.
Particularly, this CL removes:
- Logic for suppressing imageNotifyFinished():
-- ImageResource::m_isSchedulingReload
-- ImageResourceInfo::isSchedulingReload()
-- ImageResourceInfo::schedulingReloadOrShouldReloadBrokenPlaceholder()
- Resource state rewinding:
  setStatus() call in reloadIfLoFiOrPlaceholderImage()
- Direct calls to ResourceFetcher::startLoad() and
  ResourceLoader::cancel() in
  ImageResource::reloadIfLoFiOrPlaceholderImage()
- Modifications to |m_resourceRequest| after the initial request:
-- Resource::setCachePolicyBypassingCache()
-- Resource::setPreviewsStateNoTransform()
-- Resource::clearRangeRequestHeader()

Because we use the same ImageResourceContent object, the users of
images that references ImageResourceContent can get the reloaded images
and ImageResourceObservers are notified of reloaded image updates.

Other changes:
- Additionally triggers LoFi reloading in ImageResource::decodeError().
- Added null checks of getContent() to allow detaching the content.
- Added |FetchRequest::m_enforceNewResource| in order to create a new
  ImageResource (that should be attached to the existing
  ImageResourceContent).

BUG=667641, 677188
==========

[hiroshige, 2017-01-25 02:47:18]

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

[commit-bot: I haz the power, 2017-01-25 02:48:09]

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-01-25 03:07:22]

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

[commit-bot: I haz the power, 2017-01-25 03:07:23]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[hiroshige, 2017-02-11 00:19:40]

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

[commit-bot: I haz the power, 2017-02-11 00:20:28]

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

[hiroshige, 2017-02-11 00:41:06]

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

[commit-bot: I haz the power, 2017-02-11 00:41:46]

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-02-11 01:18:02]

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

[commit-bot: I haz the power, 2017-02-11 01:18:03]

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

[hiroshige, 2017-03-08 19:06:07]

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

[commit-bot: I haz the power, 2017-03-08 19:06:56]

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-08 20:07:06]

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

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

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

[hiroshige, 2017-03-09 21:13:42]

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

[commit-bot: I haz the power, 2017-03-09 21:14:08]

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-09 23:34:47]

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

[commit-bot: I haz the power, 2017-03-09 23:34:48]

Dry run: This issue passed the CQ dry run.

[hiroshige, 2017-03-13 18:10:50]

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

[commit-bot: I haz the power, 2017-03-13 18:11:23]

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-13 18:52:54]

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

[commit-bot: I haz the power, 2017-03-13 18:53:26]

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-13 20:14:48]

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

[commit-bot: I haz the power, 2017-03-13 20:14:49]

Dry run: This issue passed the CQ dry run.

[hiroshige, 2017-03-13 21:10:52]

Patchset #31 (id:680001) has been deleted

[hiroshige, 2017-03-13 21:18:31]

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

[hiroshige, 2017-03-13 21:18:42]

Patchset #32 (id:720001) has been deleted

[commit-bot: I haz the power, 2017-03-13 21:19:16]

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-13 21:27:05]

Sorry for delay. I think I handled all review comments. PTAL again.

I noticed calling ResourceFetcher::requestResource() with
Resource::m_resourceRequest causes double application of the modifications to
ResourceRequest in requestResource() (e.g. adding headers twice, when the first
request starts and when the reloading starts).
I think we should figure out what is the clear way to initiate reloading, but
for now, I created ImageResource, added it to MemoryCache/m_documentResources
and called startLoad() manually in
ImageResource::reloadIfLoFiOrPlaceholderImage().

https://codereview.chromium.org/2527353002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp (right):

https://codereview.chromium.org/2527353002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:785: if
(fetchRequest.enforceNewResource())
On 2017/01/25 02:01:49, hiroshige wrote:
> On 2016/12/28 00:14:57, Nate Chapin wrote:
> > Adding yet another clause to this function makes me sad. Could ImageResource
> > override canReuse() and get the same effect?
> 
> ACK, I'll handle this.

Removed.

[hiroshige, 2017-03-13 21:29:16]

https://codereview.chromium.org/2527353002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp (right):

https://codereview.chromium.org/2527353002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:785: if
(fetchRequest.enforceNewResource())
On 2017/03/13 21:27:04, hiroshige wrote:
> On 2017/01/25 02:01:49, hiroshige wrote:
> > On 2016/12/28 00:14:57, Nate Chapin wrote:
> > > Adding yet another clause to this function makes me sad. Could
ImageResource
> > > override canReuse() and get the same effect?
> > 
> > ACK, I'll handle this.
> 
> Removed.

In Patch Set 31, I merged FetchRequest::enforceNewResource() into
placeholderImageRequestType() and put the condition to ImageResource::canReuse()
to avoid a new clause in determineRevalidationPolicy().

However in Patch Set 32, as commented above, I use startLoad() directly and
bypass requestResource() when reloading.
Therefore, this condition is not needed in Patch Set 32 and thus it is simply
removed.

[commit-bot: I haz the power, 2017-03-13 23:04:33]

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

[commit-bot: I haz the power, 2017-03-13 23:04:34]

Dry run: This issue passed the CQ dry run.

[kouhei (in TOK), 2017-03-15 10:25:28]

mostly lg % ResourceFetcher::m_documentResources

https://codereview.chromium.org/2527353002/diff/740001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/loader/resource/ImageResource.cpp (right):

https://codereview.chromium.org/2527353002/diff/740001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/resource/ImageResource.cpp:266: if
(data())
Is this when we tried to access ::resourceBuffer while image is being loaded?

https://codereview.chromium.org/2527353002/diff/740001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/resource/ImageResource.cpp:459: bool
hasObservers = getContent() && getContent()->hasObservers();
Optional: contentHadObservers?

https://codereview.chromium.org/2527353002/diff/740001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/loader/fetch/ResourceFetcher.cpp
(right):

https://codereview.chromium.org/2527353002/diff/740001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/loader/fetch/ResourceFetcher.cpp:1342:
resource->reloadIfLoFiOrPlaceholderImage(this, Resource::kReloadAlways);
or should we replace m_documentResources using the return value?
Are we sure that we don't update m_documentResources set while iterating? or is
it safe?

[hiroshige, 2017-03-15 19:03:31]

https://codereview.chromium.org/2527353002/diff/740001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/loader/resource/ImageResource.cpp (right):

https://codereview.chromium.org/2527353002/diff/740001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/resource/ImageResource.cpp:266: if
(data())
On 2017/03/15 10:25:28, kouhei wrote:
> Is this when we tried to access ::resourceBuffer while image is being loaded?
Yes.

(Probably we check getContent() first for consistency; anyway I expect data() is
null when getContent() is null)

https://codereview.chromium.org/2527353002/diff/740001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/loader/fetch/ResourceFetcher.cpp
(right):

https://codereview.chromium.org/2527353002/diff/740001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/loader/fetch/ResourceFetcher.cpp:1342:
resource->reloadIfLoFiOrPlaceholderImage(this, Resource::kReloadAlways);
On 2017/03/15 10:25:28, kouhei wrote:
> or should we replace m_documentResources using the return value?
> Are we sure that we don't update m_documentResources set while iterating? or
is
> it safe?
In fact I am sure that we update |m_documentResources| upon reloading.
(Previously this is not needed because reloading uses the same Resource object,
but now we replace old references to ImageResource from
MemoryCache/m_documentResources with new one)

I think updates to m_documentResources should be done within reloading
(especially when we possibly call ResourceFetcher::requestResource() for
reloading possibly in the future, which updates m_documentResources), so I'll
update the code here to be safe with updating while iterating.

[yhirano, 2017-03-17 13:09:35]

https://codereview.chromium.org/2527353002/diff/740001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/loader/resource/ImageResource.cpp (right):

https://codereview.chromium.org/2527353002/diff/740001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/resource/ImageResource.cpp:248: // If
possible, delay the resetting until back at the event loop. Doing so
This comment should be inside of the outer if statement.

https://codereview.chromium.org/2527353002/diff/740001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/resource/ImageResource.cpp:483: if
(!fetcher)
Is passing nullptr valid?

https://codereview.chromium.org/2527353002/diff/740001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/loader/fetch/ResourceFetcher.cpp
(right):

https://codereview.chromium.org/2527353002/diff/740001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/loader/fetch/ResourceFetcher.cpp:1339: for
(auto& documentResource : m_documentResources) {
What is the intention of this change?

[hiroshige, 2017-03-21 07:10:21]

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

[commit-bot: I haz the power, 2017-03-21 07:10:36]

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-21 08:38:53]

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

[commit-bot: I haz the power, 2017-03-21 08:38:55]

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