Make new-style page scale work on Android.

Make new-style page scale work on Android.

- Fix CC's PinchZoomViewport and MaxScrollOffset logic to work in fixed
  layout mode.  Root layer max scroll offset is based on
  WebKit-perceived outer viewport and therefore should logically use
  layout_viewport_size_, whereas the inner one is the true user-visible
  viewport and therefore should use device_viewport_size_.

- Fix Android to send down viewport size in DIP when
  --enable-css-transform-pinch is not specified.  Default to
  non-CSS-transform mode when in impl-side painting.

- Make ContentSize() return bounds() instead of contentBounds() and
  rename to ScrollableSize for clarity.  This lets us avoid coordinate
  space conversions when computing max scroll offset.

BUG=152505
NOTRY=true

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

[aelias_OOO_until_Jul13, 2013-01-15 23:55:50]

PTAL.  WebKit-side of this change at:
https://bugs.webkit.org/show_bug.cgi?id=106951

[aelias_OOO_until_Jul13, 2013-01-17 00:21:02]

Dana, could you review this change?  We'd like to land it ASAP to unblock more
work on impl-side painting, and James said he's too busy.

[danakj, 2013-01-17 18:02:48]

I like the approach, but I think there's some inconsistency introduced here with
old code in physical pixels vs new in logical pixels that needs to be resolved?
Sorry if I'm a little nitpicky about names but I find they are the only thing
that makes understanding all these spaces possible over time.

I just reviewed cc/ as I don't think I'm familiar enough with the rest to
understand what's happening there properly at the moment.

https://codereview.chromium.org/11958004/diff/3001/cc/layer_tree_host_impl.cc
File cc/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/11958004/diff/3001/cc/layer_tree_host_impl.cc...
cc/layer_tree_host_impl.cc:257: gfx::SizeF scaledContentSize =
activeTree()->ScrollableSize();
rename scaledContentSize to what it now holds as well please

https://codereview.chromium.org/11958004/diff/3001/cc/layer_tree_host_impl.cc...
cc/layer_tree_host_impl.cc:265: m_pageScaleAnimation =
PageScaleAnimation::create(scrollTotal,
m_pinchZoomViewport.total_page_scale_factor(), viewportSize, scaledContentSize,
startTimeSeconds);
Without a change to PageScaleAnimation how does this continue to work? You're
passing in the logical scroll size now instead of the physical scroll size,
right?

https://codereview.chromium.org/11958004/diff/3001/cc/layer_tree_host_impl.cc...
cc/layer_tree_host_impl.cc:1370: gfx::SizeF scaledContentsSize =
gfx::ScaleSize(activeTree()->ScrollableSize(), pageScaleDeltaToSend);
rename scaledContentsSize here also

https://codereview.chromium.org/11958004/diff/3001/cc/layer_tree_host_impl.cc...
cc/layer_tree_host_impl.cc:1376:
scrollEnd.ClampToMax(gfx::RectF(scaledContentsSize).bottom_right() -
gfx::Rect(m_deviceViewportSize).bottom_right());
You mentioned that scrolling should all be in logical pixels now, so
scaledContentsSize should be the logical scroll size (ie not content/phsyical
pixels)?. So, should we be subtracting the logical viewport size here then?

https://codereview.chromium.org/11958004/diff/3001/cc/layer_tree_impl.cc
File cc/layer_tree_impl.cc (right):

https://codereview.chromium.org/11958004/diff/3001/cc/layer_tree_impl.cc#newc...
cc/layer_tree_impl.cc:94: view_bounds = gfx::ScaleSize(clip_layer->bounds(),
device_scale_factor());
This bound here (and above) is in physical pixels, but the else{} clause is in
logical pixels? It seems like this code could be using the layout viewport size
and not scaling the clip layer's bounds?

https://codereview.chromium.org/11958004/diff/3001/cc/layer_tree_impl.cc#newc...
cc/layer_tree_impl.cc:101: gfx::Vector2dF max_scroll =
gfx::Rect(ScrollableSize()).bottom_right() -
I do like this :)

https://codereview.chromium.org/11958004/diff/3001/cc/pinch_zoom_viewport.cc
File cc/pinch_zoom_viewport.cc (right):

https://codereview.chromium.org/11958004/diff/3001/cc/pinch_zoom_viewport.cc#...
cc/pinch_zoom_viewport.cc:53: gfx::RectF PinchZoomViewport::ZoomedViewport()
const {
80 cols in this file

https://codereview.chromium.org/11958004/diff/3001/cc/pinch_zoom_viewport.cc#...
cc/pinch_zoom_viewport.cc:54: gfx::SizeF dip_viewport_size =
gfx::ScaleSize(device_viewport_size_, 1 / device_scale_factor_);
Can we not introduce "dip" in the compositor? I find it ambiguous. Any space
other than physical pixels is dip. I'd much prefer names like "layout viewport"
or "zoomed layout viewport" where the space is clearly defined. And if it's
relative to some space, use that space with a description of its relationship to
it please.

In this case this is the unzoomed layout viewport size right?

https://codereview.chromium.org/11958004/diff/3001/cc/pinch_zoom_viewport.cc#...
cc/pinch_zoom_viewport.cc:55: gfx::SizeF layer_space_viewport_size =
gfx::ScaleSize(dip_viewport_size, 1 / total_page_scale_factor());
which layer does layer_space refer to? is this the zoomed viewport size in
layout space? let's call it something like that if so.

https://codereview.chromium.org/11958004/diff/3001/content/browser/android/co...
File content/browser/android/content_startup_flags.cc (right):

https://codereview.chromium.org/11958004/diff/3001/content/browser/android/co...
content/browser/android/content_startup_flags.cc:62:
parsed_command_line->AppendSwitch(switches::kEnableCssTransformPinch);
nit: 2 space indent

[wjmaclean, 2013-01-17 19:45:51]

Generally LGTM, once Dana's logical/physical pixel issues are resolved ... they
do make it a bit hard to follow what's happening.

https://codereview.chromium.org/11958004/diff/3001/cc/pinch_zoom_viewport.cc
File cc/pinch_zoom_viewport.cc (right):

https://codereview.chromium.org/11958004/diff/3001/cc/pinch_zoom_viewport.cc#...
cc/pinch_zoom_viewport.cc:55: gfx::SizeF layer_space_viewport_size =
gfx::ScaleSize(dip_viewport_size, 1 / total_page_scale_factor());
On 2013/01/17 18:02:49, danakj wrote:
> which layer does layer_space refer to? is this the zoomed viewport size in
> layout space? let's call it something like that if so.

Should it perhaps be "layout_space_viewport_size"?

https://codereview.chromium.org/11958004/diff/3001/cc/pinch_zoom_viewport.h
File cc/pinch_zoom_viewport.h (right):

https://codereview.chromium.org/11958004/diff/3001/cc/pinch_zoom_viewport.h#n...
cc/pinch_zoom_viewport.h:62: void set_device_viewport_size(const gfx::SizeF&
size) {
Perhaps add a comment explaining why we need to store this, as opposed to just
converting layout_viewport_size_ using psf and dsf.

[aelias_OOO_until_Jul13, 2013-01-17 19:59:45]

https://codereview.chromium.org/11958004/diff/3001/cc/layer_tree_host_impl.cc
File cc/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/11958004/diff/3001/cc/layer_tree_host_impl.cc...
cc/layer_tree_host_impl.cc:257: gfx::SizeF scaledContentSize =
activeTree()->ScrollableSize();
On 2013/01/17 18:02:49, danakj wrote:
> rename scaledContentSize to what it now holds as well please

Done.

https://codereview.chromium.org/11958004/diff/3001/cc/layer_tree_host_impl.cc...
cc/layer_tree_host_impl.cc:257: gfx::SizeF scaledContentSize =
activeTree()->ScrollableSize();
On 2013/01/17 18:02:49, danakj wrote:
> rename scaledContentSize to what it now holds as well please

Done.

https://codereview.chromium.org/11958004/diff/3001/cc/layer_tree_host_impl.cc...
cc/layer_tree_host_impl.cc:265: m_pageScaleAnimation =
PageScaleAnimation::create(scrollTotal,
m_pinchZoomViewport.total_page_scale_factor(), viewportSize, scaledContentSize,
startTimeSeconds);
On 2013/01/17 18:02:49, danakj wrote:
> Without a change to PageScaleAnimation how does this continue to work? You're
> passing in the logical scroll size now instead of the physical scroll size,
> right?

This function was already prepared for the coordinate space transition by an
earlier patch of mine.  A few lines above, notice I divide scroll by
page_scale_factor if the setting is disabled.

https://codereview.chromium.org/11958004/diff/3001/cc/layer_tree_host_impl.cc...
cc/layer_tree_host_impl.cc:1370: gfx::SizeF scaledContentsSize =
gfx::ScaleSize(activeTree()->ScrollableSize(), pageScaleDeltaToSend);
On 2013/01/17 18:02:49, danakj wrote:
> rename scaledContentsSize here also

Done.

https://codereview.chromium.org/11958004/diff/3001/cc/layer_tree_host_impl.cc...
cc/layer_tree_host_impl.cc:1376:
scrollEnd.ClampToMax(gfx::RectF(scaledContentsSize).bottom_right() -
gfx::Rect(m_deviceViewportSize).bottom_right());
On 2013/01/17 18:02:49, danakj wrote:
> You mentioned that scrolling should all be in logical pixels now, so
> scaledContentsSize should be the logical scroll size (ie not content/phsyical
> pixels)?. So, should we be subtracting the logical viewport size here then?

This method adjustScrollsForPageScaleChange is purely for the old coordinate
space world, so it doesn't need a change.

https://codereview.chromium.org/11958004/diff/3001/cc/layer_tree_impl.cc
File cc/layer_tree_impl.cc (right):

https://codereview.chromium.org/11958004/diff/3001/cc/layer_tree_impl.cc#newc...
cc/layer_tree_impl.cc:94: view_bounds = gfx::ScaleSize(clip_layer->bounds(),
device_scale_factor());
On 2013/01/17 18:02:49, danakj wrote:
> This bound here (and above) is in physical pixels, but the else{} clause is in
> logical pixels? It seems like this code could be using the layout viewport
size
> and not scaling the clip layer's bounds?

This clause is for the old coordinate space so it uses totally different
assumptions about the semantics.  I'm just leaving it unchanged to avoid
breaking Android before the transition is complete.

https://codereview.chromium.org/11958004/diff/3001/cc/pinch_zoom_viewport.cc
File cc/pinch_zoom_viewport.cc (right):

https://codereview.chromium.org/11958004/diff/3001/cc/pinch_zoom_viewport.cc#...
cc/pinch_zoom_viewport.cc:54: gfx::SizeF dip_viewport_size =
gfx::ScaleSize(device_viewport_size_, 1 / device_scale_factor_);
On 2013/01/17 18:02:49, danakj wrote:
> Can we not introduce "dip" in the compositor? I find it ambiguous. Any space
> other than physical pixels is dip. I'd much prefer names like "layout
viewport"
> or "zoomed layout viewport" where the space is clearly defined. And if it's
> relative to some space, use that space with a description of its relationship
to
> it please.
> 
> In this case this is the unzoomed layout viewport size right?

I interpret "DIP" to specifically mean physical pixels divided by device scale
factor.  Any other space is not "DIP".  We need a word for this space and I
think "DIP" is unambiguous if the acronym ("density independent pixels") is
expanded.

Anyway, I've avoided this local variable entirely for now to avoid further
debate :).

https://codereview.chromium.org/11958004/diff/3001/cc/pinch_zoom_viewport.cc#...
cc/pinch_zoom_viewport.cc:55: gfx::SizeF layer_space_viewport_size =
gfx::ScaleSize(dip_viewport_size, 1 / total_page_scale_factor());
On 2013/01/17 18:02:49, danakj wrote:
> which layer does layer_space refer to? is this the zoomed viewport size in
> layout space? let's call it something like that if so.

OK, renamed to "layout_space_device_viewport_size".

https://codereview.chromium.org/11958004/diff/3001/cc/pinch_zoom_viewport.h
File cc/pinch_zoom_viewport.h (right):

https://codereview.chromium.org/11958004/diff/3001/cc/pinch_zoom_viewport.h#n...
cc/pinch_zoom_viewport.h:62: void set_device_viewport_size(const gfx::SizeF&
size) {
On 2013/01/17 19:45:52, wjmaclean wrote:
> Perhaps add a comment explaining why we need to store this, as opposed to just
> converting layout_viewport_size_ using psf and dsf. 

Done.

https://codereview.chromium.org/11958004/diff/3001/content/browser/android/co...
File content/browser/android/content_startup_flags.cc (right):

https://codereview.chromium.org/11958004/diff/3001/content/browser/android/co...
content/browser/android/content_startup_flags.cc:62:
parsed_command_line->AppendSwitch(switches::kEnableCssTransformPinch);
On 2013/01/17 18:02:49, danakj wrote:
> nit: 2 space indent

Done.

[danakj, 2013-01-17 23:30:36]

cc/ LGTM. exciting!

https://codereview.chromium.org/11958004/diff/12001/cc/pinch_zoom_viewport.cc
File cc/pinch_zoom_viewport.cc (right):

https://codereview.chromium.org/11958004/diff/12001/cc/pinch_zoom_viewport.cc...
cc/pinch_zoom_viewport.cc:55: device_viewport_size_, 1 / (device_scale_factor_ *
total_page_scale_factor()));
nit: 80col

[no sievers, 2013-01-18 00:21:18]

lgtm for android-specific files

https://codereview.chromium.org/11958004/diff/16001/content/browser/android/c...
File content/browser/android/content_startup_flags.cc (right):

https://codereview.chromium.org/11958004/diff/16001/content/browser/android/c...
content/browser/android/content_startup_flags.cc:60:
parsed_command_line->AppendSwitch(switches::kEnablePinch);
kEnablePinch would not conflict with kEnableCssTransformPinch anywhere?

https://codereview.chromium.org/11958004/diff/16001/content/browser/android/c...
File content/browser/android/content_view_core_impl.cc (right):

https://codereview.chromium.org/11958004/diff/16001/content/browser/android/c...
content/browser/android/content_view_core_impl.cc:176:
switches::kEnableCssTransformPinch)) {
indent

[aelias_OOO_until_Jul13, 2013-01-18 00:33:37]

https://codereview.chromium.org/11958004/diff/16001/content/browser/android/c...
File content/browser/android/content_startup_flags.cc (right):

https://codereview.chromium.org/11958004/diff/16001/content/browser/android/c...
content/browser/android/content_startup_flags.cc:60:
parsed_command_line->AppendSwitch(switches::kEnablePinch);
On 2013/01/18 00:21:19, Daniel Sievers wrote:
> kEnablePinch would not conflict with kEnableCssTransformPinch anywhere?

Should be fine.  kEnablePinch turns on some pinch-specific codepaths, whereas
kEnableCssTransformPinch changes the coordinate space.  It's safer to have
kEnablePinch in case some code we use is hidden behind it.

[commit-bot: I haz the power, 2013-01-18 00:35:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/aelias@chromium.org/11958004/20001

[commit-bot: I haz the power, 2013-01-18 22:50:08]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/aelias@chromium.org/11958004/27003

[commit-bot: I haz the power, 2013-01-18 23:18:12]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_aura.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...
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-19 00:27:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/aelias@chromium.org/11958004/27003

[commit-bot: I haz the power, 2013-01-19 00:38:17]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_aura.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...
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-20 03:49:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/aelias@chromium.org/11958004/27003

[commit-bot: I haz the power, 2013-01-20 03:49:25]

Failed to apply patch for cc/layer_tree_impl.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file cc/layer_tree_impl.cc
  Hunk #1 FAILED at 86.
  Hunk #2 FAILED at 176.
  2 out of 2 hunks FAILED -- saving rejects to file cc/layer_tree_impl.cc.rej

Patch:       cc/layer_tree_impl.cc
Index: cc/layer_tree_impl.cc
diff --git a/cc/layer_tree_impl.cc b/cc/layer_tree_impl.cc
index
ad2298184324f45482c9b33c20012e6d2427c869..3c0445700ef9f6161fb7e0c8ddcfa1b62b080d0b
100644
--- a/cc/layer_tree_impl.cc
+++ b/cc/layer_tree_impl.cc
@@ -86,28 +86,21 @@ void LayerTreeImpl::UpdateMaxScrollOffset() {
   if (!root_scroll_layer() || !root_scroll_layer()->children().size())
     return;
 
-  gfx::SizeF view_bounds = device_viewport_size();
-  if (LayerImpl* clip_layer = root_scroll_layer()->parent()) {
-    // Compensate for non-overlay scrollbars.
-    if (clip_layer->masksToBounds())
-      view_bounds = gfx::ScaleSize(clip_layer->bounds(),
device_scale_factor());
-  }
-
-  gfx::Size content_bounds = ContentSize();
-  if (settings().pageScalePinchZoomEnabled) {
-    // Pinch with pageScale scrolls entirely in layout space.  ContentSize
-    // returns the bounds including the page scale factor, so calculate the
-    // pre page-scale layout size here.
-    float page_scale_factor = pinch_zoom_viewport().page_scale_factor();
-    content_bounds.set_width(content_bounds.width() / page_scale_factor);
-    content_bounds.set_height(content_bounds.height() / page_scale_factor);
-  } else {
+  gfx::SizeF view_bounds;
+  if (!settings().pageScalePinchZoomEnabled) {
+    view_bounds = device_viewport_size();
+    if (LayerImpl* clip_layer = root_scroll_layer()->parent()) {
+      // Compensate for non-overlay scrollbars.
+      if (clip_layer->masksToBounds())
+        view_bounds = gfx::ScaleSize(clip_layer->bounds(),
device_scale_factor());
+    }
     view_bounds.Scale(1 / pinch_zoom_viewport().page_scale_delta());
+  } else {
+    view_bounds = layout_viewport_size();
   }
 
-  gfx::Vector2dF max_scroll = gfx::Rect(content_bounds).bottom_right() -
+  gfx::Vector2dF max_scroll = gfx::Rect(ScrollableSize()).bottom_right() -
       gfx::RectF(view_bounds).bottom_right();
-  max_scroll.Scale(1 / device_scale_factor());
 
   // The viewport may be larger than the contents in some cases, such as
   // having a vertical scrollbar but no horizontal overflow.
@@ -176,12 +169,10 @@ const LayerTreeImpl::LayerList&
LayerTreeImpl::RenderSurfaceLayerList() const {
   return render_surface_layer_list_;
 }
 
-gfx::Size LayerTreeImpl::ContentSize() const {
-  // TODO(aelias): Hardcoding the first child here is weird. Think of
-  // a cleaner way to get the contentBounds on the Impl side.
+gfx::Size LayerTreeImpl::ScrollableSize() const {
   if (!root_scroll_layer() || root_scroll_layer()->children().empty())
     return gfx::Size();
-  return root_scroll_layer()->children()[0]->contentBounds();
+  return root_scroll_layer()->children()[0]->bounds();
 }
 
 LayerImpl* LayerTreeImpl::LayerById(int id) {

[commit-bot: I haz the power, 2013-01-20 10:07:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/aelias@chromium.org/11958004/36001

[commit-bot: I haz the power, 2013-01-20 10:38:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/aelias@chromium.org/11958004/42001

[commit-bot: I haz the power, 2013-01-20 12:46:25]

Retried try job too often on linux_chromeos for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2013-01-20 16:14:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/aelias@chromium.org/11958004/42001

[commit-bot: I haz the power, 2013-01-20 18:42:59]

Retried try job too often on linux_chromeos for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2013-01-20 20:53:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/aelias@chromium.org/11958004/42001

[commit-bot: I haz the power, 2013-01-20 22:50:19]

Retried try job too often on linux_chromeos for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[aelias_OOO_until_Jul13, 2013-01-21 07:54:23]

Landing with NOTRY as every test has gone green at least once.

[commit-bot: I haz the power, 2013-01-21 07:54:34]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/aelias@chromium.org/11958004/42007

[commit-bot: I haz the power, 2013-01-21 07:54:56]

Change committed as 177896