Replace overdraw_bottom_height with top_controls_layout_height.

Replace overdraw_bottom_height with top_controls_layout_height.

This changes CC to trust Blink's inner container layer size and apply
Android top controls size as a delta on top of it. Prior to this patch,
on Android we would clobber what Blink told CC with a from-scratch
computation "physical device size - top controls impl size - keyboard
overdraw size".

This change makes it unnecessary for the renderer to be aware of
keyboard overdraw size, but on the other hand CC now must know about how
top controls size was applied to Blink layout size, to understand the
size coming from Blink. So this patch replaces one float with the other.

This simplifies the logic and is a step towards converging OSK codepaths
between platforms (since ChromeOS didn't use overdraw_bottom_height, but
used visible_viewport_size instead -- this patch unlocks the potential
use of the latter on Android as well).

NOTRY=true
BUG=404315
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=291306

[aelias_OOO_until_Jul13, 2014-08-18 22:49:41]

PTAL.  Before I add all the rubberstamp reviewers for the field rename, first
requesting review of the substantive changes:

bokan@ for cc/
tedchoc@ for ContentViewCore and RWHVA


This is an cleanup of some platform divergence complexity that's been bothering
me since ChromeOS OSK shipped.  When pinch zoom becomes unified across
platforms, we should be able to go further in the direction of unifying OSK
behaviors and making Blink aware of top controls, but this is a nice first step
I think is worth doing.

[Ted C, 2014-08-18 23:35:41]

On 2014/08/18 22:49:41, aelias wrote:
> PTAL.  Before I add all the rubberstamp reviewers for the field rename, first
> requesting review of the substantive changes:
> 
> bokan@ for cc/
> tedchoc@ for ContentViewCore and RWHVA
> 
> 
> This is an cleanup of some platform divergence complexity that's been
bothering
> me since ChromeOS OSK shipped.  When pinch zoom becomes unified across
> platforms, we should be able to go further in the direction of unifying OSK
> behaviors and making Blink aware of top controls, but this is a nice first
step
> I think is worth doing.

I'm a bit confused here.  The overdraw height had nothing to do with top
controls,
so I'm not sure how one replaces the other.

The overdraw bottom height was because of the optimization where we don't resize
the compositor while showing the OSK, but that is independent of top controls
state.

[aelias_OOO_until_Jul13, 2014-08-18 23:45:25]

On 2014/08/18 at 23:35:41, tedchoc wrote:
> I'm a bit confused here.  The overdraw height had nothing to do with top
controls,
> so I'm not sure how one replaces the other.
> 
> The overdraw bottom height was because of the optimization where we don't
resize
> the compositor while showing the OSK, but that is independent of top controls
> state.

CC pieces together different inputs from both the browser and from Blink to
determine the final viewport size.  Blink is aware of OSK size (via the change
of Blink layout size on Android, and visible_viewport_size on ChromeOS) so the
overdraw_bottom_height value is redundant with what CC already receives from
Blink.  There was only one problem: CC could not trust the size coming from
Blink because it keeps bouncing up and down depending on top controls resize.

[Ted C, 2014-08-19 01:14:28]

lgtm - Per the offline discussion, this sounds fine to me.  I think
setViewportSizeOffset in CVC and onSizeChanged have negated the need for this
over time and seems like a good thing to clear this up then.

https://codereview.chromium.org/476113004/diff/1/content/browser/renderer_hos...
File content/browser/renderer_host/render_widget_host_view_android.cc (right):

https://codereview.chromium.org/476113004/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/render_widget_host_view_android.cc:544: return
content_view_core_->GetViewportSizeOffsetDip().height();
I would be nice to converge these two ("ViewportSizeOffset",
"TopControlsLayout") into a singly named "thing" at some point

https://codereview.chromium.org/476113004/diff/1/content/browser/renderer_hos...
File content/browser/renderer_host/render_widget_host_view_base.h (right):

https://codereview.chromium.org/476113004/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/render_widget_host_view_base.h:123: // the
browser, for example by an on-screen-keyboard (in DPI-adjusted pixels).
This needs to get updated.

https://codereview.chromium.org/476113004/diff/1/content/public/android/java/...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
(right):

https://codereview.chromium.org/476113004/diff/1/content/public/android/java/...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1576:
public void onOverdrawBottomHeightChanged(int overdrawHeightPix) {
TODO/Deprecation to remove this.

https://codereview.chromium.org/476113004/diff/1/content/public/android/java/...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2265:
controlsOffsetPix, contentOffsetYPix, 0);
Same here for the cleanup to no longer pass 0.

[bokan, 2014-08-19 12:55:23]

Much easier to understand IMO, LGTM

[dgozman, 2014-08-19 13:05:47]

https://codereview.chromium.org/476113004/diff/1/content/browser/devtools/ren...
File content/browser/devtools/renderer_overrides_handler.cc (right):

https://codereview.chromium.org/476113004/diff/1/content/browser/devtools/ren...
content/browser/devtools/renderer_overrides_handler.cc:578:
view->GetPhysicalBackingSize(), 1 / metadata.device_scale_factor);
Does GetPhysicalBackingSize() change when top controls are shown/hidden?
Screencast code assumes this value doesn't change over time.

[bokan, 2014-08-19 13:06:58]

Much easier to understand IMO, LGTM

[aelias_OOO_until_Jul13, 2014-08-19 21:28:39]

https://codereview.chromium.org/476113004/diff/1/content/browser/devtools/ren...
File content/browser/devtools/renderer_overrides_handler.cc (right):

https://codereview.chromium.org/476113004/diff/1/content/browser/devtools/ren...
content/browser/devtools/renderer_overrides_handler.cc:578:
view->GetPhysicalBackingSize(), 1 / metadata.device_scale_factor);
On 2014/08/19 at 13:05:47, dgozman wrote:
> Does GetPhysicalBackingSize() change when top controls are shown/hidden?
Screencast code assumes this value doesn't change over time.

It doesn't change.  GetPhysicalBackingSize returns the physical pixel size of
the application window regardless of top controls and keyboard.

By the way, I locally tested devtools screencasting after applying this patch
and it works fine.

https://codereview.chromium.org/476113004/diff/1/content/browser/renderer_hos...
File content/browser/renderer_host/render_widget_host_view_android.cc (right):

https://codereview.chromium.org/476113004/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/render_widget_host_view_android.cc:544: return
content_view_core_->GetViewportSizeOffsetDip().height();
On 2014/08/19 at 01:14:28, Ted C wrote:
> I would be nice to converge these two ("ViewportSizeOffset",
"TopControlsLayout") into a singly named "thing" at some point

OK, I changed the name everywhere upstream, exposing setTopControlsLayoutHeight
as a public method on ContentViewCore so that a later downstream patch can call
it.

https://codereview.chromium.org/476113004/diff/1/content/browser/renderer_hos...
File content/browser/renderer_host/render_widget_host_view_base.h (right):

https://codereview.chromium.org/476113004/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/render_widget_host_view_base.h:123: // the
browser, for example by an on-screen-keyboard (in DPI-adjusted pixels).
On 2014/08/19 at 01:14:28, Ted C wrote:
> This needs to get updated.

Done.

https://codereview.chromium.org/476113004/diff/1/content/public/android/java/...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
(right):

https://codereview.chromium.org/476113004/diff/1/content/public/android/java/...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1576:
public void onOverdrawBottomHeightChanged(int overdrawHeightPix) {
On 2014/08/19 at 01:14:28, Ted C wrote:
> TODO/Deprecation to remove this.

Done.

https://codereview.chromium.org/476113004/diff/1/content/public/android/java/...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2265:
controlsOffsetPix, contentOffsetYPix, 0);
On 2014/08/19 at 01:14:28, Ted C wrote:
> Same here for the cleanup to no longer pass 0.

Done.

[aelias_OOO_until_Jul13, 2014-08-19 23:18:09]

Adding OWNERS reviewers for the rest (mostly trivial renaming):

piman@ for content/
palmer@ for *_messages.h

[piman, 2014-08-19 23:58:45]

lgtm

https://codereview.chromium.org/476113004/diff/60001/content/browser/devtools...
File content/browser/devtools/renderer_overrides_handler.cc (right):

https://codereview.chromium.org/476113004/diff/60001/content/browser/devtools...
content/browser/devtools/renderer_overrides_handler.cc:621: host_->GetView());
nit: is the cast safe in tests? (sorry, I'm not familiar with this part of the
code, you may want a reviewer that's familiar with devtools).

[aelias_OOO_until_Jul13, 2014-08-20 03:09:46]

https://codereview.chromium.org/476113004/diff/60001/content/browser/devtools...
File content/browser/devtools/renderer_overrides_handler.cc (right):

https://codereview.chromium.org/476113004/diff/60001/content/browser/devtools...
content/browser/devtools/renderer_overrides_handler.cc:621: host_->GetView());
On 2014/08/19 at 23:58:45, piman (OOO) wrote:
> nit: is the cast safe in tests? (sorry, I'm not familiar with this part of the
code, you may want a reviewer that's familiar with devtools).

There are similar casts elsewhere in this file.  dgozman@ is a devtools person
and took an initial look earlier in this thread.  dgozman, everything else here
look OK?

[dgozman, 2014-08-20 04:47:01]

devtools lgtm

https://codereview.chromium.org/476113004/diff/1/content/browser/devtools/ren...
File content/browser/devtools/renderer_overrides_handler.cc (right):

https://codereview.chromium.org/476113004/diff/1/content/browser/devtools/ren...
content/browser/devtools/renderer_overrides_handler.cc:578:
view->GetPhysicalBackingSize(), 1 / metadata.device_scale_factor);
On 2014/08/19 21:28:39, aelias wrote:
> On 2014/08/19 at 13:05:47, dgozman wrote:
> > Does GetPhysicalBackingSize() change when top controls are shown/hidden?
> Screencast code assumes this value doesn't change over time.
> 
> It doesn't change.  GetPhysicalBackingSize returns the physical pixel size of
> the application window regardless of top controls and keyboard.
Great! I remember we erroneously called method here which returned variable size
(GetRequestedRendererSize() probably).

> 
> By the way, I locally tested devtools screencasting after applying this patch
> and it works fine.
Thank you very much! I just wanted to be on the safe side.

https://codereview.chromium.org/476113004/diff/60001/content/browser/devtools...
File content/browser/devtools/renderer_overrides_handler.cc (right):

https://codereview.chromium.org/476113004/diff/60001/content/browser/devtools...
content/browser/devtools/renderer_overrides_handler.cc:621: host_->GetView());
On 2014/08/20 03:09:46, aelias wrote:
> On 2014/08/19 at 23:58:45, piman (OOO) wrote:
> > nit: is the cast safe in tests? (sorry, I'm not familiar with this part of
the
> code, you may want a reviewer that's familiar with devtools).
> 
> There are similar casts elsewhere in this file.  dgozman@ is a devtools person
> and took an initial look earlier in this thread.  dgozman, everything else
here
> look OK?

We don't use mock views in tests for this functionality. Everything else is
looking good.

[aelias_OOO_until_Jul13, 2014-08-20 18:23:07]

OK, just need security review from palmer@ for the trivial _messages.h changes.

[palmer, 2014-08-21 22:52:44]

lgtm

[aelias_OOO_until_Jul13, 2014-08-21 22:58:02]

The CQ bit was checked by aelias@chromium.org

[commit-bot: I haz the power, 2014-08-21 22:59:36]

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

[commit-bot: I haz the power, 2014-08-22 01:40:16]

Committed patchset #6 (100001) as 291306