Refactor OverlayPanel height calculations

Refactor OverlayPanel height calculations

This changes the height calculations to not directly depend on whether
or not the toolbar is showing.

BUG=584354
Committed: https://crrev.com/57038eef935b626468268d2044e63bdef96778ae
Cr-Commit-Position: refs/heads/master@{#406168}

[mdjones, 2016-06-23 22:31:15]

mdjones@chromium.org changed reviewers:
+ dtrainor@chromium.org, twellington@chromium.org

[mdjones, 2016-06-23 22:31:16]

I realize there is an extra param on sizeChanged in Layout.java now, I want to
get thoughts on what I have before I clean that up. ptal

[Donn Denman, 2016-06-23 23:46:59]

donnd@chromium.org changed reviewers:
+ donnd@chromium.org

[Donn Denman, 2016-06-23 23:47:00]

Matt, thanks for picking up this fix!

https://codereview.chromium.org/2098683002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanel.java
(right):

https://codereview.chromium.org/2098683002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanel.java:817:
// Take the height of the toolbar into consideration.
Remove this line?

https://codereview.chromium.org/2098683002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelBase.java
(right):

https://codereview.chromium.org/2098683002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelBase.java:1100:
offset -= mLayoutYOffset;
Shouldn't this be moved up to before the Math.min?  Just a quick guess that the
pinning should be done after any adjustment to the offset.

[mdjones, 2016-06-24 01:30:26]

https://codereview.chromium.org/2098683002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanel.java
(right):

https://codereview.chromium.org/2098683002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanel.java:817:
// Take the height of the toolbar into consideration.
On 2016/06/23 23:47:00, Donn Denman wrote:
> Remove this line?

Done.

https://codereview.chromium.org/2098683002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelBase.java
(right):

https://codereview.chromium.org/2098683002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelBase.java:1100:
offset -= mLayoutYOffset;
On 2016/06/23 23:47:00, Donn Denman wrote:
> Shouldn't this be moved up to before the Math.min?  Just a quick guess that
the
> pinning should be done after any adjustment to the offset.

Done, I also joined a few of these lines for brevity.

[Theresa, 2016-06-24 23:43:04]

lgtm

https://codereview.chromium.org/2098683002/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/Layout.java
(left):

https://codereview.chromium.org/2098683002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/Layout.java:379:
float width = availableViewport.width();
As we discussed offline, my only concern was around whether this worked for
other places relying on this class. It turns out that the tab strip is the only
other class that uses this and it cares about the width (which should be the
same) rather than the height.

[mdjones, 2016-07-01 16:20:11]

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

[mdjones, 2016-07-01 16:20:11]

+changwan for Layout.java since dtrainor is ooo. ptal

[Changwan Ryu, 2016-07-07 03:29:33]

just some nits... also please test rigorously around the following cases
regarding layout's height model change:

1) context search layout when topcontrols height changes
2) OverviewListLayout... any visual glitch?

https://codereview.chromium.org/2098683002/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelBase.java
(right):

https://codereview.chromium.org/2098683002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelBase.java:196:
public void onSizeChanged(float width, float height, float
visibleViewportOffsetY) {
How about renaming it as onLayoutChanged? I don't feel that viewport offset is
part of 'size'.

https://codereview.chromium.org/2098683002/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/readermode/ReaderModePanel.java
(right):

https://codereview.chromium.org/2098683002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/readermode/ReaderModePanel.java:314:
public void onSizeChanged(float width, float height, float
visibleViewportOffsetY) {
onLayoutChanged?

https://codereview.chromium.org/2098683002/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/Layout.java
(right):

https://codereview.chromium.org/2098683002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/Layout.java:375:
public final void sizeChanged(RectF availableViewport, RectF visibleViewport,
If availableViewport is not used any more, could you clean it up?

https://codereview.chromium.org/2098683002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/Layout.java:400:
mSceneOverlays.get(i).onSizeChanged(width, height, visibleViewport.top,
orientation);
This may slightly affect overlay panel's height in certain situations, which I
guess you already expect.

[mdjones, 2016-07-07 18:33:40]

Tested on phone and tablet with multi-window which was my main concern. I also
tested rotation, showing/hiding top controls and keyboard, and resizing the
window.

https://codereview.chromium.org/2098683002/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelBase.java
(right):

https://codereview.chromium.org/2098683002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelBase.java:196:
public void onSizeChanged(float width, float height, float
visibleViewportOffsetY) {
On 2016/07/07 03:29:32, Changwan Ryu wrote:
> How about renaming it as onLayoutChanged? I don't feel that viewport offset is
> part of 'size'.

I renamed this method up to the SceneOverlay interface; I can change the names
there too if you want.

https://codereview.chromium.org/2098683002/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/readermode/ReaderModePanel.java
(right):

https://codereview.chromium.org/2098683002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/readermode/ReaderModePanel.java:314:
public void onSizeChanged(float width, float height, float
visibleViewportOffsetY) {
On 2016/07/07 03:29:32, Changwan Ryu wrote:
> onLayoutChanged?

Done.

https://codereview.chromium.org/2098683002/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/Layout.java
(right):

https://codereview.chromium.org/2098683002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/Layout.java:375:
public final void sizeChanged(RectF availableViewport, RectF visibleViewport,
On 2016/07/07 03:29:32, Changwan Ryu wrote:
> If availableViewport is not used any more, could you clean it up?

Done.

https://codereview.chromium.org/2098683002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/Layout.java:400:
mSceneOverlays.get(i).onSizeChanged(width, height, visibleViewport.top,
orientation);
On 2016/07/07 03:29:32, Changwan Ryu wrote:
> This may slightly affect overlay panel's height in certain situations, which I
> guess you already expect.

Right, this is the expected behavior. For instance, the keyboard showing will
change the height, but we are listening for things like that.

[Changwan Ryu, 2016-07-08 00:30:59]

On 2016/07/07 18:33:40, mdjones wrote:
> Tested on phone and tablet with multi-window which was my main concern. I also
> tested rotation, showing/hiding top controls and keyboard, and resizing the
> window.
> 
>
https://codereview.chromium.org/2098683002/diff/40001/chrome/android/java/src...
> File
>
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelBase.java
> (right):
> 
>
https://codereview.chromium.org/2098683002/diff/40001/chrome/android/java/src...
>
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelBase.java:196:
> public void onSizeChanged(float width, float height, float
> visibleViewportOffsetY) {
> On 2016/07/07 03:29:32, Changwan Ryu wrote:
> > How about renaming it as onLayoutChanged? I don't feel that viewport offset
is
> > part of 'size'.
> 
> I renamed this method up to the SceneOverlay interface; I can change the names
> there too if you want.
> 
>
https://codereview.chromium.org/2098683002/diff/40001/chrome/android/java/src...
> File
>
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/readermode/ReaderModePanel.java
> (right):
> 
>
https://codereview.chromium.org/2098683002/diff/40001/chrome/android/java/src...
>
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/readermode/ReaderModePanel.java:314:
> public void onSizeChanged(float width, float height, float
> visibleViewportOffsetY) {
> On 2016/07/07 03:29:32, Changwan Ryu wrote:
> > onLayoutChanged?
> 
> Done.
> 
>
https://codereview.chromium.org/2098683002/diff/40001/chrome/android/java/src...
> File
>
chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/Layout.java
> (right):
> 
>
https://codereview.chromium.org/2098683002/diff/40001/chrome/android/java/src...
>
chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/Layout.java:375:
> public final void sizeChanged(RectF availableViewport, RectF visibleViewport,
> On 2016/07/07 03:29:32, Changwan Ryu wrote:
> > If availableViewport is not used any more, could you clean it up?
> 
> Done.
> 
>
https://codereview.chromium.org/2098683002/diff/40001/chrome/android/java/src...
>
chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/Layout.java:400:
> mSceneOverlays.get(i).onSizeChanged(width, height, visibleViewport.top,
> orientation);
> On 2016/07/07 03:29:32, Changwan Ryu wrote:
> > This may slightly affect overlay panel's height in certain situations, which
I
> > guess you already expect.
> 
> Right, this is the expected behavior. For instance, the keyboard showing will
> change the height, but we are listening for things like that.

compositor/ lgtm, thanks!

[David Trainor- moved to gerrit, 2016-07-18 15:37:31]

lgtm as well.

[mdjones, 2016-07-18 15:52:31]

The CQ bit was checked by mdjones@chromium.org

[mdjones, 2016-07-18 15:52:32]

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

[commit-bot: I haz the power, 2016-07-18 15:53:08]

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-07-18 16:15:22]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-18 16:15:23]

Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)

[mdjones, 2016-07-18 23:34:34]

The CQ bit was checked by mdjones@chromium.org

[mdjones, 2016-07-18 23:34:34]

The patchset sent to the CQ was uploaded after l-g-t-m from
twellington@chromium.org, dtrainor@chromium.org, changwan@chromium.org
Link to the patchset: https://codereview.chromium.org/2098683002/#ps80001
(title: "fix tests")

[commit-bot: I haz the power, 2016-07-18 23:35:29]

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-07-19 01:05:24]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-07-19 01:06:10]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-07-19 01:08:27]

Description was changed from

==========
Refactor OverlayPanel height calculations

This changes the height calculations to not directly depend on whether
or not the toolbar is showing.

BUG=584354
==========

to

==========
Refactor OverlayPanel height calculations

This changes the height calculations to not directly depend on whether
or not the toolbar is showing.

BUG=584354

Committed: https://crrev.com/57038eef935b626468268d2044e63bdef96778ae
Cr-Commit-Position: refs/heads/master@{#406168}
==========

[commit-bot: I haz the power, 2016-07-19 01:08:29]

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