Show handles even when dragging from outside selectable region

Show handles even when dragging from outside selectable region

TEST=checked that selection handles show up when dragging from outside
     selectable region, e.g. text, and drop inside selectable region using
     a stylus while pressing a button.
BUG=397468
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=285785

[jdduke (slow), 2014-07-25 14:07:27]

https://codereview.chromium.org/416383003/diff/1/content/browser/renderer_hos...
File content/browser/renderer_host/input/gesture_text_selector.cc (right):

https://codereview.chromium.org/416383003/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/input/gesture_text_selector.cc:52:
client_->ShowSelectionHandlesAutomatically();
Any idea why the call to |client_->ShowSelectionHandlesAutomatically();| from
ET_GESTURE_SCROLL_BEGIN is insufficient?

[jdduke (slow), 2014-07-25 14:25:03]

https://codereview.chromium.org/416383003/diff/1/content/browser/renderer_hos...
File content/browser/renderer_host/input/gesture_text_selector.cc (right):

https://codereview.chromium.org/416383003/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/input/gesture_text_selector.cc:52:
client_->ShowSelectionHandlesAutomatically();
On 2014/07/25 14:07:27, jdduke wrote:
> Any idea why the call to |client_->ShowSelectionHandlesAutomatically();| from
> ET_GESTURE_SCROLL_BEGIN is insufficient?

I wonder if the |Unselect()| call is triggering a new selection update which
"consumes" the automatic showing.  In any case, with this change I would remove
the |client_->ShowSelectionHandlesAutomatically()| call entirely from
ET_GESTURE_SCROLL_BEGIN.  With that, lgtm (although it would still be good to
verify the exact order of events making the previous solution insufficient), and
please validate the callback behavior in an existing test (or new one). 

I'm still wondering if it would be better to show the handles *after* the
dragging is finished, but that's a discussion for another day.

[Changwan Ryu, 2014-07-25 22:12:47]

https://codereview.chromium.org/416383003/diff/1/content/browser/renderer_hos...
File content/browser/renderer_host/input/gesture_text_selector.cc (right):

https://codereview.chromium.org/416383003/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/input/gesture_text_selector.cc:52:
client_->ShowSelectionHandlesAutomatically();
Removed ShowSelectionHandlesAutomatically() from ET_GESTURE_SCROLL_BEGIN.

This only occurs when dragging starts in unselectable region, so I suspect that
we hide selection handles when it is not applicable (selected region is null). I
agree that showing handles on ET_GESTURE_SCROLL_END is much neater, but I'd like
to address that in the future. (Seems that refactoring is being done. :) )

On 2014/07/25 14:25:03, jdduke wrote:
> On 2014/07/25 14:07:27, jdduke wrote:
> > Any idea why the call to |client_->ShowSelectionHandlesAutomatically();|
from
> > ET_GESTURE_SCROLL_BEGIN is insufficient?
> 
> I wonder if the |Unselect()| call is triggering a new selection update which
> "consumes" the automatic showing.  In any case, with this change I would
remove
> the |client_->ShowSelectionHandlesAutomatically()| call entirely from
> ET_GESTURE_SCROLL_BEGIN.  With that, lgtm (although it would still be good to
> verify the exact order of events making the previous solution insufficient),
and
> please validate the callback behavior in an existing test (or new one). 
> 
> I'm still wondering if it would be better to show the handles *after* the
> dragging is finished, but that's a discussion for another day.

[Changwan Ryu, 2014-07-25 22:15:09]

The CQ bit was checked by changwan@chromium.org

[commit-bot: I haz the power, 2014-07-25 22:16:06]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/changwan@chromium.org/416383003/40001

[commit-bot: I haz the power, 2014-07-26 05:02:51]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_rel_swarming on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel_sw...)
  mac_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_compile_...)
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)
  linux_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu_trigger...)
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/33567)
  win_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)

[commit-bot: I haz the power, 2014-07-26 18:09:54]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_chromium_clang_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[commit-bot: I haz the power, 2014-07-26 19:31:46]

Change committed as 285785