cc: Generate tilings at other scales for impl-side painting

cc: Generate tilings at other scales for impl-side painting

This patch adds infrastructure to make it easy for PictureLayerImpl to
manage its set of tilings via PictureLayerImpl::ManageTiles().

The main functionality change is to additionally generate a low-res set
of tiles (by default 1/8 resolution).  Along with generating tilings at
different scales, tile priorities now have the correct resolution set on
them so that they can be prioritized correctly.

There's a few small bugfixes added here as well for correctness, such as
making sure that tilings are sorted in the PictureLayerTilingSet from
highest to lowest.  (This is the order in which tiles will be used.)  It
also correctly calculates a synthetic contents scale / content bounds for
the PictureLayerImpl to support multiple active contents scales.

R=nduca@chromium.org
BUG=167300
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=175927

[enne (OOO), 2012-12-28 22:45:26]

This patch was getting large, so I just left it at this.  It generates low res
tiles, but doesn't really handle pinch zoom yet.

During pinch zoom, it just doesn't create any more tilings and just uses what it
has, so that should be usable in the short term and a follow-up patch could make
ManageTilings more reasonable.

I feel like ManageTilings got really complicated really fast, and would love
advice about better heuristics or simplifications to make this easier to
understand and maintain.

[enne (OOO), 2013-01-07 21:10:37]

danakj: Do you have time to take a look at this too, since it's in line with my
previous two tiling scale changes.

This patch now has two phases for managing tilings.  ManageTilings now adds
tilings as it sees fit:
-Pending tree removes all non-ideal tilings so that it will only be activated
crisp
-Active tree during pinch will generate additional tilings if the pinch is too
zoomed out or in
-Active tree not during pinch will generate the high res / low res tilings at
the ideal scales if they don't exist

After drawing when there is not a pinch, if there are unused tilings (i.e. a
better tiling has content to cover it entirely in the visible content rect),
they get removed.  During pinch they're kept, as they can keep the content
crisper.

I will certainly take any suggestions for simpifying this, but this seems like a
reasonable heuristic to me.

[danakj, 2013-01-07 22:35:56]

Did you consider IDEAL_RES and LOW_RES instead of HIGH and LOW? It took a bit of
mental work to map between "high" and "ideal" when reading this.

https://codereview.chromium.org/11704002/diff/4001/cc/picture_layer_impl.cc
File cc/picture_layer_impl.cc (right):

https://codereview.chromium.org/11704002/diff/4001/cc/picture_layer_impl.cc#n...
cc/picture_layer_impl.cc:147: CleanUpUnusedTilings(seen_tilings);
any reason why this isn't a didDraw() thing? it's going to delay getting pixels
on the screen to do work that's not needed to get pixels on the screen.

https://codereview.chromium.org/11704002/diff/4001/cc/picture_layer_impl.cc#n...
cc/picture_layer_impl.cc:341: if (layerTreeImpl()->IsPendingTree() &&
maybe a temp bool var here for this if()? then the comment inside could probably
be obsolete.

https://codereview.chromium.org/11704002/diff/4001/cc/picture_layer_impl.cc#n...
cc/picture_layer_impl.cc:366: tiling->set_resolution(NON_IDEAL_RESOLUTION);
should this be outside the else{}?

https://codereview.chromium.org/11704002/diff/4001/cc/picture_layer_impl.cc#n...
cc/picture_layer_impl.cc:370: // Active tree creates tilings first, and the
pending tree will get them
s/ first//

I misunderstood this comment the first couple times.

https://codereview.chromium.org/11704002/diff/4001/cc/picture_layer_impl.cc#n...
cc/picture_layer_impl.cc:378: if (high_ratio >= kMaxScaleRatioDuringPinch)
can you use PositiveRatio() to simplify this area a bunch and only need one
if()?

https://codereview.chromium.org/11704002/diff/4001/cc/picture_layer_impl.cc#n...
cc/picture_layer_impl.cc:386: } else if (!low_res)
why do you only do this if !high_res? What if you are missing tiles that have
become visible now, would you not want a low res version of them even though
your high res tiles are okay still?

https://codereview.chromium.org/11704002/diff/4001/cc/picture_layer_impl.cc#n...
cc/picture_layer_impl.cc:390: // When not pinching, ensure exact contents scales
nit: period

https://codereview.chromium.org/11704002/diff/4001/cc/picture_layer_impl.cc#n...
cc/picture_layer_impl.cc:402: low_res->set_resolution(NON_IDEAL_RESOLUTION);
i don't get this. if low == high, then you set the one thing to not be either?
Why wouldn't it still be HIGH?

https://codereview.chromium.org/11704002/diff/4001/cc/picture_layer_impl.cc#n...
cc/picture_layer_impl.cc:409: std::vector<PictureLayerTiling*> used) {
i don't understand the reason for this parameter, can you explain?

it seems to me that if you have a set of tilings_, then you could iterate thru
the whole thing and remove anything that's not your current HIGH or LOW res
tiling.

What tilings would be in tilings_ but not in used, and why would you not want to
remove things that were not in |used| (and therefore unused)?

https://codereview.chromium.org/11704002/diff/4001/cc/picture_layer_impl.cc#n...
cc/picture_layer_impl.cc:410: if (ideal_contents_scale_ <= 0)
how is this possible?

https://codereview.chromium.org/11704002/diff/4001/cc/picture_layer_impl.cc#n...
cc/picture_layer_impl.cc:428: if (tiling == low_res)
can you not use the HIGH_RESOLUTION and LOW_RESOLUTION bits here instead of
doing a walk to find the low_res?

https://codereview.chromium.org/11704002/diff/4001/cc/picture_layer_tiling.cc
File cc/picture_layer_tiling.cc (right):

https://codereview.chromium.org/11704002/diff/4001/cc/picture_layer_tiling.cc...
cc/picture_layer_tiling.cc:270: float current_contents_scale,
not sure i'm a fan of the name "current". how does "current" here differ from
the value inside the member variable? that seems current as well..

maybe the member variable needs a better name instead?

https://codereview.chromium.org/11704002/diff/4001/cc/picture_layer_tiling.cc...
cc/picture_layer_tiling.cc:300: current_screen_transform,
current_layer_content_rect);
maybe current_screen_rect and last_screen_rect then as well?

https://codereview.chromium.org/11704002/diff/4001/cc/picture_layer_tiling_se...
File cc/picture_layer_tiling_set.cc (right):

https://codereview.chromium.org/11704002/diff/4001/cc/picture_layer_tiling_se...
cc/picture_layer_tiling_set.cc:85: for (size_t i = 0; i < tilings_.size(); ++i)
{
did you consider the erase(std::remove_if()) paradigm?

https://codereview.chromium.org/11704002/diff/4001/cc/picture_layer_tiling_se...
cc/picture_layer_tiling_set.cc:109: for (ideal_tiling_ = 0;
can the set ever have 0 tilings? Is it okay for ideal_tiling_ to be 0 in that
case?

https://codereview.chromium.org/11704002/diff/4001/cc/picture_layer_tiling_se...
cc/picture_layer_tiling_set.cc:115: ideal_tiling_--;
this seems less "ideal" and more "best" ? should we distinguish between them?

https://codereview.chromium.org/11704002/diff/4001/cc/picture_layer_tiling_se...
cc/picture_layer_tiling_set.cc:172: else
would "else if (current_tiling_) ... else ..." make this more clear than
switching into ?: mode suddenly in the else?

https://codereview.chromium.org/11704002/diff/4001/cc/picture_layer_tiling_se...
cc/picture_layer_tiling_set.cc:210: if (current_tiling_ >=
static_cast<int>(set_->tilings_.size()))
Does this still happen now? Seems like NextTiling() would loop back around to
ideal_tiling_ in the end, and you'd get stuck in here forever?

https://codereview.chromium.org/11704002/diff/4001/cc/picture_layer_tiling_set.h
File cc/picture_layer_tiling_set.h (right):

https://codereview.chromium.org/11704002/diff/4001/cc/picture_layer_tiling_se...
cc/picture_layer_tiling_set.h:41: void Reset();
Maybe for separate CL, but this function name could say what it does.

[enne (OOO), 2013-01-08 01:08:00]

On 2013/01/07 22:35:56, danakj wrote:
> Did you consider IDEAL_RES and LOW_RES instead of HIGH and LOW? It took a bit
of
> mental work to map between "high" and "ideal" when reading this.

In conversation, most folks say high-res/low-res.  The ideal situation is when
you have 1:1 texel/screen pixel ratio, which happens when contents scales of
your tiling == ideal contents scale.  This also happens to be the "high res"
version, since there's never any advantage to being higher than ideal.  That's
how it works in my head, anyway.

https://codereview.chromium.org/11704002/diff/4001/cc/picture_layer_impl.cc
File cc/picture_layer_impl.cc (right):

https://codereview.chromium.org/11704002/diff/4001/cc/picture_layer_impl.cc#n...
cc/picture_layer_impl.cc:147: CleanUpUnusedTilings(seen_tilings);
On 2013/01/07 22:35:56, danakj wrote:
> any reason why this isn't a didDraw() thing? it's going to delay getting
pixels
> on the screen to do work that's not needed to get pixels on the screen.

didDraw isn't called before swap, just FYI.

https://codereview.chromium.org/11704002/diff/4001/cc/picture_layer_impl.cc#n...
cc/picture_layer_impl.cc:341: if (layerTreeImpl()->IsPendingTree() &&
On 2013/01/07 22:35:56, danakj wrote:
> maybe a temp bool var here for this if()? then the comment inside could
probably
> be obsolete.

Why the temp bool? It doesn't get reused anywhere.  I'll clarify the comment, if
that's the confusion.

https://codereview.chromium.org/11704002/diff/4001/cc/picture_layer_impl.cc#n...
cc/picture_layer_impl.cc:366: tiling->set_resolution(NON_IDEAL_RESOLUTION);
On 2013/01/07 22:35:56, danakj wrote:
> should this be outside the else{}?

Technically, everything gets removed but low_res and high_res above, and
high_res and low_res get set to appropriate values below, so I think it's a
wash.  I suppose it'd be cleaner to do so and would make the code less
confusing.  Done.

https://codereview.chromium.org/11704002/diff/4001/cc/picture_layer_impl.cc#n...
cc/picture_layer_impl.cc:370: // Active tree creates tilings first, and the
pending tree will get them
On 2013/01/07 22:35:56, danakj wrote:
> s/ first//
> 
> I misunderstood this comment the first couple times.

The active tree gets calcDrawProperties called on it before the pending tree
does.  I'll try to clarify this comment.

https://codereview.chromium.org/11704002/diff/4001/cc/picture_layer_impl.cc#n...
cc/picture_layer_impl.cc:378: if (high_ratio >= kMaxScaleRatioDuringPinch)
On 2013/01/07 22:35:56, danakj wrote:
> can you use PositiveRatio() to simplify this area a bunch and only need one
> if()?

Oh, nice thought, done.

https://codereview.chromium.org/11704002/diff/4001/cc/picture_layer_impl.cc#n...
cc/picture_layer_impl.cc:386: } else if (!low_res)
On 2013/01/07 22:35:56, danakj wrote:
> why do you only do this if !high_res? What if you are missing tiles that have
> become visible now, would you not want a low res version of them even though
> your high res tiles are okay still?

Fixed to always add.

https://codereview.chromium.org/11704002/diff/4001/cc/picture_layer_impl.cc#n...
cc/picture_layer_impl.cc:390: // When not pinching, ensure exact contents scales
On 2013/01/07 22:35:56, danakj wrote:
> nit: period

Done.

https://codereview.chromium.org/11704002/diff/4001/cc/picture_layer_impl.cc#n...
cc/picture_layer_impl.cc:402: low_res->set_resolution(NON_IDEAL_RESOLUTION);
On 2013/01/07 22:35:56, danakj wrote:
> i don't get this. if low == high, then you set the one thing to not be either?
> Why wouldn't it still be HIGH?

I don't know what I was thinking.  Fixed.

https://codereview.chromium.org/11704002/diff/4001/cc/picture_layer_impl.cc#n...
cc/picture_layer_impl.cc:409: std::vector<PictureLayerTiling*> used) {
On 2013/01/07 22:35:56, danakj wrote:
> i don't understand the reason for this parameter, can you explain?
> 
> it seems to me that if you have a set of tilings_, then you could iterate thru
> the whole thing and remove anything that's not your current HIGH or LOW res
> tiling.
> 
> What tilings would be in tilings_ but not in used, and why would you not want
to
> remove things that were not in |used| (and therefore unused)?

Yeah, this is the important part and why I couldn't just remove tilings from the
active tree as a part of ManageTilings.

Let's say you zoom your ideal contents scale from 2 to 1. Right before you let
go of the pinch, you'll have tilings of scales 0.25 and 2.  When you release the
pinch and manage tilings, you'll have the following scales: 0.125, 0.250, 1, 2.

However, the 1 was *just created* and so likely has no content.  It's wasteful
to keep 2 around forever, so you want to get rid of 2 as soon as you don't need
it anymore.  When do you not need it anymore? When you don't use it for any
visible quads.

This is intimately tied in to the iterator change, where it tries to get tiles
from the ideal tiling first and then only non-ideal tilings second.  In this
case, your iteration would go through scales (in order) 1, 2, 0.25, 0.125.  If 1
can handle the entire content rect on its own, then you don't get any tiles from
2, and then it's not used, and therefore can be removed.

https://codereview.chromium.org/11704002/diff/4001/cc/picture_layer_impl.cc#n...
cc/picture_layer_impl.cc:410: if (ideal_contents_scale_ <= 0)
On 2013/01/07 22:35:56, danakj wrote:
> how is this possible?

This is vestigial.  I added it previously to work around user error, but is not
actually needed.  BALETED.

https://codereview.chromium.org/11704002/diff/4001/cc/picture_layer_impl.cc#n...
cc/picture_layer_impl.cc:428: if (tiling == low_res)
On 2013/01/07 22:35:56, danakj wrote:
> can you not use the HIGH_RESOLUTION and LOW_RESOLUTION bits here instead of
> doing a walk to find the low_res?

Oh, hey.  That's a really good idea.  It's almost as if I've already gone
through the trouble of marking the tilings I care about.  Done.

https://codereview.chromium.org/11704002/diff/4001/cc/picture_layer_tiling.cc
File cc/picture_layer_tiling.cc (right):

https://codereview.chromium.org/11704002/diff/4001/cc/picture_layer_tiling.cc...
cc/picture_layer_tiling.cc:270: float current_contents_scale,
On 2013/01/07 22:35:56, danakj wrote:
> not sure i'm a fan of the name "current". how does "current" here differ from
> the value inside the member variable? that seems current as well..
> 
> maybe the member variable needs a better name instead?

The tiling's contents scale is the scale for the tiling itself.

The two values passed in are layer scales.  I can add the word layer to their
name to make this more clear.

https://codereview.chromium.org/11704002/diff/4001/cc/picture_layer_tiling.cc...
cc/picture_layer_tiling.cc:300: current_screen_transform,
current_layer_content_rect);
On 2013/01/07 22:35:56, danakj wrote:
> maybe current_screen_rect and last_screen_rect then as well?

Done.

https://codereview.chromium.org/11704002/diff/4001/cc/picture_layer_tiling_se...
File cc/picture_layer_tiling_set.cc (right):

https://codereview.chromium.org/11704002/diff/4001/cc/picture_layer_tiling_se...
cc/picture_layer_tiling_set.cc:85: for (size_t i = 0; i < tilings_.size(); ++i)
{
On 2013/01/07 22:35:56, danakj wrote:
> did you consider the erase(std::remove_if()) paradigm?

I feel like remove_if only makes sense if you have more than one thing you're
potentially removing.  Writing a function here would be more lines than this for
loop.

https://codereview.chromium.org/11704002/diff/4001/cc/picture_layer_tiling_se...
cc/picture_layer_tiling_set.cc:109: for (ideal_tiling_ = 0;
On 2013/01/07 22:35:56, danakj wrote:
> can the set ever have 0 tilings? Is it okay for ideal_tiling_ to be 0 in that
> case?

Yeah, it can, but this works out.  If ideal_tiling is zero, then NextTiling
initializes current_tiling to zero, and since that's greater than the
tilings_.size(), it's done.

https://codereview.chromium.org/11704002/diff/4001/cc/picture_layer_tiling_se...
cc/picture_layer_tiling_set.cc:115: ideal_tiling_--;
On 2013/01/07 22:35:56, danakj wrote:
> this seems less "ideal" and more "best" ? should we distinguish between them?

The ideal tiling here is just the tiling that is closest to the ideal contents
scale.  Is your discomfort here that ideal maybe implies some "absolute ideal
under any circumstances" and this is really "best given what we have"?

I feel like "ideal" and "best" are pretty much synonymous here, but could rename
variables if you think that'd make it clearer.

https://codereview.chromium.org/11704002/diff/4001/cc/picture_layer_tiling_se...
cc/picture_layer_tiling_set.cc:172: else
On 2013/01/07 22:35:56, danakj wrote:
> would "else if (current_tiling_) ... else ..." make this more clear than
> switching into ?: mode suddenly in the else? 

Done.

https://codereview.chromium.org/11704002/diff/4001/cc/picture_layer_tiling_se...
cc/picture_layer_tiling_set.cc:210: if (current_tiling_ >=
static_cast<int>(set_->tilings_.size()))
On 2013/01/07 22:35:56, danakj wrote:
> Does this still happen now? Seems like NextTiling() would loop back around to
> ideal_tiling_ in the end, and you'd get stuck in here forever?

Yes, this case is definitely still hit.  This will always get hit for the first
checkerboard rect.  After that, region_iter.has_rect(), so you won't hit this if
statement.

If you have 5 tilings and index 3 is ideal, then NextTiling() returns: 3, 2, 1,
0, 4, 5.

I'm sorry this class is undertested; I think that would help explain how it's
supposed to work.

https://codereview.chromium.org/11704002/diff/4001/cc/picture_layer_tiling_set.h
File cc/picture_layer_tiling_set.h (right):

https://codereview.chromium.org/11704002/diff/4001/cc/picture_layer_tiling_se...
cc/picture_layer_tiling_set.h:41: void Reset();
On 2013/01/07 22:35:56, danakj wrote:
> Maybe for separate CL, but this function name could say what it does.

Which function? Reset does say what it does.  I can add a comment to Remove, if
that's what you mean.

[danakj, 2013-01-08 23:27:16]

https://codereview.chromium.org/11704002/diff/4001/cc/picture_layer_impl.cc
File cc/picture_layer_impl.cc (right):

https://codereview.chromium.org/11704002/diff/4001/cc/picture_layer_impl.cc#n...
cc/picture_layer_impl.cc:147: CleanUpUnusedTilings(seen_tilings);
On 2013/01/08 01:08:00, enne wrote:
> On 2013/01/07 22:35:56, danakj wrote:
> > any reason why this isn't a didDraw() thing? it's going to delay getting
> pixels
> > on the screen to do work that's not needed to get pixels on the screen.
> 
> didDraw isn't called before swap, just FYI.

I guess you mean "is". Doh..

https://codereview.chromium.org/11704002/diff/4001/cc/picture_layer_impl.cc#n...
cc/picture_layer_impl.cc:341: if (layerTreeImpl()->IsPendingTree() &&
On 2013/01/08 01:08:00, enne wrote:
> On 2013/01/07 22:35:56, danakj wrote:
> > maybe a temp bool var here for this if()? then the comment inside could
> probably
> > be obsolete.
> 
> Why the temp bool? It doesn't get reused anywhere.  I'll clarify the comment,
if
> that's the confusion.

I thought maybe giving a name to the combined state would be nice (comments by
variables), like

bool remove_non_ideal_high_and_low_res_tilings = IsPendingTree() &&
!PinchActive;
if (remove_non_ideal_high_and_low_res_tilings) {
}

Maybe not though..

https://codereview.chromium.org/11704002/diff/4001/cc/picture_layer_impl.cc#n...
cc/picture_layer_impl.cc:409: std::vector<PictureLayerTiling*> used) {
Oh, I see. |used| is a blacklist to not delete those. Somehow I read this as
deleting stuff from the used list.

https://codereview.chromium.org/11704002/diff/4001/cc/picture_layer_tiling_se...
File cc/picture_layer_tiling_set.cc (right):

https://codereview.chromium.org/11704002/diff/4001/cc/picture_layer_tiling_se...
cc/picture_layer_tiling_set.cc:85: for (size_t i = 0; i < tilings_.size(); ++i)
{
On 2013/01/08 01:08:00, enne wrote:
> On 2013/01/07 22:35:56, danakj wrote:
> > did you consider the erase(std::remove_if()) paradigm?
> 
> I feel like remove_if only makes sense if you have more than one thing you're
> potentially removing.  Writing a function here would be more lines than this
for
> loop.

Ah, ok good point.

https://codereview.chromium.org/11704002/diff/4001/cc/picture_layer_tiling_se...
cc/picture_layer_tiling_set.cc:115: ideal_tiling_--;
On 2013/01/08 01:08:00, enne wrote:
> On 2013/01/07 22:35:56, danakj wrote:
> > this seems less "ideal" and more "best" ? should we distinguish between
them?
> 
> The ideal tiling here is just the tiling that is closest to the ideal contents
> scale.  Is your discomfort here that ideal maybe implies some "absolute ideal
> under any circumstances" and this is really "best given what we have"?
> 
> I feel like "ideal" and "best" are pretty much synonymous here, but could
rename
> variables if you think that'd make it clearer.

I felt like ideal is used elsewhere to refer to the thing that exactly matches
the contents scale 1:1. whereas here, it's not, so best seemed more appropriate?

i may be misattributing accuracy on the other "ideal" cases though?

https://codereview.chromium.org/11704002/diff/4001/cc/picture_layer_tiling_se...
cc/picture_layer_tiling_set.cc:166: // Prefer hi-res tiles, but prefer the least
hi-res if bigger than ideal,
I'm still not sure what "prefer the least hi-res if bigger than ideal" means.
You could spell out the ordering like:

// Order returned by this method is:
// 1. Ideal
// 2. Tilings < Ideal in decreasing order.
// 3. Tilings > Ideal in increasing order.
// 4. tilings.size()

Or, maybe it's not needed at all with the else changes. At the moment I don't
think this comment is adding much cuz I don't get it, even though I understand
the ordering.

https://codereview.chromium.org/11704002/diff/4001/cc/picture_layer_tiling_set.h
File cc/picture_layer_tiling_set.h (right):

https://codereview.chromium.org/11704002/diff/4001/cc/picture_layer_tiling_se...
cc/picture_layer_tiling_set.h:36: void RemoveAll();
How about RemoveAllTilings() and RemoveTiling(Tiling*) ?

https://codereview.chromium.org/11704002/diff/4001/cc/picture_layer_tiling_se...
cc/picture_layer_tiling_set.h:41: void Reset();
On 2013/01/08 01:08:00, enne wrote:
> On 2013/01/07 22:35:56, danakj wrote:
> > Maybe for separate CL, but this function name could say what it does.
> 
> Which function? Reset does say what it does.  I can add a comment to Remove,
if
> that's what you mean.

// Remove all tiles, but keep all tilings.
void Reset();

could be

void RemoveAllTiles();
or
void RemoveAllTilesButNotTilings();

Reset() is just a little vague.

[enne (OOO), 2013-01-08 23:54:38]

https://codereview.chromium.org/11704002/diff/4001/cc/picture_layer_tiling_se...
File cc/picture_layer_tiling_set.cc (right):

https://codereview.chromium.org/11704002/diff/4001/cc/picture_layer_tiling_se...
cc/picture_layer_tiling_set.cc:166: // Prefer hi-res tiles, but prefer the least
hi-res if bigger than ideal,
On 2013/01/08 23:27:16, danakj wrote:
> I'm still not sure what "prefer the least hi-res if bigger than ideal" means.
> You could spell out the ordering like:
> 
> // Order returned by this method is:
> // 1. Ideal
> // 2. Tilings < Ideal in decreasing order.
> // 3. Tilings > Ideal in increasing order.
> // 4. tilings.size()
> 
> Or, maybe it's not needed at all with the else changes. At the moment I don't
> think this comment is adding much cuz I don't get it, even though I understand
> the ordering.

Done.

https://codereview.chromium.org/11704002/diff/4001/cc/picture_layer_tiling_set.h
File cc/picture_layer_tiling_set.h (right):

https://codereview.chromium.org/11704002/diff/4001/cc/picture_layer_tiling_se...
cc/picture_layer_tiling_set.h:36: void RemoveAll();
On 2013/01/08 23:27:16, danakj wrote:
> How about RemoveAllTilings() and RemoveTiling(Tiling*) ?

Done.

[danakj, 2013-01-09 00:22:49]

Some nitty things, and one real question

https://codereview.chromium.org/11704002/diff/12001/cc/picture_layer_impl.cc
File cc/picture_layer_impl.cc (right):

https://codereview.chromium.org/11704002/diff/12001/cc/picture_layer_impl.cc#...
cc/picture_layer_impl.cc:337: // Remove any tilings that don't exactly match the
contents scale from the
nit: Remove any tilings from the pending tree that don't...

https://codereview.chromium.org/11704002/diff/12001/cc/picture_layer_impl.cc#...
cc/picture_layer_impl.cc:348: else if (tiling->contents_scale() ==
low_res_contents_scale)
nit: don't need the else

https://codereview.chromium.org/11704002/diff/12001/cc/picture_layer_impl.cc#...
cc/picture_layer_impl.cc:354: tilings_.Remove(remove_list[i]);
n^2 removal :/ i take it the sizes here aren't large enough to care though?

https://codereview.chromium.org/11704002/diff/12001/cc/picture_layer_impl.cc#...
cc/picture_layer_impl.cc:357: // Find existing tilings closest to ideal high /
low res
nit: period

https://codereview.chromium.org/11704002/diff/12001/cc/picture_layer_impl.cc#...
cc/picture_layer_impl.cc:390: low_res = AddTiling(low_res_contents_scale);
why do you add a low res during pinch here? but not when the high-res is bad?

https://codereview.chromium.org/11704002/diff/12001/cc/picture_layer_impl.cc#...
cc/picture_layer_impl.cc:401: std::vector<PictureLayerTiling*> used) {
nit: used_tilings (match the name in the header)

https://codereview.chromium.org/11704002/diff/12001/cc/picture_layer_impl.cc#...
cc/picture_layer_impl.cc:414: tilings_.Remove(to_remove[i]);
n^2 again, it's okay?

https://codereview.chromium.org/11704002/diff/12001/cc/picture_layer_tiling.cc
File cc/picture_layer_tiling.cc (right):

https://codereview.chromium.org/11704002/diff/12001/cc/picture_layer_tiling.c...
cc/picture_layer_tiling.cc:269: float last_layer_contents_scale,
ya this helps, thanks

https://codereview.chromium.org/11704002/diff/12001/cc/picture_layer_tiling.h
File cc/picture_layer_tiling.h (right):

https://codereview.chromium.org/11704002/diff/12001/cc/picture_layer_tiling.h...
cc/picture_layer_tiling.h:95: float last_contents_scale,
last_layer_...

https://codereview.chromium.org/11704002/diff/12001/cc/picture_layer_tiling.h...
cc/picture_layer_tiling.h:96: float current_contents_scale,
current_layer_... (match the .cc)

[enne (OOO), 2013-01-09 00:38:37]

https://codereview.chromium.org/11704002/diff/12001/cc/picture_layer_impl.cc
File cc/picture_layer_impl.cc (right):

https://codereview.chromium.org/11704002/diff/12001/cc/picture_layer_impl.cc#...
cc/picture_layer_impl.cc:337: // Remove any tilings that don't exactly match the
contents scale from the
On 2013/01/09 00:22:49, danakj wrote:
> nit: Remove any tilings from the pending tree that don't...

Done.

https://codereview.chromium.org/11704002/diff/12001/cc/picture_layer_impl.cc#...
cc/picture_layer_impl.cc:348: else if (tiling->contents_scale() ==
low_res_contents_scale)
On 2013/01/09 00:22:49, danakj wrote:
> nit: don't need the else

No, but I'd have to put it on multiple lines anyway (thanks 80 col), and I
prefer to break up early outs to make debugging easier and increase readability.

https://codereview.chromium.org/11704002/diff/12001/cc/picture_layer_impl.cc#...
cc/picture_layer_impl.cc:354: tilings_.Remove(remove_list[i]);
On 2013/01/09 00:22:49, danakj wrote:
> n^2 removal :/ i take it the sizes here aren't large enough to care though?

Yes, n <= 4 99% of the time, and maybe n <= 6 if you make a bunch of really
quick pinches.  I should probably bound that at some point, but I can't imagine
it'll come up much.

https://codereview.chromium.org/11704002/diff/12001/cc/picture_layer_impl.cc#...
cc/picture_layer_impl.cc:357: // Find existing tilings closest to ideal high /
low res
On 2013/01/09 00:22:49, danakj wrote:
> nit: period

Done.

https://codereview.chromium.org/11704002/diff/12001/cc/picture_layer_impl.cc#...
cc/picture_layer_impl.cc:390: low_res = AddTiling(low_res_contents_scale);
On 2013/01/09 00:22:49, danakj wrote:
> why do you add a low res during pinch here? but not when the high-res is bad?

If the high res is bad, then you at least still have a high res.  If you get to
this else block during pinch, then you do not have any tilings at all (since if
you have one, high = low = that one tiling).  If you start pinching prior before
any active tree exists, it'd be nice to generate tilings and start putting up
*something*.

https://codereview.chromium.org/11704002/diff/12001/cc/picture_layer_impl.cc#...
cc/picture_layer_impl.cc:401: std::vector<PictureLayerTiling*> used) {
On 2013/01/09 00:22:49, danakj wrote:
> nit: used_tilings (match the name in the header)

Variable name short for 80 col.  /o\  Done.

https://codereview.chromium.org/11704002/diff/12001/cc/picture_layer_impl.cc#...
cc/picture_layer_impl.cc:414: tilings_.Remove(to_remove[i]);
On 2013/01/09 00:22:49, danakj wrote:
> n^2 again, it's okay?

Yes.

[danakj, 2013-01-09 00:40:20]

https://codereview.chromium.org/11704002/diff/12001/cc/picture_layer_impl.cc
File cc/picture_layer_impl.cc (right):

https://codereview.chromium.org/11704002/diff/12001/cc/picture_layer_impl.cc#...
cc/picture_layer_impl.cc:348: else if (tiling->contents_scale() ==
low_res_contents_scale)
On 2013/01/09 00:38:38, enne wrote:
> On 2013/01/09 00:22:49, danakj wrote:
> > nit: don't need the else
> 
> No, but I'd have to put it on multiple lines anyway (thanks 80 col), and I
> prefer to break up early outs to make debugging easier and increase
readability.

I mean you don't need the "else" part. Just the if.

[danakj, 2013-01-09 00:41:42]

LGTM

[commit-bot: I haz the power, 2013-01-09 00:54:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/enne@chromium.org/11704002/12004

[commit-bot: I haz the power, 2013-01-09 01:15:37]

Sorry for I got bad news for ya.
Compile failed with a clobber build on android_clang_dbg.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_cl...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-01-09 01:23:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/enne@chromium.org/11704002/15022

[commit-bot: I haz the power, 2013-01-09 02:26:08]

Sorry for I got bad news for ya.
Compile failed with a clobber build on win_rel.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-01-09 02:36:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/enne@chromium.org/11704002/17004

[commit-bot: I haz the power, 2013-01-09 07:51:21]

Failed to apply patch for cc/picture_layer_tiling_set.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file cc/picture_layer_tiling_set.cc
  Hunk #2 FAILED at 49.
  Hunk #3 FAILED at 63.
  2 out of 5 hunks FAILED -- saving rejects to file
cc/picture_layer_tiling_set.cc.rej

Patch:       cc/picture_layer_tiling_set.cc
Index: cc/picture_layer_tiling_set.cc
diff --git a/cc/picture_layer_tiling_set.cc b/cc/picture_layer_tiling_set.cc
index
9edf8479d7352583cc1effdc2e4139d884dcbb6d..9c78978c24cc01b7088e5793865551d51cb3d0e0
100644
--- a/cc/picture_layer_tiling_set.cc
+++ b/cc/picture_layer_tiling_set.cc
@@ -6,6 +6,18 @@
 
 namespace cc {
 
+namespace {
+
+class LargestToSmallestScaleFunctor {
+ public:
+  bool operator() (PictureLayerTiling* left, PictureLayerTiling* right) {
+    return left->contents_scale() > right->contents_scale();
+  }
+};
+
+}  // namespace
+
+
 PictureLayerTilingSet::PictureLayerTilingSet(
     PictureLayerTilingClient * client)
     : client_(client) {
@@ -37,6 +49,8 @@ void PictureLayerTilingSet::Clone(
   tilings_.append(tiling->Clone());
   tilings_.last()->SetClient(client_);
   tilings_.last()->Invalidate(invalidation);
+
+  std::sort(tilings_.begin(), tilings_.end(), LargestToSmallestScaleFunctor());
 }
 
 void PictureLayerTilingSet::SetLayerBounds(gfx::Size layer_bounds) {
@@ -51,27 +65,58 @@ gfx::Size PictureLayerTilingSet::LayerBounds() const {
   return layer_bounds_;
 }
 
-const PictureLayerTiling* PictureLayerTilingSet::AddTiling(
+PictureLayerTiling* PictureLayerTilingSet::AddTiling(
     float contents_scale,
     gfx::Size tile_size) {
   tilings_.append(PictureLayerTiling::Create(contents_scale, tile_size));
-  tilings_.last()->SetClient(client_);
-  tilings_.last()->SetLayerBounds(layer_bounds_);
-  return tilings_.last();
+  PictureLayerTiling* appended = tilings_.last();
+  appended->SetClient(client_);
+  appended->SetLayerBounds(layer_bounds_);
+
+  std::sort(tilings_.begin(), tilings_.end(), LargestToSmallestScaleFunctor());
+  return appended;
+}
+
+void PictureLayerTilingSet::RemoveAllTilings() {
+  tilings_.clear();
 }
 
-void PictureLayerTilingSet::Reset() {
+void PictureLayerTilingSet::Remove(PictureLayerTiling* tiling) {
+  for (size_t i = 0; i < tilings_.size(); ++i) {
+    if (tilings_[i] == tiling) {
+      tilings_.remove(i);
+      return;
+    }
+  }
+}
+
+void PictureLayerTilingSet::RemoveAllTiles() {
   for (size_t i = 0; i < tilings_.size(); ++i)
     tilings_[i]->Reset();
 }
 
-PictureLayerTilingSet::Iterator::Iterator(const PictureLayerTilingSet* set,
-                                          float contents_scale,
-                                          gfx::Rect content_rect)
+PictureLayerTilingSet::Iterator::Iterator(
+    const PictureLayerTilingSet* set,
+    float contents_scale,
+    gfx::Rect content_rect,
+    float ideal_contents_scale)
     : set_(set),
       contents_scale_(contents_scale),
+      ideal_contents_scale_(ideal_contents_scale),
       current_tiling_(-1) {
   missing_region_.Union(content_rect);
+
+  for (ideal_tiling_ = 0;
+       static_cast<size_t>(ideal_tiling_) < set_->tilings_.size();
+       ++ideal_tiling_) {
+    PictureLayerTiling* tiling = set_->tilings_[ideal_tiling_];
+    if (tiling->contents_scale() < ideal_contents_scale_) {
+      if (ideal_tiling_ > 0)
+        ideal_tiling_--;
+      break;
+    }
+  }
+
   ++(*this);
 }
 
@@ -111,6 +156,30 @@ Tile* PictureLayerTilingSet::Iterator::operator*() const {
   return *tiling_iter_;
 }
 
+PictureLayerTiling* PictureLayerTilingSet::Iterator::CurrentTiling() {
+  if (current_tiling_ < 0)
+    return NULL;
+  if (static_cast<size_t>(current_tiling_) >= set_->tilings_.size())
+    return NULL;
+  return set_->tilings_[current_tiling_];
+}
+
+int PictureLayerTilingSet::Iterator::NextTiling() const {
+  // Order returned by this method is:
+  // 1. Ideal tiling index
+  // 2. Tiling index < Ideal in decreasing order (higher res than ideal)
+  // 3. Tiling index > Ideal in increasing order (lower res than ideal)
+  // 4. Tiling index > tilings.size() (invalid index)
+  if (current_tiling_ < 0)
+    return ideal_tiling_;
+  else if (current_tiling_ > ideal_tiling_)
+    return current_tiling_ + 1;
+  else if (current_tiling_)
+    return current_tiling_ - 1;
+  else
+    return ideal_tiling_ + 1;
+}
+
 PictureLayerTilingSet::Iterator& PictureLayerTilingSet::Iterator::operator++()
{
   bool first_time = current_tiling_ < 0;
 
@@ -133,7 +202,7 @@ PictureLayerTilingSet::Iterator&
PictureLayerTilingSet::Iterator::operator++() {
     // tiling and set up to iterate through all of the remaining holes.
     // This will also happen the first time through the loop.
     if (!region_iter_.has_rect()) {
-      current_tiling_++;
+      current_tiling_ = NextTiling();
       current_region_.Swap(missing_region_);
       missing_region_.Clear();
       region_iter_ = Region::Iterator(current_region_);

[commit-bot: I haz the power, 2013-01-09 17:20:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/enne@chromium.org/11704002/20023

[commit-bot: I haz the power, 2013-01-09 23:54:35]

Change committed as 175927