Split ImageLoader::SetImage() and set flags consistently in tests

Split ImageLoader::SetImage() and set flags consistently in tests

Previously, ImageLoader::SetImage() is used in three ways:
1. SetImage(nullptr),
2. SetImage(non-null) for ImageDocument, and
3. SetImage(non-null) for unit tests for non-ImageDocument.
and SetImage() sets has_pending_load_event_ = false and
image_complete_ = true for all of these.

This flag setting is consistent for 1., but not for 3., and causes
assertion failures when we apply stronger assertions in [1].

This CL fixes this by splitting SetImage() for separate methods:
1. ClearImage(),
2. SetImageForImageDocument(), and
3. SetImageForTest(),
and introducing UpdateImageState() that sets:
- image_
- has_pending_load_event_
- image_complete_

This CL
- Doesn't change non-test behavior, i.e. keeps the behavior for
  Cases 1 and 2.
  Particularly, this leaves the flag values that are apparently
  inconsistent but needed for the current implementation in Case 2 in
  SetImageForImageDocument().
- Changes the test-only behavior of Case 3, to set
    has_pending_load_event_ = true and
    image_complete_ = false in SetImageForTest().
  This doesn't affect the tested behavior but fixes the assertion
  failures in [1].

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

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

[hiroshige, 2017-05-08 23:05:40]

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

[commit-bot: I haz the power, 2017-05-08 23:15:17]

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-08 23:20:28]

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

[commit-bot: I haz the power, 2017-05-08 23:35:35]

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-08 23:59:57]

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

[hiroshige, 2017-05-09 00:00:20]

Description was changed from

==========
Refactor ImageLoader::SetImage()

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

to

==========
Refactor ImageLoader::SetImage()

Previously, ImageLoader::SetImage() is used in three ways:
1. SetImage(nullptr),
2. SetImage(non-null) for ImageDocument, and
3. SetImage(non-null) for unit tests for non-ImageDocument.
and SetImage() sets has_pending_load_event_ = false and
image_complete_ = true for all of these.

This flag setting is consistent for 1., but not for 3., and causes
assertion failures when we apply stronger assertions in [1].

This CL fixes this by splitting SetImage() for separate methods:
1. ClearImage(),
2. SetImageForImageDocument(), and
3. SetImageForTest(),
and introducing SetImageInternal() that sets:
- image_
- has_pending_load_event_
- image_complete_

This CL
- Doesn't change non-test behavior, i.e. keeps the behavior for
  Cases 1 and 2.
- Changes the test-only behavior of Case 3, to set
    has_pending_load_event_ = true and
    image_complete_ = false in SetImageForTest().
  This doesn't affect the tested behavior but fixes the assertion
  failures in [1].

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

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

[hiroshige, 2017-05-09 00:00:20]

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

[hiroshige, 2017-05-09 00:04:56]

Description was changed from

==========
Refactor ImageLoader::SetImage()

Previously, ImageLoader::SetImage() is used in three ways:
1. SetImage(nullptr),
2. SetImage(non-null) for ImageDocument, and
3. SetImage(non-null) for unit tests for non-ImageDocument.
and SetImage() sets has_pending_load_event_ = false and
image_complete_ = true for all of these.

This flag setting is consistent for 1., but not for 3., and causes
assertion failures when we apply stronger assertions in [1].

This CL fixes this by splitting SetImage() for separate methods:
1. ClearImage(),
2. SetImageForImageDocument(), and
3. SetImageForTest(),
and introducing SetImageInternal() that sets:
- image_
- has_pending_load_event_
- image_complete_

This CL
- Doesn't change non-test behavior, i.e. keeps the behavior for
  Cases 1 and 2.
- Changes the test-only behavior of Case 3, to set
    has_pending_load_event_ = true and
    image_complete_ = false in SetImageForTest().
  This doesn't affect the tested behavior but fixes the assertion
  failures in [1].

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

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

to

==========
Split ImageLoader::SetImage() and set flags consistently in tests

Previously, ImageLoader::SetImage() is used in three ways:
1. SetImage(nullptr),
2. SetImage(non-null) for ImageDocument, and
3. SetImage(non-null) for unit tests for non-ImageDocument.
and SetImage() sets has_pending_load_event_ = false and
image_complete_ = true for all of these.

This flag setting is consistent for 1., but not for 3., and causes
assertion failures when we apply stronger assertions in [1].

This CL fixes this by splitting SetImage() for separate methods:
1. ClearImage(),
2. SetImageForImageDocument(), and
3. SetImageForTest(),
and introducing SetImageInternal() that sets:
- image_
- has_pending_load_event_
- image_complete_

This CL
- Doesn't change non-test behavior, i.e. keeps the behavior for
  Cases 1 and 2.
- Changes the test-only behavior of Case 3, to set
    has_pending_load_event_ = true and
    image_complete_ = false in SetImageForTest().
  This doesn't affect the tested behavior but fixes the assertion
  failures in [1].

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

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

[commit-bot: I haz the power, 2017-05-09 00:05:49]

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-09 00:07:45]

Description was changed from

==========
Split ImageLoader::SetImage() and set flags consistently in tests

Previously, ImageLoader::SetImage() is used in three ways:
1. SetImage(nullptr),
2. SetImage(non-null) for ImageDocument, and
3. SetImage(non-null) for unit tests for non-ImageDocument.
and SetImage() sets has_pending_load_event_ = false and
image_complete_ = true for all of these.

This flag setting is consistent for 1., but not for 3., and causes
assertion failures when we apply stronger assertions in [1].

This CL fixes this by splitting SetImage() for separate methods:
1. ClearImage(),
2. SetImageForImageDocument(), and
3. SetImageForTest(),
and introducing SetImageInternal() that sets:
- image_
- has_pending_load_event_
- image_complete_

This CL
- Doesn't change non-test behavior, i.e. keeps the behavior for
  Cases 1 and 2.
- Changes the test-only behavior of Case 3, to set
    has_pending_load_event_ = true and
    image_complete_ = false in SetImageForTest().
  This doesn't affect the tested behavior but fixes the assertion
  failures in [1].

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

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

to

==========
Split ImageLoader::SetImage() and set flags consistently in tests

Previously, ImageLoader::SetImage() is used in three ways:
1. SetImage(nullptr),
2. SetImage(non-null) for ImageDocument, and
3. SetImage(non-null) for unit tests for non-ImageDocument.
and SetImage() sets has_pending_load_event_ = false and
image_complete_ = true for all of these.

This flag setting is consistent for 1., but not for 3., and causes
assertion failures when we apply stronger assertions in [1].

This CL fixes this by splitting SetImage() for separate methods:
1. ClearImage(),
2. SetImageForImageDocument(), and
3. SetImageForTest(),
and introducing SetImageInternal() that sets:
- image_
- has_pending_load_event_
- image_complete_

This CL
- Doesn't change non-test behavior, i.e. keeps the behavior for
  Cases 1 and 2.
  Particularly, this leaves the flag values that are apparently
  inconsistent but needed for the current implementation in Case 2 in
  SetImageForImageDocument().
- Changes the test-only behavior of Case 3, to set
    has_pending_load_event_ = true and
    image_complete_ = false in SetImageForTest().
  This doesn't affect the tested behavior but fixes the assertion
  failures in [1].

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

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

[hiroshige, 2017-05-09 00:08:16]

PTAL.

[hiroshige, 2017-05-09 00:46:48]

hiroshige@chromium.org changed reviewers:
+ fs@opera.com

[hiroshige, 2017-05-09 00:48:54]

+fs@ if you interested.

A concern here is the names are confusing: SetImage.*() are
not simple setters.
ClearImage() corresponds to previous SetImage(nullptr), and
thus also not a simple resetter of image_.
Any suggestions?

[commit-bot: I haz the power, 2017-05-09 01:20:54]

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

[commit-bot: I haz the power, 2017-05-09 01:20:55]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[fs, 2017-05-09 08:29:56]

lgtm

On 2017/05/09 at 00:48:54, hiroshige wrote:
> +fs@ if you interested.
> 
> A concern here is the names are confusing: SetImage.*() are
> not simple setters.
> ClearImage() corresponds to previous SetImage(nullptr), and
> thus also not a simple resetter of image_.
> Any suggestions?

Thanks for doing this! I think this code is in dire need of some refactoring,
and regardless of naming this is an improvement...

As for the naming issue, I'd say it's probably fine to use Set/Clear prefixes
here even though they do more than set/clear something. Maybe eventually when a
reasonable structure has formed we can pick better names. For SetImageInternal
though maybe we could call it UpdateImageState or UpdateStateFromImage or
something like that. (Personally I think I would've kept the setting of
|has_pending_load_event_| out of it, because it feels like that is where the
different ways of setting an Image are diverging.)

https://codereview.chromium.org/2864253003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/HTMLImageElement.h (right):

https://codereview.chromium.org/2864253003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLImageElement.h:86: void
SetImageForTest(ImageResourceContent* i) {
Maybe rename |i| to something more appropriate here while we're at it...

[yhirano, 2017-05-09 08:44:47]

lgtm

[hiroshige, 2017-05-09 22:58:53]

Thanks!

> For SetImageInternal though maybe we could call it UpdateImageState or
UpdateStateFromImage or something like that.
UpdateImageState looks good. I'll rename.

> (Personally I think I would've kept the setting of
> |has_pending_load_event_| out of it, because it feels like that is where the
> different ways of setting an Image are diverging.)
It will be removed soon by https://codereview.chromium.org/2859093003.

[hiroshige, 2017-05-09 23:05:36]

Description was changed from

==========
Split ImageLoader::SetImage() and set flags consistently in tests

Previously, ImageLoader::SetImage() is used in three ways:
1. SetImage(nullptr),
2. SetImage(non-null) for ImageDocument, and
3. SetImage(non-null) for unit tests for non-ImageDocument.
and SetImage() sets has_pending_load_event_ = false and
image_complete_ = true for all of these.

This flag setting is consistent for 1., but not for 3., and causes
assertion failures when we apply stronger assertions in [1].

This CL fixes this by splitting SetImage() for separate methods:
1. ClearImage(),
2. SetImageForImageDocument(), and
3. SetImageForTest(),
and introducing SetImageInternal() that sets:
- image_
- has_pending_load_event_
- image_complete_

This CL
- Doesn't change non-test behavior, i.e. keeps the behavior for
  Cases 1 and 2.
  Particularly, this leaves the flag values that are apparently
  inconsistent but needed for the current implementation in Case 2 in
  SetImageForImageDocument().
- Changes the test-only behavior of Case 3, to set
    has_pending_load_event_ = true and
    image_complete_ = false in SetImageForTest().
  This doesn't affect the tested behavior but fixes the assertion
  failures in [1].

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

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

to

==========
Split ImageLoader::SetImage() and set flags consistently in tests

Previously, ImageLoader::SetImage() is used in three ways:
1. SetImage(nullptr),
2. SetImage(non-null) for ImageDocument, and
3. SetImage(non-null) for unit tests for non-ImageDocument.
and SetImage() sets has_pending_load_event_ = false and
image_complete_ = true for all of these.

This flag setting is consistent for 1., but not for 3., and causes
assertion failures when we apply stronger assertions in [1].

This CL fixes this by splitting SetImage() for separate methods:
1. ClearImage(),
2. SetImageForImageDocument(), and
3. SetImageForTest(),
and introducing UpdateImageState() that sets:
- image_
- has_pending_load_event_
- image_complete_

This CL
- Doesn't change non-test behavior, i.e. keeps the behavior for
  Cases 1 and 2.
  Particularly, this leaves the flag values that are apparently
  inconsistent but needed for the current implementation in Case 2 in
  SetImageForImageDocument().
- Changes the test-only behavior of Case 3, to set
    has_pending_load_event_ = true and
    image_complete_ = false in SetImageForTest().
  This doesn't affect the tested behavior but fixes the assertion
  failures in [1].

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

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

[hiroshige, 2017-05-09 23:07:57]

https://codereview.chromium.org/2864253003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/HTMLImageElement.h (right):

https://codereview.chromium.org/2864253003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLImageElement.h:86: void
SetImageForTest(ImageResourceContent* i) {
On 2017/05/09 08:29:55, fs wrote:
> Maybe rename |i| to something more appropriate here while we're at it...

Done.

[hiroshige, 2017-05-09 23:36:55]

Description was changed from

==========
Split ImageLoader::SetImage() and set flags consistently in tests

Previously, ImageLoader::SetImage() is used in three ways:
1. SetImage(nullptr),
2. SetImage(non-null) for ImageDocument, and
3. SetImage(non-null) for unit tests for non-ImageDocument.
and SetImage() sets has_pending_load_event_ = false and
image_complete_ = true for all of these.

This flag setting is consistent for 1., but not for 3., and causes
assertion failures when we apply stronger assertions in [1].

This CL fixes this by splitting SetImage() for separate methods:
1. ClearImage(),
2. SetImageForImageDocument(), and
3. SetImageForTest(),
and introducing UpdateImageState() that sets:
- image_
- has_pending_load_event_
- image_complete_

This CL
- Doesn't change non-test behavior, i.e. keeps the behavior for
  Cases 1 and 2.
  Particularly, this leaves the flag values that are apparently
  inconsistent but needed for the current implementation in Case 2 in
  SetImageForImageDocument().
- Changes the test-only behavior of Case 3, to set
    has_pending_load_event_ = true and
    image_complete_ = false in SetImageForTest().
  This doesn't affect the tested behavior but fixes the assertion
  failures in [1].

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

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

to

==========
Split ImageLoader::SetImage() and set flags consistently in tests

Previously, ImageLoader::SetImage() is used in three ways:
1. SetImage(nullptr),
2. SetImage(non-null) for ImageDocument, and
3. SetImage(non-null) for unit tests for non-ImageDocument.
and SetImage() sets has_pending_load_event_ = false and
image_complete_ = true for all of these.

This flag setting is consistent for 1., but not for 3., and causes
assertion failures when we apply stronger assertions in [1].

This CL fixes this by splitting SetImage() for separate methods:
1. ClearImage(),
2. SetImageForImageDocument(), and
3. SetImageForTest(),
and introducing UpdateImageState() that sets:
- image_
- has_pending_load_event_
- image_complete_

This CL
- Doesn't change non-test behavior, i.e. keeps the behavior for
  Cases 1 and 2.
  Particularly, this leaves the flag values that are apparently
  inconsistent but needed for the current implementation in Case 2 in
  SetImageForImageDocument().
- Changes the test-only behavior of Case 3, to set
    has_pending_load_event_ = true and
    image_complete_ = false in SetImageForTest().
  This doesn't affect the tested behavior but fixes the assertion
  failures in [1].

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

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

[kouhei (in TOK), 2017-05-10 16:17:28]

lgtm

[hiroshige, 2017-05-10 16:56:36]

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

[commit-bot: I haz the power, 2017-05-10 16:57:33]

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

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

[commit-bot: I haz the power, 2017-05-10 19:46: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_...)

[hiroshige, 2017-05-10 22:23:42]

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:24: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-05-10 22:24:52]

Minor update in ImageLoader::SetImageForImageDocument():
Updated the comment and added a DCHECK().

[commit-bot: I haz the power, 2017-05-11 00:36:55]

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

[commit-bot: I haz the power, 2017-05-11 00:36:57]

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, 2017-05-11 16:04:04]

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

[commit-bot: I haz the power, 2017-05-11 16:04:55]

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 17:57:25]

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

[commit-bot: I haz the power, 2017-05-11 17:57:27]

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, 2017-05-11 18:00:11]

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

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

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

[commit-bot: I haz the power, 2017-05-11 20:13: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_...)

[hiroshige, 2017-05-11 23:24:31]

The CQ bit was checked by hiroshige@chromium.org

[hiroshige, 2017-05-11 23:24:31]

The patchset sent to the CQ was uploaded after l-g-t-m from fs@opera.com,
yhirano@chromium.org, kouhei@chromium.org
Link to the patchset: https://codereview.chromium.org/2864253003/#ps100001
(title: "Update comment")

[commit-bot: I haz the power, 2017-05-11 23:25:43]

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 01:56:05]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1494545071086310,
"parent_rev": "229d9745b7d438ac230baa3f5c21945868e48256", "commit_rev":
"d5af742666d2acde551ac33e82f3d8662ffa0f15"}

[commit-bot: I haz the power, 2017-05-12 01:56:12]

Description was changed from

==========
Split ImageLoader::SetImage() and set flags consistently in tests

Previously, ImageLoader::SetImage() is used in three ways:
1. SetImage(nullptr),
2. SetImage(non-null) for ImageDocument, and
3. SetImage(non-null) for unit tests for non-ImageDocument.
and SetImage() sets has_pending_load_event_ = false and
image_complete_ = true for all of these.

This flag setting is consistent for 1., but not for 3., and causes
assertion failures when we apply stronger assertions in [1].

This CL fixes this by splitting SetImage() for separate methods:
1. ClearImage(),
2. SetImageForImageDocument(), and
3. SetImageForTest(),
and introducing UpdateImageState() that sets:
- image_
- has_pending_load_event_
- image_complete_

This CL
- Doesn't change non-test behavior, i.e. keeps the behavior for
  Cases 1 and 2.
  Particularly, this leaves the flag values that are apparently
  inconsistent but needed for the current implementation in Case 2 in
  SetImageForImageDocument().
- Changes the test-only behavior of Case 3, to set
    has_pending_load_event_ = true and
    image_complete_ = false in SetImageForTest().
  This doesn't affect the tested behavior but fixes the assertion
  failures in [1].

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

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

to

==========
Split ImageLoader::SetImage() and set flags consistently in tests

Previously, ImageLoader::SetImage() is used in three ways:
1. SetImage(nullptr),
2. SetImage(non-null) for ImageDocument, and
3. SetImage(non-null) for unit tests for non-ImageDocument.
and SetImage() sets has_pending_load_event_ = false and
image_complete_ = true for all of these.

This flag setting is consistent for 1., but not for 3., and causes
assertion failures when we apply stronger assertions in [1].

This CL fixes this by splitting SetImage() for separate methods:
1. ClearImage(),
2. SetImageForImageDocument(), and
3. SetImageForTest(),
and introducing UpdateImageState() that sets:
- image_
- has_pending_load_event_
- image_complete_

This CL
- Doesn't change non-test behavior, i.e. keeps the behavior for
  Cases 1 and 2.
  Particularly, this leaves the flag values that are apparently
  inconsistent but needed for the current implementation in Case 2 in
  SetImageForImageDocument().
- Changes the test-only behavior of Case 3, to set
    has_pending_load_event_ = true and
    image_complete_ = false in SetImageForTest().
  This doesn't affect the tested behavior but fixes the assertion
  failures in [1].

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

BUG=624697, 719759

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

[commit-bot: I haz the power, 2017-05-12 01:56:13]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/d5af742666d2acde551ac33e82f3...