Fix UpdateTilePriorities viewport in Android WebView.

cc/layers/picture_layer_impl.cc

Index: cc/layers/picture_layer_impl.cc
diff --git a/cc/layers/picture_layer_impl.cc b/cc/layers/picture_layer_impl.cc
index 82eef41e66fb833e33d691cb51c73ff6a475629e..e6454b13e9f700449ba17e860b26b0160103b929 100644
--- a/cc/layers/picture_layer_impl.cc
+++ b/cc/layers/picture_layer_impl.cc
@@ -293,6 +293,9 @@ void PictureLayerImpl::AppendQuads(QuadSink* quad_sink,
 
 void PictureLayerImpl::UpdateTilePriorities() {
   DCHECK(!needs_post_commit_initialization_);
+  if (!layer_tree_impl()->device_viewport_valid_for_tile_management())

[enne (OOO), 2013/09/03 16:58:47]

If you're going to early out here, can you also DCHECK that each tiling
has_ever_been_updated?

I think you also need to handle this case in CalculateContentsScales, possibly
by skipping the ManageTilings call.  If the scale from the external transform is
different than the normal one, then layers will create tilings needlessly.

[aelias_OOO_until_Jul13, 2013/09/03 19:52:09]

Done.

+    return;
+
   if (!tilings_->num_tilings())
     return;
 
@@ -318,12 +321,13 @@ void PictureLayerImpl::UpdateTilePriorities() {
 
   gfx::Transform current_screen_space_transform = screen_space_transform();
 
+  gfx::Size device_viewport_size = layer_tree_impl()->DeviceViewport().size();

[enne (OOO), 2013/09/03 16:58:47]

Can you either use DeviceViewport() directly without dropping its location or
DCHECK that the location is the origin?

In general, it seems awkward to pass a full viewport rect, but then only use its
size in some places.  Having a full transform and a viewport rect seems like
there's redundant information.  Could we keep the assumption that the viewport
is always at the origin and just bake any viewport offsets into the external
transform?  Isn't that what AdjustTransform is already doing?

[aelias_OOO_until_Jul13, 2013/09/03 19:52:09]

This is a pretty subtle question but I've convinced myself the current
implementation is appropriate and we cannot drop the offset from the viewport
rect.

A key point here is that there are two confusingly similar spaces which I've
been calling in the code "draw space" and "window space".  In both spaces a
single 1x1 unit corresponds to a pixel, but draw space is always rooted at the
origin, whereas window space is offsetted and flipped.  Layers and quads are
targeted to draw space, then transformed into the unit square by the projection
matrix, then transformed into window space by the glViewport (in GLRenderer) or
equivalently the window matrix (in SoftwareRenderer).  When I use
"DeviceViewport().size()" it's because that's a draw-space thing, when I use the
full DeviceViewport() rect it's for a window-space thing.

Furthermore, the transform is in draw space.  The transform + viewport size
tells us which content pixels are visible.  If we baked the window-space offset
of the viewport into the transform, we would end up with a strange idea of what
pixels are onscreen (very likely extending outside the document bounds).  So we
split off the origin of the visibility rect on the physical screen into a
separate value applied later.

(One possible path to address some of your concerns would be to redefine "draw
space" to not necessarily be rooted at the origin.  If we did that, then we
wouldn't need to extract DeviceViewport().size() and we wouldn't need to adjust
the transform.  I tried to do this a month ago  but the assumption of
rooted-at-origin is deeply embedded in CC.  It didn't seem a good idea to change
a lot of code to use a rect instead of a size, introducing a lot of
opportunities for bugs if somebody forgets to consider the rect offset since
it's usually zero, when I can instead apply the offset in a superficial and
reliable way in DirectRenderer.)

[enne (OOO), 2013/09/03 21:47:31]

Aha!  Thanks, I really appreciate the explanation of the distinction here.

I do agree with you that redefining draw space to not be at the origin might
confuse somebody, but at the same time having these two different spaces come
from the same variable (more or less) seems also prone to error.

Is there some way to name or differentiate these better? I mean, maybe it's as
simple as having DeviceViewport only used by DirectRenderer and
DeviceViewportSize used elsewhere.

I think I could get behind a policy where DeviceViewport was only used by
DirectRenderer and device_viewport_size as before is used everywhere else, with
a comment in LTHI to document these different spaces.

Maybe they could also be called DeviceDrawViewport(Size?) and
DeviceWindowViewport? I realize that's getting kind of wordy, and maybe that's
overkill.

[aelias_OOO_until_Jul13, 2013/09/04 01:22:23]

OK, I went with "DrawViewportSize" and "DeviceViewport", and added more comments
in the LTHI header.  Now only the Renderer subclasses use DeviceViewport() and
you can only obtain the size via the LayerTreeImpl.  I also renamed
"DeviceTransform" to "DrawTransform" to connote it's a draw space concept.

   gfx::Rect viewport_in_content_space;
   gfx::Transform screen_to_layer(gfx::Transform::kSkipInitialization);
   if (screen_space_transform().GetInverse(&screen_to_layer)) {
-    gfx::Rect device_viewport(layer_tree_impl()->device_viewport_size());
-    viewport_in_content_space = gfx::ToEnclosingRect(
-        MathUtil::ProjectClippedRect(screen_to_layer, device_viewport));
+    viewport_in_content_space =
+        gfx::ToEnclosingRect(MathUtil::ProjectClippedRect(
+            screen_to_layer, gfx::Rect(device_viewport_size)));
   }
 
   WhichTree tree =
@@ -332,7 +336,7 @@ void PictureLayerImpl::UpdateTilePriorities() {
       layer_tree_impl()->settings().max_tiles_for_interest_area;
   tilings_->UpdateTilePriorities(
       tree,
-      layer_tree_impl()->device_viewport_size(),
+      device_viewport_size,
       viewport_in_content_space,
       visible_content_rect(),
       last_bounds_,