cc: Move image decodes from tile manager to a separate file.

cc: Move image decodes from tile manager to a separate file.

This patch moves image decode functionality to ImageDecodeController.
This is intended to make modifications to image decodes simpler.

R=enne, weiliangc
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/c8cf52ab5b17cc122027b3c743dced5713628aa7
Cr-Commit-Position: refs/heads/master@{#341636}

[vmpstr, 2015-07-20 21:44:42]

Please take a look.

[vmpstr, 2015-07-30 22:10:17]

On 2015/07/20 21:44:42, vmpstr wrote:
> Please take a look.

ping. I think this will eventually be the direction that we want to move in, so
I'd like to get this refactor landed.

[ericrk, 2015-07-30 22:59:29]

ericrk@chromium.org changed reviewers:
+ ericrk@chromium.org

[ericrk, 2015-07-30 22:59:30]

Makes sense to me w/ a few small nits - LGTM

https://codereview.chromium.org/1246633004/diff/1/cc/tiles/image_decode_contr...
File cc/tiles/image_decode_controller.cc (right):

https://codereview.chromium.org/1246633004/diff/1/cc/tiles/image_decode_contr...
cc/tiles/image_decode_controller.cc:69: if (decode_task)
super nit: Why not just do "if (!decode_task) decode_task = Create...."

allowing for only one return and simplifying flow a bit?

https://codereview.chromium.org/1246633004/diff/1/cc/tiles/image_decode_contr...
File cc/tiles/image_decode_controller.h (right):

https://codereview.chromium.org/1246633004/diff/1/cc/tiles/image_decode_contr...
cc/tiles/image_decode_controller.h:17: class CC_EXPORT ImageDecodeController {
does this need CC_EXPORT as it seems to be only used by tile_manager?

https://codereview.chromium.org/1246633004/diff/1/cc/tiles/image_decode_contr...
cc/tiles/image_decode_controller.h:27: void DecodePixelRef(SkPixelRef*
pixel_ref) const;
nit: maybe it's the same difference, but should this just be static rather than
const? doesn't refer to any class state. - I guess it could though, so if you're
leaving it open to that, sounds good.

[vmpstr, 2015-07-30 23:08:34]

https://codereview.chromium.org/1246633004/diff/1/cc/tiles/image_decode_contr...
File cc/tiles/image_decode_controller.cc (right):

https://codereview.chromium.org/1246633004/diff/1/cc/tiles/image_decode_contr...
cc/tiles/image_decode_controller.cc:69: if (decode_task)
On 2015/07/30 22:59:30, ericrk wrote:
> super nit: Why not just do "if (!decode_task) decode_task = Create...."
> 
> allowing for only one return and simplifying flow a bit?

Done.

https://codereview.chromium.org/1246633004/diff/1/cc/tiles/image_decode_contr...
File cc/tiles/image_decode_controller.h (right):

https://codereview.chromium.org/1246633004/diff/1/cc/tiles/image_decode_contr...
cc/tiles/image_decode_controller.h:17: class CC_EXPORT ImageDecodeController {
On 2015/07/30 22:59:30, ericrk wrote:
> does this need CC_EXPORT as it seems to be only used by tile_manager?

Removed. We'll probably be unittesting this as well :P so I'll re-add it when we
actually need it.

https://codereview.chromium.org/1246633004/diff/1/cc/tiles/image_decode_contr...
cc/tiles/image_decode_controller.h:27: void DecodePixelRef(SkPixelRef*
pixel_ref) const;
On 2015/07/30 22:59:30, ericrk wrote:
> nit: maybe it's the same difference, but should this just be static rather
than
> const? doesn't refer to any class state. - I guess it could though, so if
you're
> leaving it open to that, sounds good.

Eventually the goal is to make this act like a cache with DecodePixelRef storing
results for future use. So I actually removed const from this as well to leave
the decision of what to do in this function up to the controller.

[vmpstr, 2015-07-31 23:35:54]

The CQ bit was checked by vmpstr@chromium.org

[vmpstr, 2015-07-31 23:35:55]

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

[commit-bot: I haz the power, 2015-07-31 23:36:16]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1246633004/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1246633004/40001

[commit-bot: I haz the power, 2015-08-01 01:34:36]

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

[commit-bot: I haz the power, 2015-08-01 01:34:37]

Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[vmpstr, 2015-08-03 17:43:36]

The CQ bit was checked by vmpstr@chromium.org

[commit-bot: I haz the power, 2015-08-03 17:43:56]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1246633004/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1246633004/40001

[enne (OOO), 2015-08-03 18:02:46]

lgtm

https://codereview.chromium.org/1246633004/diff/40001/cc/tiles/image_decode_c...
File cc/tiles/image_decode_controller.cc (right):

https://codereview.chromium.org/1246633004/diff/40001/cc/tiles/image_decode_c...
cc/tiles/image_decode_controller.cc:67: auto& decode_task =
image_decode_tasks_[layer_id][generation_id];
Sorry to style nitpick, but I feel like this CL uses auto a bit too much.  In
particular, auto for smart pointers seems like something that should be avoided.

https://codereview.chromium.org/1246633004/diff/40001/cc/tiles/image_decode_c...
cc/tiles/image_decode_controller.cc:104: // is no longer in use. This ensures
that we only decode a pixel ref once.
It will be nice to be able to get rid of this at some point in the future.

https://codereview.chromium.org/1246633004/diff/40001/cc/tiles/image_decode_c...
cc/tiles/image_decode_controller.cc:114: auto& pixel_ref_tasks =
layer_it->second;
More auto nitpicking: This auto hides more than it helps, in my opinion.

[vmpstr, 2015-08-03 18:04:37]

The CQ bit was unchecked by vmpstr@chromium.org

[enne (OOO), 2015-08-03 18:05:32]

The CQ bit was unchecked by enne@chromium.org

[vmpstr, 2015-08-03 18:19:18]

PTAL

https://codereview.chromium.org/1246633004/diff/40001/cc/tiles/image_decode_c...
File cc/tiles/image_decode_controller.cc (right):

https://codereview.chromium.org/1246633004/diff/40001/cc/tiles/image_decode_c...
cc/tiles/image_decode_controller.cc:67: auto& decode_task =
image_decode_tasks_[layer_id][generation_id];
On 2015/08/03 18:02:46, enne wrote:
> Sorry to style nitpick, but I feel like this CL uses auto a bit too much.  In
> particular, auto for smart pointers seems like something that should be
avoided.

Done.

https://codereview.chromium.org/1246633004/diff/40001/cc/tiles/image_decode_c...
cc/tiles/image_decode_controller.cc:104: // is no longer in use. This ensures
that we only decode a pixel ref once.
On 2015/08/03 18:02:46, enne wrote:
> It will be nice to be able to get rid of this at some point in the future.

Added a todo

https://codereview.chromium.org/1246633004/diff/40001/cc/tiles/image_decode_c...
cc/tiles/image_decode_controller.cc:114: auto& pixel_ref_tasks =
layer_it->second;
On 2015/08/03 18:02:46, enne wrote:
> More auto nitpicking: This auto hides more than it helps, in my opinion.

Done.

[enne (OOO), 2015-08-03 18:31:25]

The CQ bit was checked by enne@chromium.org

[enne (OOO), 2015-08-03 18:31:26]

The patchset sent to the CQ was uploaded after l-g-t-m from enne@chromium.org,
ericrk@chromium.org
Link to the patchset: https://codereview.chromium.org/1246633004/#ps60001
(title: "update")

[commit-bot: I haz the power, 2015-08-03 18:33:19]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1246633004/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1246633004/60001

[commit-bot: I haz the power, 2015-08-03 19:23:58]

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

[commit-bot: I haz the power, 2015-08-03 19:23:58]

Try jobs failed on following builders:
  linux_blink_rel on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/7...)

[vmpstr, 2015-08-03 22:28:02]

The CQ bit was checked by vmpstr@chromium.org

[commit-bot: I haz the power, 2015-08-03 22:28:28]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1246633004/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1246633004/60001

[commit-bot: I haz the power, 2015-08-03 23:13:06]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-08-03 23:13:48]

Patchset 4 (id:??) landed as
https://crrev.com/c8cf52ab5b17cc122027b3c743dced5713628aa7
Cr-Commit-Position: refs/heads/master@{#341636}