cc: Use an enum for tracking images vetoed for async decodes.

cc: Use an enum for tracking images vetoed for async decodes.

This adds an option for permanent vetoes to come from blink.

BUG=702386
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2817473002
Cr-Commit-Position: refs/heads/master@{#464656}
Committed: https://chromium.googlesource.com/chromium/src/+/d525c83dc997c24ecd618d45c2d932cd2e7feccc

[Khushal, 2017-04-11 18:18:50]

Description was changed from

==========
cc: Use an enum for tracking images vetoed for async decodes.

This adds an option for permanent vetoes to come from blink.

BUG=702386
==========

to

==========
cc: Use an enum for tracking images vetoed for async decodes.

This adds an option for permanent vetoes to come from blink.

BUG=702386
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[Khushal, 2017-04-11 18:19:23]

khushalsagar@chromium.org changed reviewers:
+ vmpstr@chromium.org

[vmpstr, 2017-04-11 18:42:55]

Add some tests as well, please.

https://codereview.chromium.org/2817473002/diff/1/cc/tiles/checker_image_trac...
File cc/tiles/checker_image_tracker.cc (right):

https://codereview.chromium.org/2817473002/diff/1/cc/tiles/checker_image_trac...
cc/tiles/checker_image_tracker.cc:136: return
image_async_decode_state_[image_id] == AsyncDecodeVetoState::ASYNC;
Erm, in lines 128-136 you do 3 finds on the same image. Can you cache some of
the results

https://codereview.chromium.org/2817473002/diff/1/cc/tiles/checker_image_trac...
cc/tiles/checker_image_tracker.cc:145: // The image might have already been
decoded, in which case we don't need to
Can you reword the comment to take into account that we're not checking whether
it's been decoded, rather we're checking if it can be async

https://codereview.chromium.org/2817473002/diff/1/cc/tiles/checker_image_trac...
File cc/tiles/checker_image_tracker.h (right):

https://codereview.chromium.org/2817473002/diff/1/cc/tiles/checker_image_trac...
cc/tiles/checker_image_tracker.h:57: enum class AsyncDecodeVetoState {
s/AsyncDecodeVetoState/DecodePolicy/

https://codereview.chromium.org/2817473002/diff/1/cc/tiles/checker_image_trac...
cc/tiles/checker_image_tracker.h:62: SYNC_DECODED_ONCE,
Is there any semantic difference between the last two values? Like is ASYNC/SYNC
enough or is there a subtle difference? If there's a difference, please comment
on it.

https://codereview.chromium.org/2817473002/diff/1/cc/tiles/checker_image_trac...
cc/tiles/checker_image_tracker.h:94: // A map of ImageId to its permitted
AsyncDecodeVetoState.
nit: s/permitted //

[Khushal, 2017-04-12 20:31:17]

The existing tests already take care of the changes. Since this change does not
add any use of the SYNC_PERMANENT mode.

https://codereview.chromium.org/2817473002/diff/1/cc/tiles/checker_image_trac...
File cc/tiles/checker_image_tracker.cc (right):

https://codereview.chromium.org/2817473002/diff/1/cc/tiles/checker_image_trac...
cc/tiles/checker_image_tracker.cc:136: return
image_async_decode_state_[image_id] == AsyncDecodeVetoState::ASYNC;
On 2017/04/11 18:42:55, vmpstr wrote:
> Erm, in lines 128-136 you do 3 finds on the same image. Can you cache some of
> the results

Done.

https://codereview.chromium.org/2817473002/diff/1/cc/tiles/checker_image_trac...
cc/tiles/checker_image_tracker.cc:145: // The image might have already been
decoded, in which case we don't need to
On 2017/04/11 18:42:55, vmpstr wrote:
> Can you reword the comment to take into account that we're not checking
whether
> it's been decoded, rather we're checking if it can be async

Done.

https://codereview.chromium.org/2817473002/diff/1/cc/tiles/checker_image_trac...
File cc/tiles/checker_image_tracker.h (right):

https://codereview.chromium.org/2817473002/diff/1/cc/tiles/checker_image_trac...
cc/tiles/checker_image_tracker.h:57: enum class AsyncDecodeVetoState {
On 2017/04/11 18:42:55, vmpstr wrote:
> s/AsyncDecodeVetoState/DecodePolicy/

Done.

https://codereview.chromium.org/2817473002/diff/1/cc/tiles/checker_image_trac...
cc/tiles/checker_image_tracker.h:62: SYNC_DECODED_ONCE,
On 2017/04/11 18:42:55, vmpstr wrote:
> Is there any semantic difference between the last two values? Like is
ASYNC/SYNC
> enough or is there a subtle difference? If there's a difference, please
comment
> on it.

Done.

https://codereview.chromium.org/2817473002/diff/1/cc/tiles/checker_image_trac...
cc/tiles/checker_image_tracker.h:94: // A map of ImageId to its permitted
AsyncDecodeVetoState.
On 2017/04/11 18:42:55, vmpstr wrote:
> nit: s/permitted //

Done.

[vmpstr, 2017-04-13 18:28:12]

lgtm

https://codereview.chromium.org/2817473002/diff/20001/cc/tiles/checker_image_...
File cc/tiles/checker_image_tracker.cc (right):

https://codereview.chromium.org/2817473002/diff/20001/cc/tiles/checker_image_...
cc/tiles/checker_image_tracker.cc:130: auto it =
image_async_decode_state_.find(image_id);
nit: Call ::insert() on it so you always get an iterator, and check the bool
return type to see if it is a new insert or not, that way you don't have to do
another find on line 135.

https://codereview.chromium.org/2817473002/diff/20001/cc/tiles/checker_image_...
cc/tiles/checker_image_tracker.cc:153:
DCHECK_NE(image_async_decode_state_.count(image_id), 0u);
just do a find, then use the iterator for both the dcheck and line 154 if

[Khushal, 2017-04-14 01:06:42]

https://codereview.chromium.org/2817473002/diff/20001/cc/tiles/checker_image_...
File cc/tiles/checker_image_tracker.cc (right):

https://codereview.chromium.org/2817473002/diff/20001/cc/tiles/checker_image_...
cc/tiles/checker_image_tracker.cc:130: auto it =
image_async_decode_state_.find(image_id);
On 2017/04/13 18:28:12, vmpstr wrote:
> nit: Call ::insert() on it so you always get an iterator, and check the bool
> return type to see if it is a new insert or not, that way you don't have to do
> another find on line 135.

Done.

https://codereview.chromium.org/2817473002/diff/20001/cc/tiles/checker_image_...
cc/tiles/checker_image_tracker.cc:153:
DCHECK_NE(image_async_decode_state_.count(image_id), 0u);
On 2017/04/13 18:28:12, vmpstr wrote:
> just do a find, then use the iterator for both the dcheck and line 154 if

Done.

[Khushal, 2017-04-14 01:06:47]

The CQ bit was checked by khushalsagar@chromium.org

[Khushal, 2017-04-14 01:06:47]

The patchset sent to the CQ was uploaded after l-g-t-m from vmpstr@chromium.org
Link to the patchset: https://codereview.chromium.org/2817473002/#ps40001
(title: "addressed comments")

[commit-bot: I haz the power, 2017-04-14 01:07:22]

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-04-14 02:09:08]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1492132007311510,
"parent_rev": "94309c21892ee97c409b3f7dc509e414dd9095a9", "commit_rev":
"d525c83dc997c24ecd618d45c2d932cd2e7feccc"}

[commit-bot: I haz the power, 2017-04-14 02:09:59]

Description was changed from

==========
cc: Use an enum for tracking images vetoed for async decodes.

This adds an option for permanent vetoes to come from blink.

BUG=702386
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
cc: Use an enum for tracking images vetoed for async decodes.

This adds an option for permanent vetoes to come from blink.

BUG=702386
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

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

[commit-bot: I haz the power, 2017-04-14 02:10:00]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/d525c83dc997c24ecd618d45c2d9...