(Reland) Always create top controls manager

(Reland) Always create top controls manager.

This CL removes the calculate_top_controls_position flag and instead
always creates the top controls manager. The methods of the manager
become no-ops if the top controls height is 0 so that it becomes
effectively enabled when a top controls height gets set.

This fixes the attached bug because some Android Chrome UI pages don't
have top controls but the manager was still created. These pages set
the height to 0 to hide the top controls but the methods of the
manager were still getting called. This caused a crash in Blink since
ScrollBy would try to calculate the shown ratio with a 0 height, causing
a NaN to propagate into Blink.

BUG=460007, 461511
Committed: https://crrev.com/7610e74eacde3468853c9765bd3f726c14df04c1
Cr-Commit-Position: refs/heads/master@{#319257}

Committed: https://crrev.com/aa5d1b033c0894dfde32bfe0b1d2cd8b2d9a1eb3
Cr-Commit-Position: refs/heads/master@{#319738}

[bokan, 2015-02-26 19:10:39]

bokan@chromium.org changed reviewers:
+ aelias@chromium.org

[bokan, 2015-02-26 19:10:39]

This was causing crashes further down as the NaN propagated into Blink. Looks
like the UI pages don't have TopControls so the height never gets set to a non-0
number. Is this the right fix? I've noticed we don't even create
TopControlsManager if settings.calculate_top_controls_position isn't set, should
we just turn that off instead? Let me know and I'll add a test.

[aelias_OOO_until_Jul13, 2015-02-26 20:51:05]

This fix looks OK.  I prefer this to the setting, in fact I think it might be a
good idea to get rid of the setting and create a TopControlsManager on all
platforms with height 0, for simplicity/consistency.

[bokan, 2015-03-04 14:57:47]

PTAL, I made LTHI always create the top controls manager as you suggested.

[aelias_OOO_until_Jul13, 2015-03-04 20:05:53]

https://codereview.chromium.org/961023002/diff/80001/cc/input/top_controls_ma...
File cc/input/top_controls_manager.cc (right):

https://codereview.chromium.org/961023002/diff/80001/cc/input/top_controls_ma...
cc/input/top_controls_manager.cc:72: if (!TopControlsEnabled())
This one shouldn't be suppressed by height.  We should still be respecting the
state update even at height 0 (so the ratio is correct if the height changes
later).

https://codereview.chromium.org/961023002/diff/80001/cc/input/top_controls_ma...
cc/input/top_controls_manager.cc:100: if (!TopControlsEnabled())
I think we should remove all of the extra early-returns you added outside of
ScrollBy.  In particular, if the height ever changes to nonzero when a scroll
already is in progress, that will work better if we run as much of the usual
logic as possible.

https://codereview.chromium.org/961023002/diff/80001/cc/input/top_controls_ma...
cc/input/top_controls_manager.cc:124: client_->SetCurrentTopControlsShownRatio(
For similar reasons of preserving state in case height changes in mid-gesture,
then could you narrowly not call this if TopControlsHeight() == 0, instead of
early-returning out of the whole method?

https://codereview.chromium.org/961023002/diff/80001/cc/input/top_controls_ma...
cc/input/top_controls_manager.cc:175: DCHECK(TopControlsEnabled());
I'd suggest removing this DCHECK.  If you're worried about unnecessary animation
frames (although I don't think it would happen in practice), you could change
SetupAnimation just set the ratio to 0/1 without animating if height is 0, and
also have a unit test if you do that.

https://codereview.chromium.org/961023002/diff/80001/cc/input/top_controls_ma...
cc/input/top_controls_manager.cc:257: bool
TopControlsManager::TopControlsEnabled() {
I don't think we should have a method called this, it's not a mode anymore.

[bokan, 2015-03-04 21:48:30]

https://codereview.chromium.org/961023002/diff/80001/cc/input/top_controls_ma...
File cc/input/top_controls_manager.cc (right):

https://codereview.chromium.org/961023002/diff/80001/cc/input/top_controls_ma...
cc/input/top_controls_manager.cc:72: if (!TopControlsEnabled())
On 2015/03/04 20:05:53, aelias wrote:
> This one shouldn't be suppressed by height.  We should still be respecting the
> state update even at height 0 (so the ratio is correct if the height changes
> later).

Done.

https://codereview.chromium.org/961023002/diff/80001/cc/input/top_controls_ma...
cc/input/top_controls_manager.cc:100: if (!TopControlsEnabled())
On 2015/03/04 20:05:53, aelias wrote:
> I think we should remove all of the extra early-returns you added outside of
> ScrollBy.  In particular, if the height ever changes to nonzero when a scroll
> already is in progress, that will work better if we run as much of the usual
> logic as possible.

Done.

https://codereview.chromium.org/961023002/diff/80001/cc/input/top_controls_ma...
cc/input/top_controls_manager.cc:124: client_->SetCurrentTopControlsShownRatio(
On 2015/03/04 20:05:53, aelias wrote:
> For similar reasons of preserving state in case height changes in mid-gesture,
> then could you narrowly not call this if TopControlsHeight() == 0, instead of
> early-returning out of the whole method?

I agree in other cases, but I think preserving state here would look weird; when
the top controls suddenly get a height the page would jump under the users
finger as all the accumulated delta is applied. We'd also need more careful
logic in LTHI as we'd return more delta from this method than we were given.

https://codereview.chromium.org/961023002/diff/80001/cc/input/top_controls_ma...
cc/input/top_controls_manager.cc:175: DCHECK(TopControlsEnabled());
On 2015/03/04 20:05:53, aelias wrote:
> I'd suggest removing this DCHECK.  If you're worried about unnecessary
animation
> frames (although I don't think it would happen in practice), you could change
> SetupAnimation just set the ratio to 0/1 without animating if height is 0, and
> also have a unit test if you do that.

Done.

https://codereview.chromium.org/961023002/diff/80001/cc/input/top_controls_ma...
cc/input/top_controls_manager.cc:257: bool
TopControlsManager::TopControlsEnabled() {
On 2015/03/04 20:05:53, aelias wrote:
> I don't think we should have a method called this, it's not a mode anymore.

Done.

[aelias_OOO_until_Jul13, 2015-03-04 21:57:06]

OK, lgtm.

[bokan, 2015-03-04 22:05:06]

bokan@chromium.org changed reviewers:
+ avi@chromium.org, dpolukhin@chromium.org, yfriedman@chromium.org

[bokan, 2015-03-04 22:05:06]

+yfriedman@ for ApplicationInitialization.java and ContentSwitches.java

+avi@ for render_widget_compositor.cc

+dpolukhin@ for chrome_restart_request.cc

[Avi (use Gerrit), 2015-03-04 22:11:04]

lgtm

stampity stamp

[Dmitry Polukhin, 2015-03-05 08:44:53]

LGTM chrome/browser/chromeos/login/chrome_restart_request.cc

[Yaron, 2015-03-05 14:44:20]

lgtm

[bokan, 2015-03-05 15:23:03]

The CQ bit was checked by bokan@chromium.org

[commit-bot: I haz the power, 2015-03-05 15:23:28]

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

[commit-bot: I haz the power, 2015-03-05 16:19:42]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-03-05 16:20:35]

Patchset 6 (id:??) landed as
https://crrev.com/7610e74eacde3468853c9765bd3f726c14df04c1
Cr-Commit-Position: refs/heads/master@{#319257}

[Dirk Pranke, 2015-03-05 23:59:24]

A revert of this CL (patchset #6 id:100001) has been created in
https://codereview.chromium.org/986433004/ by dpranke@chromium.org.

The reason for reverting is: This seems to be causing the
virtual/threaded/printing/return-from-printing-mode.html layout test to fail:

http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=Cus....

[bokan, 2015-03-06 21:03:27]

Patchset #7 (id:120001) has been deleted

[bokan, 2015-03-06 21:06:21]

Alexandre, fixed up the patch to skip updateViewportContainerSizes if top
controls aren't used (like before). still l-g-t-m?

[aelias_OOO_until_Jul13, 2015-03-06 21:51:05]

Could you set all the bound deltas to zero before early-returning?  That's more
correct in the dynamic case.

[bokan, 2015-03-06 22:25:59]

On 2015/03/06 21:51:05, aelias wrote:
> Could you set all the bound deltas to zero before early-returning?  That's
more
> correct in the dynamic case.

Done.

[aelias_OOO_until_Jul13, 2015-03-06 22:32:29]

lgtm

[bokan, 2015-03-09 12:38:08]

The CQ bit was checked by bokan@chromium.org

[bokan, 2015-03-09 12:38:08]

The patchset sent to the CQ was uploaded after l-g-t-m from
dpolukhin@chromium.org, yfriedman@chromium.org, avi@chromium.org
Link to the patchset: https://codereview.chromium.org/961023002/#ps180001
(title: "Zero out deltas before returning")

[commit-bot: I haz the power, 2015-03-09 12:38:14]

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

[bokan, 2015-03-09 12:39:03]

The CQ bit was unchecked by bokan@chromium.org

[bokan, 2015-03-09 15:33:50]

The CQ bit was checked by bokan@chromium.org

[bokan, 2015-03-09 15:33:51]

The patchset sent to the CQ was uploaded after l-g-t-m from
dpolukhin@chromium.org, aelias@chromium.org, yfriedman@chromium.org,
avi@chromium.org
Link to the patchset: https://codereview.chromium.org/961023002/#ps200001
(title: "Rebase")

[commit-bot: I haz the power, 2015-03-09 15:34:21]

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

[bokan, 2015-03-09 17:31:03]

The CQ bit was unchecked by bokan@chromium.org

[bokan, 2015-03-09 18:45:04]

The CQ bit was checked by bokan@chromium.org

[bokan, 2015-03-09 18:45:06]

The patchset sent to the CQ was uploaded after l-g-t-m from
dpolukhin@chromium.org, aelias@chromium.org, yfriedman@chromium.org,
avi@chromium.org
Link to the patchset: https://codereview.chromium.org/961023002/#ps220001
(title: "Fixing bad patch (Patches since "rebase" were from wrong branch)")

[commit-bot: I haz the power, 2015-03-09 18:45:36]

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

[commit-bot: I haz the power, 2015-03-09 21:51:54]

Committed patchset #11 (id:220001)

[commit-bot: I haz the power, 2015-03-09 21:52:38]

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