Pipe bottom control info to ChromeFullscreenManager

Pipe bottom control info to ChromeFullscreenManager

This change feeds the bottom controls offset into the
ChromeFullscreenManager. This does not change the existing behavior.

BUG=652892
Committed: https://crrev.com/82562db5ddbe02469e1ba648b2d1e593a176f5f6
Cr-Commit-Position: refs/heads/master@{#430993}

[mdjones, 2016-10-28 00:58:21]

mdjones@chromium.org changed reviewers:
+ ianwen@chromium.org

[mdjones, 2016-10-28 00:58:21]

Not sure you know anything about this class, if not I'll pass it to ted. ptal

[mdjones, 2016-10-28 23:24:20]

mdjones@chromium.org changed reviewers:
+ tedchoc@chromium.org

[mdjones, 2016-10-28 23:24:20]

+tedchoc

[Ted C, 2016-10-31 23:47:58]

https://codereview.chromium.org/2454263003/diff/40001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java (right):

https://codereview.chromium.org/2454263003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java:2817: &&
Float.isNaN(mPreviousFullscreenBottomControlsOffsetY)) {
I think this should be a || here too.  This was just to set it to a sane value
if any are NaN.  Granted, that assumes we are getting a 0 for browser controls
offset if they don't exist.  If not, then it would always be NaN and be more of
an issue.

[mdjones, 2016-11-01 21:35:00]

https://codereview.chromium.org/2454263003/diff/40001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java (right):

https://codereview.chromium.org/2454263003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java:2817: &&
Float.isNaN(mPreviousFullscreenBottomControlsOffsetY)) {
On 2016/10/31 23:47:58, Ted C wrote:
> I think this should be a || here too.  This was just to set it to a sane value
> if any are NaN.  Granted, that assumes we are getting a 0 for browser controls
> offset if they don't exist.  If not, then it would always be NaN and be more
of
> an issue.

You're right. When unused, the offsets remain NaN instead of 0. I updated the
logic to reflect this.

[Ted C, 2016-11-01 21:36:45]

lgtm

[Ian Wen, 2016-11-02 18:12:41]

https://codereview.chromium.org/2454263003/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/fullscreen/ChromeFullscreenManager.java
(right):

https://codereview.chromium.org/2454263003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/fullscreen/ChromeFullscreenManager.java:650:
float topContentOffset) {
Shouldn't we make this symmetric? Why is that we don't care about
bottomControlsOffset?

https://codereview.chromium.org/2454263003/diff/60001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java (right):

https://codereview.chromium.org/2454263003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java:2828:
mPreviousFullscreenBottomControlsOffsetY,
I am a little confused about mPreviousFullscreenBottomControlsOffsetY.

First I wonder if we could make the name shorter? LOL

Second, are we planning to support some sort of dynamic change of controls
position? If not I would assume setFullscreenManager is only called during
activity initialization. Then why do we need the
mPreviousFullscreenBottomControlsOffsetY? Can't we set it an arbitrary initial
value?

[mdjones, 2016-11-02 18:46:36]

https://codereview.chromium.org/2454263003/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/fullscreen/ChromeFullscreenManager.java
(right):

https://codereview.chromium.org/2454263003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/fullscreen/ChromeFullscreenManager.java:650:
float topContentOffset) {
On 2016/11/02 18:12:41, Ian Wen wrote:
> Shouldn't we make this symmetric? Why is that we don't care about
> bottomControlsOffset?

Currently it isn't used anywhere. The only time we need to know the offset is
when the content is translated down (when there are top controls). In the case
of bottom controls, it doesn't need to move.

https://codereview.chromium.org/2454263003/diff/60001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java (right):

https://codereview.chromium.org/2454263003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java:2828:
mPreviousFullscreenBottomControlsOffsetY,
On 2016/11/02 18:12:41, Ian Wen wrote:
> I am a little confused about mPreviousFullscreenBottomControlsOffsetY.
> 
> First I wonder if we could make the name shorter? LOL
> 
> Second, are we planning to support some sort of dynamic change of controls
> position? If not I would assume setFullscreenManager is only called during
> activity initialization. Then why do we need the
> mPreviousFullscreenBottomControlsOffsetY? Can't we set it an arbitrary initial
> value?

Renamed the 3 mPreviousFullscreen* to mPrevious*.

The fullscreen manager is reset when a tab is closed, removed, or selected.
Those variables recall the previous state. I suppose an argument could be made
about the controls always being completely shown in these cases, but I'd rather
leave that to a separate change.

[Ian Wen, 2016-11-02 18:50:49]

On 2016/11/02 18:46:36, mdjones wrote:
>
https://codereview.chromium.org/2454263003/diff/60001/chrome/android/java/src...
> File
>
chrome/android/java/src/org/chromium/chrome/browser/fullscreen/ChromeFullscreenManager.java
> (right):
> 
>
https://codereview.chromium.org/2454263003/diff/60001/chrome/android/java/src...
>
chrome/android/java/src/org/chromium/chrome/browser/fullscreen/ChromeFullscreenManager.java:650:
> float topContentOffset) {
> On 2016/11/02 18:12:41, Ian Wen wrote:
> > Shouldn't we make this symmetric? Why is that we don't care about
> > bottomControlsOffset?
> 
> Currently it isn't used anywhere. The only time we need to know the offset is
> when the content is translated down (when there are top controls). In the case
> of bottom controls, it doesn't need to move.
> 
>
https://codereview.chromium.org/2454263003/diff/60001/chrome/android/java/src...
> File chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java (right):
> 
>
https://codereview.chromium.org/2454263003/diff/60001/chrome/android/java/src...
> chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java:2828:
> mPreviousFullscreenBottomControlsOffsetY,
> On 2016/11/02 18:12:41, Ian Wen wrote:
> > I am a little confused about mPreviousFullscreenBottomControlsOffsetY.
> > 
> > First I wonder if we could make the name shorter? LOL
> > 
> > Second, are we planning to support some sort of dynamic change of controls
> > position? If not I would assume setFullscreenManager is only called during
> > activity initialization. Then why do we need the
> > mPreviousFullscreenBottomControlsOffsetY? Can't we set it an arbitrary
initial
> > value?
> 
> Renamed the 3 mPreviousFullscreen* to mPrevious*.
> 
> The fullscreen manager is reset when a tab is closed, removed, or selected.
> Those variables recall the previous state. I suppose an argument could be made
> about the controls always being completely shown in these cases, but I'd
rather
> leave that to a separate change.

Okay I see. Thanks for the explanation!

[Ian Wen, 2016-11-02 18:50:58]

lgtm

[mdjones, 2016-11-05 02:01:39]

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

[mdjones, 2016-11-05 02:01:40]

+aelias

[mdjones, 2016-11-09 18:21:00]

The CQ bit was checked by mdjones@chromium.org

[mdjones, 2016-11-09 18:21:00]

The patchset sent to the CQ was uploaded after l-g-t-m from tedchoc@chromium.org
Link to the patchset: https://codereview.chromium.org/2454263003/#ps80001
(title: "address comments")

[commit-bot: I haz the power, 2016-11-09 18:21:23]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-09 18:54:04]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-11-09 19:17:06]

Description was changed from

==========
Pipe bottom control info to ChromeFullscreenManager

This change feeds the bottom controls offset into the
ChromeFullscreenManager. This does not change the existing behavior.

BUG=652892
==========

to

==========
Pipe bottom control info to ChromeFullscreenManager

This change feeds the bottom controls offset into the
ChromeFullscreenManager. This does not change the existing behavior.

BUG=652892

Committed: https://crrev.com/82562db5ddbe02469e1ba648b2d1e593a176f5f6
Cr-Commit-Position: refs/heads/master@{#430993}
==========

[commit-bot: I haz the power, 2016-11-09 19:17:07]

Patchset 5 (id:??) landed as
https://crrev.com/82562db5ddbe02469e1ba648b2d1e593a176f5f6
Cr-Commit-Position: refs/heads/master@{#430993}