cc: Move (partially) the predecode image tracking to ImageManager.

cc: Move (partially) the predecode image tracking to ImageManager.

This patch moves some of the predecode image tracking to ImageManager.
The other part depends on crbug.com/647402. Once the TileTaskRunner
can keep a ref on scheduled tiles, then we can remove
TileManager::locked_image_tasks_ as well.

R=enne
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel

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

[vmpstr, 2016-09-15 21:39:29]

Description was changed from

==========
cc: Move (partially) the predecode image tracking to ImageManager.

This patch moves some of the predecode image tracking to ImageManager.
The other part depends on crbug.com/647402. Once the TileTaskRunner
can keep a ref on scheduled tiles, then we can remove
TileManager::locked_image_tasks_ as well.

R=enne
==========

to

==========
cc: Move (partially) the predecode image tracking to ImageManager.

This patch moves some of the predecode image tracking to ImageManager.
The other part depends on crbug.com/647402. Once the TileTaskRunner
can keep a ref on scheduled tiles, then we can remove
TileManager::locked_image_tasks_ as well.

R=enne
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

[vmpstr, 2016-09-15 21:41:27]

Please take a look.

[enne (OOO), 2016-09-16 17:45:45]

https://codereview.chromium.org/2345833002/diff/1/cc/tiles/image_manager.cc
File cc/tiles/image_manager.cc (right):

https://codereview.chromium.org/2345833002/diff/1/cc/tiles/image_manager.cc#n...
cc/tiles/image_manager.cc:14: SetPredecodeImages(std::vector<DrawImage>(),
Why do you need to do this? Why would there be locked images before an image
controller is set? (Or why wouldn't you do this when the image decode controller
changed vs is set for the first time?)

Do you mean controller and not controller_?

[vmpstr, 2016-09-16 18:11:50]

PTAL

https://codereview.chromium.org/2345833002/diff/1/cc/tiles/image_manager.cc
File cc/tiles/image_manager.cc (right):

https://codereview.chromium.org/2345833002/diff/1/cc/tiles/image_manager.cc#n...
cc/tiles/image_manager.cc:14: SetPredecodeImages(std::vector<DrawImage>(),
On 2016/09/16 17:45:44, enne wrote:
> Why do you need to do this? Why would there be locked images before an image
> controller is set? (Or why wouldn't you do this when the image decode
controller
> changed vs is set for the first time?)
> 
> Do you mean controller and not controller_?

Errr, yeah I meant controller here. I've added a test for this.

[enne (OOO), 2016-09-16 18:21:37]

https://codereview.chromium.org/2345833002/diff/20001/cc/tiles/image_manager.cc
File cc/tiles/image_manager.cc (right):

https://codereview.chromium.org/2345833002/diff/20001/cc/tiles/image_manager....
cc/tiles/image_manager.cc:13: if (!controller) {
Sure, but do you also need to do this when the image controller changes as well?
If it goes non-null => non-null?

[vmpstr, 2016-09-16 18:38:49]

https://codereview.chromium.org/2345833002/diff/20001/cc/tiles/image_manager.cc
File cc/tiles/image_manager.cc (right):

https://codereview.chromium.org/2345833002/diff/20001/cc/tiles/image_manager....
cc/tiles/image_manager.cc:13: if (!controller) {
On 2016/09/16 18:21:37, enne wrote:
> Sure, but do you also need to do this when the image controller changes as
well?
> If it goes non-null => non-null?

Well... I mean I can add a DCHECK, but normally if it switches from non-null to
non-null, it's too late to do anything with the old controller because it's
already been deleted. Tile manager can't get SetResources twice without getting
a FinishTasksAndCleanUp in between.

[enne (OOO), 2016-09-16 18:42:52]

On 2016/09/16 at 18:38:49, vmpstr wrote:
>
https://codereview.chromium.org/2345833002/diff/20001/cc/tiles/image_manager.cc
> File cc/tiles/image_manager.cc (right):
> 
>
https://codereview.chromium.org/2345833002/diff/20001/cc/tiles/image_manager....
> cc/tiles/image_manager.cc:13: if (!controller) {
> On 2016/09/16 18:21:37, enne wrote:
> > Sure, but do you also need to do this when the image controller changes as
well?
> > If it goes non-null => non-null?
> 
> Well... I mean I can add a DCHECK, but normally if it switches from non-null
to non-null, it's too late to do anything with the old controller because it's
already been deleted. Tile manager can't get SetResources twice without getting
a FinishTasksAndCleanUp in between.

Whoa whoa whoa, that sounds not good.  It seems like the image manager should
never have a deleted controller.  Maybe enforce that you can only switch null ->
non-null -> null?

[vmpstr, 2016-09-16 18:55:24]

On 2016/09/16 18:42:52, enne wrote:
> On 2016/09/16 at 18:38:49, vmpstr wrote:
> >
>
https://codereview.chromium.org/2345833002/diff/20001/cc/tiles/image_manager.cc
> > File cc/tiles/image_manager.cc (right):
> > 
> >
>
https://codereview.chromium.org/2345833002/diff/20001/cc/tiles/image_manager....
> > cc/tiles/image_manager.cc:13: if (!controller) {
> > On 2016/09/16 18:21:37, enne wrote:
> > > Sure, but do you also need to do this when the image controller changes as
> well?
> > > If it goes non-null => non-null?
> > 
> > Well... I mean I can add a DCHECK, but normally if it switches from non-null
> to non-null, it's too late to do anything with the old controller because it's
> already been deleted. Tile manager can't get SetResources twice without
getting
> a FinishTasksAndCleanUp in between.
> 
> Whoa whoa whoa, that sounds not good.  It seems like the image manager should
> never have a deleted controller.  Maybe enforce that you can only switch null
->
> non-null -> null?

I'm not sure what you mean. You mean DCHECKs? It already can't happen, because
this class is function is called by tile manager's SetResources and
FinishTasksAndCleanUp which already have DCHECKs to ensure that two SetResources
can't happen without a FinishTasksAndCleanUp in between the two.

[enne (OOO), 2016-09-16 19:00:29]

On 2016/09/16 at 18:55:24, vmpstr wrote:
> On 2016/09/16 18:42:52, enne wrote:
> > On 2016/09/16 at 18:38:49, vmpstr wrote:
> > >
> >
https://codereview.chromium.org/2345833002/diff/20001/cc/tiles/image_manager.cc
> > > File cc/tiles/image_manager.cc (right):
> > > 
> > >
> >
https://codereview.chromium.org/2345833002/diff/20001/cc/tiles/image_manager....
> > > cc/tiles/image_manager.cc:13: if (!controller) {
> > > On 2016/09/16 18:21:37, enne wrote:
> > > > Sure, but do you also need to do this when the image controller changes
as
> > well?
> > > > If it goes non-null => non-null?
> > > 
> > > Well... I mean I can add a DCHECK, but normally if it switches from
non-null
> > to non-null, it's too late to do anything with the old controller because
it's
> > already been deleted. Tile manager can't get SetResources twice without
getting
> > a FinishTasksAndCleanUp in between.
> > 
> > Whoa whoa whoa, that sounds not good.  It seems like the image manager
should
> > never have a deleted controller.  Maybe enforce that you can only switch
null ->
> > non-null -> null?
> 
> I'm not sure what you mean. You mean DCHECKs? It already can't happen, because
this class is function is called by tile manager's SetResources and
FinishTasksAndCleanUp which already have DCHECKs to ensure that two SetResources
can't happen without a FinishTasksAndCleanUp in between the two.

But now you're depending on the caller to do the right thing and that logic is
elsewhere? So maybe it doesn't happen now but you've set it up so that somebody
could use the image manager incorrectly in the future?

[vmpstr, 2016-09-16 19:48:04]

On 2016/09/16 19:00:29, enne wrote:
> On 2016/09/16 at 18:55:24, vmpstr wrote:
> > On 2016/09/16 18:42:52, enne wrote:
> > > On 2016/09/16 at 18:38:49, vmpstr wrote:
> > > >
> > >
>
https://codereview.chromium.org/2345833002/diff/20001/cc/tiles/image_manager.cc
> > > > File cc/tiles/image_manager.cc (right):
> > > > 
> > > >
> > >
>
https://codereview.chromium.org/2345833002/diff/20001/cc/tiles/image_manager....
> > > > cc/tiles/image_manager.cc:13: if (!controller) {
> > > > On 2016/09/16 18:21:37, enne wrote:
> > > > > Sure, but do you also need to do this when the image controller
changes
> as
> > > well?
> > > > > If it goes non-null => non-null?
> > > > 
> > > > Well... I mean I can add a DCHECK, but normally if it switches from
> non-null
> > > to non-null, it's too late to do anything with the old controller because
> it's
> > > already been deleted. Tile manager can't get SetResources twice without
> getting
> > > a FinishTasksAndCleanUp in between.
> > > 
> > > Whoa whoa whoa, that sounds not good.  It seems like the image manager
> should
> > > never have a deleted controller.  Maybe enforce that you can only switch
> null ->
> > > non-null -> null?
> > 
> > I'm not sure what you mean. You mean DCHECKs? It already can't happen,
because
> this class is function is called by tile manager's SetResources and
> FinishTasksAndCleanUp which already have DCHECKs to ensure that two
SetResources
> can't happen without a FinishTasksAndCleanUp in between the two.
> 
> But now you're depending on the caller to do the right thing and that logic is
> elsewhere? So maybe it doesn't happen now but you've set it up so that
somebody
> could use the image manager incorrectly in the future?

I added a DCHECK. Please take a look.

[enne (OOO), 2016-09-16 19:51:27]

lgtm

[vmpstr, 2016-09-16 19:51:46]

The CQ bit was checked by vmpstr@chromium.org

[commit-bot: I haz the power, 2016-09-16 19:52: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, 2016-09-16 20:36:21]

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

[commit-bot: I haz the power, 2016-09-16 20:36:22]

Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[vmpstr, 2016-09-19 18:53:04]

The CQ bit was checked by vmpstr@chromium.org

[vmpstr, 2016-09-19 18:53:05]

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

[commit-bot: I haz the power, 2016-09-19 18:53:33]

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, 2016-09-19 20:05:13]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-09-19 21:03:19]

Description was changed from

==========
cc: Move (partially) the predecode image tracking to ImageManager.

This patch moves some of the predecode image tracking to ImageManager.
The other part depends on crbug.com/647402. Once the TileTaskRunner
can keep a ref on scheduled tiles, then we can remove
TileManager::locked_image_tasks_ as well.

R=enne
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

to

==========
cc: Move (partially) the predecode image tracking to ImageManager.

This patch moves some of the predecode image tracking to ImageManager.
The other part depends on crbug.com/647402. Once the TileTaskRunner
can keep a ref on scheduled tiles, then we can remove
TileManager::locked_image_tasks_ as well.

R=enne
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel

Committed: https://crrev.com/f8c96db734b49a003b679ca4f504184778bc87c7
Cr-Commit-Position: refs/heads/master@{#419539}
==========

[commit-bot: I haz the power, 2016-09-19 21:03:20]

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