cc: Fix cleanup path for image decode requests in ImageController.

cc: Fix cleanup path for image decode requests in ImageController.

The change fixes a couple of things in the ImageController:

1) We try to mark each image decode task as completed when tearing down
the work queued for the image worker in StopWorkerTasks. Since multiple
requests for the same image will re-use the tasks, we try to complete
the same task again.

2) Make sure that we tear down the worker tasks when switching caches,
not just when the cache is reset.

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

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

[Khushal, 2017-02-17 19:44:02]

Description was changed from

==========
cc: Fix cleanup path for image decode requests in ImageController.

The change fixes a couple of things in the ImageController:

1) We try to mark each image decode task as completed when tearing down
the work queued for the image worker in StopWorkerTasks. Since multiple
requests for the same image will re-use the tasks, we try to complete
the same task again.

2) Make sure that we tear down the worker tasks when switching caches,
not just when the cache is reset.

BUG=693682
==========

to

==========
cc: Fix cleanup path for image decode requests in ImageController.

The change fixes a couple of things in the ImageController:

1) We try to mark each image decode task as completed when tearing down
the work queued for the image worker in StopWorkerTasks. Since multiple
requests for the same image will re-use the tasks, we try to complete
the same task again.

2) Make sure that we tear down the worker tasks when switching caches,
not just when the cache is reset.

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

[Khushal, 2017-02-17 19:44:28]

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

[vmpstr, 2017-02-17 21:15:13]

https://codereview.chromium.org/2699533010/diff/1/cc/tiles/image_controller.cc
File cc/tiles/image_controller.cc (right):

https://codereview.chromium.org/2699533010/diff/1/cc/tiles/image_controller.c...
cc/tiles/image_controller.cc:126: } else if (cache_ && cache_ != cache) {
Don't do this... We specifically talked about it and we only support setting
non-null to null or vice versa

[Khushal, 2017-02-17 21:17:35]

https://codereview.chromium.org/2699533010/diff/1/cc/tiles/image_controller.cc
File cc/tiles/image_controller.cc (right):

https://codereview.chromium.org/2699533010/diff/1/cc/tiles/image_controller.c...
cc/tiles/image_controller.cc:126: } else if (cache_ && cache_ != cache) {
On 2017/02/17 21:15:13, vmpstr wrote:
> Don't do this... We specifically talked about it and we only support setting
> non-null to null or vice versa

Should I add a DCHECK for that then? Makes it obvious for someone reading this
too.

[vmpstr, 2017-02-17 21:22:09]

lgtm

https://codereview.chromium.org/2699533010/diff/1/cc/tiles/image_controller.cc
File cc/tiles/image_controller.cc (right):

https://codereview.chromium.org/2699533010/diff/1/cc/tiles/image_controller.c...
cc/tiles/image_controller.cc:126: } else if (cache_ && cache_ != cache) {
On 2017/02/17 21:17:35, Khushal wrote:
> On 2017/02/17 21:15:13, vmpstr wrote:
> > Don't do this... We specifically talked about it and we only support setting
> > non-null to null or vice versa
> 
> Should I add a DCHECK for that then? Makes it obvious for someone reading this
> too.

Yeah I'm not sure why we don't have a DCHECK, there must have been a reason.
Actually, I think what you have here is fine. Can you just make sure to call
SetPredecodeImages as well (just combine the two if statements)

[Khushal, 2017-02-22 21:09:03]

On 2017/02/17 21:22:09, vmpstr wrote:
> lgtm
> 
> https://codereview.chromium.org/2699533010/diff/1/cc/tiles/image_controller.cc
> File cc/tiles/image_controller.cc (right):
> 
>
https://codereview.chromium.org/2699533010/diff/1/cc/tiles/image_controller.c...
> cc/tiles/image_controller.cc:126: } else if (cache_ && cache_ != cache) {
> On 2017/02/17 21:17:35, Khushal wrote:
> > On 2017/02/17 21:15:13, vmpstr wrote:
> > > Don't do this... We specifically talked about it and we only support
setting
> > > non-null to null or vice versa
> > 
> > Should I add a DCHECK for that then? Makes it obvious for someone reading
this
> > too.
> 
> Yeah I'm not sure why we don't have a DCHECK, there must have been a reason.
> Actually, I think what you have here is fine. Can you just make sure to call
> SetPredecodeImages as well (just combine the two if statements)

I added the DCHECK that we had discussed, one of cache_ or cache should be null,
and removed the else block there.

[Khushal, 2017-02-22 21:09:08]

The CQ bit was checked by khushalsagar@chromium.org

[Khushal, 2017-02-22 21:09:09]

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/2699533010/#ps20001
(title: "..")

[commit-bot: I haz the power, 2017-02-22 21:10:19]

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-22 22:29:42]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1487797748816060,
"parent_rev": "86a6cab9976c0355b241d85c30655c2ae51d0005", "commit_rev":
"fbe2b0877565ac300ce38da21e4bf0fb93e7de40"}

[commit-bot: I haz the power, 2017-02-22 22:30:22]

Description was changed from

==========
cc: Fix cleanup path for image decode requests in ImageController.

The change fixes a couple of things in the ImageController:

1) We try to mark each image decode task as completed when tearing down
the work queued for the image worker in StopWorkerTasks. Since multiple
requests for the same image will re-use the tasks, we try to complete
the same task again.

2) Make sure that we tear down the worker tasks when switching caches,
not just when the cache is reset.

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

to

==========
cc: Fix cleanup path for image decode requests in ImageController.

The change fixes a couple of things in the ImageController:

1) We try to mark each image decode task as completed when tearing down
the work queued for the image worker in StopWorkerTasks. Since multiple
requests for the same image will re-use the tasks, we try to complete
the same task again.

2) Make sure that we tear down the worker tasks when switching caches,
not just when the cache is reset.

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

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

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

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/fbe2b0877565ac300ce38da21e4b...