Explicitly clear the image in the first updateImage() call

Explicitly clear the image in the first updateImage() call

Previously, updateImage() called in appendData()/finish()/error()
creates a new blink::Image only when the given SharedBuffer is non-empty and
|m_image| is nullptr.
However, this requires |m_image| to be cleared to enforce a new image
creation in LoFi/placeholder image reloading and would complicate
refactoring of LoFi reloading in https://codereview.chromium.org/2527353002.

This CL makes ImageResource to explicitly use ClearAndUpdateImage when calling
updateImage() for the first time for a corresponding response (or a
corresponding part in case of a multipart image).
As a result, we no longer need to clear |m_image| when reloading LoFi images.

BUG=667641

[hiroshige, 2016-12-05 08:59:24]

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

[commit-bot: I haz the power, 2016-12-05 08:59:34]

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 09:19:36]

Description was changed from

==========
Explicitly use ClearExistingImage for the first updateImage() call

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

to

==========
Explicitly use ClearExistingImage for the first updateImage() call

Previously, updateImage() called in appendData()/finish()/error()
creates a new blink::Image only when the given SharedBuffer is non-empty and
|m_image| is nullptr.
However, this requires |m_image| to be cleared to enforce a new image
creation in LoFi/placeholder image reloading and would complicate
refactoring of LoFi reloading in https://codereview.chromium.org/2527353002.

This CL makes ImageResource to explicitly use ClearExistingImage when calling
updateImage() for the first time for a corresponding response (or a
corresponding part in case of a multipart image).
As a result, we no longer need to clear |m_image| when reloading LoFi images.

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

[hiroshige, 2016-12-05 09:19:36]

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

[hiroshige, 2016-12-05 09:19:53]

PTAL.

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

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

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

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

[hajimehoshi, 2016-12-06 07:26:00]

https://codereview.chromium.org/2552653002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/fetch/ImageResource.h (right):

https://codereview.chromium.org/2552653002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/fetch/ImageResource.h:27: #include
"core/fetch/ImageResourceContent.h"
I couldn't find this file.

[hiroshige, 2016-12-07 07:12:43]

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:12: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, 2016-12-07 07:47:54]

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

[commit-bot: I haz the power, 2016-12-07 07:47:55]

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:02:53]

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:03: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, 2016-12-09 09:06:10]

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

[commit-bot: I haz the power, 2016-12-09 09:06:11]

Dry run: This issue passed the CQ dry run.

[yhirano, 2016-12-13 12:28:19]

lgtm

[hiroshige, 2016-12-16 19:01:44]

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:02:57]

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

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:34: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-16 22:45:03]

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

[commit-bot: I haz the power, 2016-12-16 22:45:03]

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

[hiroshige, 2016-12-16 22:51:45]

Description was changed from

==========
Explicitly use ClearExistingImage for the first updateImage() call

Previously, updateImage() called in appendData()/finish()/error()
creates a new blink::Image only when the given SharedBuffer is non-empty and
|m_image| is nullptr.
However, this requires |m_image| to be cleared to enforce a new image
creation in LoFi/placeholder image reloading and would complicate
refactoring of LoFi reloading in https://codereview.chromium.org/2527353002.

This CL makes ImageResource to explicitly use ClearExistingImage when calling
updateImage() for the first time for a corresponding response (or a
corresponding part in case of a multipart image).
As a result, we no longer need to clear |m_image| when reloading LoFi images.

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

to

==========
Explicitly clear the image in the first updateImage() call

Previously, updateImage() called in appendData()/finish()/error()
creates a new blink::Image only when the given SharedBuffer is non-empty and
|m_image| is nullptr.
However, this requires |m_image| to be cleared to enforce a new image
creation in LoFi/placeholder image reloading and would complicate
refactoring of LoFi reloading in https://codereview.chromium.org/2527353002.

This CL makes ImageResource to explicitly use ClearAndUpdateImage when calling
updateImage() for the first time for a corresponding response (or a
corresponding part in case of a multipart image).
As a result, we no longer need to clear |m_image| when reloading LoFi images.

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

[hiroshige, 2016-12-16 22:52:56]

Rebased after toyoshim's directory move and
https://codereview.chromium.org/2587503002.
PTAL again.

[hiroshige, 2016-12-16 22:53:06]

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

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

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-17 01:00:13]

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

[commit-bot: I haz the power, 2016-12-17 01:00:13]

Dry run: This issue passed the CQ dry run.

[kouhei (in TOK), 2016-12-19 01:32:04]

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

[kouhei (in TOK), 2016-12-19 01:32:04]

https://codereview.chromium.org/2552653002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/loader/resource/ImageResource.h (right):

https://codereview.chromium.org/2552653002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/resource/ImageResource.h:152: bool
m_isUpdateImageCalled = false;
This flag name is quite confusing, as onePartIn~() also updates this.
Would inverting this flag & renaming this to "m_shouldEnforceClearImage" make
sense?

Would you document somewhere why we have this
ImageResourceContent::ClearImageOption in the first place?

[kouhei (in TOK), 2016-12-19 01:33:50]

On 2016/12/19 01:32:04, kouhei wrote:
>
https://codereview.chromium.org/2552653002/diff/140001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/loader/resource/ImageResource.h (right):
> 
>
https://codereview.chromium.org/2552653002/diff/140001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/loader/resource/ImageResource.h:152: bool
> m_isUpdateImageCalled = false;
> This flag name is quite confusing, as onePartIn~() also updates this.
> Would inverting this flag & renaming this to "m_shouldEnforceClearImage" make
> sense?
> 
> Would you document somewhere why we have this
> ImageResourceContent::ClearImageOption in the first place?

<intent-to-unblock>
Feel free to ping me on Hangout/VC for this, or go ahead and land this CL given
lgt yhirano@.

[hiroshige, 2016-12-27 23:24:44]

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

[commit-bot: I haz the power, 2016-12-27 23:24: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-27 23:25:31]

https://codereview.chromium.org/2552653002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/loader/resource/ImageResource.h (right):

https://codereview.chromium.org/2552653002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/resource/ImageResource.h:152: bool
m_isUpdateImageCalled = false;
On 2016/12/19 01:32:04, kouhei wrote:
> This flag name is quite confusing, as onePartIn~() also updates this.
> Would inverting this flag & renaming this to "m_shouldEnforceClearImage" make
> sense?
Done in Patch Set 10.

> Would you document somewhere why we have this
> ImageResourceContent::ClearImageOption in the first place?

Done (in https://codereview.chromium.org/2587503002/ Patch Set 6).

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

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

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

Dry run: This issue passed the CQ dry run.

[kouhei (in TOK), 2016-12-30 01:45:56]

lgtm

[hajimehoshi, 2017-01-10 06:39:13]

lgtm

[hiroshige, 2017-01-25 02:53:10]

Thanks for reviewing.

However, I intended to use this CL in order to change the timing of clearing
m_image of ImageResourceContent in reloading, but I found a problem there, and I
changed my mind.
Now I'll keep the timing of clearing m_image there, and thus this CL is not
needed by that CL.

Therefore I close this CL uncommitted.