cc: Make image controller return a status with the callback.

cc: Make image controller return a status with the callback.

This patch adds a status to the callback when image decode requests
happen. This makes it possible for the caller to react appropriately
to failures.

R=khushalsagar@chromium.org, enne@chromium.org
BUG=693677
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

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

[vmpstr, 2017-02-17 23:17:27]

Description was changed from

==========
cc: Make image controller return a status with the callback.

This patch adds a status to the callback when image decode requests
happen. This makes it possible for the caller to react appropriately
to failures.

R=khushalsagar@chromium.org, enne@chromium.org
BUG=693677
==========

to

==========
cc: Make image controller return a status with the callback.

This patch adds a status to the callback when image decode requests
happen. This makes it possible for the caller to react appropriately
to failures.

R=khushalsagar@chromium.org, enne@chromium.org
BUG=693677
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[vmpstr, 2017-02-17 23:24:41]

Please take a look.

[Khushal, 2017-02-17 23:56:38]

https://codereview.chromium.org/2703633004/diff/40001/cc/tiles/decoded_image_...
File cc/tiles/decoded_image_tracker.h (right):

https://codereview.chromium.org/2703633004/diff/40001/cc/tiles/decoded_image_...
cc/tiles/decoded_image_tracker.h:33: const base::Callback<void(bool)>&
callback);
Add a comment about what the bool in the callback signifies?

https://codereview.chromium.org/2703633004/diff/40001/cc/tiles/image_controll...
File cc/tiles/image_controller.cc (right):

https://codereview.chromium.org/2703633004/diff/40001/cc/tiles/image_controll...
cc/tiles/image_controller.cc:283: // into memory. In all cases, this implies
that the decode was a failure.
What if we tried to decode the image and it was corrupt or something, it
wouldn't cover that case right? It probably doesn't matter but I was reading the
comment and thought about this.

https://codereview.chromium.org/2703633004/diff/40001/cc/tiles/image_controll...
cc/tiles/image_controller.cc:284: if
(!request.draw_image.image()->isLazyGenerated())
Could you add a ImageControllerTest for this?

[vmpstr, 2017-02-24 02:43:49]

PTAL

https://codereview.chromium.org/2703633004/diff/40001/cc/tiles/decoded_image_...
File cc/tiles/decoded_image_tracker.h (right):

https://codereview.chromium.org/2703633004/diff/40001/cc/tiles/decoded_image_...
cc/tiles/decoded_image_tracker.h:33: const base::Callback<void(bool)>&
callback);
On 2017/02/17 23:56:38, Khushal wrote:
> Add a comment about what the bool in the callback signifies?

Done.

https://codereview.chromium.org/2703633004/diff/40001/cc/tiles/image_controll...
File cc/tiles/image_controller.cc (right):

https://codereview.chromium.org/2703633004/diff/40001/cc/tiles/image_controll...
cc/tiles/image_controller.cc:283: // into memory. In all cases, this implies
that the decode was a failure.
On 2017/02/17 23:56:38, Khushal wrote:
> What if we tried to decode the image and it was corrupt or something, it
> wouldn't cover that case right? It probably doesn't matter but I was reading
the
> comment and thought about this.

As discussed offline, I think this is fine, since the failure case is guaranteed
to be a failure.

https://codereview.chromium.org/2703633004/diff/40001/cc/tiles/image_controll...
cc/tiles/image_controller.cc:284: if
(!request.draw_image.image()->isLazyGenerated())
On 2017/02/17 23:56:38, Khushal wrote:
> Could you add a ImageControllerTest for this?

Done.

[Khushal, 2017-02-25 00:21:40]

lgtm.

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

https://codereview.chromium.org/2703633004/diff/60001/cc/tiles/checker_image_...
cc/tiles/checker_image_tracker.cc:81: ImageController::ImageDecodeResult result)
{
May be DCHECK here that we never get decode not required, because in that case
we never should have queued a decode.

[vmpstr, 2017-02-27 21:25:28]

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

https://codereview.chromium.org/2703633004/diff/60001/cc/tiles/checker_image_...
cc/tiles/checker_image_tracker.cc:81: ImageController::ImageDecodeResult result)
{
On 2017/02/25 00:21:40, Khushal wrote:
> May be DCHECK here that we never get decode not required, because in that case
> we never should have queued a decode.

Done.

[vmpstr, 2017-02-27 21:25:33]

The CQ bit was checked by vmpstr@chromium.org

[vmpstr, 2017-02-27 21:25:33]

The patchset sent to the CQ was uploaded after l-g-t-m from
khushalsagar@chromium.org
Link to the patchset: https://codereview.chromium.org/2703633004/#ps80001
(title: "update")

[commit-bot: I haz the power, 2017-02-27 21:26:08]

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-27 22:51:34]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1488230733322830,
"parent_rev": "d00656ece1ae3378d3206cb5aca23d4566158b6d", "commit_rev":
"fa6e385073a6d05bdee9d60e3814e7a77a035be6"}

[commit-bot: I haz the power, 2017-02-27 22:52:19]

Description was changed from

==========
cc: Make image controller return a status with the callback.

This patch adds a status to the callback when image decode requests
happen. This makes it possible for the caller to react appropriately
to failures.

R=khushalsagar@chromium.org, enne@chromium.org
BUG=693677
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
cc: Make image controller return a status with the callback.

This patch adds a status to the callback when image decode requests
happen. This makes it possible for the caller to react appropriately
to failures.

R=khushalsagar@chromium.org, enne@chromium.org
BUG=693677
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

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

[commit-bot: I haz the power, 2017-02-27 22:52:20]

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