Refactor ImageLoader::UpdateFromElement(kUpdateForcedReload)

Refactor ImageLoader::UpdateFromElement(kUpdateForcedReload)

Previously, there were two cases when |loading_image_document_| is true:
Case 1. Initial ImageDocument loading. |image_| is not associated with
   ResourceLoader, and the response/data are supplied by ImageDocument
   instead.
Case 2. Reloading. |image_| is created normally via ResourceFetcher
   and thus is associated with ResourceLoader. This is same as
   non-ImageDocument image loading.

This CL clears |loading_image_document_| and
|image_resource_for_image_document_| in the second case, to
- Make |image_| always under control of ImageDocument if
  |loading_image_document_| is true.
- Avoid |image_| from being controlled by both of ImageDocument and
  ResourceLoader when |loading_image_document_| is false.
- Simplify the invariants and make the assertion in [1] hold.

[1] https://codereview.chromium.org/2859383002/

BUG=624697, 719759, 485295
Review-Url: https://codereview.chromium.org/2877583002
Cr-Commit-Position: refs/heads/master@{#471455}
Committed: https://chromium.googlesource.com/chromium/src/+/e263cd1373b7f330054bc1b252c19abf1ac9ba09

[hiroshige, 2017-05-10 21:46:46]

Description was changed from

==========
Refactor ImageLoader for HTMLImageElement::ForceReload() case

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

to

==========
Refactor ImageLoader for HTMLImageElement::ForceReload() case

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

[hiroshige, 2017-05-10 21:47:13]

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

[commit-bot: I haz the power, 2017-05-10 21:48:25]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[hiroshige, 2017-05-10 22:13:58]

Description was changed from

==========
Refactor ImageLoader for HTMLImageElement::ForceReload() case

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

to

==========
Refactor ImageLoader::UpdateFromElement(kUpdateForcedReload)

Previously, there were two cases when |loading_image_document_| is true:
Case 1. Initial ImageDocument loading. |image_| is not associated with
   ResourceLoader, and the response/data are supplied by ImageDocument
   instead.
Case 2. Reloading. |image_| is created normally via ResourceFetcher
   and thus is associated with ResourceLoader. This is the same as
   non-ImageDocument image loading.

This CL clears |loading_image_document_| and
|image_resource_for_image_document_| in the second case, to
- Make |image_| always under control of ImageDocument if
  |loading_image_document_| is true.
- Avoid |image_| from being controlled by both of ImageDocument and
  ResourceLoader when |loading_image_document_| is false.
- Simplify the invariants and make the assertion in [1] hold.

[1] https://codereview.chromium.org/2859383002/

BUG=624697, 719759, 485295
==========

[hiroshige, 2017-05-10 22:15:53]

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

[hiroshige, 2017-05-10 22:31:08]

Description was changed from

==========
Refactor ImageLoader::UpdateFromElement(kUpdateForcedReload)

Previously, there were two cases when |loading_image_document_| is true:
Case 1. Initial ImageDocument loading. |image_| is not associated with
   ResourceLoader, and the response/data are supplied by ImageDocument
   instead.
Case 2. Reloading. |image_| is created normally via ResourceFetcher
   and thus is associated with ResourceLoader. This is the same as
   non-ImageDocument image loading.

This CL clears |loading_image_document_| and
|image_resource_for_image_document_| in the second case, to
- Make |image_| always under control of ImageDocument if
  |loading_image_document_| is true.
- Avoid |image_| from being controlled by both of ImageDocument and
  ResourceLoader when |loading_image_document_| is false.
- Simplify the invariants and make the assertion in [1] hold.

[1] https://codereview.chromium.org/2859383002/

BUG=624697, 719759, 485295
==========

to

==========
Refactor ImageLoader::UpdateFromElement(kUpdateForcedReload)

Previously, there were two cases when |loading_image_document_| is true:
Case 1. Initial ImageDocument loading. |image_| is not associated with
   ResourceLoader, and the response/data are supplied by ImageDocument
   instead.
Case 2. Reloading. |image_| is created normally via ResourceFetcher
   and thus is associated with ResourceLoader. This is same as
   non-ImageDocument image loading.

This CL clears |loading_image_document_| and
|image_resource_for_image_document_| in the second case, to
- Make |image_| always under control of ImageDocument if
  |loading_image_document_| is true.
- Avoid |image_| from being controlled by both of ImageDocument and
  ResourceLoader when |loading_image_document_| is false.
- Simplify the invariants and make the assertion in [1] hold.

[1] https://codereview.chromium.org/2859383002/

BUG=624697, 719759, 485295
==========

[hiroshige, 2017-05-10 22:33:24]

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

[commit-bot: I haz the power, 2017-05-10 22:34:14]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[hiroshige, 2017-05-10 23:11:58]

hiroshige@chromium.org changed reviewers:
+ kinuko@chromium.org, kouhei@chromium.org, megjablon@chromium.org,
yhirano@chromium.org

[hiroshige, 2017-05-10 23:13:09]

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

[commit-bot: I haz the power, 2017-05-11 00:45:28]

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

[commit-bot: I haz the power, 2017-05-11 00:45:29]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[hiroshige, 2017-05-11 16:12:06]

PTAL. (bot failures look irrelevant)

[hiroshige, 2017-05-11 18:12:46]

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

[commit-bot: I haz the power, 2017-05-11 18:14:01]

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-05-11 19:26:42]

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

[commit-bot: I haz the power, 2017-05-11 19:26:43]

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

[kinuko, 2017-05-12 08:28:34]

lgtm

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

https://codereview.chromium.org/2877583002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/ImageLoader.cpp:429: // Prepares for
reloading ImageDocument.
nit: I think we generally use imperative format for the code comment (while
descriptive format for header comments), i.e. 'Prepare for ...'

(I think our internal style guide used to have this but I can't find this
anymore, so fyi/optional)

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

https://codereview.chromium.org/2877583002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/ImageLoader.h:106: //  
|image_resource_for_image_document_| is null.
Thanks for adding this comment!

[kinuko, 2017-05-12 08:28:34]

lgtm

[yhirano, 2017-05-12 10:38:11]

lgtm

[kouhei (in TOK), 2017-05-12 13:05:52]

lgtm

[hiroshige, 2017-05-12 17:17:41]

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

[commit-bot: I haz the power, 2017-05-12 17:18:36]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[hiroshige, 2017-05-12 20:00:23]

The CQ bit was unchecked by hiroshige@chromium.org

[hiroshige, 2017-05-12 20:00:28]

The CQ bit was checked by hiroshige@chromium.org

[hiroshige, 2017-05-12 20:00:28]

The patchset sent to the CQ was uploaded after l-g-t-m from kinuko@chromium.org,
kouhei@chromium.org, yhirano@chromium.org
Link to the patchset: https://codereview.chromium.org/2877583002/#ps80001
(title: "Rebase")

[commit-bot: I haz the power, 2017-05-12 20:00:50]

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-05-12 21:52:25]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1494619228208870,
"parent_rev": "7a442e87ccdbce2f4f5371131d01f343b2f6eab1", "commit_rev":
"e263cd1373b7f330054bc1b252c19abf1ac9ba09"}

[commit-bot: I haz the power, 2017-05-12 21:52:37]

Description was changed from

==========
Refactor ImageLoader::UpdateFromElement(kUpdateForcedReload)

Previously, there were two cases when |loading_image_document_| is true:
Case 1. Initial ImageDocument loading. |image_| is not associated with
   ResourceLoader, and the response/data are supplied by ImageDocument
   instead.
Case 2. Reloading. |image_| is created normally via ResourceFetcher
   and thus is associated with ResourceLoader. This is same as
   non-ImageDocument image loading.

This CL clears |loading_image_document_| and
|image_resource_for_image_document_| in the second case, to
- Make |image_| always under control of ImageDocument if
  |loading_image_document_| is true.
- Avoid |image_| from being controlled by both of ImageDocument and
  ResourceLoader when |loading_image_document_| is false.
- Simplify the invariants and make the assertion in [1] hold.

[1] https://codereview.chromium.org/2859383002/

BUG=624697, 719759, 485295
==========

to

==========
Refactor ImageLoader::UpdateFromElement(kUpdateForcedReload)

Previously, there were two cases when |loading_image_document_| is true:
Case 1. Initial ImageDocument loading. |image_| is not associated with
   ResourceLoader, and the response/data are supplied by ImageDocument
   instead.
Case 2. Reloading. |image_| is created normally via ResourceFetcher
   and thus is associated with ResourceLoader. This is same as
   non-ImageDocument image loading.

This CL clears |loading_image_document_| and
|image_resource_for_image_document_| in the second case, to
- Make |image_| always under control of ImageDocument if
  |loading_image_document_| is true.
- Avoid |image_| from being controlled by both of ImageDocument and
  ResourceLoader when |loading_image_document_| is false.
- Simplify the invariants and make the assertion in [1] hold.

[1] https://codereview.chromium.org/2859383002/

BUG=624697, 719759, 485295

Review-Url: https://codereview.chromium.org/2877583002
Cr-Commit-Position: refs/heads/master@{#471455}
Committed:
https://chromium.googlesource.com/chromium/src/+/e263cd1373b7f330054bc1b252c1...
==========

[commit-bot: I haz the power, 2017-05-12 21:52:38]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/e263cd1373b7f330054bc1b252c1...