cc: Removed low priority bin from tile management.

cc: Removed low priority bin from tile management.

This patch removes low priority bin from tile management, increasing
the perftests by about 5% (since the bin comparator doesn't have to
check this anymore).

TileManager unittests, and all of cc_unittests pass.

R=reveman@chromium.org
BUG=268887
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=219897

[vmpstr, 2013-08-22 22:56:28]

Please take a look.

[reveman, 2013-08-26 16:55:46]

This looks great if you just cleanup GetPrioritizedTileSet a bit.

https://codereview.chromium.org/22831041/diff/1/cc/resources/tile_manager.cc
File cc/resources/tile_manager.cc (right):

https://codereview.chromium.org/22831041/diff/1/cc/resources/tile_manager.cc#...
cc/resources/tile_manager.cc:316: mts.bin = mts.tree_bin[PENDING_TREE];
can we clean this up somehow? maybe bump the bin up after the switch statement
if !is_in_never_bin_on_both_trees()?

[vmpstr, 2013-08-26 17:30:40]

PTAL.

[vmpstr, 2013-08-26 17:30:58]

https://codereview.chromium.org/22831041/diff/1/cc/resources/tile_manager.cc
File cc/resources/tile_manager.cc (right):

https://codereview.chromium.org/22831041/diff/1/cc/resources/tile_manager.cc#...
cc/resources/tile_manager.cc:316: mts.bin = mts.tree_bin[PENDING_TREE];
On 2013/08/26 16:55:46, David Reveman wrote:
> can we clean this up somehow? maybe bump the bin up after the switch statement
> if !is_in_never_bin_on_both_trees()?

Done.

[reveman, 2013-08-26 20:56:28]

https://codereview.chromium.org/22831041/diff/6001/cc/resources/tile_manager.cc
File cc/resources/tile_manager.cc (right):

https://codereview.chromium.org/22831041/diff/6001/cc/resources/tile_manager....
cc/resources/tile_manager.cc:334: if (mts.bin == NEVER_BIN &&
!dual_bin_is_never)
would:

bool is_in_never_bin_on_both_trees =
    mts.tree_bin[ACTIVE_TREE] == NEVER_BIN &&
    mts.tree_bin[PENDING_TREE] == NEVER_BIN;

if (mts.bin == NEVER_BIN && !is_in_never_bin_on_both_trees)
  mts.bin = tile_is_active ? AT_LAST_AND_ACTIVE_BIN : AT_LAST_BIN;

give the same result?

[vmpstr, 2013-08-26 21:08:08]

PTAL.

https://codereview.chromium.org/22831041/diff/6001/cc/resources/tile_manager.cc
File cc/resources/tile_manager.cc (right):

https://codereview.chromium.org/22831041/diff/6001/cc/resources/tile_manager....
cc/resources/tile_manager.cc:334: if (mts.bin == NEVER_BIN &&
!dual_bin_is_never)
On 2013/08/26 20:56:29, David Reveman wrote:
> would:
> 
> bool is_in_never_bin_on_both_trees =
>     mts.tree_bin[ACTIVE_TREE] == NEVER_BIN &&
>     mts.tree_bin[PENDING_TREE] == NEVER_BIN;
> 
> if (mts.bin == NEVER_BIN && !is_in_never_bin_on_both_trees)
>   mts.bin = tile_is_active ? AT_LAST_AND_ACTIVE_BIN : AT_LAST_BIN;
> 
> give the same result?

Yep. That looks better to me, I've changed it.

[reveman, 2013-08-26 23:22:13]

lgtm

https://codereview.chromium.org/22831041/diff/17001/cc/resources/prioritized_...
File cc/resources/prioritized_tile_set.cc (right):

https://codereview.chromium.org/22831041/diff/17001/cc/resources/prioritized_...
cc/resources/prioritized_tile_set.cc:61: case NUM_BINS:
nit: "default: NOTREACHED()" would be more consistent at the moment.

[danakj, 2013-08-27 02:23:43]

https://codereview.chromium.org/22831041/diff/17001/cc/resources/prioritized_...
File cc/resources/prioritized_tile_set.cc (right):

https://codereview.chromium.org/22831041/diff/17001/cc/resources/prioritized_...
cc/resources/prioritized_tile_set.cc:61: case NUM_BINS:
On 2013/08/26 23:22:13, David Reveman wrote:
> nit: "default: NOTREACHED()" would be more consistent at the moment.

:(

[tomhudson, 2013-08-27 10:00:40]

On 2013/08/27 02:23:43, danakj wrote:
>
https://codereview.chromium.org/22831041/diff/17001/cc/resources/prioritized_...
> cc/resources/prioritized_tile_set.cc:61: case NUM_BINS:
> On 2013/08/26 23:22:13, David Reveman wrote:
> > nit: "default: NOTREACHED()" would be more consistent at the moment.
> 
> :(

What Dana said; I haven't seen cc/ come to a consensus on that, and the majority
in the still-ongoing discussion seems to have a principled argument against
default:.

[reveman, 2013-08-27 18:58:23]

On 2013/08/27 10:00:40, tomhudson wrote:
> On 2013/08/27 02:23:43, danakj wrote:
> >
>
https://codereview.chromium.org/22831041/diff/17001/cc/resources/prioritized_...
> > cc/resources/prioritized_tile_set.cc:61: case NUM_BINS:
> > On 2013/08/26 23:22:13, David Reveman wrote:
> > > nit: "default: NOTREACHED()" would be more consistent at the moment.
> > 
> > :(
> 
> What Dana said; I haven't seen cc/ come to a consensus on that, and the
majority
> in the still-ongoing discussion seems to have a principled argument against
> default:.

If there's consensus on removing all default cases and adding a bunch of
NUM_THINGS cases, then we should do that throughout all of cc/ separately. What
makes me unhappy are inconsistencies.

[vmpstr, 2013-08-27 23:35:16]

On 2013/08/27 18:58:23, David Reveman wrote:
> On 2013/08/27 10:00:40, tomhudson wrote:
> > On 2013/08/27 02:23:43, danakj wrote:
> > >
> >
>
https://codereview.chromium.org/22831041/diff/17001/cc/resources/prioritized_...
> > > cc/resources/prioritized_tile_set.cc:61: case NUM_BINS:
> > > On 2013/08/26 23:22:13, David Reveman wrote:
> > > > nit: "default: NOTREACHED()" would be more consistent at the moment.
> > > 
> > > :(
> > 
> > What Dana said; I haven't seen cc/ come to a consensus on that, and the
> majority
> > in the still-ongoing discussion seems to have a principled argument against
> > default:.
> 
> If there's consensus on removing all default cases and adding a bunch of
> NUM_THINGS cases, then we should do that throughout all of cc/ separately.
What
> makes me unhappy are inconsistencies.

I changed this to default. We can still do a bulk update if we can agree on how
to handle this type of case.

[commit-bot: I haz the power, 2013-08-27 23:41:29]

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

[commit-bot: I haz the power, 2013-08-28 00:38:45]

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

[commit-bot: I haz the power, 2013-08-28 03:44:28]

Change committed as 219897