cc: Add more eviction categories to picture layer impl.

cc: Add more eviction categories to picture layer impl.

Since we use priority_for_tree_priority to get the priority_bin,
we might end up in situations where we have a non-now priority_bin
with a tile that is required for activation (this is because
required for activation property is read off the pending priority).

In particular in smoothness takes priority, if a tile is only
available on the pending tree, then it will have EVENTUALLY
priority_bin with inf distance. However, at the same time
it can be required for activation (NOW on the active tree).

We need to ensure that this is handled explicitly and correctly.
This patch adds the extra required categories, as well as
makes an array instead of separate variables for each of
the categories.

R=reveman

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=290011

[vmpstr, 2014-08-14 06:23:49]

PTAL.

https://codereview.chromium.org/471833002/diff/1/cc/resources/picture_layer_t...
File cc/resources/picture_layer_tiling.h (right):

https://codereview.chromium.org/471833002/diff/1/cc/resources/picture_layer_t...
cc/resources/picture_layer_tiling.h:66: enum { NUM_EVICTION_CATEGORIES =
NOW_AND_REQUIRED_FOR_ACTIVATION + 1 };
I'm not sure what our style guide says about enums anymore :) I prefer this
approach, but if you have another preference I'll be happy to change it.

https://codereview.chromium.org/471833002/diff/1/cc/resources/tile_manager_un...
File cc/resources/tile_manager_unittest.cc (right):

https://codereview.chromium.org/471833002/diff/1/cc/resources/tile_manager_un...
cc/resources/tile_manager_unittest.cc:901:
pending_layer_->MarkVisibleResourcesAsRequired();
This line alone (without the other changes in the function) would break the
unittest, since we would not be considering required for activation changes.

[reveman, 2014-08-14 09:01:50]

https://codereview.chromium.org/471833002/diff/1/cc/resources/picture_layer_t...
File cc/resources/picture_layer_tiling.cc (right):

https://codereview.chromium.org/471833002/diff/1/cc/resources/picture_layer_t...
cc/resources/picture_layer_tiling.cc:42: return a_priority.priority_bin >
b_priority.priority_bin;
should this not be a DCHECK too?

https://codereview.chromium.org/471833002/diff/1/cc/resources/picture_layer_t...
File cc/resources/picture_layer_tiling.h (right):

https://codereview.chromium.org/471833002/diff/1/cc/resources/picture_layer_t...
cc/resources/picture_layer_tiling.h:66: enum { NUM_EVICTION_CATEGORIES =
NOW_AND_REQUIRED_FOR_ACTIVATION + 1 };
On 2014/08/14 06:23:48, vmpstr wrote:
> I'm not sure what our style guide says about enums anymore :) I prefer this
> approach, but if you have another preference I'll be happy to change it.

We don't have a nice consistent mechanism for this. I was trying to create this
at one point (https://codereview.chromium.org/22911037/) but never had the time
to make it build on win and push for it as a standard in cc and/or chromium.

The problem with the approach you use here is that one might easily forget to
update NUM_EVICTION_CATEGORIES when adding more enum values.

enum EvictionCategory {
  EVENTUALLY,
  SOON,
  NOW,
  NOW_AND_REQUIRED_FOR_ACTIVATION,
  kEvictionCategoryMax = NOW_AND_REQUIRED_FOR_ACTIVATION
}

is a bit better imo but suffers from the same problem. Adding
NUM_EVICTION_CATEGORIES at the end is another option but the problem with that
is obvious.

I prefer if we avoid array indexing using enum values until we have a nice
mechanism to do this. How about we keep this as before but create a bug for
adding a proper mechanism for using enum values as array indexes and put a TODO
here? You can assign the bug to me and I'll try to finish up that old cl.

[danakj, 2014-08-14 16:55:26]

https://codereview.chromium.org/471833002/diff/1/cc/resources/picture_layer_t...
File cc/resources/picture_layer_tiling.h (right):

https://codereview.chromium.org/471833002/diff/1/cc/resources/picture_layer_t...
cc/resources/picture_layer_tiling.h:66: enum { NUM_EVICTION_CATEGORIES =
NOW_AND_REQUIRED_FOR_ACTIVATION + 1 };
On 2014/08/14 09:01:50, reveman wrote:
> On 2014/08/14 06:23:48, vmpstr wrote:
> > I'm not sure what our style guide says about enums anymore :) I prefer this
> > approach, but if you have another preference I'll be happy to change it.
> 
> We don't have a nice consistent mechanism for this. I was trying to create
this
> at one point (https://codereview.chromium.org/22911037/) but never had the
time
> to make it build on win and push for it as a standard in cc and/or chromium.
> 
> The problem with the approach you use here is that one might easily forget to
> update NUM_EVICTION_CATEGORIES when adding more enum values.
> 
> enum EvictionCategory {
>   EVENTUALLY,
>   SOON,
>   NOW,
>   NOW_AND_REQUIRED_FOR_ACTIVATION,
>   kEvictionCategoryMax = NOW_AND_REQUIRED_FOR_ACTIVATION
> }
> 
> is a bit better imo but suffers from the same problem. Adding
> NUM_EVICTION_CATEGORIES at the end is another option but the problem with that
> is obvious.
> 
> I prefer if we avoid array indexing using enum values until we have a nice
> mechanism to do this. How about we keep this as before but create a bug for
> adding a proper mechanism for using enum values as array indexes and put a
TODO
> here? You can assign the bug to me and I'll try to finish up that old cl.

Security team (tsepez) has been adding a LAST_FOO_BAR to enums like

enum Value {
a
b
LAST_VALUE=b
};

so we should probly be consistent with that.

[vmpstr, 2014-08-14 22:05:50]

PTAL

https://codereview.chromium.org/471833002/diff/1/cc/resources/picture_layer_t...
File cc/resources/picture_layer_tiling.cc (right):

https://codereview.chromium.org/471833002/diff/1/cc/resources/picture_layer_t...
cc/resources/picture_layer_tiling.cc:42: return a_priority.priority_bin >
b_priority.priority_bin;
On 2014/08/14 09:01:50, reveman wrote:
> should this not be a DCHECK too?

Good point, yes!

https://codereview.chromium.org/471833002/diff/1/cc/resources/picture_layer_t...
File cc/resources/picture_layer_tiling.h (right):

https://codereview.chromium.org/471833002/diff/1/cc/resources/picture_layer_t...
cc/resources/picture_layer_tiling.h:66: enum { NUM_EVICTION_CATEGORIES =
NOW_AND_REQUIRED_FOR_ACTIVATION + 1 };
On 2014/08/14 16:55:26, danakj wrote:
> On 2014/08/14 09:01:50, reveman wrote:
> > On 2014/08/14 06:23:48, vmpstr wrote:
> > > I'm not sure what our style guide says about enums anymore :) I prefer
this
> > > approach, but if you have another preference I'll be happy to change it.
> > 
> > We don't have a nice consistent mechanism for this. I was trying to create
> this
> > at one point (https://codereview.chromium.org/22911037/) but never had the
> time
> > to make it build on win and push for it as a standard in cc and/or chromium.
> > 
> > The problem with the approach you use here is that one might easily forget
to
> > update NUM_EVICTION_CATEGORIES when adding more enum values.
> > 
> > enum EvictionCategory {
> >   EVENTUALLY,
> >   SOON,
> >   NOW,
> >   NOW_AND_REQUIRED_FOR_ACTIVATION,
> >   kEvictionCategoryMax = NOW_AND_REQUIRED_FOR_ACTIVATION
> > }
> > 
> > is a bit better imo but suffers from the same problem. Adding
> > NUM_EVICTION_CATEGORIES at the end is another option but the problem with
that
> > is obvious.
> > 
> > I prefer if we avoid array indexing using enum values until we have a nice
> > mechanism to do this. How about we keep this as before but create a bug for
> > adding a proper mechanism for using enum values as array indexes and put a
> TODO
> > here? You can assign the bug to me and I'll try to finish up that old cl.
> 
> Security team (tsepez) has been adding a LAST_FOO_BAR to enums like
> 
> enum Value {
> a
> b
> LAST_VALUE=b
> };
> 
> so we should probly be consistent with that.

Removed this for now, added a TODO instead.

https://codereview.chromium.org/471833002/diff/20001/cc/resources/picture_lay...
File cc/resources/picture_layer_tiling.h (right):

https://codereview.chromium.org/471833002/diff/20001/cc/resources/picture_lay...
cc/resources/picture_layer_tiling.h:342: // TODO(reveman): Remove this in favour
of an array of eviction_tiles_ when we
reveman, as discussed, I put your name here. Let me know if you have time to fix
it up

[reveman, 2014-08-15 09:05:24]

lgtm with nit

https://codereview.chromium.org/471833002/diff/20001/cc/resources/picture_lay...
File cc/resources/picture_layer_tiling.h (right):

https://codereview.chromium.org/471833002/diff/20001/cc/resources/picture_lay...
cc/resources/picture_layer_tiling.h:346: std::vector<Tile*>
eviction_tiles_now_and_rfa_;
nit: i know it's long but i would still prefer to spell it out.

[vmpstr, 2014-08-15 17:16:01]

https://codereview.chromium.org/471833002/diff/20001/cc/resources/picture_lay...
File cc/resources/picture_layer_tiling.h (right):

https://codereview.chromium.org/471833002/diff/20001/cc/resources/picture_lay...
cc/resources/picture_layer_tiling.h:346: std::vector<Tile*>
eviction_tiles_now_and_rfa_;
On 2014/08/15 09:05:23, reveman wrote:
> nit: i know it's long but i would still prefer to spell it out.

Done.

[vmpstr, 2014-08-15 17:16:06]

The CQ bit was checked by vmpstr@chromium.org

[commit-bot: I haz the power, 2014-08-15 17:17:17]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/vmpstr@chromium.org/471833002/40001

[commit-bot: I haz the power, 2014-08-15 20:29:04]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_chromium_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[commit-bot: I haz the power, 2014-08-15 21:22:09]

Committed patchset #3 (40001) as 290011