Skip expanded state for Contextual Search when swiping down

Skip expanded state for Contextual Search when swiping down

This change is for feature parity with Chrome Home. When
swiping down on the Contextual Search panel the expanded
state is skipped as it is unlikely that that user wants
to see half as much content but not interact with the
web page.

BUG=705553
Review-Url: https://codereview.chromium.org/2781563002
Cr-Commit-Position: refs/heads/master@{#459931}
Committed: https://chromium.googlesource.com/chromium/src/+/61e342e6e7a1a0d6dff3159d87437b5f8442b442

[Donn Denman, 2017-03-27 18:32:30]

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

[Donn Denman, 2017-03-27 18:32:31]

Added myself as a reviewer and noted a few nits.  Let me know if you want me to
just pick this up and land!

LGTM % nits

https://codereview.chromium.org/2781563002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java
(right):

https://codereview.chromium.org/2781563002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java:229:
if (mCurrentSwipeVelocity > 0 && state == PanelState.EXPANDED) return false;
Nit: add a blank line after this line (to flag the return).

https://codereview.chromium.org/2781563002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java:235:
if (mCurrentSwipeVelocity > 0) return 0.30f;
Nit: Maybe a blank line here too.

Is the mCurrentSwipeVelocity likely to be greater than .3?  If so maybe return
that instead, and use .3 as a threshold?

https://codereview.chromium.org/2781563002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java:251:

Nit: blank line not needed here.

[mdjones, 2017-03-27 20:09:47]

https://codereview.chromium.org/2781563002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java
(right):

https://codereview.chromium.org/2781563002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java:229:
if (mCurrentSwipeVelocity > 0 && state == PanelState.EXPANDED) return false;
On 2017/03/27 18:32:31, Donn Denman wrote:
> Nit: add a blank line after this line (to flag the return).

Done.

https://codereview.chromium.org/2781563002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java:235:
if (mCurrentSwipeVelocity > 0) return 0.30f;
On 2017/03/27 18:32:31, Donn Denman wrote:
> Nit: Maybe a blank line here too.
Added.

> 
> Is the mCurrentSwipeVelocity likely to be greater than .3?  If so maybe return
> that instead, and use .3 as a threshold?

We only care if the velocity is positive or negative (up or down). The threshold
value is a different concept; it means how far the panel must be swiped to the
next state before automatically animating there when released.

https://codereview.chromium.org/2781563002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java:251:

On 2017/03/27 18:32:31, Donn Denman wrote:
> Nit: blank line not needed here.

Done.

[Donn Denman, 2017-03-27 20:11:02]

Thanks Matt!  (LGTM)

[mdjones, 2017-03-27 20:21:52]

Description was changed from

==========
Skip expanded state for Contextual Search when swiping down

BUG=705553
==========

to

==========
Skip expanded state for Contextual Search when swiping down

This change is for feature parity with Chrome Home. When
swiping down on the Contextual Search panel the expanded
state is skipped as it is unlikely that that user wants
to see half as much content but not interact with the
web page.

BUG=705553
==========

[mdjones, 2017-03-27 20:22:31]

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

[mdjones, 2017-03-27 20:22:32]

+twellington Should we instead close the panel or keep it minimized?

[Theresa, 2017-03-27 20:30:24]

On 2017/03/27 20:22:32, mdjones wrote:
> +twellington Should we instead close the panel or keep it minimized?

I think we should keep it minimized (unless the fling velocity is high enough
that it would have closed prior to this change). With the introduction of quick
actions, it's possible (maybe not probable) that users would want to do
something like:
1) Highlight phone number
2) Open panel to search results for number
3) Minimize panel and tap on left side of bar to call the number

[Donn Denman, 2017-03-27 20:40:28]

On 2017/03/27 20:30:24, Theresa wrote:
> On 2017/03/27 20:22:32, mdjones wrote:
> > +twellington Should we instead close the panel or keep it minimized?
> 
> I think we should keep it minimized (unless the fling velocity is high enough
> that it would have closed prior to this change). With the introduction of
quick
> actions, it's possible (maybe not probable) that users would want to do
> something like:
> 1) Highlight phone number
> 2) Open panel to search results for number
> 3) Minimize panel and tap on left side of bar to call the number

+1

The Bar hides quite easily so this choice seems best.

[Theresa, 2017-03-27 21:54:33]

lgtm

[mdjones, 2017-03-27 22:32:31]

The CQ bit was checked by mdjones@chromium.org

[commit-bot: I haz the power, 2017-03-27 22:33:57]

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, 2017-03-27 23:21:52]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1490653951757310,
"parent_rev": "f4c12323bc9ef46575de81489ce7afbf57484d5c", "commit_rev":
"61e342e6e7a1a0d6dff3159d87437b5f8442b442"}

[commit-bot: I haz the power, 2017-03-27 23:23:23]

Description was changed from

==========
Skip expanded state for Contextual Search when swiping down

This change is for feature parity with Chrome Home. When
swiping down on the Contextual Search panel the expanded
state is skipped as it is unlikely that that user wants
to see half as much content but not interact with the
web page.

BUG=705553
==========

to

==========
Skip expanded state for Contextual Search when swiping down

This change is for feature parity with Chrome Home. When
swiping down on the Contextual Search panel the expanded
state is skipped as it is unlikely that that user wants
to see half as much content but not interact with the
web page.

BUG=705553

Review-Url: https://codereview.chromium.org/2781563002
Cr-Commit-Position: refs/heads/master@{#459931}
Committed:
https://chromium.googlesource.com/chromium/src/+/61e342e6e7a1a0d6dff3159d8743...
==========

[commit-bot: I haz the power, 2017-03-27 23:23:26]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/61e342e6e7a1a0d6dff3159d8743...