implement the logic to set tile priorities based on current matrix

implement the logic to set tile priorities based on current matrix


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

[qinmin, 2012-11-29 20:52:15]

PTAL

[epennerAtGoogle, 2012-11-29 22:07:50]

https://codereview.chromium.org/11414238/diff/1/cc/picture_layer_tiling.cc
File cc/picture_layer_tiling.cc (right):

https://codereview.chromium.org/11414238/diff/1/cc/picture_layer_tiling.cc#ne...
cc/picture_layer_tiling.cc:294: gfx::Rect screen_rect =
MathUtil::mapClippedRect(
I think this might bite us performance-wise if we do this for every tile. Same
goes for using doubles in time-bounds calculation.

However, better to not prematurely optimize so we can wait for it to show up on
a trace.

https://codereview.chromium.org/11414238/diff/1/cc/picture_layer_tiling.cc#ne...
cc/picture_layer_tiling.cc:303: 
I think for painting we are better off using manhattan distance (x+y) since
tiles show up in rows/columns. We could just copy manhattanDistance(rect,rect)
inside priority_calculator.cc?

https://codereview.chromium.org/11414238/diff/1/cc/picture_layer_tiling.cc#ne...
cc/picture_layer_tiling.cc:324: double
PictureLayerTiling::TimeForBoundsToIntersect(gfx::Rect previous_bounds,
This is super clever.

https://codereview.chromium.org/11414238/diff/1/cc/picture_layer_tiling.cc#ne...
cc/picture_layer_tiling.cc:338: // overlap between these time ranges, the bounds
must have intersect with
I was initially confused by the comment but now I think I get it. Each edge
calculates a one-sided range and then when you intersect them all you get the
the predicted time range of intersection?

It seems like a line/rect intersect would likely be close enough to this. But
then it needs adjustments to be conservative, so if it's not a perf issue I
really like this.

[qinmin, 2012-11-29 22:44:17]

https://codereview.chromium.org/11414238/diff/1/cc/picture_layer_tiling.cc
File cc/picture_layer_tiling.cc (right):

https://codereview.chromium.org/11414238/diff/1/cc/picture_layer_tiling.cc#ne...
cc/picture_layer_tiling.cc:303: 
I like the idea of manhattan distance, it saves the squareroot operation and
makes more sense for tiling.
Copied it over from priority_calculator.cc as that file might go away.

On 2012/11/29 22:07:50, epennerAtGoogle wrote:
> I think for painting we are better off using manhattan distance (x+y) since
> tiles show up in rows/columns. We could just copy manhattanDistance(rect,rect)
> inside priority_calculator.cc?

[epennerAtGoogle, 2012-11-29 22:52:54]

I think this is a pretty good start. Enne wdyt? 

My concerns are only performance related, so might as well see if it becomes a
performance problem first.

https://codereview.chromium.org/11414238/diff/1/cc/picture_layer_tiling.cc
File cc/picture_layer_tiling.cc (right):

https://codereview.chromium.org/11414238/diff/1/cc/picture_layer_tiling.cc#ne...
cc/picture_layer_tiling.cc:292: gfx::Rect layer_rect = gfx::ToEnclosingRect(
Question for Enne. Is this scale correct in order to use the screen space
transform?

[danakj, 2012-11-29 22:57:30]

https://codereview.chromium.org/11414238/diff/1/cc/picture_layer_tiling.cc
File cc/picture_layer_tiling.cc (right):

https://codereview.chromium.org/11414238/diff/1/cc/picture_layer_tiling.cc#ne...
cc/picture_layer_tiling.cc:295: current_transform, layer_rect);
What is current_transform? If it's the screen space transform
(current_screenspace_transform?) then you want to give it the content_rect, not
the layer_rect.. unless this isn't the same screenSpaceTransform() found on
other layers.

[epennerAtGoogle, 2012-11-29 23:02:58]

https://codereview.chromium.org/11414238/diff/1/cc/picture_layer_tiling.cc
File cc/picture_layer_tiling.cc (right):

https://codereview.chromium.org/11414238/diff/1/cc/picture_layer_tiling.cc#ne...
cc/picture_layer_tiling.cc:295: current_transform, layer_rect);
On 2012/11/29 22:57:30, danakj wrote:
> What is current_transform? If it's the screen space transform
> (current_screenspace_transform?) then you want to give it the content_rect,
not
> the layer_rect.. unless this isn't the same screenSpaceTransform() found on
> other layers.

The trouble is there is multiple content scales, so what does content_rect mean
in this context? So we figured screenSpaceTransform for this layer must be for
content_scale = 1.

[qinmin, 2012-11-30 00:35:00]

https://codereview.chromium.org/11414238/diff/1/cc/picture_layer_tiling.cc
File cc/picture_layer_tiling.cc (right):

https://codereview.chromium.org/11414238/diff/1/cc/picture_layer_tiling.cc#ne...
cc/picture_layer_tiling.cc:295: current_transform, layer_rect);
Ok, passing the content_scale from the layer to this function. I think the
content_scale_ in this class is pretty confusing. Probably call it tiling_scale_
might be better. Anyway, we need to remove the content_scale_ applied to the
content_rect, and then apply the layer content_scale here.

On 2012/11/29 22:57:30, danakj wrote:
> What is current_transform? If it's the screen space transform
> (current_screenspace_transform?) then you want to give it the content_rect,
not
> the layer_rect.. unless this isn't the same screenSpaceTransform() found on
> other layers.

[nduca, 2012-11-30 08:47:24]

Who are our deciders here? I think its Enne... this makes things MUCH better, so
I'd like to see this land on Friday if humanly possible. But, Enne's are scarce
resources tomorrow. Is anyone != Enne qualified to approve-ish?

[epennerAtGoogle, 2012-11-30 16:48:34]

lgtm

I think we can land and follow up? Especially if Dana LGTMs as well. 

Nat: My last thought is that the math functions
(Range/TimeForBoundsToIntersect/TimeRangeValueLargerThanThreshold) should really
be in a utility class instead of picture_layer_tiling. I would say
CCPriorityCalculator actually, but do we want to keep the code separate for now?
Also, if Tile had a back pointer to their owning layer, Tiles could calculate
their own priorities which might be nice.

https://codereview.chromium.org/11414238/diff/7002/cc/picture_layer_impl.cc
File cc/picture_layer_impl.cc (right):

https://codereview.chromium.org/11414238/diff/7002/cc/picture_layer_impl.cc#n...
cc/picture_layer_impl.cc:97: if (last_update_time_ != 0) {
Should we move this condition into UpdateTilePriorities? We can still calculate
the distance, just not the velocity.

https://codereview.chromium.org/11414238/diff/7002/cc/picture_layer_impl.h
File cc/picture_layer_impl.h (right):

https://codereview.chromium.org/11414238/diff/7002/cc/picture_layer_impl.h#ne...
cc/picture_layer_impl.h:47: gfx::Transform last_transform_;
Can you add "screenspace" to the name. There are so many transforms it's
important to know which one.

[danakj, 2012-11-30 18:39:00]

https://codereview.chromium.org/11414238/diff/7002/cc/picture_layer_impl.cc
File cc/picture_layer_impl.cc (right):

https://codereview.chromium.org/11414238/diff/7002/cc/picture_layer_impl.cc#n...
cc/picture_layer_impl.cc:99: contentsScaleX(),
can you DCHECK that contentsScaleX() == contentsScaleY()?

[nduca, 2012-11-30 18:50:21]

Definitely keep it out of priority calculator for the reasons you mention.

Tiles cant have backpointers because a tile can be in two layers.

[qinmin, 2012-11-30 19:24:13]

https://codereview.chromium.org/11414238/diff/7002/cc/picture_layer_impl.cc
File cc/picture_layer_impl.cc (right):

https://codereview.chromium.org/11414238/diff/7002/cc/picture_layer_impl.cc#n...
cc/picture_layer_impl.cc:97: if (last_update_time_ != 0) {
using timedelta 0 when this happens, so that we won't calculate velocity in that
case.

On 2012/11/30 16:48:34, epennerAtGoogle wrote:
> Should we move this condition into UpdateTilePriorities? We can still
calculate
> the distance, just not the velocity.

https://codereview.chromium.org/11414238/diff/7002/cc/picture_layer_impl.cc#n...
cc/picture_layer_impl.cc:99: contentsScaleX(),
On 2012/11/30 18:39:00, danakj wrote:
> can you DCHECK that contentsScaleX() == contentsScaleY()?

Done.

https://codereview.chromium.org/11414238/diff/7002/cc/picture_layer_impl.h
File cc/picture_layer_impl.h (right):

https://codereview.chromium.org/11414238/diff/7002/cc/picture_layer_impl.h#ne...
cc/picture_layer_impl.h:47: gfx::Transform last_transform_;
On 2012/11/30 16:48:34, epennerAtGoogle wrote:
> Can you add "screenspace" to the name. There are so many transforms it's
> important to know which one.

Done.

[epenner, 2012-11-30 19:29:31]

https://codereview.chromium.org/11414238/diff/7002/cc/picture_layer_tiling.cc
File cc/picture_layer_tiling.cc (right):

https://codereview.chromium.org/11414238/diff/7002/cc/picture_layer_tiling.cc...
cc/picture_layer_tiling.cc:310: TilePriority priority;
Sorry if this is annoying, but after looking at the code, I think
tile_priority.h/cc would be a nice place for most of the math bits eg.
static TilePriority TilePriority::CalculatePriority...
wdyt?

[qinmin, 2012-11-30 20:32:35]

https://codereview.chromium.org/11414238/diff/7002/cc/picture_layer_tiling.cc
File cc/picture_layer_tiling.cc (right):

https://codereview.chromium.org/11414238/diff/7002/cc/picture_layer_tiling.cc...
cc/picture_layer_tiling.cc:310: TilePriority priority;
Done, the math thingy are moved to TilePriority. And unittest also moved there

On 2012/11/30 19:29:31, epenner wrote:
> Sorry if this is annoying, but after looking at the code, I think
> tile_priority.h/cc would be a nice place for most of the math bits eg.
> static TilePriority TilePriority::CalculatePriority...
> wdyt?

[epennerAtGoogle, 2012-11-30 20:56:34]

lgtm

Looking good to me. Certainly the structure seems right, and we can iterate on
the actual math etc. if there is any issues.

[danakj, 2012-12-01 00:07:31]

https://codereview.chromium.org/11414238/diff/10002/cc/picture_layer_tiling.cc
File cc/picture_layer_tiling.cc (right):

https://codereview.chromium.org/11414238/diff/10002/cc/picture_layer_tiling.c...
cc/picture_layer_tiling.cc:288: gfx::ScaleRect(content_rect, layer_content_scale
/ contents_scale_));
Maybe you should just pass in both content scale X and Y and scale the rect by
both layer_scale_x / contents_scale_ and layer_scale_y / contents_scale_.

https://codereview.chromium.org/11414238/diff/10002/cc/picture_layer_tiling.c...
cc/picture_layer_tiling.cc:292: last_screen_transform, layer_content_rect);
Don't you need the last_layer_content_rect here to be precise? What if the
bounds or layer contents scale or tiling contents scale changed?

[qinmin, 2012-12-01 01:07:02]

https://codereview.chromium.org/11414238/diff/10002/cc/picture_layer_tiling.cc
File cc/picture_layer_tiling.cc (right):

https://codereview.chromium.org/11414238/diff/10002/cc/picture_layer_tiling.c...
cc/picture_layer_tiling.cc:288: gfx::ScaleRect(content_rect, layer_content_scale
/ contents_scale_));
On 2012/12/01 00:07:31, danakj wrote:
> Maybe you should just pass in both content scale X and Y and scale the rect by
> both layer_scale_x / contents_scale_ and layer_scale_y / contents_scale_.

Done.

https://codereview.chromium.org/11414238/diff/10002/cc/picture_layer_tiling.c...
cc/picture_layer_tiling.cc:292: last_screen_transform, layer_content_rect);
Yes, this is some of our concerns.
If tiling_content_scale is changed, a new tiling will be created. So we
shouldn't be worried about that.
For layer_contents_scale change or layer bounds change, we can simply invalidate
the previous information by setting time_delta to 0. So we will not compute the
velocity in this case. 
I have changed picture_layer_impl.cc to override the setContentsScale(),
setContentBounds() and setBounds() function.


On 2012/12/01 00:07:31, danakj wrote:
> Don't you need the last_layer_content_rect here to be precise? What if the
> bounds or layer contents scale or tiling contents scale changed?

[danakj, 2012-12-01 01:20:19]

https://codereview.chromium.org/11414238/diff/11014/cc/picture_layer_tiling.cc
File cc/picture_layer_tiling.cc (right):

https://codereview.chromium.org/11414238/diff/11014/cc/picture_layer_tiling.c...
cc/picture_layer_tiling.cc:288: gfx::Rect layer_content_rect =
gfx::ToEnclosingRect(
Why go to a Rect and take the enclosing rect here? Seems like you could just
keep it as a RectF and not have to do rounding.

[qinmin, 2012-12-01 01:36:44]

https://codereview.chromium.org/11414238/diff/11014/cc/picture_layer_tiling.cc
File cc/picture_layer_tiling.cc (right):

https://codereview.chromium.org/11414238/diff/11014/cc/picture_layer_tiling.c...
cc/picture_layer_tiling.cc:288: gfx::Rect layer_content_rect =
gfx::ToEnclosingRect(
if we keep it rectF, then the following calculations will also needs to be
rectF. 
But using rectF doesn't add too much value to calculate the visible_in_seconds
and distance_to_visible_in_seconds. These values should just be a rough
estimation, especially since we are using manhattan instead of euclidean
distance, and velocity calculation does not take friction into consideration.

On 2012/12/01 01:20:19, danakj wrote:
> Why go to a Rect and take the enclosing rect here? Seems like you could just
> keep it as a RectF and not have to do rounding.

[danakj, 2012-12-01 01:58:25]

https://codereview.chromium.org/11414238/diff/11014/cc/picture_layer_tiling.cc
File cc/picture_layer_tiling.cc (right):

https://codereview.chromium.org/11414238/diff/11014/cc/picture_layer_tiling.c...
cc/picture_layer_tiling.cc:288: gfx::Rect layer_content_rect =
gfx::ToEnclosingRect(
On 2012/12/01 01:36:44, qinmin wrote:
> if we keep it rectF, then the following calculations will also needs to be
> rectF. 
> But using rectF doesn't add too much value to calculate the visible_in_seconds
> and distance_to_visible_in_seconds. These values should just be a rough
> estimation, especially since we are using manhattan instead of euclidean
> distance, and velocity calculation does not take friction into consideration.

The mapRect function is going to convert it to float at each step anyways. And
then you just lost precision, I don't see the advantange. I'd only go back to
ints if it's going to add precision. Otherwise keep it all floats as long as you
can.

[qinmin, 2012-12-01 02:31:20]

https://codereview.chromium.org/11414238/diff/11014/cc/picture_layer_tiling.cc
File cc/picture_layer_tiling.cc (right):

https://codereview.chromium.org/11414238/diff/11014/cc/picture_layer_tiling.c...
cc/picture_layer_tiling.cc:288: gfx::Rect layer_content_rect =
gfx::ToEnclosingRect(
ok, switched to rectF for all the remaining calculations

On 2012/12/01 01:58:25, danakj wrote:
> On 2012/12/01 01:36:44, qinmin wrote:
> > if we keep it rectF, then the following calculations will also needs to be
> > rectF. 
> > But using rectF doesn't add too much value to calculate the
visible_in_seconds
> > and distance_to_visible_in_seconds. These values should just be a rough
> > estimation, especially since we are using manhattan instead of euclidean
> > distance, and velocity calculation does not take friction into
consideration.
> 
> The mapRect function is going to convert it to float at each step anyways. And
> then you just lost precision, I don't see the advantange. I'd only go back to
> ints if it's going to add precision. Otherwise keep it all floats as long as
you
> can.

[danakj, 2012-12-01 02:37:37]

The scale stuff seems alright to me, I can't really speak to the picture layer
specific stuff, as I haven't been keeping up with plans/etc for it. Not sure if
you want to have enne look at it too.

Thanks for the scaling fixes :)

https://codereview.chromium.org/11414238/diff/8004/cc/tile_priority.h
File cc/tile_priority.h (right):

https://codereview.chromium.org/11414238/diff/8004/cc/tile_priority.h#newcode8
cc/tile_priority.h:8: #include <limits>
seems unused

[qinmin, 2012-12-01 02:45:24]

https://codereview.chromium.org/11414238/diff/8004/cc/tile_priority.h
File cc/tile_priority.h (right):

https://codereview.chromium.org/11414238/diff/8004/cc/tile_priority.h#newcode8
cc/tile_priority.h:8: #include <limits>
this is for the std::numeric_limits in TilePriority ctor.

On 2012/12/01 02:37:37, danakj wrote:
> seems unused

[enne (OOO), 2012-12-03 19:57:27]

https://codereview.chromium.org/11414238/diff/8004/cc/picture_layer_tiling.cc
File cc/picture_layer_tiling.cc (right):

https://codereview.chromium.org/11414238/diff/8004/cc/picture_layer_tiling.cc...
cc/picture_layer_tiling.cc:278: gfx::Rect countent_rect = ContentRect();
countent => content

https://codereview.chromium.org/11414238/diff/8004/cc/picture_layer_tiling.cc...
cc/picture_layer_tiling.cc:285: for (int j = 0; j <= bottom; ++j) {
For correctness, do you need to iterate over all tiles here rather than just the
visible ones? Won't previous tiles that have been marked as
distance_to_visible=0 but have now moved off screen have a stale priority?

https://codereview.chromium.org/11414238/diff/8004/cc/picture_layer_tiling.cc...
cc/picture_layer_tiling.cc:287: gfx::Rect content_rect =
tiling_data_.TileBounds(i, j);
I think your math is wrong here for layer content scales != 1.  Can you add a
test where this is the case?

TileBounds is a tile's bounds within some tiling's particular scale.  If you
want to use screen transform, then you need to also transform to the layer's
scale.  See the dest_to_content_scale_ math elsewhere in PictureLayerTiling.

https://codereview.chromium.org/11414238/diff/8004/cc/picture_layer_tiling.cc...
cc/picture_layer_tiling.cc:301: previous_rect, screen_rect, time_delta,
view_rect);
Oh, interesting!  So rather than caring about all of the clips along the way,
you just look at where the screen space is and how quickly something is moving
to get into the screen space rect.  I suppose on complicated pages this is
wrong, but it seems like it should solve the use cases we care about.  This
seems like a good start.

https://codereview.chromium.org/11414238/diff/8004/cc/picture_layer_tiling.h
File cc/picture_layer_tiling.h (right):

https://codereview.chromium.org/11414238/diff/8004/cc/picture_layer_tiling.h#...
cc/picture_layer_tiling.h:89: void UpdateTilePriorities(const gfx::Size&
view_port,
naming nit: device_viewport? layout_viewport?

https://codereview.chromium.org/11414238/diff/8004/cc/picture_layer_tiling.h#...
cc/picture_layer_tiling.h:90: float layer_content_scaleX,
scaleX => scale_x

https://codereview.chromium.org/11414238/diff/8004/cc/tile_priority.cc
File cc/tile_priority.cc (right):

https://codereview.chromium.org/11414238/diff/8004/cc/tile_priority.cc#newcode56
cc/tile_priority.cc:56: int TilePriority::manhattanDistance(const gfx::RectF& a,
const gfx::RectF& b) {
Can you write a quick unit test for this math?

[enne (OOO), 2012-12-03 22:08:53]

https://codereview.chromium.org/11414238/diff/8004/cc/picture_layer_tiling.cc
File cc/picture_layer_tiling.cc (right):

https://codereview.chromium.org/11414238/diff/8004/cc/picture_layer_tiling.cc...
cc/picture_layer_tiling.cc:287: gfx::Rect content_rect =
tiling_data_.TileBounds(i, j);
On 2012/12/03 19:57:28, enne wrote:
> I think your math is wrong here for layer content scales != 1.  Can you add a
> test where this is the case?
> 
> TileBounds is a tile's bounds within some tiling's particular scale.  If you
> want to use screen transform, then you need to also transform to the layer's
> scale.  See the dest_to_content_scale_ math elsewhere in PictureLayerTiling.

Actually, scratch that.  I think your math is right.  You are converting from
tiling content scale to layer content scale.

[qinmin, 2012-12-04 00:37:29]

Thanks, Enne

https://codereview.chromium.org/11414238/diff/8004/cc/picture_layer_tiling.cc
File cc/picture_layer_tiling.cc (right):

https://codereview.chromium.org/11414238/diff/8004/cc/picture_layer_tiling.cc...
cc/picture_layer_tiling.cc:278: gfx::Rect countent_rect = ContentRect();
On 2012/12/03 19:57:28, enne wrote:
> countent => content

Done.

https://codereview.chromium.org/11414238/diff/8004/cc/picture_layer_tiling.cc...
cc/picture_layer_tiling.cc:285: for (int j = 0; j <= bottom; ++j) {
On 2012/12/03 19:57:28, enne wrote:
> For correctness, do you need to iterate over all tiles here rather than just
the
> visible ones? Won't previous tiles that have been marked as
> distance_to_visible=0 but have now moved off screen have a stale priority?

Done. looping throught the whole hash map to set priorities of all tiles.

https://codereview.chromium.org/11414238/diff/8004/cc/picture_layer_tiling.cc...
cc/picture_layer_tiling.cc:301: previous_rect, screen_rect, time_delta,
view_rect);
Yes, this is an efficient way to deal with the scaling/scrolling case. For other
transformations, this might not work well.

On 2012/12/03 19:57:28, enne wrote:
> Oh, interesting!  So rather than caring about all of the clips along the way,
> you just look at where the screen space is and how quickly something is moving
> to get into the screen space rect.  I suppose on complicated pages this is
> wrong, but it seems like it should solve the use cases we care about.  This
> seems like a good start.

https://codereview.chromium.org/11414238/diff/8004/cc/picture_layer_tiling.h
File cc/picture_layer_tiling.h (right):

https://codereview.chromium.org/11414238/diff/8004/cc/picture_layer_tiling.h#...
cc/picture_layer_tiling.h:89: void UpdateTilePriorities(const gfx::Size&
view_port,
On 2012/12/03 19:57:28, enne wrote:
> naming nit: device_viewport? layout_viewport?

Done.

https://codereview.chromium.org/11414238/diff/8004/cc/picture_layer_tiling.h#...
cc/picture_layer_tiling.h:90: float layer_content_scaleX,
On 2012/12/03 19:57:28, enne wrote:
> scaleX => scale_x

Done.

https://codereview.chromium.org/11414238/diff/8004/cc/tile_priority.cc
File cc/tile_priority.cc (right):

https://codereview.chromium.org/11414238/diff/8004/cc/tile_priority.cc#newcode56
cc/tile_priority.cc:56: int TilePriority::manhattanDistance(const gfx::RectF& a,
const gfx::RectF& b) {
On 2012/12/03 19:57:28, enne wrote:
> Can you write a quick unit test for this math?

Done.

[enne (OOO), 2012-12-04 01:13:27]

https://codereview.chromium.org/11414238/diff/6015/cc/picture_layer_impl.h
File cc/picture_layer_impl.h (right):

https://codereview.chromium.org/11414238/diff/6015/cc/picture_layer_impl.h#ne...
cc/picture_layer_impl.h:41: void setBounds(const gfx::Size&);
These three functions shadow the LayerImpl implementation.  I do not see how
they can get called.

I suspect you need to set these directly from PictureLayer::pushPropertiesTo
prior to calling Layer::pushPropertiesTo.

https://codereview.chromium.org/11414238/diff/6015/cc/picture_layer_tiling.cc
File cc/picture_layer_tiling.cc (right):

https://codereview.chromium.org/11414238/diff/6015/cc/picture_layer_tiling.cc...
cc/picture_layer_tiling.cc:272: const gfx::Size& device_view_port,
Sorry for the nit, but viewport is one word everywhere else in cc.

https://codereview.chromium.org/11414238/diff/6015/cc/tile_priority.cc
File cc/tile_priority.cc (right):

https://codereview.chromium.org/11414238/diff/6015/cc/tile_priority.cc#newcode74
cc/tile_priority.cc:74: if (previous_bounds.Intersects(target_bounds) ||
time_delta == 0)
Why is it the case that if the previous bounds but not the current bounds
intersect the target bounds that it's kMaxTime?

[qinmin, 2012-12-04 01:39:53]

https://codereview.chromium.org/11414238/diff/6015/cc/picture_layer_impl.h
File cc/picture_layer_impl.h (right):

https://codereview.chromium.org/11414238/diff/6015/cc/picture_layer_impl.h#ne...
cc/picture_layer_impl.h:41: void setBounds(const gfx::Size&);
These functions are to check whether bounds/scale changes. Layer don't have
scale info. Anyway, changed the code to record last scale and last bounds.
On 2012/12/04 01:13:27, enne wrote:
> These three functions shadow the LayerImpl implementation.  I do not see how
> they can get called.
> 
> I suspect you need to set these directly from PictureLayer::pushPropertiesTo
> prior to calling Layer::pushPropertiesTo.

https://codereview.chromium.org/11414238/diff/6015/cc/picture_layer_tiling.cc
File cc/picture_layer_tiling.cc (right):

https://codereview.chromium.org/11414238/diff/6015/cc/picture_layer_tiling.cc...
cc/picture_layer_tiling.cc:272: const gfx::Size& device_view_port,
On 2012/12/04 01:13:27, enne wrote:
> Sorry for the nit, but viewport is one word everywhere else in cc.

Done.

https://codereview.chromium.org/11414238/diff/6015/cc/tile_priority.cc
File cc/tile_priority.cc (right):

https://codereview.chromium.org/11414238/diff/6015/cc/tile_priority.cc#newcode74
cc/tile_priority.cc:74: if (previous_bounds.Intersects(target_bounds) ||
time_delta == 0)
so if the previous bounds intersect, and the current biounds is not, which means
the bounds is moving away from the target, so we set it to maxTime. This is
always true for scrolling/scaling case, but may not hold for rotation. But since
we have distance, we can use that info in case.


On 2012/12/04 01:13:27, enne wrote:
> Why is it the case that if the previous bounds but not the current bounds
> intersect the target bounds that it's kMaxTime?

[enne (OOO), 2012-12-04 02:26:33]

lgtm

[commit-bot: I haz the power, 2012-12-04 03:22:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/qinmin@chromium.org/11414238/14003

[commit-bot: I haz the power, 2012-12-04 05:59:45]

Retried try job too often on win_rel for step(s) browser_tests

[commit-bot: I haz the power, 2012-12-04 06:04:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/qinmin@chromium.org/11414238/14003

[commit-bot: I haz the power, 2012-12-04 08:53:02]

Change committed as 170908