touchui: Fine-tune selection-controller visibility.

touchui: Fine-tune selection-controller visibility.

This makes two changes:
 + Makes sure the selection bound has a minimum size before the controller is
   made visible.
 + Delays updating the touch-selection controller a little bit to account for the
   erroneous selection-change messages from webkit.

BUG=none
TEST=manually

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=99894

[sky, 2011-08-31 14:16:28]

> erroneous selection-change messages

Is this is a bug in WebKit we should be fixing?

  -Scott

[varunjain, 2011-08-31 16:52:21]

On 2011/08/31 14:16:28, sky wrote:
> > erroneous selection-change messages
> 
> Is this is a bug in WebKit we should be fixing?
> 
>   -Scott

webkit sends a selection of size 1 pixel "sometimes". I am not sure if this is a
bug or webkit is sending the cursor size. However, if we want to fix this in
chrome, I think it should be done in RenderView::SyncSelection. There should be
no IPC sent if the selection size is below a threshold. That way, the selection
controller will not need to be delayed.
Also, I think the threshold of 4 is too big probably.

[sadrul, 2011-08-31 17:04:11]

On 2011/08/31 16:52:21, varunjain wrote:
> On 2011/08/31 14:16:28, sky wrote:
> > > erroneous selection-change messages
> > 
> > Is this is a bug in WebKit we should be fixing?
> > 
> >   -Scott
> 
> webkit sends a selection of size 1 pixel "sometimes".

Indeed it does. This particular workaround (the delayed update), however, is for
a different error: enter some text into a textfield, press Home to move to the
beginning of the textfield, press another key (e.g. 'w'). The last keypress will
cause webkit to send a selection of 1 character (with the appropriate width,
e.g. ~6px), immediately followed by a selection of 0 characters. The related
webkit code is twisty enough that I do not know if it is [easily] solvable on
the webkit end.

> I am not sure if this is a
> bug or webkit is sending the cursor size. However, if we want to fix this in
> chrome, I think it should be done in RenderView::SyncSelection. There should
be
> no IPC sent if the selection size is below a threshold. That way, the
selection
> controller will not need to be delayed.

I think the '1px' discrepancy is caused by errors accumulated from
floating-point operations. RenderView can adjust the range in such cases, or
RWHVV can decide whether the range makes sense or not. Either way sounds OK to
me. The minimum selection-size is a fix for this issue. The delayed-task in this
CL is not related to this problem.

> Also, I think the threshold of 4 is too big probably.

Perhaps. But selecting something smaller than 4px with touch sounds like a
reasonably difficult enough task that it's probably an OK threshold.

[sadrul, 2011-09-02 04:30:15]

On 2011/08/31 17:04:11, sadrul wrote:
> On 2011/08/31 16:52:21, varunjain wrote:
> > On 2011/08/31 14:16:28, sky wrote:
> > > > erroneous selection-change messages
> > > 
> > > Is this is a bug in WebKit we should be fixing?
> > > 
> > >   -Scott
> > 
> > webkit sends a selection of size 1 pixel "sometimes".
> 
> Indeed it does. This particular workaround (the delayed update), however, is
for
> a different error: enter some text into a textfield, press Home to move to the
> beginning of the textfield, press another key (e.g. 'w'). The last keypress
will
> cause webkit to send a selection of 1 character (with the appropriate width,
> e.g. ~6px), immediately followed by a selection of 0 characters. The related
> webkit code is twisty enough that I do not know if it is [easily] solvable on
> the webkit end.

I have filed https://bugs.webkit.org/show_bug.cgi?id=67464 to keep track of this
bug.

> > I am not sure if this is a
> > bug or webkit is sending the cursor size. However, if we want to fix this in
> > chrome, I think it should be done in RenderView::SyncSelection. There should
> be
> > no IPC sent if the selection size is below a threshold. That way, the
> selection
> > controller will not need to be delayed.
> 
> I think the '1px' discrepancy is caused by errors accumulated from
> floating-point operations. RenderView can adjust the range in such cases, or
> RWHVV can decide whether the range makes sense or not. Either way sounds OK to
> me. The minimum selection-size is a fix for this issue. The delayed-task in
this
> CL is not related to this problem.
> 
> > Also, I think the threshold of 4 is too big probably.
> 
> Perhaps. But selecting something smaller than 4px with touch sounds like a
> reasonably difficult enough task that it's probably an OK threshold.

[rjkroege, 2011-09-02 19:15:25]

This seems like a reasonable work around for until webkit bug
https://bugs.webkit.org/show_bug.cgi?id=67464 gets fixed (several weeks at a
minimum I would think.)

But it's worth adding a fixme about it?

http://codereview.chromium.org/7778039/diff/1/chrome/browser/renderer_host/re...
File chrome/browser/renderer_host/render_widget_host_view_views.cc (right):

http://codereview.chromium.org/7778039/diff/1/chrome/browser/renderer_host/re...
chrome/browser/renderer_host/render_widget_host_view_views.cc:343: // Webkit can
send spurious selection-change on text-input (e.g. when
So per our discussion, this is a temporary fix pending a webkit fix right? So
maybe add a fixme?

http://codereview.chromium.org/7778039/diff/1/chrome/browser/renderer_host/re...
File chrome/browser/renderer_host/render_widget_host_view_views.h (right):

http://codereview.chromium.org/7778039/diff/1/chrome/browser/renderer_host/re...
chrome/browser/renderer_host/render_widget_host_view_views.h:208: void
UpdateTouchSelectionController();
how about a doc string?

[sadrul, 2011-09-02 19:39:44]

http://codereview.chromium.org/7778039/diff/1/chrome/browser/renderer_host/re...
File chrome/browser/renderer_host/render_widget_host_view_views.cc (right):

http://codereview.chromium.org/7778039/diff/1/chrome/browser/renderer_host/re...
chrome/browser/renderer_host/render_widget_host_view_views.cc:343: // Webkit can
send spurious selection-change on text-input (e.g. when
On 2011/09/02 19:15:25, rjkroege wrote:
> So per our discussion, this is a temporary fix pending a webkit fix right? So
> maybe add a fixme?

Yep. Done

http://codereview.chromium.org/7778039/diff/1/chrome/browser/renderer_host/re...
File chrome/browser/renderer_host/render_widget_host_view_views.h (right):

http://codereview.chromium.org/7778039/diff/1/chrome/browser/renderer_host/re...
chrome/browser/renderer_host/render_widget_host_view_views.h:208: void
UpdateTouchSelectionController();
On 2011/09/02 19:15:25, rjkroege wrote:
> how about a doc string?

Done.

[sadrul, 2011-09-06 17:34:07]

Friendly ping!

(I can split the fix into two CLs for the two bugs if that'd be better)

[sky, 2011-09-06 17:43:27]

http://codereview.chromium.org/7778039/diff/6004/chrome/browser/renderer_host...
File chrome/browser/renderer_host/render_widget_host_view_views.cc (right):

http://codereview.chromium.org/7778039/diff/6004/chrome/browser/renderer_host...
chrome/browser/renderer_host/render_widget_host_view_views.cc:359:
update_touch_selection_.NewRunnableMethod(
Make sure we don't send this if we've lost focus between when the selection
changes and UpdateTouchSelectionController fires.

http://codereview.chromium.org/7778039/diff/6004/views/touchui/touch_selectio...
File views/touchui/touch_selection_controller_impl.cc (right):

http://codereview.chromium.org/7778039/diff/6004/views/touchui/touch_selectio...
views/touchui/touch_selection_controller_impl.cc:281: UpdateContextMenu(p1, p2);
Should this be moved to 194ish and around line 290 get a HideContextMenu?

[sadrul, 2011-09-06 20:23:31]

http://codereview.chromium.org/7778039/diff/6004/chrome/browser/renderer_host...
File chrome/browser/renderer_host/render_widget_host_view_views.cc (right):

http://codereview.chromium.org/7778039/diff/6004/chrome/browser/renderer_host...
chrome/browser/renderer_host/render_widget_host_view_views.cc:359:
update_touch_selection_.NewRunnableMethod(
On 2011/09/06 17:43:27, sky wrote:
> Make sure we don't send this if we've lost focus between when the selection
> changes and UpdateTouchSelectionController fires.

Good point. Done.

http://codereview.chromium.org/7778039/diff/6004/views/touchui/touch_selectio...
File views/touchui/touch_selection_controller_impl.cc (right):

http://codereview.chromium.org/7778039/diff/6004/views/touchui/touch_selectio...
views/touchui/touch_selection_controller_impl.cc:281: UpdateContextMenu(p1, p2);
On 2011/09/06 17:43:27, sky wrote:
> Should this be moved to 194ish and around line 290 get a HideContextMenu?

I do not think I understand: do you mean do the same check for p1,p2 in
UpdateContextMenu (done)?

[sky, 2011-09-06 20:33:36]

http://codereview.chromium.org/7778039/diff/6004/views/touchui/touch_selectio...
File views/touchui/touch_selection_controller_impl.cc (right):

http://codereview.chromium.org/7778039/diff/6004/views/touchui/touch_selectio...
views/touchui/touch_selection_controller_impl.cc:281: UpdateContextMenu(p1, p2);
On 2011/09/06 20:23:31, sadrul wrote:
> On 2011/09/06 17:43:27, sky wrote:
> > Should this be moved to 194ish and around line 290 get a HideContextMenu?
> 
> I do not think I understand: do you mean do the same check for p1,p2 in
> UpdateContextMenu (done)?

I'm suggesting:

if (dragging_handle_) {
  ...
} else {
  int delta_x ...
  if (abs(delta...)) {
    HideConextMenu();
    ...
    return;
  }
  UpdateContextMenu(p1, p2);
  ...

[sadrul, 2011-09-06 20:36:51]

http://codereview.chromium.org/7778039/diff/6004/views/touchui/touch_selectio...
File views/touchui/touch_selection_controller_impl.cc (right):

http://codereview.chromium.org/7778039/diff/6004/views/touchui/touch_selectio...
views/touchui/touch_selection_controller_impl.cc:281: UpdateContextMenu(p1, p2);
On 2011/09/06 20:33:36, sky wrote:
> On 2011/09/06 20:23:31, sadrul wrote:
> > On 2011/09/06 17:43:27, sky wrote:
> > > Should this be moved to 194ish and around line 290 get a HideContextMenu?
> > 
> > I do not think I understand: do you mean do the same check for p1,p2 in
> > UpdateContextMenu (done)?
> 
> I'm suggesting:
> 
> if (dragging_handle_) {
>   ...
> } else {
>   int delta_x ...
>   if (abs(delta...)) {
>     HideConextMenu();
>     ...
>     return;
>   }
>   UpdateContextMenu(p1, p2);
>   ...

Ah, I see. It looks like UpdateContextMenu already does a HideContextMenu, so
this probably doesn't need to change? (Or should UpdateContextMenu not do the
hiding?)

[sky, 2011-09-06 20:45:38]

On 2011/09/06 20:36:51, sadrul wrote:
>
http://codereview.chromium.org/7778039/diff/6004/views/touchui/touch_selectio...
> File views/touchui/touch_selection_controller_impl.cc (right):
> 
>
http://codereview.chromium.org/7778039/diff/6004/views/touchui/touch_selectio...
> views/touchui/touch_selection_controller_impl.cc:281: UpdateContextMenu(p1,
p2);
> On 2011/09/06 20:33:36, sky wrote:
> > On 2011/09/06 20:23:31, sadrul wrote:
> > > On 2011/09/06 17:43:27, sky wrote:
> > > > Should this be moved to 194ish and around line 290 get a
HideContextMenu?
> > > 
> > > I do not think I understand: do you mean do the same check for p1,p2 in
> > > UpdateContextMenu (done)?
> > 
> > I'm suggesting:
> > 
> > if (dragging_handle_) {
> >   ...
> > } else {
> >   int delta_x ...
> >   if (abs(delta...)) {
> >     HideConextMenu();
> >     ...
> >     return;
> >   }
> >   UpdateContextMenu(p1, p2);
> >   ...
> 
> Ah, I see. It looks like UpdateContextMenu already does a HideContextMenu, so
> this probably doesn't need to change? (Or should UpdateContextMenu not do the
> hiding?)

It seems like you could return early while UpdateContextMenu might restart the
timer. Don't you want to timer stopped if you return early?

  -Scott

[sadrul, 2011-09-06 20:50:41]

On 2011/09/06 20:45:38, sky wrote:
> On 2011/09/06 20:36:51, sadrul wrote:
> >
>
http://codereview.chromium.org/7778039/diff/6004/views/touchui/touch_selectio...
> > File views/touchui/touch_selection_controller_impl.cc (right):
> > 
> >
>
http://codereview.chromium.org/7778039/diff/6004/views/touchui/touch_selectio...
> > views/touchui/touch_selection_controller_impl.cc:281: UpdateContextMenu(p1,
> p2);
> > On 2011/09/06 20:33:36, sky wrote:
> > > On 2011/09/06 20:23:31, sadrul wrote:
> > > > On 2011/09/06 17:43:27, sky wrote:
> > > > > Should this be moved to 194ish and around line 290 get a
> HideContextMenu?
> > > > 
> > > > I do not think I understand: do you mean do the same check for p1,p2 in
> > > > UpdateContextMenu (done)?
> > > 
> > > I'm suggesting:
> > > 
> > > if (dragging_handle_) {
> > >   ...
> > > } else {
> > >   int delta_x ...
> > >   if (abs(delta...)) {
> > >     HideConextMenu();
> > >     ...
> > >     return;
> > >   }
> > >   UpdateContextMenu(p1, p2);
> > >   ...
> > 
> > Ah, I see. It looks like UpdateContextMenu already does a HideContextMenu,
so
> > this probably doesn't need to change? (Or should UpdateContextMenu not do
the
> > hiding?)
> 
> It seems like you could return early while UpdateContextMenu might restart the
> timer. Don't you want to timer stopped if you return early?

With the current change, the timer will stop because of the call to
HideContextMenu from UpdateContextMenu, and will not restart since
IsEmptySelection in UpdateContextMenu will return true for the early-return
case.

> 
>   -Scott

[sky, 2011-09-06 21:17:29]

LGTM