Change long press behavior when touch text selection is enabled:

Change long press behavior when touch text selection is enabled.

Long press is overloaded to do the following three functions: start drag/drop, select word and show context menu. They are performed in the following order:
1. If touch drag drop is enabled and long press happens on a draggable element, start drag drop.
2. Else if touch text editing is enabled and long press happens over selectable text, select closest word.
3. Else, show context menu.

BUG=115237
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=148740

[hayato, 2013-04-12 07:53:48]

Let me take a look, but I am afraid that it will take some time for me to
understand how 'touch text selection' has been working so far.

Looks like the review requires how editing works. Editing is one of the most
difficult part for me. :)

[varunjain, 2013-04-17 02:41:06]

friendly ping :)

On 2013/04/12 07:53:48, hayato wrote:
> Let me take a look, but I am afraid that it will take some time for me to
> understand how 'touch text selection' has been working so far.
> 
> Looks like the review requires how editing works. Editing is one of the most
> difficult part for me. :)

[hayato, 2013-04-17 04:03:49]

Ops. Sorry. Let me take a look today.

On 2013/04/17 02:41:06, varunjain wrote:
> friendly ping :)
> 
> On 2013/04/12 07:53:48, hayato wrote:
> > Let me take a look, but I am afraid that it will take some time for me to
> > understand how 'touch text selection' has been working so far.
> > 
> > Looks like the review requires how editing works. Editing is one of the most
> > difficult part for me. :)

[hayato, 2013-04-17 04:51:03]

In overall, looks good, but there are some code I am not familiar with.

e.g. I am not sure why we need subframe() in HitTests.

It might be good If we can have a review from other reviewers.

https://codereview.chromium.org/14089009/diff/2001/Source/WebCore/page/EventH...
File Source/WebCore/page/EventHandler.cpp (right):

https://codereview.chromium.org/14089009/diff/2001/Source/WebCore/page/EventH...
Source/WebCore/page/EventHandler.cpp:2485: HitTestRequest
request(HitTestRequest::ReadOnly | HitTestRequest::DisallowShadowContent);
Could we remove DisallowShadowContent from this HitTest?
I don't see any reason to disallow shadow content here.
MouseEvent should happen in the shadow tree.

https://codereview.chromium.org/14089009/diff/2001/Source/WebCore/page/EventH...
Source/WebCore/page/EventHandler.cpp:2507: bool shouldLongPressSelectWord =
m_frame->settings() && m_frame->settings()->touchEditingEnabled();
Line 2507 to 2510 could be merged into #IF .. #ELSE ... #ENDIF block.
e.g.
#if OS(ANDROID)
  bool shouldLongPressSelectWord = true
#else
  bool shouldLongPressSelectWord = ...
#endif

https://codereview.chromium.org/14089009/diff/2001/Source/WebCore/page/EventH...
Source/WebCore/page/EventHandler.cpp:2528: bool
EventHandler::handleGestureLongTap(const PlatformGestureEvent& gestureEvent)
It's optional, but looks like we can share some of code between
handleGestureLongPress and handleGestureLongTap.

[varunjain, 2013-04-17 17:45:36]

Hi Hayato, Thanks for the review. About your subframe hittest question. It is
required to have the gesture descend correctly into iframes. It is covered by
the *-iframe test.

https://codereview.chromium.org/14089009/diff/2001/Source/WebCore/page/EventH...
File Source/WebCore/page/EventHandler.cpp (right):

https://codereview.chromium.org/14089009/diff/2001/Source/WebCore/page/EventH...
Source/WebCore/page/EventHandler.cpp:2485: HitTestRequest
request(HitTestRequest::ReadOnly | HitTestRequest::DisallowShadowContent);
On 2013/04/17 04:51:03, hayato wrote:
> Could we remove DisallowShadowContent from this HitTest?
> I don't see any reason to disallow shadow content here.
> MouseEvent should happen in the shadow tree.

I am not sure but wouldnt DisallowShadowContent stop the hit test from
descending into shadow dom? If that is indeed correct, I think we should keep
it. There is no reason this hit test should go into shadow dom.

https://codereview.chromium.org/14089009/diff/2001/Source/WebCore/page/EventH...
Source/WebCore/page/EventHandler.cpp:2507: bool shouldLongPressSelectWord =
m_frame->settings() && m_frame->settings()->touchEditingEnabled();
On 2013/04/17 04:51:03, hayato wrote:
> Line 2507 to 2510 could be merged into #IF .. #ELSE ... #ENDIF block.
> e.g.
> #if OS(ANDROID)
>   bool shouldLongPressSelectWord = true
> #else
>   bool shouldLongPressSelectWord = ...
> #endif

Done.

https://codereview.chromium.org/14089009/diff/2001/Source/WebCore/page/EventH...
Source/WebCore/page/EventHandler.cpp:2528: bool
EventHandler::handleGestureLongTap(const PlatformGestureEvent& gestureEvent)
On 2013/04/17 04:51:03, hayato wrote:
> It's optional, but looks like we can share some of code between
> handleGestureLongPress and handleGestureLongTap.

Done.

[varunjain, 2013-04-17 22:06:14]

removed HitTestRequest::DisallowShadowContent after talking to Dimitri.

On 2013/04/17 17:45:36, varunjain wrote:
> Hi Hayato, Thanks for the review. About your subframe hittest question. It is
> required to have the gesture descend correctly into iframes. It is covered by
> the *-iframe test.
> 
>
https://codereview.chromium.org/14089009/diff/2001/Source/WebCore/page/EventH...
> File Source/WebCore/page/EventHandler.cpp (right):
> 
>
https://codereview.chromium.org/14089009/diff/2001/Source/WebCore/page/EventH...
> Source/WebCore/page/EventHandler.cpp:2485: HitTestRequest
> request(HitTestRequest::ReadOnly | HitTestRequest::DisallowShadowContent);
> On 2013/04/17 04:51:03, hayato wrote:
> > Could we remove DisallowShadowContent from this HitTest?
> > I don't see any reason to disallow shadow content here.
> > MouseEvent should happen in the shadow tree.
> 
> I am not sure but wouldnt DisallowShadowContent stop the hit test from
> descending into shadow dom? If that is indeed correct, I think we should keep
> it. There is no reason this hit test should go into shadow dom.
> 
>
https://codereview.chromium.org/14089009/diff/2001/Source/WebCore/page/EventH...
> Source/WebCore/page/EventHandler.cpp:2507: bool shouldLongPressSelectWord =
> m_frame->settings() && m_frame->settings()->touchEditingEnabled();
> On 2013/04/17 04:51:03, hayato wrote:
> > Line 2507 to 2510 could be merged into #IF .. #ELSE ... #ENDIF block.
> > e.g.
> > #if OS(ANDROID)
> >   bool shouldLongPressSelectWord = true
> > #else
> >   bool shouldLongPressSelectWord = ...
> > #endif
> 
> Done.
> 
>
https://codereview.chromium.org/14089009/diff/2001/Source/WebCore/page/EventH...
> Source/WebCore/page/EventHandler.cpp:2528: bool
> EventHandler::handleGestureLongTap(const PlatformGestureEvent& gestureEvent)
> On 2013/04/17 04:51:03, hayato wrote:
> > It's optional, but looks like we can share some of code between
> > handleGestureLongPress and handleGestureLongTap.
> 
> Done.

[varunjain, 2013-04-17 22:25:15]

+Levi

[dglazkov, 2013-04-18 18:39:17]

LGTM, unless Levi yells angrily in the next few hours.

[commit-bot: I haz the power, 2013-04-19 03:44:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/varunjain@chromium.org/14089009/20001

[commit-bot: I haz the power, 2013-04-19 03:58:32]

Sorry for I got bad news for ya.
Compile failed with a clobber build on mac_layout_rel.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_layout...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-04-19 05:18:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/varunjain@chromium.org/14089009/20001

[commit-bot: I haz the power, 2013-04-19 05:18:45]

"webkit_tests" failed. Giving up immediately.
Builder is linux_layout_rel, revision is HEAD

[commit-bot: I haz the power, 2013-04-19 18:03:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/varunjain@chromium.org/14089009/33001

[commit-bot: I haz the power, 2013-04-19 18:31:53]

Change committed as 148740