🏡 Disable BottomSheet touches while context menu is open.

[Home] Disable BottomSheet touches while context menu is open.

BUG=695818
Review-Url: https://codereview.chromium.org/2810713002
Cr-Commit-Position: refs/heads/master@{#463979}
Committed: https://chromium.googlesource.com/chromium/src/+/6bdd60bde9cc56d72767bb7e13b777fd47074006

[Michael van Ouwerkerk, 2017-04-10 13:26:15]

The CQ bit was checked by mvanouwerkerk@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-10 13:26:32]

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

[Michael van Ouwerkerk, 2017-04-10 13:27:12]

mvanouwerkerk@chromium.org changed reviewers:
+ dgn@chromium.org, twellington@chromium.org

[Michael van Ouwerkerk, 2017-04-10 13:27:13]

Nicolas, could you take a look please for the ContextMenuManager changes?

Theresa, could you take a look please for the BottomSheet changes?

[commit-bot: I haz the power, 2017-04-10 14:10:20]

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

[commit-bot: I haz the power, 2017-04-10 14:10:21]

Dry run: This issue passed the CQ dry run.

[Theresa, 2017-04-10 19:55:44]

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

[Theresa, 2017-04-10 19:55:44]

+mdjones@ -- will you please look at the changes to BottomSheet?

https://codereview.chromium.org/2810713002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/widget/bottomsheet/BottomSheet.java
(right):

https://codereview.chromium.org/2810713002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/widget/bottomsheet/BottomSheet.java:187:
private boolean mTouchEnabled = true;
nit: mIsTouchEnabled to match boolean below this one.

https://codereview.chromium.org/2810713002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/widget/bottomsheet/BottomSheet.java:1013:
* mode, or when "find in page" is visible, or when the toolbar is hidden.
nit: Remove the "or" before "when 'find in page'..."

[mdjones, 2017-04-10 20:12:04]

https://codereview.chromium.org/2810713002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/widget/bottomsheet/BottomSheet.java
(right):

https://codereview.chromium.org/2810713002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/widget/bottomsheet/BottomSheet.java:349:
if (!mTouchEnabled || !canMoveSheet()) return false;
The only thing being controlled in this class is the ability to move the sheet
up or down; when false is returned from the *TouchEvent functions, the event is
passed to the children. That being said, there are probably two options:

1. mTouchEnabled is rolled into canMoveSheet() since that's what it is
controlling and rename it.

2. Return true in the *TouchEvent functions if mTouchEnabled is true and then do
nothing. This would likely allow you to remove some of the logic in the
children.

[Michael van Ouwerkerk, 2017-04-11 10:49:50]

Description was changed from

==========
Disable BottomSheet touches while context menu is open.

BUG=695818
==========

to

==========
[Home] Disable BottomSheet touches while context menu is open.

BUG=695818
==========

[Michael van Ouwerkerk, 2017-04-11 11:03:07]

Thanks! Please take another look.

https://codereview.chromium.org/2810713002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/widget/bottomsheet/BottomSheet.java
(right):

https://codereview.chromium.org/2810713002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/widget/bottomsheet/BottomSheet.java:187:
private boolean mTouchEnabled = true;
On 2017/04/10 19:55:44, Theresa wrote:
> nit: mIsTouchEnabled to match boolean below this one.

Done.

https://codereview.chromium.org/2810713002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/widget/bottomsheet/BottomSheet.java:349:
if (!mTouchEnabled || !canMoveSheet()) return false;
On 2017/04/10 20:12:04, mdjones wrote:
> The only thing being controlled in this class is the ability to move the sheet
> up or down; when false is returned from the *TouchEvent functions, the event
is
> passed to the children. That being said, there are probably two options:
> 
> 1. mTouchEnabled is rolled into canMoveSheet() since that's what it is
> controlling and rename it.
> 
> 2. Return true in the *TouchEvent functions if mTouchEnabled is true and then
do
> nothing. This would likely allow you to remove some of the logic in the
> children.

Returning true seems like the cleanest solution so we can remove some similar
logic further down.

https://codereview.chromium.org/2810713002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/widget/bottomsheet/BottomSheet.java:1013:
* mode, or when "find in page" is visible, or when the toolbar is hidden.
On 2017/04/10 19:55:44, Theresa wrote:
> nit: Remove the "or" before "when 'find in page'..."

Done.

[Michael van Ouwerkerk, 2017-04-11 11:03:16]

The CQ bit was checked by mvanouwerkerk@chromium.org to run a CQ dry run

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

Dry run: 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-04-11 11:38:03]

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

[commit-bot: I haz the power, 2017-04-11 11:38:04]

Dry run: This issue passed the CQ dry run.

[mdjones, 2017-04-11 15:37:03]

lgtm % nit

https://codereview.chromium.org/2810713002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/widget/bottomsheet/BottomSheet.java
(right):

https://codereview.chromium.org/2810713002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/widget/bottomsheet/BottomSheet.java:349:
if (!mIsTouchEnabled) return true;
nit: doc, something like, "If touch is disabled, onInterceptTouchEvent acts like
a black hole and consumes touch events without doing anything with them."

[dgn, 2017-04-11 22:28:43]

suggestions lgtm

[Michael van Ouwerkerk, 2017-04-12 09:25:06]

Thanks!

https://codereview.chromium.org/2810713002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/widget/bottomsheet/BottomSheet.java
(right):

https://codereview.chromium.org/2810713002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/widget/bottomsheet/BottomSheet.java:349:
if (!mIsTouchEnabled) return true;
On 2017/04/11 15:37:03, mdjones wrote:
> nit: doc, something like, "If touch is disabled, onInterceptTouchEvent acts
like
> a black hole and consumes touch events without doing anything with them."

Done.

[Michael van Ouwerkerk, 2017-04-12 09:25:10]

The CQ bit was checked by mvanouwerkerk@chromium.org

[Michael van Ouwerkerk, 2017-04-12 09:25:10]

The patchset sent to the CQ was uploaded after l-g-t-m from
mdjones@chromium.org, dgn@chromium.org
Link to the patchset: https://codereview.chromium.org/2810713002/#ps40001
(title: "Add some comments.")

[commit-bot: I haz the power, 2017-04-12 09:25:31]

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-04-12 10:02:56]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1491989110575900,
"parent_rev": "b33836726c84241997ec990495acd9f9616008d0", "commit_rev":
"6bdd60bde9cc56d72767bb7e13b777fd47074006"}

[commit-bot: I haz the power, 2017-04-12 10:03:51]

Description was changed from

==========
[Home] Disable BottomSheet touches while context menu is open.

BUG=695818
==========

to

==========
[Home] Disable BottomSheet touches while context menu is open.

BUG=695818

Review-Url: https://codereview.chromium.org/2810713002
Cr-Commit-Position: refs/heads/master@{#463979}
Committed:
https://chromium.googlesource.com/chromium/src/+/6bdd60bde9cc56d72767bb7e13b7...
==========

[commit-bot: I haz the power, 2017-04-12 10:03:52]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/6bdd60bde9cc56d72767bb7e13b7...