Allow swipe threshold adjustment

Allow swipe threshold adjustment

When the OverlayPanel is only using two states, the amount of swipe
it takes to move from one panel state to another is excessive. This
change allows adjustment of how sensitive the panels are to a swipe.

BUG=565585
Committed: https://crrev.com/4782f344419a9990300da9e9312f7083a38ec47d
Cr-Commit-Position: refs/heads/master@{#371325}

[mdjones, 2016-01-22 18:53:15]

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

[mdjones, 2016-01-22 18:53:15]

PTAL

[Theresa, 2016-01-22 19:59:15]

lgtm

[Donn Denman, 2016-01-22 21:22:51]

Matt, it's really great that you wrote a detailed set of tests for this change!

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

https://codereview.chromium.org/1614953004/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelBase.java:891:
* @return The percent of the distance the panel has to be to its next state
before animating
Nit: we're not really using percents, we're using fractions, so I would change
this to read "The fraction of the distance..." and "Default is the panel must be
half way to..." on the next line.

https://codereview.chromium.org/1614953004/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelBase.java:914:
prevState = nextState;
Nit: I think it's more readable to move this down to just before setting
nextState.

https://codereview.chromium.org/1614953004/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelBase.java:919:
// The values in PanelState are ascending, so this should work.
Please add a comment to PanelState to say the states should be kept in order.

https://codereview.chromium.org/1614953004/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelBase.java:931:
if (velocity < 0.0f) {
Consider simplifying by folding the two cases, e.g:
float thresholdToNextState = velocity < 0.0f ? getThresholdToNextState() : 1.0 -
getThresholdToNextState();
if ((desiredPanelHeight - lowerBound) / distance > thresholdToNextState) {
    return nextState;
} else {
    return prevState;
}

https://codereview.chromium.org/1614953004/diff/40001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelBaseTest.java
(right):

https://codereview.chromium.org/1614953004/diff/40001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelBaseTest.java:106:
nextState = mExpandPanel.findNearestPanelStateFromHeight(310,
DOWNWARD_VELOCITY);
It's a bit hard for me to understand what you're actually testing here.  I think
updating the comments would help.  Maybe add some constants for these positions
instead of explaining them in comments, e.g. ABOVE_PEEK_EXPANDED_MIDPOINT,
BELOW_PEEK_EXPANDED_MIDPOINT.  Also, is it true that the velocity does not have
any effect here because we're in a 3-state panel which has equal bias between up
and down?  Please comment along these lines, e.g. "Nearest state when above the
midpoint always is above regardless of velocity."  and maybe group those two
cases together.

https://codereview.chromium.org/1614953004/diff/40001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelBaseTest.java:130:
PanelState nextState = mNoExpandPanel.findNearestPanelStateFromHeight(350,
UPWARD_VELOCITY);
Please update comments here too (along the lines of above).  Also, I would think
that now the velocity would make a difference, so I'd expect tests of the same
height with different velocity giving a different outcome.

[mdjones, 2016-01-22 22:51:13]

Questionable addressing of testing comments. PTAL

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

https://codereview.chromium.org/1614953004/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelBase.java:891:
* @return The percent of the distance the panel has to be to its next state
before animating
On 2016/01/22 21:22:51, Donn Denman wrote:
> Nit: we're not really using percents, we're using fractions, so I would change
> this to read "The fraction of the distance..." and "Default is the panel must
be
> half way to..." on the next line.

Done.

https://codereview.chromium.org/1614953004/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelBase.java:914:
prevState = nextState;
On 2016/01/22 21:22:51, Donn Denman wrote:
> Nit: I think it's more readable to move this down to just before setting
> nextState.

Done.

https://codereview.chromium.org/1614953004/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelBase.java:919:
// The values in PanelState are ascending, so this should work.
On 2016/01/22 21:22:51, Donn Denman wrote:
> Please add a comment to PanelState to say the states should be kept in order.

Done.

https://codereview.chromium.org/1614953004/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelBase.java:931:
if (velocity < 0.0f) {
On 2016/01/22 21:22:50, Donn Denman wrote:
> Consider simplifying by folding the two cases, e.g:
> float thresholdToNextState = velocity < 0.0f ? getThresholdToNextState() : 1.0
-
> getThresholdToNextState();
> if ((desiredPanelHeight - lowerBound) / distance > thresholdToNextState) {
>     return nextState;
> } else {
>     return prevState;
> }

Done.

https://codereview.chromium.org/1614953004/diff/40001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelBaseTest.java
(right):

https://codereview.chromium.org/1614953004/diff/40001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelBaseTest.java:106:
nextState = mExpandPanel.findNearestPanelStateFromHeight(310,
DOWNWARD_VELOCITY);
On 2016/01/22 21:22:51, Donn Denman wrote:
> It's a bit hard for me to understand what you're actually testing here.  I
think
> updating the comments would help.  Maybe add some constants for these
positions
> instead of explaining them in comments, e.g. ABOVE_PEEK_EXPANDED_MIDPOINT,
> BELOW_PEEK_EXPANDED_MIDPOINT.  Also, is it true that the velocity does not
have
> any effect here because we're in a 3-state panel which has equal bias between
up
> and down?  Please comment along these lines, e.g. "Nearest state when above
the
> midpoint always is above regardless of velocity."  and maybe group those two
> cases together.

Expressed in the new logic, the contextual search panel must be half way to the
next state in a given direction before it animates there; this matches the old
logic. The magnitude of the velocity is not considered, only if it is positive
or negative.

I'm not terribly fond of making a list of 12 constants; there would have to be
one per test case. Instead I compute them, but I'm not sure I like this because
it is similar to the logic we are trying to test...

https://codereview.chromium.org/1614953004/diff/40001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelBaseTest.java:130:
PanelState nextState = mNoExpandPanel.findNearestPanelStateFromHeight(350,
UPWARD_VELOCITY);
On 2016/01/22 21:22:51, Donn Denman wrote:
> Please update comments here too (along the lines of above).  Also, I would
think
> that now the velocity would make a difference, so I'd expect tests of the same
> height with different velocity giving a different outcome.

The magnitude of the velocity is not considered in the computation, only +/-.

[Donn Denman, 2016-01-22 23:23:41]

LGTM, Thanks for the update!  Sorry if this seems nit-picky!

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

https://codereview.chromium.org/1614953004/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelBase.java:933:

Nit: I would drop this blank line.

https://codereview.chromium.org/1614953004/diff/60001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelBaseTest.java
(right):

https://codereview.chromium.org/1614953004/diff/60001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelBaseTest.java:102:
float peekToExpHeight = MOCK_EXPANDED_HEIGHT - MOCK_PEEKED_HEIGHT;
Nit: these can be final (and all upper case?) I think.

https://codereview.chromium.org/1614953004/diff/60001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelBaseTest.java:129:
// Between EXPANDED and MAXIMIZED past the threshold in the down direction.
For completeness do you think it would help to add two cases just like these two
but in the up direction (like we have between peeking and expanded)?

https://codereview.chromium.org/1614953004/diff/60001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelBaseTest.java:146:
public void testNonExpandingPanelMovesToCorrectState() {
I think it would be helpful to move this up, since it's the simpler test.

https://codereview.chromium.org/1614953004/diff/60001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelBaseTest.java:148:
float height = MOCK_MAXIMIZED_HEIGHT - MOCK_PEEKED_HEIGHT;
Nit: this can be final too.

https://codereview.chromium.org/1614953004/diff/60001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelBaseTest.java:149:
float peekToMaxBound = threshold * height + MOCK_PEEKED_HEIGHT;
Let's see if I understand this now: I think what's confusing me is that the
velocity direction is tied to the boundary: When swiping up the boundary is near
the bottom when swiping down the boundary is near the top, that way you don't
need to swipe very far.  Let me know if that isn't right!  Therefor, these tests
need to test the appropriate direction at each boundary.

Can you add a block comment that explains this, maybe right before
peekToMaxBound and maxToPeekBound are computed?

[mdjones, 2016-01-23 00:25:21]

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

https://codereview.chromium.org/1614953004/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelBase.java:933:

On 2016/01/22 23:23:41, Donn Denman wrote:
> Nit: I would drop this blank line.

Done.

https://codereview.chromium.org/1614953004/diff/60001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelBaseTest.java
(right):

https://codereview.chromium.org/1614953004/diff/60001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelBaseTest.java:102:
float peekToExpHeight = MOCK_EXPANDED_HEIGHT - MOCK_PEEKED_HEIGHT;
On 2016/01/22 23:23:41, Donn Denman wrote:
> Nit: these can be final (and all upper case?) I think.

Made final, left camel case (as per my interpretation of style guide).

https://codereview.chromium.org/1614953004/diff/60001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelBaseTest.java:129:
// Between EXPANDED and MAXIMIZED past the threshold in the down direction.
On 2016/01/22 23:23:41, Donn Denman wrote:
> For completeness do you think it would help to add two cases just like these
two
> but in the up direction (like we have between peeking and expanded)?

Done.

https://codereview.chromium.org/1614953004/diff/60001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelBaseTest.java:146:
public void testNonExpandingPanelMovesToCorrectState() {
On 2016/01/22 23:23:41, Donn Denman wrote:
> I think it would be helpful to move this up, since it's the simpler test.

Done.

https://codereview.chromium.org/1614953004/diff/60001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelBaseTest.java:148:
float height = MOCK_MAXIMIZED_HEIGHT - MOCK_PEEKED_HEIGHT;
On 2016/01/22 23:23:41, Donn Denman wrote:
> Nit: this can be final too.

Done.

https://codereview.chromium.org/1614953004/diff/60001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelBaseTest.java:149:
float peekToMaxBound = threshold * height + MOCK_PEEKED_HEIGHT;
On 2016/01/22 23:23:41, Donn Denman wrote:
> Let's see if I understand this now: I think what's confusing me is that the
> velocity direction is tied to the boundary: When swiping up the boundary is
near
> the bottom when swiping down the boundary is near the top, that way you don't
> need to swipe very far.  Let me know if that isn't right!  Therefor, these
tests
> need to test the appropriate direction at each boundary.
> 
> Can you add a block comment that explains this, maybe right before
> peekToMaxBound and maxToPeekBound are computed?

That is correct. Comment added.

[gone, 2016-01-23 00:39:53]

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

[gone, 2016-01-23 00:39:53]

Contextual search folks have already done a review for this contextual search
code.

rs lgtm

[mdjones, 2016-01-25 21:01:11]

The CQ bit was checked by mdjones@chromium.org

[mdjones, 2016-01-25 21:01:11]

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

[commit-bot: I haz the power, 2016-01-25 21:03:40]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1614953004/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1614953004/80001

[commit-bot: I haz the power, 2016-01-25 21:51:23]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-01-25 21:53:14]

Description was changed from

==========
Allow swipe threshold adjustment

When the OverlayPanel is only using two states, the amount of swipe
it takes to move from one panel state to another is excessive. This
change allows adjustment of how sensitive the panels are to a swipe.

BUG=565585
==========

to

==========
Allow swipe threshold adjustment

When the OverlayPanel is only using two states, the amount of swipe
it takes to move from one panel state to another is excessive. This
change allows adjustment of how sensitive the panels are to a swipe.

BUG=565585

Committed: https://crrev.com/4782f344419a9990300da9e9312f7083a38ec47d
Cr-Commit-Position: refs/heads/master@{#371325}
==========

[commit-bot: I haz the power, 2016-01-25 21:53:15]

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