Allow changing top controls height

Allow changing top controls height

Allow changing of the top controls height after the renderer's LayerTreeHost is built.  Currently it
is built based on a command line flag, but this means different activities that have different top
controls heights have trouble setting the proper command line if they're running in the same process.

BUG=430635
Committed: https://crrev.com/cb7779b8cfcc25981ef727b20cbe82231776b577
Cr-Commit-Position: refs/heads/master@{#306737}

[David Trainor- moved to gerrit, 2014-11-20 01:02:48]

dtrainor@chromium.org changed reviewers:
+ aelias@chromium.org, bokan@chromium.org, tedchoc@chromium.org

[David Trainor- moved to gerrit, 2014-11-20 01:02:49]

Sending this out as a preliminary fyi (still need to add layer_tree_host_impl
unit tests).

David, could you check that adjusting the top_controls_delta is the correct
thing to do?  I'm not 100% sure if I'm doing the right thing wrt those variables
and I had a hard time getting all of the existing unit tests to pass, so I'm a
little worried I'm not understanding exactly how it's supposed to work.  Thanks!

[bokan, 2014-11-20 15:09:32]

This looks fine to me, anytime the LTHImpl is changing the top controls offset
it should do so by modifying the delta.

One question I have, you've effectively replaced tc_layout_height (a float) with
tc_shrinks_blink_size (a bool). Was this necessary or is this just a clean up? I
think this is potentially clearer, though it means that resizes can only occur
when top controls are fully shown or fully hidden. This is probably fine as
that's how this works today; just want to make sure I understand.

https://chromiumcodereview.appspot.com/714003002/diff/80001/cc/trees/layer_tr...
File cc/trees/layer_tree_host_impl.cc (right):

https://chromiumcodereview.appspot.com/714003002/diff/80001/cc/trees/layer_tr...
cc/trees/layer_tree_host_impl.cc:269: SetControlsTopOffset(0.f);
This should become a no-op now so it should be removed. I had a lot of issues
with top controls initializing hidden, or hiding shortly after load. It looks
like the compositor is now initializing the controls to hidden (and zero height)
until the browser gives it a size. If the event flow here is resolved so top
controls initialize properly then you can resolve this bug too.

https://chromiumcodereview.appspot.com/714003002/diff/80001/content/browser/r...
File content/browser/renderer_host/render_widget_host_view_android.cc (right):

https://chromiumcodereview.appspot.com/714003002/diff/80001/content/browser/r...
content/browser/renderer_host/render_widget_host_view_android.cc:550: return
0.f;
This should be a bool right?

[David Trainor- moved to gerrit, 2014-11-20 20:57:21]

On 2014/11/20 15:09:32, bokan wrote:
> This looks fine to me, anytime the LTHImpl is changing the top controls offset
> it should do so by modifying the delta.
> 
> One question I have, you've effectively replaced tc_layout_height (a float)
with
> tc_shrinks_blink_size (a bool). Was this necessary or is this just a clean up?
I
> think this is potentially clearer, though it means that resizes can only occur
> when top controls are fully shown or fully hidden. This is probably fine as
> that's how this works today; just want to make sure I understand.

Yeah this was a cleanup :).  It was just getting confusing having two height
values around when one was either 0 or max height.

> 
>
https://chromiumcodereview.appspot.com/714003002/diff/80001/cc/trees/layer_tr...
> File cc/trees/layer_tree_host_impl.cc (right):
> 
>
https://chromiumcodereview.appspot.com/714003002/diff/80001/cc/trees/layer_tr...
> cc/trees/layer_tree_host_impl.cc:269: SetControlsTopOffset(0.f);
> This should become a no-op now so it should be removed. I had a lot of issues
> with top controls initializing hidden, or hiding shortly after load. It looks
> like the compositor is now initializing the controls to hidden (and zero
height)
> until the browser gives it a size. If the event flow here is resolved so top
> controls initialize properly then you can resolve this bug too.
> 
>
https://chromiumcodereview.appspot.com/714003002/diff/80001/content/browser/r...
> File content/browser/renderer_host/render_widget_host_view_android.cc (right):
> 
>
https://chromiumcodereview.appspot.com/714003002/diff/80001/content/browser/r...
> content/browser/renderer_host/render_widget_host_view_android.cc:550: return
> 0.f;
> This should be a bool right?

[David Trainor- moved to gerrit, 2014-11-20 20:57:28]

https://chromiumcodereview.appspot.com/714003002/diff/80001/cc/trees/layer_tr...
File cc/trees/layer_tree_host_impl.cc (right):

https://chromiumcodereview.appspot.com/714003002/diff/80001/cc/trees/layer_tr...
cc/trees/layer_tree_host_impl.cc:269: SetControlsTopOffset(0.f);
On 2014/11/20 15:09:32, bokan wrote:
> This should become a no-op now so it should be removed. I had a lot of issues
> with top controls initializing hidden, or hiding shortly after load. It looks
> like the compositor is now initializing the controls to hidden (and zero
height)
> until the browser gives it a size. If the event flow here is resolved so top
> controls initialize properly then you can resolve this bug too.

Good point.  Commented this out and tested it and it seemed to work.  Any
particular test you'd like me to run?

https://chromiumcodereview.appspot.com/714003002/diff/80001/content/browser/r...
File content/browser/renderer_host/render_widget_host_view_android.cc (right):

https://chromiumcodereview.appspot.com/714003002/diff/80001/content/browser/r...
content/browser/renderer_host/render_widget_host_view_android.cc:550: return
0.f;
On 2014/11/20 15:09:32, bokan wrote:
> This should be a bool right?

Done.

[bokan, 2014-11-20 21:31:39]

https://codereview.chromium.org/714003002/diff/80001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/714003002/diff/80001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl.cc:269: SetControlsTopOffset(0.f);
On 2014/11/20 20:57:27, David Trainor wrote:
> On 2014/11/20 15:09:32, bokan wrote:
> > This should become a no-op now so it should be removed. I had a lot of
issues
> > with top controls initializing hidden, or hiding shortly after load. It
looks
> > like the compositor is now initializing the controls to hidden (and zero
> height)
> > until the browser gives it a size. If the event flow here is resolved so top
> > controls initialize properly then you can resolve this bug too.
> 
> Good point.  Commented this out and tested it and it seemed to work.  Any
> particular test you'd like me to run?

I would try the repro steps for crbug.com/416944 and crbug.com/423483. There was
some raciness here so it isn't necessarily 100% reproducible but if it works
after a couple of tries I think it's fine.

[aelias_OOO_until_Jul13, 2014-11-21 04:19:21]

https://codereview.chromium.org/714003002/diff/120001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/714003002/diff/120001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl.cc:1673: void
LayerTreeHostImpl::SetTopControlsShrinkBlinkSize(bool shrink) {
I don't see any calls to this method in this patch.  You want to call it during
activation like the other method.  I expect this will cause bottom fixed-pos
elements to behave incorrectly, try for example mobile http://forbes.com/.

https://codereview.chromium.org/714003002/diff/120001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl.cc:1673: void
LayerTreeHostImpl::SetTopControlsShrinkBlinkSize(bool shrink) {
I don't see any calls to this method in this patch.  You want to call it during
activation like the other method.  I expect this will cause bottom fixed-pos
elements to behave incorrectly, try for example mobile http://forbes.com/.

https://codereview.chromium.org/714003002/diff/120001/content/renderer/render...
File content/renderer/render_widget.cc (right):

https://codereview.chromium.org/714003002/diff/120001/content/renderer/render...
content/renderer/render_widget.cc:705: ? static_cast<float>(top_controls_height)
: 0.f);
static_cast<float> seems to do nothing here?

[David Trainor- moved to gerrit, 2014-11-22 00:35:30]

https://chromiumcodereview.appspot.com/714003002/diff/120001/cc/trees/layer_t...
File cc/trees/layer_tree_host_impl.cc (right):

https://chromiumcodereview.appspot.com/714003002/diff/120001/cc/trees/layer_t...
cc/trees/layer_tree_host_impl.cc:1673: void
LayerTreeHostImpl::SetTopControlsShrinkBlinkSize(bool shrink) {
On 2014/11/21 04:19:21, aelias wrote:
> I don't see any calls to this method in this patch.  You want to call it
during
> activation like the other method.  I expect this will cause bottom fixed-pos
> elements to behave incorrectly, try for example mobile http://forbes.com/.

Good call.  Removed.

https://chromiumcodereview.appspot.com/714003002/diff/120001/content/renderer...
File content/renderer/render_widget.cc (right):

https://chromiumcodereview.appspot.com/714003002/diff/120001/content/renderer...
content/renderer/render_widget.cc:705: ? static_cast<float>(top_controls_height)
: 0.f);
On 2014/11/21 04:19:21, aelias wrote:
> static_cast<float> seems to do nothing here?

Done.

[aelias_OOO_until_Jul13, 2014-11-22 00:39:02]

lgtm

[Ted C, 2014-11-26 00:38:25]

lgtm

https://codereview.chromium.org/714003002/diff/160001/content/browser/android...
File content/browser/android/content_view_core_impl.cc (right):

https://codereview.chromium.org/714003002/diff/160001/content/browser/android...
content/browser/android/content_view_core_impl.cc:762: return 0;
return false

[David Trainor- moved to gerrit, 2014-12-01 04:20:26]

dtrainor@chromium.org changed reviewers:
+ jcivelli@chromium.org, mlamouri@chromium.org, palmer@chromium.org,
sievers@chromium.org

[David Trainor- moved to gerrit, 2014-12-01 04:20:27]

owners ptal thanks!

mlamouri: content/browser/screen_orientation
palmer: content/common
jcivelli: content/public/test
sievers: content/renderer

[mlamouri (slow - plz ping), 2014-12-01 10:56:16]

content/browser/screen_orientation/ lgtm

[Jay Civelli, 2014-12-01 16:56:51]

LGTM for content/public/test

[palmer, 2014-12-01 19:57:54]

LGTM mod naming nit.

https://codereview.chromium.org/714003002/diff/260001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_android.cc (right):

https://codereview.chromium.org/714003002/diff/260001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_android.cc:672: bool
RenderWidgetHostViewAndroid::GetTopControlsShrinkBlinkSize() const {
The name |GetTopControlsShrinkBlinkSize| is really confusing. "get...size"
sounds like it should return a number.

[no sievers, 2014-12-01 21:10:24]

On 2014/12/01 04:20:27, David Trainor wrote:
> owners ptal thanks!
> 
> mlamouri: content/browser/screen_orientation
> palmer: content/common
> jcivelli: content/public/test
> sievers: content/renderer

lgtm

[David Trainor- moved to gerrit, 2014-12-02 05:53:25]

https://codereview.chromium.org/714003002/diff/260001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_android.cc (right):

https://codereview.chromium.org/714003002/diff/260001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_android.cc:672: bool
RenderWidgetHostViewAndroid::GetTopControlsShrinkBlinkSize() const {
On 2014/12/01 19:57:54, Chromium Palmer wrote:
> The name |GetTopControlsShrinkBlinkSize| is really confusing. "get...size"
> sounds like it should return a number.

Yeah I can see that being a bit confusing.  I could call it
getTopControlsShrinkBlinkViewport, but that still kind of makes it sound like it
should return a Rect or a Size object.  Maybe getTopControlsShrinkBlinkLayout?

[aelias_OOO_until_Jul13, 2014-12-02 07:11:04]

On 2014/12/02 05:53:25, David Trainor wrote:
>
https://codereview.chromium.org/714003002/diff/260001/content/browser/rendere...
> File content/browser/renderer_host/render_widget_host_view_android.cc (right):
> 
>
https://codereview.chromium.org/714003002/diff/260001/content/browser/rendere...
> content/browser/renderer_host/render_widget_host_view_android.cc:672: bool
> RenderWidgetHostViewAndroid::GetTopControlsShrinkBlinkSize() const {
> On 2014/12/01 19:57:54, Chromium Palmer wrote:
> > The name |GetTopControlsShrinkBlinkSize| is really confusing. "get...size"
> > sounds like it should return a number.
> 
> Yeah I can see that being a bit confusing.  I could call it
> getTopControlsShrinkBlinkViewport, but that still kind of makes it sound like
it
> should return a Rect or a Size object.  Maybe getTopControlsShrinkBlinkLayout?

Maybe the problem is more with the "get"?  DoTopControlsShrinkBlinkSize?

[David Trainor- moved to gerrit, 2014-12-04 01:06:06]

The CQ bit was checked by dtrainor@chromium.org

[commit-bot: I haz the power, 2014-12-04 01:07:07]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/714003002/300001

[commit-bot: I haz the power, 2014-12-04 01:08:11]

Committed patchset #16 (id:300001)

[commit-bot: I haz the power, 2014-12-04 01:09:09]

Patchset 16 (id:??) landed as
https://crrev.com/cb7779b8cfcc25981ef727b20cbe82231776b577
Cr-Commit-Position: refs/heads/master@{#306737}