Text selected with double-tap should not cause vibration

Text selected with double-tap should not cause vibration

Previously a haptic feedback (vibration) was performed whenever selection handles
were shown. This was problematic since double tapping on text would also cause the
selection handles to be shown. Now haptic feedback only occurs when a long press
gesture is handled. This required changing some context menu logic to return whether
or not a context menu was actually shown.

BUG=606175
Committed: https://crrev.com/6de57089abaea05decce400f1bae7889b34e839b
Cr-Commit-Position: refs/heads/master@{#395490}

[amaralp, 2016-05-17 17:18:45]

Description was changed from

==========
Text selected with double-tap should not cause vibration

Haptic feedback (vibration) is now only performed when a long press is consumed.
BUG=606175
==========

to

==========
Text selected with double-tap should not cause vibration

Haptic feedback (vibration) is now only performed when a long press is consumed.
BUG=606175
==========

[amaralp, 2016-05-17 17:18:46]

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

[amaralp, 2016-05-17 23:57:59]

On 2016/05/17 17:18:46, amaralp wrote:
> mailto:amaralp@chromium.org changed reviewers:
> + mailto:aelias@chromium.org

PTAL

[aelias_OOO_until_Jul13, 2016-05-18 01:22:33]

https://codereview.chromium.org/1986783002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java
(left):

https://codereview.chromium.org/1986783002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java:168:
public boolean shouldShowContextMenu(ContextMenuParams params) {
Can you add more details to the CL description explaining why your change is
structured as it is, for the benefit of future code reviewers?  (Since this will
also need OWNERS review from a few other people to land.)

https://codereview.chromium.org/1986783002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/page/ContextMenuController.cpp (right):

https://codereview.chromium.org/1986783002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/page/ContextMenuController.cpp:174: void
ContextMenuController::showContextMenu(Event* event)
I'd suggest adding a unit test for the new logic in
ContextMenuControllerTest.cpp.  I suggest checking whether showContextMenu()
executed or not by looking for its side effects.  (I suggest also looking at
WebViewTest.cpp and WebFrameTest.cpp for inspiration on how Blink unit tests can
be written.)

https://codereview.chromium.org/1986783002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/ContextMenuClientImpl.cpp (right):

https://codereview.chromium.org/1986783002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/ContextMenuClientImpl.cpp:142: return true;
As discussed offline, it should be fine for this to be false.

https://codereview.chromium.org/1986783002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/ContextMenuClientImpl.cpp:366: #if OS(ANDROID)
Instead of #if OS(ANDROID), let's make this logic conditional on whether the
event type is a LongPress or a Click.  That event information isn't available in
this scope, so I'd suggest adding an additional argument to this method passing
it in (probably just the event type enum).

https://codereview.chromium.org/1986783002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/ContextMenuClientImpl.cpp:370: return true;
I think this should be an early-return above the showContextMenu() call, to
substitute for the Java shouldShowContextMenu() logic you deleted.  How about
putting this early-return at the beginning next to
ContextMenuAllowedScope::isContextMenuAllowed()?

[amaralp, 2016-05-18 22:14:19]

Description was changed from

==========
Text selected with double-tap should not cause vibration

Haptic feedback (vibration) is now only performed when a long press is consumed.
BUG=606175
==========

to

==========
Text selected with double-tap should not cause vibration

Previously a haptic feedback (vibration) was performed whenever selection
handles were shown. This was problematic since double tapping on text would also
cause the selection handles to be shown. Now haptic feedback only occurs when a
long press gesture is handled. This required changing some context menu logic to
return whether or not a context menu was actually shown.

BUG=606175
==========

[amaralp, 2016-05-18 22:16:18]

PTAL, will add tests shortly

https://codereview.chromium.org/1986783002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java
(left):

https://codereview.chromium.org/1986783002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java:168:
public boolean shouldShowContextMenu(ContextMenuParams params) {
On 2016/05/18 01:22:32, aelias wrote:
> Can you add more details to the CL description explaining why your change is
> structured as it is, for the benefit of future code reviewers?  (Since this
will
> also need OWNERS review from a few other people to land.)

Done.

https://codereview.chromium.org/1986783002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/page/ContextMenuController.cpp (right):

https://codereview.chromium.org/1986783002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/page/ContextMenuController.cpp:174: void
ContextMenuController::showContextMenu(Event* event)
On 2016/05/18 01:22:32, aelias wrote:
> I'd suggest adding a unit test for the new logic in
> ContextMenuControllerTest.cpp.  I suggest checking whether showContextMenu()
> executed or not by looking for its side effects.  (I suggest also looking at
> WebViewTest.cpp and WebFrameTest.cpp for inspiration on how Blink unit tests
can
> be written.)

Acknowledged.

https://codereview.chromium.org/1986783002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/ContextMenuClientImpl.cpp (right):

https://codereview.chromium.org/1986783002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/ContextMenuClientImpl.cpp:142: return true;
On 2016/05/18 01:22:32, aelias wrote:
> As discussed offline, it should be fine for this to be false.

Done.

https://codereview.chromium.org/1986783002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/ContextMenuClientImpl.cpp:366: #if OS(ANDROID)
On 2016/05/18 01:22:33, aelias wrote:
> Instead of #if OS(ANDROID), let's make this logic conditional on whether the
> event type is a LongPress or a Click.  That event information isn't available
in
> this scope, so I'd suggest adding an additional argument to this method
passing
> it in (probably just the event type enum).

Done.

https://codereview.chromium.org/1986783002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/ContextMenuClientImpl.cpp:370: return true;
On 2016/05/18 01:22:33, aelias wrote:
> I think this should be an early-return above the showContextMenu() call, to
> substitute for the Java shouldShowContextMenu() logic you deleted.  How about
> putting this early-return at the beginning next to
> ContextMenuAllowedScope::isContextMenuAllowed()?

I rely on the data variable being populated so I can't do this at the very top
like ContextMenuAllowedScope::isContextMenuAllowed(), but I moved it up a little
bit.

[aelias_OOO_until_Jul13, 2016-05-19 02:25:21]

https://codereview.chromium.org/1986783002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/page/ContextMenuController.cpp (right):

https://codereview.chromium.org/1986783002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/page/ContextMenuController.cpp:182: if
(m_client->showContextMenu(m_contextMenu.get(), fromTouch) && event)
This "&& event" is too late because there would already have been a null pointer
crash from event->isMouseEvent().

https://codereview.chromium.org/1986783002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/ContextMenuClientImpl.cpp (right):

https://codereview.chromium.org/1986783002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/ContextMenuClientImpl.cpp:268: }
Nit: Blink style is that one-line control clauses should not use braces.

https://codereview.chromium.org/1986783002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/ContextMenuClientImpl.h (right):

https://codereview.chromium.org/1986783002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/ContextMenuClientImpl.h:45: bool
showContextMenu(const ContextMenu*, bool) override;
The meaning of this boolean is nonobvious, so it should have a name in the
declaration.

[amaralp, 2016-05-19 18:21:10]

https://codereview.chromium.org/1986783002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/page/ContextMenuController.cpp (right):

https://codereview.chromium.org/1986783002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/page/ContextMenuController.cpp:182: if
(m_client->showContextMenu(m_contextMenu.get(), fromTouch) && event)
On 2016/05/19 02:25:21, aelias wrote:
> This "&& event" is too late because there would already have been a null
pointer
> crash from event->isMouseEvent().

Done.

https://codereview.chromium.org/1986783002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/ContextMenuClientImpl.cpp (right):

https://codereview.chromium.org/1986783002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/ContextMenuClientImpl.cpp:268: }
On 2016/05/19 02:25:21, aelias wrote:
> Nit: Blink style is that one-line control clauses should not use braces.

Done.

https://codereview.chromium.org/1986783002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/ContextMenuClientImpl.h (right):

https://codereview.chromium.org/1986783002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/ContextMenuClientImpl.h:45: bool
showContextMenu(const ContextMenu*, bool) override;
On 2016/05/19 02:25:21, aelias wrote:
> The meaning of this boolean is nonobvious, so it should have a name in the
> declaration.

Done.

[amaralp, 2016-05-20 02:17:51]

Added tests. PTAL

[aelias_OOO_until_Jul13, 2016-05-20 03:27:03]

Description was changed from

==========
Text selected with double-tap should not cause vibration

Previously a haptic feedback (vibration) was performed whenever selection
handles were shown. This was problematic since double tapping on text would also
cause the selection handles to be shown. Now haptic feedback only occurs when a
long press gesture is handled. This required changing some context menu logic to
return whether or not a context menu was actually shown.

BUG=606175
==========

to

==========
Text selected with double-tap should not cause vibration

Previously a haptic feedback (vibration) was performed whenever selection
handles
were shown. This was problematic since double tapping on text would also cause
the
selection handles to be shown. Now haptic feedback only occurs when a long press
gesture is handled. This required changing some context menu logic to return
whether or not a context menu was actually shown.

BUG=606175
==========

[aelias_OOO_until_Jul13, 2016-05-20 03:27:11]

Description was changed from

==========
Text selected with double-tap should not cause vibration

Previously a haptic feedback (vibration) was performed whenever selection
handles
were shown. This was problematic since double tapping on text would also cause
the
selection handles to be shown. Now haptic feedback only occurs when a long press
gesture is handled. This required changing some context menu logic to return
whether or not a context menu was actually shown.

BUG=606175
==========

to

==========
Text selected with double-tap should not cause vibration

Previously a haptic feedback (vibration) was performed whenever selection
handles
were shown. This was problematic since double tapping on text would also cause
the
selection handles to be shown. Now haptic feedback only occurs when a long press
gesture is handled. This required changing some context menu logic to return
whether
or not a context menu was actually shown.

BUG=606175
==========

[aelias_OOO_until_Jul13, 2016-05-20 03:32:59]

aelias@chromium.org changed reviewers:
+ rbyers@chromium.org, tedchoc@chromium.org

[aelias_OOO_until_Jul13, 2016-05-20 03:33:00]

Source/web lgtm, thanks.  Adding additional OWNERS:

- tedchoc@ for browser-side changes
- rbyers@ for Source/core

https://codereview.chromium.org/1986783002/diff/80001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/tab/TabContextMenuPopulator.java
(right):

https://codereview.chromium.org/1986783002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/tab/TabContextMenuPopulator.java:46:
}
nit: you are committing a newline at the end of a few files.  This is probably
something your text editor did automatically when you saved this file.  Please
configure your text editor and/or git not to do this, since it's distracting in
code review and pollutes git blame.

[Rick Byers, 2016-05-20 14:01:58]

Source/core LGTM with nit

https://codereview.chromium.org/1986783002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/page/ContextMenuClient.h (right):

https://codereview.chromium.org/1986783002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/page/ContextMenuClient.h:36: virtual bool
showContextMenu(const ContextMenu*, bool) = 0;
nit: please add a comment documenting what these bools mean (or in the case of
the argument, just give it an explicit name "fromTouch").

[amaralp, 2016-05-20 16:57:08]

https://codereview.chromium.org/1986783002/diff/80001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/tab/TabContextMenuPopulator.java
(right):

https://codereview.chromium.org/1986783002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/tab/TabContextMenuPopulator.java:46:
}
On 2016/05/20 03:33:00, aelias wrote:
> nit: you are committing a newline at the end of a few files.  This is probably
> something your text editor did automatically when you saved this file.  Please
> configure your text editor and/or git not to do this, since it's distracting
in
> code review and pollutes git blame.

Done.

https://codereview.chromium.org/1986783002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/page/ContextMenuClient.h (right):

https://codereview.chromium.org/1986783002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/page/ContextMenuClient.h:36: virtual bool
showContextMenu(const ContextMenu*, bool) = 0;
On 2016/05/20 14:01:58, Rick Byers wrote:
> nit: please add a comment documenting what these bools mean (or in the case of
> the argument, just give it an explicit name "fromTouch").

Done.

[Rick Byers, 2016-05-20 19:42:17]

https://codereview.chromium.org/1986783002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/page/ContextMenuClient.h (right):

https://codereview.chromium.org/1986783002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/page/ContextMenuClient.h:36: virtual bool
showContextMenu(const ContextMenu*, bool) = 0;
On 2016/05/20 16:57:08, amaralp wrote:
> On 2016/05/20 14:01:58, Rick Byers wrote:
> > nit: please add a comment documenting what these bools mean (or in the case
of
> > the argument, just give it an explicit name "fromTouch").
> 
> Done.

Thanks!  Still LGTM

[Ted C, 2016-05-23 22:58:50]

On 2016/05/20 19:42:17, Rick Byers (out until May 24) wrote:
>
https://codereview.chromium.org/1986783002/diff/80001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/page/ContextMenuClient.h (right):
> 
>
https://codereview.chromium.org/1986783002/diff/80001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/page/ContextMenuClient.h:36: virtual bool
> showContextMenu(const ContextMenu*, bool) = 0;
> On 2016/05/20 16:57:08, amaralp wrote:
> > On 2016/05/20 14:01:58, Rick Byers wrote:
> > > nit: please add a comment documenting what these bools mean (or in the
case
> of
> > > the argument, just give it an explicit name "fromTouch").
> > 
> > Done.
> 
> Thanks!  Still LGTM

browser side - lgtm

[amaralp, 2016-05-23 23:06:48]

The CQ bit was checked by amaralp@chromium.org

[amaralp, 2016-05-23 23:06:48]

The patchset sent to the CQ was uploaded after l-g-t-m from aelias@chromium.org
Link to the patchset: https://codereview.chromium.org/1986783002/#ps120001
(title: "added description of return value of showContextMenu")

[commit-bot: I haz the power, 2016-05-23 23:07:33]

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

[commit-bot: I haz the power, 2016-05-24 00:45:52]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-05-24 00:48:06]

Description was changed from

==========
Text selected with double-tap should not cause vibration

Previously a haptic feedback (vibration) was performed whenever selection
handles
were shown. This was problematic since double tapping on text would also cause
the
selection handles to be shown. Now haptic feedback only occurs when a long press
gesture is handled. This required changing some context menu logic to return
whether
or not a context menu was actually shown.

BUG=606175
==========

to

==========
Text selected with double-tap should not cause vibration

Previously a haptic feedback (vibration) was performed whenever selection
handles
were shown. This was problematic since double tapping on text would also cause
the
selection handles to be shown. Now haptic feedback only occurs when a long press
gesture is handled. This required changing some context menu logic to return
whether
or not a context menu was actually shown.

BUG=606175

Committed: https://crrev.com/6de57089abaea05decce400f1bae7889b34e839b
Cr-Commit-Position: refs/heads/master@{#395490}
==========

[commit-bot: I haz the power, 2016-05-24 00:48:08]

Patchset 7 (id:??) landed as
https://crrev.com/6de57089abaea05decce400f1bae7889b34e839b
Cr-Commit-Position: refs/heads/master@{#395490}