Phase II Step 2: Remove setIsPlaceholder() in updateImage()

Phase II Step 2: Remove setIsPlaceholder() in updateImage()

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

Previsouly, |m_isPlaceholder| is cleared in updateImage(), if:
- isEntireResource() is true, AND
- (1) The HTTP status code is not 4XX or 5XX OR
  (2) the response has no DecodeError,
to prevent creating of placeholder and reloading.

However, this causes timing and code dependencies from
ImageResourceContent::updateImage()
to |ImageResource::m_isPlaceholder|
and ImageResource::reloadIfLoFiOrPlaceholderImage().

This CL removes the dependencies and thus
ImageResourceInfo::setIsPlaceholder(),
by making |ImageResource::m_isPlaceholder| a tri-state enum
(renamed as |ImageResource::placeholderOption|):
- DoNotReloadPlaceholder (previously m_isPlaceholder == false)
- ReloadPlaceholderOnDecodeError (new state)
- ShowAndReloadPlaceholderAlways (previously m_isPlaceholder == true)
and setting it when receiving ResourceResponse.

This CL preserves the behavior of a placeholder ImageResource:

If isEntireResource() && (1):
  Behavior: Placeholder is not created && !shouldReloadBrokenPlaceholder().
  Previously: because |m_isPlaceholder| is cleared in updateImage().
  Now: because |m_placeholderOption| is set to |DoNotReloadPlaceholder|.

If isEntireResource() && not (1) && (2):
  Behavior: Placeholder is not created && !shouldReloadBrokenPlaceholder().
  Previously: because |m_isPlaceholder| is cleared in updateImage().
  Now: because |m_placeholderOption| is set to |ReloadPlaceholderOnDecodeError|
       and DecodeError does NOT occur because (2) is true.

If isEntireResource() && not (1) && not (2):
  Behavior: Placeholder is not created && shouldReloadBrokenPlaceholder().
  Previously: because updateImage() creates placeholder only if (2) is true
              and |m_isPlaceholder| is NOT cleared in updateImage().
  Now: because |m_placeholderOption| is set to |ReloadPlaceholderOnDecodeError|
       and DecodeError does occur because (2) is false.

Otherwise:
  Not affected.

BUG=667641, 677188
Review-Url: https://codereview.chromium.org/2650113002
Cr-Commit-Position: refs/heads/master@{#455282}
Committed: https://chromium.googlesource.com/chromium/src/+/810f7ecd76b0aba74f0a215dbd00b8e8cc5705ba

[hiroshige, 2017-01-24 21:20:29]

Description was changed from

==========
Remove timing dependencies around ImageResourceInfo::setIsPlaceholder()

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

to

==========
Remove timing dependencies around ImageResourceInfo::setIsPlaceholder()

Previsouly, |m_isPlaceholder| is cleared in updateImage(), if:
- isEntireResource() is true, AND
- (1) The HTTP status code is not 4XX or 5XX OR
  (2) the response has no DecodeError,
to prevent creating of placeholder and reloading.

However, this causes timing and code dependencies from
ImageResourceContent::updateImage()
to |ImageResource::m_isPlaceholder|
and ImageResource::reloadIfLoFiOrPlaceholderImage().

This CL makes |ImageResource::m_isPlaceholder| a tri-state enum
(renamed as |ImageResource::placeholderOption|:
- DoNotReloadPlaceholder (previously m_isPlaceholder == false)
- ReloadPlaceholderOnDecodeError (new state)
- ShowAndReloadPlaceholderAlways (previously m_isPlaceholder == true)
and set it when receiving ResourceResponse, to remove these dependencies.

This CL preserves the behavior of placeholder ImageResource:

If isEntireResource() && (1):
  Behavior: Placeholder is not created && !shouldReloadBrokenPlaceholder().
  Previously: because |m_isPlaceholder| is cleared in updateImage().
  Now: because |m_placeholderOption| is set to |DoNotReloadPlaceholder|.

If isEntireResource() && not (1) && (2):
  Behavior: Placeholder is not created && !shouldReloadBrokenPlaceholder().
  Previously: because |m_isPlaceholder| is cleared in updateImage().
  Now: because |m_placeholderOption| is set to |ReloadPlaceholderOnDecodeError|
       and DecodeError does NOT occur because (2) is true.

If isEntireResource() && not (1) && not (2):
  Behavior: Placeholder is not created && shouldReloadBrokenPlaceholder().
  Previously: because updateImage() creates placeholder only if (2) is true
              and |m_isPlaceholder| is NOT cleared in updateImage().
  Now: because |m_placeholderOption| is set to |ReloadPlaceholderOnDecodeError|
       and DecodeError does occur because (2) is false.

This CL doesn't affect the behavior when |isPlaceholder| is false or
isEntireResource() is false.

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

[hiroshige, 2017-01-24 23:47:38]

Description was changed from

==========
Remove timing dependencies around ImageResourceInfo::setIsPlaceholder()

Previsouly, |m_isPlaceholder| is cleared in updateImage(), if:
- isEntireResource() is true, AND
- (1) The HTTP status code is not 4XX or 5XX OR
  (2) the response has no DecodeError,
to prevent creating of placeholder and reloading.

However, this causes timing and code dependencies from
ImageResourceContent::updateImage()
to |ImageResource::m_isPlaceholder|
and ImageResource::reloadIfLoFiOrPlaceholderImage().

This CL makes |ImageResource::m_isPlaceholder| a tri-state enum
(renamed as |ImageResource::placeholderOption|:
- DoNotReloadPlaceholder (previously m_isPlaceholder == false)
- ReloadPlaceholderOnDecodeError (new state)
- ShowAndReloadPlaceholderAlways (previously m_isPlaceholder == true)
and set it when receiving ResourceResponse, to remove these dependencies.

This CL preserves the behavior of placeholder ImageResource:

If isEntireResource() && (1):
  Behavior: Placeholder is not created && !shouldReloadBrokenPlaceholder().
  Previously: because |m_isPlaceholder| is cleared in updateImage().
  Now: because |m_placeholderOption| is set to |DoNotReloadPlaceholder|.

If isEntireResource() && not (1) && (2):
  Behavior: Placeholder is not created && !shouldReloadBrokenPlaceholder().
  Previously: because |m_isPlaceholder| is cleared in updateImage().
  Now: because |m_placeholderOption| is set to |ReloadPlaceholderOnDecodeError|
       and DecodeError does NOT occur because (2) is true.

If isEntireResource() && not (1) && not (2):
  Behavior: Placeholder is not created && shouldReloadBrokenPlaceholder().
  Previously: because updateImage() creates placeholder only if (2) is true
              and |m_isPlaceholder| is NOT cleared in updateImage().
  Now: because |m_placeholderOption| is set to |ReloadPlaceholderOnDecodeError|
       and DecodeError does occur because (2) is false.

This CL doesn't affect the behavior when |isPlaceholder| is false or
isEntireResource() is false.

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

to

==========
Phase II Step 2: Remove setIsPlaceholder() in updateImage()

Previsouly, |m_isPlaceholder| is cleared in updateImage(), if:
- isEntireResource() is true, AND
- (1) The HTTP status code is not 4XX or 5XX OR
  (2) the response has no DecodeError,
to prevent creating of placeholder and reloading.

However, this causes timing and code dependencies from
ImageResourceContent::updateImage()
to |ImageResource::m_isPlaceholder|
and ImageResource::reloadIfLoFiOrPlaceholderImage().

This CL makes |ImageResource::m_isPlaceholder| a tri-state enum
(renamed as |ImageResource::placeholderOption|:
- DoNotReloadPlaceholder (previously m_isPlaceholder == false)
- ReloadPlaceholderOnDecodeError (new state)
- ShowAndReloadPlaceholderAlways (previously m_isPlaceholder == true)
and set it when receiving ResourceResponse, to remove these dependencies.

This CL preserves the behavior of placeholder ImageResource:

If isEntireResource() && (1):
  Behavior: Placeholder is not created && !shouldReloadBrokenPlaceholder().
  Previously: because |m_isPlaceholder| is cleared in updateImage().
  Now: because |m_placeholderOption| is set to |DoNotReloadPlaceholder|.

If isEntireResource() && not (1) && (2):
  Behavior: Placeholder is not created && !shouldReloadBrokenPlaceholder().
  Previously: because |m_isPlaceholder| is cleared in updateImage().
  Now: because |m_placeholderOption| is set to |ReloadPlaceholderOnDecodeError|
       and DecodeError does NOT occur because (2) is true.

If isEntireResource() && not (1) && not (2):
  Behavior: Placeholder is not created && shouldReloadBrokenPlaceholder().
  Previously: because updateImage() creates placeholder only if (2) is true
              and |m_isPlaceholder| is NOT cleared in updateImage().
  Now: because |m_placeholderOption| is set to |ReloadPlaceholderOnDecodeError|
       and DecodeError does occur because (2) is false.

This CL doesn't affect the behavior when |isPlaceholder| is false or
isEntireResource() is false.

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

[hiroshige, 2017-01-25 00:29:09]

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

[commit-bot: I haz the power, 2017-01-25 00:30:04]

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 00:59:10]

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

[commit-bot: I haz the power, 2017-01-25 00:59:12]

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

[hiroshige, 2017-01-25 01:41:12]

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:42: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-25 02:07:52]

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

[commit-bot: I haz the power, 2017-01-25 02:07:53]

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

[hiroshige, 2017-01-25 02:12:33]

Description was changed from

==========
Phase II Step 2: Remove setIsPlaceholder() in updateImage()

Previsouly, |m_isPlaceholder| is cleared in updateImage(), if:
- isEntireResource() is true, AND
- (1) The HTTP status code is not 4XX or 5XX OR
  (2) the response has no DecodeError,
to prevent creating of placeholder and reloading.

However, this causes timing and code dependencies from
ImageResourceContent::updateImage()
to |ImageResource::m_isPlaceholder|
and ImageResource::reloadIfLoFiOrPlaceholderImage().

This CL makes |ImageResource::m_isPlaceholder| a tri-state enum
(renamed as |ImageResource::placeholderOption|:
- DoNotReloadPlaceholder (previously m_isPlaceholder == false)
- ReloadPlaceholderOnDecodeError (new state)
- ShowAndReloadPlaceholderAlways (previously m_isPlaceholder == true)
and set it when receiving ResourceResponse, to remove these dependencies.

This CL preserves the behavior of placeholder ImageResource:

If isEntireResource() && (1):
  Behavior: Placeholder is not created && !shouldReloadBrokenPlaceholder().
  Previously: because |m_isPlaceholder| is cleared in updateImage().
  Now: because |m_placeholderOption| is set to |DoNotReloadPlaceholder|.

If isEntireResource() && not (1) && (2):
  Behavior: Placeholder is not created && !shouldReloadBrokenPlaceholder().
  Previously: because |m_isPlaceholder| is cleared in updateImage().
  Now: because |m_placeholderOption| is set to |ReloadPlaceholderOnDecodeError|
       and DecodeError does NOT occur because (2) is true.

If isEntireResource() && not (1) && not (2):
  Behavior: Placeholder is not created && shouldReloadBrokenPlaceholder().
  Previously: because updateImage() creates placeholder only if (2) is true
              and |m_isPlaceholder| is NOT cleared in updateImage().
  Now: because |m_placeholderOption| is set to |ReloadPlaceholderOnDecodeError|
       and DecodeError does occur because (2) is false.

This CL doesn't affect the behavior when |isPlaceholder| is false or
isEntireResource() is false.

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

to

==========
Phase II Step 2: Remove setIsPlaceholder() in updateImage()

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

Previsouly, |m_isPlaceholder| is cleared in updateImage(), if:
- isEntireResource() is true, AND
- (1) The HTTP status code is not 4XX or 5XX OR
  (2) the response has no DecodeError,
to prevent creating of placeholder and reloading.

However, this causes timing and code dependencies from
ImageResourceContent::updateImage()
to |ImageResource::m_isPlaceholder|
and ImageResource::reloadIfLoFiOrPlaceholderImage().

This CL makes |ImageResource::m_isPlaceholder| a tri-state enum
(renamed as |ImageResource::placeholderOption|:
- DoNotReloadPlaceholder (previously m_isPlaceholder == false)
- ReloadPlaceholderOnDecodeError (new state)
- ShowAndReloadPlaceholderAlways (previously m_isPlaceholder == true)
and set it when receiving ResourceResponse, to remove these dependencies.

This CL preserves the behavior of placeholder ImageResource:

If isEntireResource() && (1):
  Behavior: Placeholder is not created && !shouldReloadBrokenPlaceholder().
  Previously: because |m_isPlaceholder| is cleared in updateImage().
  Now: because |m_placeholderOption| is set to |DoNotReloadPlaceholder|.

If isEntireResource() && not (1) && (2):
  Behavior: Placeholder is not created && !shouldReloadBrokenPlaceholder().
  Previously: because |m_isPlaceholder| is cleared in updateImage().
  Now: because |m_placeholderOption| is set to |ReloadPlaceholderOnDecodeError|
       and DecodeError does NOT occur because (2) is true.

If isEntireResource() && not (1) && not (2):
  Behavior: Placeholder is not created && shouldReloadBrokenPlaceholder().
  Previously: because updateImage() creates placeholder only if (2) is true
              and |m_isPlaceholder| is NOT cleared in updateImage().
  Now: because |m_placeholderOption| is set to |ReloadPlaceholderOnDecodeError|
       and DecodeError does occur because (2) is false.

This CL doesn't affect the behavior when |isPlaceholder| is false or
isEntireResource() is false.

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

[hiroshige, 2017-01-25 02:26:58]

Description was changed from

==========
Phase II Step 2: Remove setIsPlaceholder() in updateImage()

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

Previsouly, |m_isPlaceholder| is cleared in updateImage(), if:
- isEntireResource() is true, AND
- (1) The HTTP status code is not 4XX or 5XX OR
  (2) the response has no DecodeError,
to prevent creating of placeholder and reloading.

However, this causes timing and code dependencies from
ImageResourceContent::updateImage()
to |ImageResource::m_isPlaceholder|
and ImageResource::reloadIfLoFiOrPlaceholderImage().

This CL makes |ImageResource::m_isPlaceholder| a tri-state enum
(renamed as |ImageResource::placeholderOption|:
- DoNotReloadPlaceholder (previously m_isPlaceholder == false)
- ReloadPlaceholderOnDecodeError (new state)
- ShowAndReloadPlaceholderAlways (previously m_isPlaceholder == true)
and set it when receiving ResourceResponse, to remove these dependencies.

This CL preserves the behavior of placeholder ImageResource:

If isEntireResource() && (1):
  Behavior: Placeholder is not created && !shouldReloadBrokenPlaceholder().
  Previously: because |m_isPlaceholder| is cleared in updateImage().
  Now: because |m_placeholderOption| is set to |DoNotReloadPlaceholder|.

If isEntireResource() && not (1) && (2):
  Behavior: Placeholder is not created && !shouldReloadBrokenPlaceholder().
  Previously: because |m_isPlaceholder| is cleared in updateImage().
  Now: because |m_placeholderOption| is set to |ReloadPlaceholderOnDecodeError|
       and DecodeError does NOT occur because (2) is true.

If isEntireResource() && not (1) && not (2):
  Behavior: Placeholder is not created && shouldReloadBrokenPlaceholder().
  Previously: because updateImage() creates placeholder only if (2) is true
              and |m_isPlaceholder| is NOT cleared in updateImage().
  Now: because |m_placeholderOption| is set to |ReloadPlaceholderOnDecodeError|
       and DecodeError does occur because (2) is false.

This CL doesn't affect the behavior when |isPlaceholder| is false or
isEntireResource() is false.

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

to

==========
Phase II Step 2: Remove setIsPlaceholder() in updateImage()

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

Previsouly, |m_isPlaceholder| is cleared in updateImage(), if:
- isEntireResource() is true, AND
- (1) The HTTP status code is not 4XX or 5XX OR
  (2) the response has no DecodeError,
to prevent creating of placeholder and reloading.

However, this causes timing and code dependencies from
ImageResourceContent::updateImage()
to |ImageResource::m_isPlaceholder|
and ImageResource::reloadIfLoFiOrPlaceholderImage().

This CL removes the dependencies and thus
ImageResourceInfo::setIsPlaceholder(),
by making |ImageResource::m_isPlaceholder| a tri-state enum
(renamed as |ImageResource::placeholderOption|):
- DoNotReloadPlaceholder (previously m_isPlaceholder == false)
- ReloadPlaceholderOnDecodeError (new state)
- ShowAndReloadPlaceholderAlways (previously m_isPlaceholder == true)
and setting it when receiving ResourceResponse.

This CL preserves the behavior of a placeholder ImageResource:

If isEntireResource() && (1):
  Behavior: Placeholder is not created && !shouldReloadBrokenPlaceholder().
  Previously: because |m_isPlaceholder| is cleared in updateImage().
  Now: because |m_placeholderOption| is set to |DoNotReloadPlaceholder|.

If isEntireResource() && not (1) && (2):
  Behavior: Placeholder is not created && !shouldReloadBrokenPlaceholder().
  Previously: because |m_isPlaceholder| is cleared in updateImage().
  Now: because |m_placeholderOption| is set to |ReloadPlaceholderOnDecodeError|
       and DecodeError does NOT occur because (2) is true.

If isEntireResource() && not (1) && not (2):
  Behavior: Placeholder is not created && shouldReloadBrokenPlaceholder().
  Previously: because updateImage() creates placeholder only if (2) is true
              and |m_isPlaceholder| is NOT cleared in updateImage().
  Now: because |m_placeholderOption| is set to |ReloadPlaceholderOnDecodeError|
       and DecodeError does occur because (2) is false.

Otherwise:
Not affected.

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

[hiroshige, 2017-01-25 02:27:14]

Description was changed from

==========
Phase II Step 2: Remove setIsPlaceholder() in updateImage()

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

Previsouly, |m_isPlaceholder| is cleared in updateImage(), if:
- isEntireResource() is true, AND
- (1) The HTTP status code is not 4XX or 5XX OR
  (2) the response has no DecodeError,
to prevent creating of placeholder and reloading.

However, this causes timing and code dependencies from
ImageResourceContent::updateImage()
to |ImageResource::m_isPlaceholder|
and ImageResource::reloadIfLoFiOrPlaceholderImage().

This CL removes the dependencies and thus
ImageResourceInfo::setIsPlaceholder(),
by making |ImageResource::m_isPlaceholder| a tri-state enum
(renamed as |ImageResource::placeholderOption|):
- DoNotReloadPlaceholder (previously m_isPlaceholder == false)
- ReloadPlaceholderOnDecodeError (new state)
- ShowAndReloadPlaceholderAlways (previously m_isPlaceholder == true)
and setting it when receiving ResourceResponse.

This CL preserves the behavior of a placeholder ImageResource:

If isEntireResource() && (1):
  Behavior: Placeholder is not created && !shouldReloadBrokenPlaceholder().
  Previously: because |m_isPlaceholder| is cleared in updateImage().
  Now: because |m_placeholderOption| is set to |DoNotReloadPlaceholder|.

If isEntireResource() && not (1) && (2):
  Behavior: Placeholder is not created && !shouldReloadBrokenPlaceholder().
  Previously: because |m_isPlaceholder| is cleared in updateImage().
  Now: because |m_placeholderOption| is set to |ReloadPlaceholderOnDecodeError|
       and DecodeError does NOT occur because (2) is true.

If isEntireResource() && not (1) && not (2):
  Behavior: Placeholder is not created && shouldReloadBrokenPlaceholder().
  Previously: because updateImage() creates placeholder only if (2) is true
              and |m_isPlaceholder| is NOT cleared in updateImage().
  Now: because |m_placeholderOption| is set to |ReloadPlaceholderOnDecodeError|
       and DecodeError does occur because (2) is false.

Otherwise:
Not affected.

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

to

==========
Phase II Step 2: Remove setIsPlaceholder() in updateImage()

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

Previsouly, |m_isPlaceholder| is cleared in updateImage(), if:
- isEntireResource() is true, AND
- (1) The HTTP status code is not 4XX or 5XX OR
  (2) the response has no DecodeError,
to prevent creating of placeholder and reloading.

However, this causes timing and code dependencies from
ImageResourceContent::updateImage()
to |ImageResource::m_isPlaceholder|
and ImageResource::reloadIfLoFiOrPlaceholderImage().

This CL removes the dependencies and thus
ImageResourceInfo::setIsPlaceholder(),
by making |ImageResource::m_isPlaceholder| a tri-state enum
(renamed as |ImageResource::placeholderOption|):
- DoNotReloadPlaceholder (previously m_isPlaceholder == false)
- ReloadPlaceholderOnDecodeError (new state)
- ShowAndReloadPlaceholderAlways (previously m_isPlaceholder == true)
and setting it when receiving ResourceResponse.

This CL preserves the behavior of a placeholder ImageResource:

If isEntireResource() && (1):
  Behavior: Placeholder is not created && !shouldReloadBrokenPlaceholder().
  Previously: because |m_isPlaceholder| is cleared in updateImage().
  Now: because |m_placeholderOption| is set to |DoNotReloadPlaceholder|.

If isEntireResource() && not (1) && (2):
  Behavior: Placeholder is not created && !shouldReloadBrokenPlaceholder().
  Previously: because |m_isPlaceholder| is cleared in updateImage().
  Now: because |m_placeholderOption| is set to |ReloadPlaceholderOnDecodeError|
       and DecodeError does NOT occur because (2) is true.

If isEntireResource() && not (1) && not (2):
  Behavior: Placeholder is not created && shouldReloadBrokenPlaceholder().
  Previously: because updateImage() creates placeholder only if (2) is true
              and |m_isPlaceholder| is NOT cleared in updateImage().
  Now: because |m_placeholderOption| is set to |ReloadPlaceholderOnDecodeError|
       and DecodeError does occur because (2) is false.

Otherwise:
  Not affected.

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

[hiroshige, 2017-01-25 02:34:48]

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

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

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

[commit-bot: I haz the power, 2017-01-25 02:57:13]

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

[hiroshige, 2017-02-11 00:01:17]

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

[hiroshige, 2017-02-11 00:01:18]

PTAL.

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

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:08: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-02-11 00:33:16]

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

[commit-bot: I haz the power, 2017-02-11 00:33:17]

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-02-11 00:40:15]

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:40:52]

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

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

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

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

[yhirano, 2017-02-13 11:36:57]

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

https://codereview.chromium.org/2650113002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/resource/ImageResource.cpp:428:
m_placeholderOption = ReloadPlaceholderOnDecodeError;
Is DoNotReloadPlaceholder => ReloadPlaceholderOnDecodeError transition valid?

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

https://codereview.chromium.org/2650113002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/resource/ImageResource.h:157: enum
PlaceholderOption {
enum class?

[hiroshige, 2017-02-23 20:52:27]

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

[hiroshige, 2017-02-23 20:52:38]

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

https://codereview.chromium.org/2650113002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/resource/ImageResource.cpp:428:
m_placeholderOption = ReloadPlaceholderOnDecodeError;
On 2017/02/13 11:36:57, yhirano wrote:
> Is DoNotReloadPlaceholder => ReloadPlaceholderOnDecodeError transition valid?

Oh, looks invalid. Fixed.

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

https://codereview.chromium.org/2650113002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/resource/ImageResource.h:157: enum
PlaceholderOption {
On 2017/02/13 11:36:57, yhirano wrote:
> enum class?

Done.

[commit-bot: I haz the power, 2017-02-23 20:52: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, 2017-02-23 22:26:08]

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

[commit-bot: I haz the power, 2017-02-23 22:26:09]

Dry run: This issue passed the CQ dry run.

[yhirano, 2017-02-24 17:17:27]

lgtm

[hiroshige, 2017-02-24 18:31:44]

Thanks!

sclittle@, could you also take a look?

[hiroshige, 2017-03-01 18:42:46]

On 2017/02/24 18:31:44, hiroshige wrote:
> Thanks!
> 
> sclittle@, could you also take a look?

Friendly ping. sclittle@, could you take a look?

[hiroshige, 2017-03-01 18:46:53]

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

[commit-bot: I haz the power, 2017-03-01 18:47:22]

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-01 20:11:41]

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

[commit-bot: I haz the power, 2017-03-01 20:11:42]

Dry run: This issue passed the CQ dry run.

[hiroshige, 2017-03-02 00:08:00]

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

[commit-bot: I haz the power, 2017-03-02 00:08: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-03-02 02:07:05]

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

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

Dry run: This issue passed the CQ dry run.

[sclittle, 2017-03-07 21:16:12]

LGTM! Sorry for the delayed review.

[hiroshige, 2017-03-07 21:19:31]

The CQ bit was checked by hiroshige@chromium.org

[hiroshige, 2017-03-07 21:19:31]

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

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

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-07 23:23:19]

CQ is committing da patch.

Bot data: {"patchset_id": 220001, "attempt_start_ts": 1488921571144810,
"parent_rev": "81aeead4d36ae4401894ab0b059d9721a30c9521", "commit_rev":
"810f7ecd76b0aba74f0a215dbd00b8e8cc5705ba"}

[commit-bot: I haz the power, 2017-03-07 23:24:21]

Description was changed from

==========
Phase II Step 2: Remove setIsPlaceholder() in updateImage()

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

Previsouly, |m_isPlaceholder| is cleared in updateImage(), if:
- isEntireResource() is true, AND
- (1) The HTTP status code is not 4XX or 5XX OR
  (2) the response has no DecodeError,
to prevent creating of placeholder and reloading.

However, this causes timing and code dependencies from
ImageResourceContent::updateImage()
to |ImageResource::m_isPlaceholder|
and ImageResource::reloadIfLoFiOrPlaceholderImage().

This CL removes the dependencies and thus
ImageResourceInfo::setIsPlaceholder(),
by making |ImageResource::m_isPlaceholder| a tri-state enum
(renamed as |ImageResource::placeholderOption|):
- DoNotReloadPlaceholder (previously m_isPlaceholder == false)
- ReloadPlaceholderOnDecodeError (new state)
- ShowAndReloadPlaceholderAlways (previously m_isPlaceholder == true)
and setting it when receiving ResourceResponse.

This CL preserves the behavior of a placeholder ImageResource:

If isEntireResource() && (1):
  Behavior: Placeholder is not created && !shouldReloadBrokenPlaceholder().
  Previously: because |m_isPlaceholder| is cleared in updateImage().
  Now: because |m_placeholderOption| is set to |DoNotReloadPlaceholder|.

If isEntireResource() && not (1) && (2):
  Behavior: Placeholder is not created && !shouldReloadBrokenPlaceholder().
  Previously: because |m_isPlaceholder| is cleared in updateImage().
  Now: because |m_placeholderOption| is set to |ReloadPlaceholderOnDecodeError|
       and DecodeError does NOT occur because (2) is true.

If isEntireResource() && not (1) && not (2):
  Behavior: Placeholder is not created && shouldReloadBrokenPlaceholder().
  Previously: because updateImage() creates placeholder only if (2) is true
              and |m_isPlaceholder| is NOT cleared in updateImage().
  Now: because |m_placeholderOption| is set to |ReloadPlaceholderOnDecodeError|
       and DecodeError does occur because (2) is false.

Otherwise:
  Not affected.

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

to

==========
Phase II Step 2: Remove setIsPlaceholder() in updateImage()

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

Previsouly, |m_isPlaceholder| is cleared in updateImage(), if:
- isEntireResource() is true, AND
- (1) The HTTP status code is not 4XX or 5XX OR
  (2) the response has no DecodeError,
to prevent creating of placeholder and reloading.

However, this causes timing and code dependencies from
ImageResourceContent::updateImage()
to |ImageResource::m_isPlaceholder|
and ImageResource::reloadIfLoFiOrPlaceholderImage().

This CL removes the dependencies and thus
ImageResourceInfo::setIsPlaceholder(),
by making |ImageResource::m_isPlaceholder| a tri-state enum
(renamed as |ImageResource::placeholderOption|):
- DoNotReloadPlaceholder (previously m_isPlaceholder == false)
- ReloadPlaceholderOnDecodeError (new state)
- ShowAndReloadPlaceholderAlways (previously m_isPlaceholder == true)
and setting it when receiving ResourceResponse.

This CL preserves the behavior of a placeholder ImageResource:

If isEntireResource() && (1):
  Behavior: Placeholder is not created && !shouldReloadBrokenPlaceholder().
  Previously: because |m_isPlaceholder| is cleared in updateImage().
  Now: because |m_placeholderOption| is set to |DoNotReloadPlaceholder|.

If isEntireResource() && not (1) && (2):
  Behavior: Placeholder is not created && !shouldReloadBrokenPlaceholder().
  Previously: because |m_isPlaceholder| is cleared in updateImage().
  Now: because |m_placeholderOption| is set to |ReloadPlaceholderOnDecodeError|
       and DecodeError does NOT occur because (2) is true.

If isEntireResource() && not (1) && not (2):
  Behavior: Placeholder is not created && shouldReloadBrokenPlaceholder().
  Previously: because updateImage() creates placeholder only if (2) is true
              and |m_isPlaceholder| is NOT cleared in updateImage().
  Now: because |m_placeholderOption| is set to |ReloadPlaceholderOnDecodeError|
       and DecodeError does occur because (2) is false.

Otherwise:
  Not affected.

BUG=667641, 677188

Review-Url: https://codereview.chromium.org/2650113002
Cr-Commit-Position: refs/heads/master@{#455282}
Committed:
https://chromium.googlesource.com/chromium/src/+/810f7ecd76b0aba74f0a215dbd00...
==========

[commit-bot: I haz the power, 2017-03-07 23:24:22]

Committed patchset #12 (id:220001) as
https://chromium.googlesource.com/chromium/src/+/810f7ecd76b0aba74f0a215dbd00...