Throttle SelectRange IPC

Throttle SelectRange IPC

This code is used by the Android port. We add a SelectRange_ACK to
throttle the rate at which the selection changes. Without throttling,
the selection handle updates can fall behind the user's touch input.

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

[Iain Merrick, 2012-07-30 12:56:26]

Adding olilan who worked on this code for the Android port, and sky for
content/browser/OWNERS review.

[sky, 2012-08-01 02:37:32]

LGTM - but I'm adding Darin to make sure he can't think of any reasons not to do
this. Wait for Darin to respond before landing.

[Iain Merrick, 2012-08-01 09:07:32]

On 2012/08/01 02:37:32, sky wrote:
> LGTM - but I'm adding Darin to make sure he can't think of any reasons not to
do
> this. Wait for Darin to respond before landing.

Thanks! Will do.

[darin (slow to review), 2012-08-02 06:30:20]

http://codereview.chromium.org/10823077/diff/1/content/browser/renderer_host/...
File content/browser/renderer_host/render_widget_host_impl.h (right):

http://codereview.chromium.org/10823077/diff/1/content/browser/renderer_host/...
content/browser/renderer_host/render_widget_host_impl.h:555: void
OnMsgSelectRangeAck();
nit: it seems like this should be listed after OnMsgBeginSmoothScroll since you
place it after that function in the .cc file.

http://codereview.chromium.org/10823077/diff/1/content/browser/renderer_host/...
content/browser/renderer_host/render_widget_host_impl.h:693: gfx::Point
pend_select_start_;
nit: please use words.  pend -> pending?

http://codereview.chromium.org/10823077/diff/1/content/renderer/render_view_i...
File content/renderer/render_view_impl.cc (right):

http://codereview.chromium.org/10823077/diff/1/content/renderer/render_view_i...
content/renderer/render_view_impl.cc:1275: Send(new
ViewHostMsg_SelectRange_ACK(routing_id_));
nit: I suggest sending the ACK before calling selectRange(). That way you allow
the next SelectRange IPC to overlap slightly with the processing of the current
SelectRange IPC.  In other cases, such an approach has been useful.

[Iain Merrick, 2012-08-02 10:25:42]

http://codereview.chromium.org/10823077/diff/1/content/browser/renderer_host/...
File content/browser/renderer_host/render_widget_host_impl.h (right):

http://codereview.chromium.org/10823077/diff/1/content/browser/renderer_host/...
content/browser/renderer_host/render_widget_host_impl.h:555: void
OnMsgSelectRangeAck();
On 2012/08/02 06:30:20, darin wrote:
> nit: it seems like this should be listed after OnMsgBeginSmoothScroll since
you
> place it after that function in the .cc file.

Done.

http://codereview.chromium.org/10823077/diff/1/content/browser/renderer_host/...
content/browser/renderer_host/render_widget_host_impl.h:693: gfx::Point
pend_select_start_;
On 2012/08/02 06:30:20, darin wrote:
> nit: please use words.  pend -> pending?

Done. In fact I'll rename things a bit because using 'pending' in two different
senses seems very confusing.

http://codereview.chromium.org/10823077/diff/1/content/renderer/render_view_i...
File content/renderer/render_view_impl.cc (right):

http://codereview.chromium.org/10823077/diff/1/content/renderer/render_view_i...
content/renderer/render_view_impl.cc:1275: Send(new
ViewHostMsg_SelectRange_ACK(routing_id_));
On 2012/08/02 06:30:20, darin wrote:
> nit: I suggest sending the ACK before calling selectRange(). That way you
allow
> the next SelectRange IPC to overlap slightly with the processing of the
current
> SelectRange IPC.  In other cases, such an approach has been useful.

Done.

[darin (slow to review), 2012-08-02 16:37:17]

http://codereview.chromium.org/10823077/diff/10001/content/browser/renderer_h...
File content/browser/renderer_host/render_widget_host_impl.cc (right):

http://codereview.chromium.org/10823077/diff/10001/content/browser/renderer_h...
content/browser/renderer_host/render_widget_host_impl.cc:1521: gfx::Point start
= next_select_range_->first;
consider using .Pass() here instead:

  scoped_ptr<SelectionRange> next(next_selection_range_.Pass());
  SelectRange(next->start, next->end);

http://codereview.chromium.org/10823077/diff/10001/content/browser/renderer_h...
File content/browser/renderer_host/render_widget_host_impl.h (right):

http://codereview.chromium.org/10823077/diff/10001/content/browser/renderer_h...
content/browser/renderer_host/render_widget_host_impl.h:695:
scoped_ptr<std::pair<gfx::Point, gfx::Point> > next_select_range_;
Please just define a struct instead of using std::pair.  The usage of 'first'
and 'second' in code is rarely readable.

How about this:
  struct SelectionRange {
    gfx::Point start, end;
  };
  scoped_ptr<SelectionRange> next_selection_range_;

^^^ reads better to say that "SelectRange sends a selection range," which is why
I named the type SelectionRange instead of SelectRange.

[Iain Merrick, 2012-08-06 11:28:45]

http://codereview.chromium.org/10823077/diff/10001/content/browser/renderer_h...
File content/browser/renderer_host/render_widget_host_impl.cc (right):

http://codereview.chromium.org/10823077/diff/10001/content/browser/renderer_h...
content/browser/renderer_host/render_widget_host_impl.cc:1521: gfx::Point start
= next_select_range_->first;
On 2012/08/02 16:37:17, darin wrote:
> consider using .Pass() here instead:
> 
>   scoped_ptr<SelectionRange> next(next_selection_range_.Pass());
>   SelectRange(next->start, next->end);

Done.

http://codereview.chromium.org/10823077/diff/10001/content/browser/renderer_h...
File content/browser/renderer_host/render_widget_host_impl.h (right):

http://codereview.chromium.org/10823077/diff/10001/content/browser/renderer_h...
content/browser/renderer_host/render_widget_host_impl.h:695:
scoped_ptr<std::pair<gfx::Point, gfx::Point> > next_select_range_;
On 2012/08/02 16:37:17, darin wrote:
> Please just define a struct instead of using std::pair.  The usage of 'first'
> and 'second' in code is rarely readable.
> 
> How about this:
>   struct SelectionRange {
>     gfx::Point start, end;
>   };
>   scoped_ptr<SelectionRange> next_selection_range_;
> 
> ^^^ reads better to say that "SelectRange sends a selection range," which is
why
> I named the type SelectionRange instead of SelectRange.

Done.

[darin (slow to review), 2012-08-06 18:27:40]

http://codereview.chromium.org/10823077/diff/16002/content/browser/renderer_h...
File content/browser/renderer_host/render_widget_host_impl.cc (right):

http://codereview.chromium.org/10823077/diff/16002/content/browser/renderer_h...
content/browser/renderer_host/render_widget_host_impl.cc:1829: if
(select_range_pending_) {
by the way, if the renderer crashes, you probably want to reset these member
variables.  see RenderWidgetHostImpl::RendererExited

http://codereview.chromium.org/10823077/diff/16002/content/browser/renderer_h...
File content/browser/renderer_host/render_widget_host_impl.h (right):

http://codereview.chromium.org/10823077/diff/16002/content/browser/renderer_h...
content/browser/renderer_host/render_widget_host_impl.h:10: #include <utility>
nit: not needed

http://codereview.chromium.org/10823077/diff/16002/content/browser/renderer_h...
content/browser/renderer_host/render_widget_host_impl.h:698:
scoped_ptr<SelectionRange> next_select_range_;
nit: next_selection_range_  <--  type is "SelectionRange" so variable name
should be consistent.

[Iain Merrick, 2012-08-13 13:17:23]

Sorry for the delay, I didn't see the email about the last batch of comments.

http://codereview.chromium.org/10823077/diff/16002/content/browser/renderer_h...
File content/browser/renderer_host/render_widget_host_impl.cc (right):

http://codereview.chromium.org/10823077/diff/16002/content/browser/renderer_h...
content/browser/renderer_host/render_widget_host_impl.cc:1829: if
(select_range_pending_) {
On 2012/08/06 18:27:41, darin wrote:
> by the way, if the renderer crashes, you probably want to reset these member
> variables.  see RenderWidgetHostImpl::RendererExited

Done.

http://codereview.chromium.org/10823077/diff/16002/content/browser/renderer_h...
File content/browser/renderer_host/render_widget_host_impl.h (right):

http://codereview.chromium.org/10823077/diff/16002/content/browser/renderer_h...
content/browser/renderer_host/render_widget_host_impl.h:10: #include <utility>
On 2012/08/06 18:27:41, darin wrote:
> nit: not needed

Done.

http://codereview.chromium.org/10823077/diff/16002/content/browser/renderer_h...
content/browser/renderer_host/render_widget_host_impl.h:698:
scoped_ptr<SelectionRange> next_select_range_;
On 2012/08/06 18:27:41, darin wrote:
> nit: next_selection_range_  <--  type is "SelectionRange" so variable name
> should be consistent.

Done.

[darin (slow to review), 2012-08-13 16:25:57]

LGTM

[commit-bot: I haz the power, 2012-08-13 16:27:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/husky@chromium.org/10823077/9

[commit-bot: I haz the power, 2012-08-13 17:56:18]

Change committed as 151304