Splits SmoothGestureController from RenderWidgetHostImpl

Splits SmoothGestureController from RenderWidgetHostImpl

Elements were pushed when the platform sends an input event,
but they're are popped when the event are ACKd.
There are other internal queues such as "touch_event_queue_" and
"gesture_filter_" that may coalesce / ACK events using their own internal
order.
This results in in_process_event_types_ not being popped correctly, and
prevents Telemetry from synthesizing Motion events on android.
The solution is to move the synthetic events out of RenderWidgetHostImpl and use a simple counter there.

BUG=166521
TEST=content_unittest, SmoothScrollGestureControllerTest

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=192041

[bulach, 2013-01-11 10:46:39]

varunjain / sky: the bits I'm changing were added at:
https://chromiumcodereview.appspot.com/10917102/diff/6055/content/browser/ren...

for a lot more details, please refer to:
http://code.google.com/p/chromium/issues/detail?id=166521
and
https://codereview.chromium.org/11793003/

nduca / skyostil: just so you know when to to re-read the patch above..

funny note: yay! message sent to sky && skyostil, and not by accident! :)

[Sami, 2013-01-11 11:12:40]

Looks good to me, but I'll defer to the folks who know more about this area.

[sky, 2013-01-11 17:01:27]

I'm not a good reviewer for this code. Maybe Darin.

[bulach, 2013-01-15 12:30:32]

darin, would you mind taking a look please?

[darin (slow to review), 2013-01-15 19:19:53]

It is quite concerning to me that input events are not ACK'd in the order they
are generated.  That seems troublesome.  What causes this?  @jamesr also shares
my concern.  What's going on here?

https://codereview.chromium.org/11858007/diff/2001/content/browser/renderer_h...
File content/browser/renderer_host/render_widget_host_impl.cc (right):

https://codereview.chromium.org/11858007/diff/2001/content/browser/renderer_h...
content/browser/renderer_host/render_widget_host_impl.cc:1691: void
RenderWidgetHostImpl::OnInputEventAck(
nit: This function is now too long.  You need to break it up.  Maybe your chunk
of new code could be a helper function instead.

[jamesr, 2013-01-15 19:26:10]

https://codereview.chromium.org/11858007/diff/2001/content/browser/renderer_h...
File content/browser/renderer_host/render_widget_host_impl.cc (right):

https://codereview.chromium.org/11858007/diff/2001/content/browser/renderer_h...
content/browser/renderer_host/render_widget_host_impl.cc:1709: // that may ACK
in their own internal order. When traversing
what does this mean?  The render process should ACK input events in the order it
receives them in - why would other browser-process queues matter?

[bulach, 2013-01-15 19:35:42]

thanks darin! my bad, I should've explained in more details here besides the
discussion that happened in the linked bug..

let me try to summarize our current understanding.
- we push events to this in_process_event_types_ as they are created by the
platform, so let's suppose the platform sends "T1", "G1", "T2" (T=touch,
G=gesture).

- then, when we're ACKing T1, there's a separate "touch_event_queue_" that may
also pop T2.. however, "in_process_event_types_" has now G1 on top, and won't
pop that T2...

- this will ultimately prevent synthesized events by Telemetry from being
"ticked" (it only happens when in_process_event_types_ is empty), which prevents
the scroll benchmark from finishing.

it seems that in_process_event_types_ was added before touch_event_queue_ /
gesture_event_filter_, and it's now used in a slightly different semantic that
this patch now tries to fix.

from an interactive point of view (i.e., outside telemetry / synthesized harness
events), in_process_event_types_ doesn't affect anything, but it does get out of
sync (new event streams tend to pop queued events, but that's obviously not
guaranteed)...

I hope that helps, but please let me know if I could clarify this further..

[jamesr, 2013-01-16 04:21:26]

I don't understand why we have multiple queues of even types at all in the
browser process.  All communication with the renderer has to be strictly
in-order.  It looks like sadrul@ added the touch_event_queue_ - what is going on
with it?

[jamesr, 2013-01-16 04:28:17]

There's a key_event_ queue on RenderWidgetHostImpl because we sent multiple key
events without waiting for ACKs, but AFAIK that's only true for keyboard events
since otherwise we run into interleaving bugs due to the fact that a single
logical keypress maps to a sequence of key events (keydown, keypressed, etc
etc).  This isn't true for any other event type that I know of -
touches/gestures are pretty standalone - and even for key events the current
mechanism is not perfect.  What are these other queues doing?

[sadrul, 2013-01-16 05:40:12]

On 2013/01/16 04:28:17, jamesr wrote:
> There's a key_event_ queue on RenderWidgetHostImpl because we sent multiple
key
> events without waiting for ACKs, but AFAIK that's only true for keyboard
events
> since otherwise we run into interleaving bugs due to the fact that a single
> logical keypress maps to a sequence of key events (keydown, keypressed, etc
> etc).  This isn't true for any other event type that I know of -
> touches/gestures are pretty standalone - and even for key events the current
> mechanism is not perfect.  What are these other queues doing?

The touch-event-queue is essentially used to coalesce touch-move events when
possible (much like mouse-wheel events).

Similarly, gesture-event-filter is used to coalesce scroll-update events. It
does some additional filtering too (e.g. apply some heuristic to detect a bounce
and drop events appropriately).

The events either touch-event-queue/gesture-event-filter coalesces/drops (i.e.
the events that are not dispatched to the renderer) do not get added to the
|in_process_event_types_| queue.

I am not sure exactly what bug this patch is trying to fix, but I wonder if the
compositor-thread hit-testing done for touch-events has anything to do with
this. I have commented on the bug about this
(http://code.google.com/p/chromium/issues/detail?id=166521#c14).

[bulach, 2013-01-16 09:27:37]

thanks james, sadrul!

the bug I'm trying to solve here, from 10000m :)
- when in_process_event_types_ isn't empty, the "synthesized events" for
Telemetry (our perf test harness) don't fully complete. that leads to benchmarks
never finishing... note though that even interactively (i.e., the full browser
manually outside any harness), this in_process_event_types_ gets out of sync..
it doesn't have any side effect in that case though, besides potentially
growing.

- the reason in_process_event_types_ isn't empty is due to these other internal
queues that may unroll their internal events...


- in_process_event_types_ was added here, for a very different reason:
https://chromiumcodereview.appspot.com/10917102/diff/6055/content/browser/ren...

- the other queues were added afterwards, and I guess since there were no
visible side effects, we never hit this case before...

I hope that helps, but maybe varunjain can provide more context on this?

[sadrul, 2013-01-17 01:21:14]

On 2013/01/16 09:27:37, bulach wrote:
> thanks james, sadrul!
> 
> the bug I'm trying to solve here, from 10000m :)
> - when in_process_event_types_ isn't empty, the "synthesized events" for
> Telemetry (our perf test harness) don't fully complete. that leads to
benchmarks
> never finishing... note though that even interactively (i.e., the full browser
> manually outside any harness), this in_process_event_types_ gets out of sync..
> it doesn't have any side effect in that case though, besides potentially
> growing.
> 
> - the reason in_process_event_types_ isn't empty is due to these other
internal
> queues that may unroll their internal events...
> 
> 
> - in_process_event_types_ was added here, for a very different reason:
>
https://chromiumcodereview.appspot.com/10917102/diff/6055/content/browser/ren...
> 
> - the other queues were added afterwards, and I guess since there were no
> visible side effects, we never hit this case before...
> 
> I hope that helps, but maybe varunjain can provide more context on this?

I did some more digging, and it looks like RenderWidget can throttle ACKs for
input events in some cases (e.g. for mouse-move/wheel events), and that is what
causes the browser to receive out-of-order ACKs.

For example, I could easily make in_process_event_types_ receive ACKs in
incorrect order by using the wheelscroll on an external mouse-devices to
generate a lot of wheel events while simultaneously moving the trackpad mouse on
chromeos (i.e. not using touch/gesture events at all).

[bulach, 2013-01-17 09:34:17]

thanks for the help sadrul!

so do you think this fix is a valid attempt or should I look into some other
direction?

I'm not too familiar with this code, I'm coming from the "telemetry" side where
we need to synthesize events to simulate scroll for our benchmarks:
https://codereview.chromium.org/11793003/

[sadrul, 2013-01-17 17:31:40]

On 2013/01/17 09:34:17, bulach wrote:
> thanks for the help sadrul!
> 
> so do you think this fix is a valid attempt or should I look into some other
> direction?

I think this is reasonable. Possible alternate solutions:

 * Looks like telemetry doesn't really care about the event-type. So it could
simply use an event-counter that gets increased for each sent event and is
decreased for each ACKed event (instead of depending on
|in_process_event_types_|).
 * Move the current logic in RenderViewHostImpl that tries to decide whether a
context menu was generated from a gesture event, mouse event or keyboard event
into RenderViewImpl in the renderer side.

[bulach, 2013-01-17 18:31:21]

thanks sadrul! I don't know all the subtleties for moving the context menu
detection, but the counter is a good idea! would you be ok if I implement that
for telemetry?

[bulach, 2013-01-18 11:37:09]

sadrul / nduca: how about moving all these telemetry-related "smooth scroll
gesture" into its own class?

I guess it'd be simpler to control things in a smaller context... let me draft
this up and will upload the patch soon.

[bulach, 2013-01-18 19:48:56]

hey sadrul / nduca: apologies for the biggie and for a complete change on
direction...

I hope this will look better :)
so rather than keep messing up deep down in RenderWidgetHostViewImpl, I split
the necessary bits into a SmoothScroolGestureController so that it can be tested
in isolation and also disentangles the dependencies with the other queues...

please let me know your thoughts on this approach!

thanks

[bulach, 2013-02-04 10:23:46]

sorry to ping again, but any chance someone could look into this?

[rjkroege, 2013-02-11 22:47:27]

Sorry for the delay. I think we are observing an instance of
http://en.wikipedia.org/wiki/Diffusion_of_responsibility in the review for this
patch. :-)

https://codereview.chromium.org/11858007/diff/24001/content/browser/renderer_...
File content/browser/renderer_host/render_widget_host_impl.cc (right):

https://codereview.chromium.org/11858007/diff/24001/content/browser/renderer_...
content/browser/renderer_host/render_widget_host_impl.cc:281: int
RenderWidgetHostImpl::SyntheticScrollMessageInterval() const {
who accesses this? perhaps they could speak to the SSGC directly?

https://codereview.chromium.org/11858007/diff/24001/content/browser/renderer_...
File content/browser/renderer_host/render_widget_host_impl.h (right):

https://codereview.chromium.org/11858007/diff/24001/content/browser/renderer_...
content/browser/renderer_host/render_widget_host_impl.h:861:
scoped_ptr<SmoothScrollGestureController> smooth_scroll_gesture_controller_;
The SmoothScrollGestureController is a lot like an implementation of fling in
the browser. (Perhaps fling should move from renderer to browser someday?)

https://codereview.chromium.org/11858007/diff/24001/content/browser/renderer_...
File content/browser/renderer_host/smooth_scroll_gesture_controller.cc (right):

https://codereview.chromium.org/11858007/diff/24001/content/browser/renderer_...
content/browser/renderer_host/smooth_scroll_gesture_controller.cc:37:
view->CreateSmoothScrollGesture(
I see that this makes wheel events. Note that wheel scrolling and touch
scrolling are (correctly) diverging and (particularly in main-thread-mediated
scrolling case) have become quite different.

https://codereview.chromium.org/11858007/diff/24001/content/browser/renderer_...
content/browser/renderer_host/smooth_scroll_gesture_controller.cc:55:
--pending_input_event_count_;
should you verify that this has not gone negative?

https://codereview.chromium.org/11858007/diff/24001/content/browser/renderer_...
content/browser/renderer_host/smooth_scroll_gesture_controller.cc:100:
std::vector<int> ids_that_are_done;
possibly dumb question: why do you want to support more than one at a time?

https://codereview.chromium.org/11858007/diff/24001/content/browser/renderer_...
content/browser/renderer_host/smooth_scroll_gesture_controller.cc:128:
tick_active_smooth_scroll_gestures_task_posted_ = true;
it would be nicer to not replicate this chunk of code?

[bulach, 2013-02-12 16:38:49]

yay, thanks Robert! :) seeing you as an OWNER, would you be willing if I use you
to focus (un-diffuse?) responsibilities here? :)

so before I dig into the actual comments, let me try an overview once again, I
hope we'll agree on the high level architecture and then we can drill down to
the implementation... bear with me, but feel free to skip things you already
know! ;)

Telemetry is a cross-chrome-platform harness we're building, mostly focused on
performance, so that we'll be able to write a single perf test that would run
consistently on chrome across all platforms.
(broad brush, but this where this patch is).

Telemetry "speaks" DevTools Remote Debugging, and adds some niceties, such as an
API to inject "synthetic" events (right now, named "SmoothScrollGesture")...

on android, instead of "mouse wheel" events at the browser layer, we'd really
like to synthesize platforms events higher up in our Java layer for "touch down,
move, up"..

my original patch (see long, long discussion in the attached bug) got reverted
because those events were not being "ticked", that is, we were not sending the
full event stream as required to scroll the page under test.

the core of the issue is that "in_process_event_types_" gets conflated with
different "touch" and "gesture", and then checking for it being empty is not the
right condition for these synthetic events.. in fact, "in_process_event_types_"
gets out of sync (see sadrul on #12 above).

the latest patch tries to decouple all of these complexity, in hopefully a
better architecture with improved tests.

the smooth_scroll_gesture_controller.h would then only kick in for such
"synthetic" events (we're planning on adding synthetic "pinch" too)....


so... I hope this shines a little bit of light on what this is all about. if so,
please do let me know if you're happy with this split, and if I should go ahead
and address your comments, or if you rather have some completely different
approach altogether :)

[rjkroege, 2013-02-13 18:35:50]

On 2013/02/12 16:38:49, bulach wrote:
> yay, thanks Robert! :) seeing you as an OWNER, would you be willing if I use
you
> to focus (un-diffuse?) responsibilities here? :)
> 

I'll do my best. If your change extends beyond touch/gestures, we should bring
in someone else to look it over.

> 
> so before I dig into the actual comments, let me try an overview once again, I
> hope we'll agree on the high level architecture and then we can drill down to
> the implementation... bear with me, but feel free to skip things you already
> know! ;)

Thanks for the details. I'd like to think that I've heard some of them before.
:-)

> 
> Telemetry is a cross-chrome-platform harness we're building, mostly focused on
> performance, so that we'll be able to write a single perf test that would run
> consistently on chrome across all platforms.
> (broad brush, but this where this patch is).
> 
> Telemetry "speaks" DevTools Remote Debugging, and adds some niceties, such as
an
> API to inject "synthetic" events (right now, named "SmoothScrollGesture")...
> 
> on android, instead of "mouse wheel" events at the browser layer, we'd really
> like to synthesize platforms events higher up in our Java layer for "touch
down,
> move, up"..

Do you mean to do this with touch events or gesture events? I think both are
interesting and useful but note that the queueing is going to get subtle. Let me
know how I can help make this happen on CrOS/Aura.

> 
> my original patch (see long, long discussion in the attached bug) got reverted
> because those events were not being "ticked", that is, we were not sending the
> full event stream as required to scroll the page under test.
> 
> the core of the issue is that "in_process_event_types_" gets conflated with
> different "touch" and "gesture", and then checking for it being empty is not
the
> right condition for these synthetic events.. in fact,
"in_process_event_types_"
> gets out of sync (see sadrul on #12 above).
> 
> the latest patch tries to decouple all of these complexity, in hopefully a
> better architecture with improved tests.

I think I get the gist of this code and the general approach seems reasonable at
least to me except for the following:

*  I don't understand how you know if the scroll has actually scrolled anything?
You send wheels that add up to pixels_scrolled_ pixels but I can't see how the
code figures out how much the page has actually been scrolled.

*  Is there recording of how many events reach the renderer vs how many your
generate? (i.e. how coarse the scroll is?) It seemed that this could be wired
into pending_input_event_count_

Also: (dumb maybe) why do you want to support multiple SmoothScrollGestures at
once?

I'm really glad to see this code moved out of render_widget_host_impl. RWHi
seems like a class needing to be divided up.


> 
> the smooth_scroll_gesture_controller.h would then only kick in for such
> "synthetic" events (we're planning on adding synthetic "pinch" too)....
> 
> 
> so... I hope this shines a little bit of light on what this is all about. if
so,
> please do let me know if you're happy with this split, and if I should go
ahead
> and address your comments, or if you rather have some completely different
> approach altogether :)

[bulach, 2013-02-15 11:18:40]

thanks! inline:


On Wed, Feb 13, 2013 at 6:35 PM, <rjkroege@chromium.org> wrote:

> On 2013/02/12 16:38:49, bulach wrote:
>
>> yay, thanks Robert! :) seeing you as an OWNER, would you be willing if I
>> use
>>
> you
>
>> to focus (un-diffuse?) responsibilities here? :)
>>
>
>
> I'll do my best. If your change extends beyond touch/gestures, we should
> bring
> in someone else to look it over.


This patch itself is only to allow Telemetry, in the scrolling benchmarks,
to send "touch down, touch move, touch up" instead of "wheel events" :)
separately, we'll want to have "pinch to zoom" as well, but that's totally
orthogonal..



>
>
>
>  so before I dig into the actual comments, let me try an overview once
>> again, I
>> hope we'll agree on the high level architecture and then we can drill
>> down to
>> the implementation... bear with me, but feel free to skip things you
>> already
>> know! ;)
>>
>
> Thanks for the details. I'd like to think that I've heard some of them
> before.
> :-)


:)



>
>
>
>  Telemetry is a cross-chrome-platform harness we're building, mostly
>> focused on
>> performance, so that we'll be able to write a single perf test that would
>> run
>> consistently on chrome across all platforms.
>> (broad brush, but this where this patch is).
>>
>
>  Telemetry "speaks" DevTools Remote Debugging, and adds some niceties,
>> such as
>>
> an
>
>> API to inject "synthetic" events (right now, named
>> "SmoothScrollGesture")...
>>
>
>  on android, instead of "mouse wheel" events at the browser layer, we'd
>> really
>> like to synthesize platforms events higher up in our Java layer for "touch
>>
> down,
>
>> move, up"..
>>
>
> Do you mean to do this with touch events or gesture events? I think both
> are
> interesting and useful but note that the queueing is going to get subtle.
> Let me
> know how I can help make this happen on CrOS/Aura.


at first, we just want to synthesize the touch stream: down, move * n,
up....



>
>
>
>  my original patch (see long, long discussion in the attached bug) got
>> reverted
>> because those events were not being "ticked", that is, we were not
>> sending the
>> full event stream as required to scroll the page under test.
>>
>
>  the core of the issue is that "in_process_event_types_" gets conflated
>> with
>> different "touch" and "gesture", and then checking for it being empty is
>> not
>>
> the
>
>> right condition for these synthetic events.. in fact,
>>
> "in_process_event_types_"
>
>> gets out of sync (see sadrul on #12 above).
>>
>
>  the latest patch tries to decouple all of these complexity, in hopefully a
>> better architecture with improved tests.
>>
>
> I think I get the gist of this code and the general approach seems
> reasonable at
> least to me except for the following:
>
> *  I don't understand how you know if the scroll has actually scrolled
> anything?
> You send wheels that add up to pixels_scrolled_ pixels but I can't see how
> the
> code figures out how much the page has actually been scrolled.
>

The actual "has scrolled" is done deep down in the javascript side of the
benchmark ran by telemetry...
http://src.chromium.org/viewvc/chrome/trunk/src/tools/telemetry/telemetry/scr...

afaict, the pixels here is just a "best estimate" in order to keep the
"velocity" smooth, i.e., we'd try to send  scroll deltas to keep a fixed
rate of scrolling... nduca@ can provide more details on that.



> *  Is there recording of how many events reach the renderer vs how many
> your
> generate? (i.e. how coarse the scroll is?) It seemed that this could be
> wired
> into pending_input_event_count_
>

nope, it's not recording / keeping track of the events themselves. I think
the main goal here is to measure framerate during scroll, not necessarily
the event stream throughput (but I could've misunderstood your question,
sorry..)


>
> Also: (dumb maybe) why do you want to support multiple
> SmoothScrollGestures at
> once?
>

good question :)
nduca?


>
> I'm really glad to see this code moved out of render_widget_host_impl. RWHi
> seems like a class needing to be divided up.


yay! :) thanks, glad to hear you think it's a good direction as well...
as soon as we clarify the questions above from nat, I'll dig into this to
clear up your previous comments.


>
>
>
>
>  the smooth_scroll_gesture_**controller.h would then only kick in for such
>> "synthetic" events (we're planning on adding synthetic "pinch" too)....
>>
>
>
>  so... I hope this shines a little bit of light on what this is all about.
>> if
>>
> so,
>
>> please do let me know if you're happy with this split, and if I should go
>>
> ahead
>
>> and address your comments, or if you rather have some completely different
>> approach altogether :)
>>
>
>
>
>
https://codereview.chromium.**org/11858007/<https://codereview.chromium.org/1...
>

[nduca, 2013-02-15 19:27:06]

We dont have to support multiple gestures at once, but if we dont, then we
should add adequate protections on the js-side when it is kicked off to prevent
issues, as well as failure return values for if it fails.

[bulach, 2013-02-18 10:59:16]

nat, robert, jamesr:
again, thanks for the comments!

so it seems we could simplify all of the stack by supporting just one single
"synthetic event" at any time..
this is going to be a biggie and will cross renderer and browser, so hopefully
we won't diffuse responsibilities even more :)
let me outline the plan here:
- keep smooth_scroll_gesture_controller.h as is, but rather than
SmoothScrollGestureMap, it will contain just a single SmoothScrollGesture.

- change RenderWidget::BeginSmoothScroll to return a bool

- also on render_widget, rather than PendingSmoothScrollGestureMap, it'll
contain a single gesture.

- RenderWidget::BeginSmoothScroll returns false if there's already one pending
and then it's up to the java script (currently,
tools/telemetry/telemetry/page/scrolling_action.js) assert that
chrome.gpuBenchmarking.smoothScrollBy returns true (it's already creating a
single event anyways)...


in terms of test coverage, the unittest introduced in #2 and the existing
telemetry benchmarks should pretty much cover all the functionality.

how does it sound? should I go ahead with it?

I guess it'll require robert for content/renderer_host, jamesr for
content/renderer and nduca for telemetry/, so it'd be great to have your
thoughts before uploading yet-another-biggie. :)

thanks!

[bulach, 2013-02-18 18:07:52]

quick follow up on #23 above, I just went ahead and did the "one smooth scroll
at a time", which deletes even more code :)
ptal

[rjkroege, 2013-02-21 19:26:33]

this looks good to me.

https://codereview.chromium.org/11858007/diff/35002/content/browser/renderer_...
File content/browser/renderer_host/render_widget_host_impl.cc (right):

https://codereview.chromium.org/11858007/diff/35002/content/browser/renderer_...
content/browser/renderer_host/render_widget_host_impl.cc:1211:
smooth_scroll_gesture_controller_->OnForwardInputEvent();
In the SmoothScrollGestureController, you say that this should be called when we
forward an input event. This is then in the wrong place if there are events
other than wheels in the stream and the page has been responding slowly enough
to the wheels to require coalescing -- you need to call it in the loop down
below and the final SendInpu


Aside: given that the benchmarking support is a turn off/on sort of feature,
perhaps implementing this counting by making the SmoothScrollGestureController a
subclass of RenderViewHostObserver?

https://codereview.chromium.org/11858007/diff/35002/content/common/view_messa...
File content/common/view_messages.h (right):

https://codereview.chromium.org/11858007/diff/35002/content/common/view_messa...
content/common/view_messages.h:850:
IPC_MESSAGE_ROUTED0(ViewMsg_SmoothScrollCompleted)
using a RenderViewHostObserver could save you needing to add this here too?

[rjkroege, 2013-02-21 19:27:41]

On 2013/02/21 19:26:33, rjkroege wrote:
> this looks good to me.

oops. sent too soon. But I think you should fix the location of the entry point
to OnForwardInputEvent.

> 
>
https://codereview.chromium.org/11858007/diff/35002/content/browser/renderer_...
> File content/browser/renderer_host/render_widget_host_impl.cc (right):
> 
>
https://codereview.chromium.org/11858007/diff/35002/content/browser/renderer_...
> content/browser/renderer_host/render_widget_host_impl.cc:1211:
> smooth_scroll_gesture_controller_->OnForwardInputEvent();
> In the SmoothScrollGestureController, you say that this should be called when
we
> forward an input event. This is then in the wrong place if there are events
> other than wheels in the stream and the page has been responding slowly enough
> to the wheels to require coalescing -- you need to call it in the loop down
> below and the final SendInpu
> 
> 
> Aside: given that the benchmarking support is a turn off/on sort of feature,
> perhaps implementing this counting by making the SmoothScrollGestureController
a
> subclass of RenderViewHostObserver?
> 
>
https://codereview.chromium.org/11858007/diff/35002/content/common/view_messa...
> File content/common/view_messages.h (right):
> 
>
https://codereview.chromium.org/11858007/diff/35002/content/common/view_messa...
> content/common/view_messages.h:850:
> IPC_MESSAGE_ROUTED0(ViewMsg_SmoothScrollCompleted)
> using a RenderViewHostObserver could save you needing to add this here too?

[rjkroege, 2013-02-21 19:57:47]

On 2013/02/21 19:26:33, rjkroege wrote:
> this looks good to me.
> 
>
https://codereview.chromium.org/11858007/diff/35002/content/browser/renderer_...
> File content/browser/renderer_host/render_widget_host_impl.cc (right):
> 
>
https://codereview.chromium.org/11858007/diff/35002/content/browser/renderer_...
> content/browser/renderer_host/render_widget_host_impl.cc:1211:
> smooth_scroll_gesture_controller_->OnForwardInputEvent();
> In the SmoothScrollGestureController, you say that this should be called when
we
> forward an input event. This is then in the wrong place if there are events
> other than wheels in the stream and the page has been responding slowly enough
> to the wheels to require coalescing -- you need to call it in the loop down
> below and the final SendInpu
> 
> 

I was wrong about this aside. Please ignore this advice. 

> Aside: given that the benchmarking support is a turn off/on sort of feature,
> perhaps implementing this counting by making the SmoothScrollGestureController
a
> subclass of RenderViewHostObserver?
> 
>
https://codereview.chromium.org/11858007/diff/35002/content/common/view_messa...
> File content/common/view_messages.h (right):
> 
>
https://codereview.chromium.org/11858007/diff/35002/content/common/view_messa...
> content/common/view_messages.h:850:
> IPC_MESSAGE_ROUTED0(ViewMsg_SmoothScrollCompleted)
> using a RenderViewHostObserver could save you needing to add this here too?

Here though you could conceivably use an observer. But I'm not sure if it's
worthwhile.

[bulach, 2013-02-22 16:55:14]

thanks! one comment addressed and some clarifications inline, please let me know
your thoughts:

https://codereview.chromium.org/11858007/diff/35002/content/browser/renderer_...
File content/browser/renderer_host/render_widget_host_impl.cc (right):

https://codereview.chromium.org/11858007/diff/35002/content/browser/renderer_...
content/browser/renderer_host/render_widget_host_impl.cc:1211:
smooth_scroll_gesture_controller_->OnForwardInputEvent();
On 2013/02/21 19:26:33, rjkroege wrote:
> In the SmoothScrollGestureController, you say that this should be called when
we
> forward an input event. This is then in the wrong place if there are events
> other than wheels in the stream and the page has been responding slowly enough
> to the wheels to require coalescing -- you need to call it in the loop down
> below and the final SendInpu
> 
> 
> Aside: given that the benchmarking support is a turn off/on sort of feature,
> perhaps implementing this counting by making the SmoothScrollGestureController
a
> subclass of RenderViewHostObserver?

Done.

https://codereview.chromium.org/11858007/diff/35002/content/browser/renderer_...
File content/browser/renderer_host/smooth_scroll_gesture_controller.cc (right):

https://codereview.chromium.org/11858007/diff/35002/content/browser/renderer_...
content/browser/renderer_host/smooth_scroll_gesture_controller.cc:56:
DCHECK(pending_input_event_count_ > 0);
I changed this to an "if" rather than a DCHECK.
there's a test RenderWidgetHostTest.IgnoreKeyEventsWeDidntSend, which would fail
with this...
we could keep a better track of events here, but again, we shall never receive
an ACK for something we never sent in the context of synthetic events. wdyt?

https://codereview.chromium.org/11858007/diff/35002/content/common/view_messa...
File content/common/view_messages.h (right):

https://codereview.chromium.org/11858007/diff/35002/content/common/view_messa...
content/common/view_messages.h:850:
IPC_MESSAGE_ROUTED0(ViewMsg_SmoothScrollCompleted)
On 2013/02/21 19:26:33, rjkroege wrote:
> using a RenderViewHostObserver could save you needing to add this here too?

hmm.. this is just being changed, not added, but anyways.. :)
I can move this to a separate file, say "smooth_scroll_messages.h" if you
prefer?
however, as for making the |gesture controller| a RenderViewHostObserver, I'm
not quite clear if that would be worth it: my understanding is that this needs
to be hooked at RenderWidgetHostImpl, which is a parallel hierarchy to
RenderViewHost, so it wouldn't be possible to instantiate the |gesture
controller| at construction time... I mean, my understanding is that it'd be
only possible after the bottom of the diamond, |RenderViewHostImpl|, is
instantiated.

however, I may have missed some "view x host x impl" combination here, please
let me know if this is simpler than I'm seeing :)

[rjkroege, 2013-02-22 17:24:29]

On 2013/02/22 16:55:14, bulach wrote:
> thanks! one comment addressed and some clarifications inline, please let me
know
> your thoughts:
> 
>
https://codereview.chromium.org/11858007/diff/35002/content/browser/renderer_...
> File content/browser/renderer_host/render_widget_host_impl.cc (right):
> 
>
https://codereview.chromium.org/11858007/diff/35002/content/browser/renderer_...
> content/browser/renderer_host/render_widget_host_impl.cc:1211:
> smooth_scroll_gesture_controller_->OnForwardInputEvent();
> On 2013/02/21 19:26:33, rjkroege wrote:
> > In the SmoothScrollGestureController, you say that this should be called
when
> we
> > forward an input event. This is then in the wrong place if there are events
> > other than wheels in the stream and the page has been responding slowly
enough
> > to the wheels to require coalescing -- you need to call it in the loop down
> > below and the final SendInpu
> > 
> > 
> > Aside: given that the benchmarking support is a turn off/on sort of feature,
> > perhaps implementing this counting by making the
SmoothScrollGestureController
> a
> > subclass of RenderViewHostObserver?
> 
> Done.
> 
>
https://codereview.chromium.org/11858007/diff/35002/content/browser/renderer_...
> File content/browser/renderer_host/smooth_scroll_gesture_controller.cc
(right):
> 
>
https://codereview.chromium.org/11858007/diff/35002/content/browser/renderer_...
> content/browser/renderer_host/smooth_scroll_gesture_controller.cc:56:
> DCHECK(pending_input_event_count_ > 0);
> I changed this to an "if" rather than a DCHECK.
> there's a test RenderWidgetHostTest.IgnoreKeyEventsWeDidntSend, which would
fail
> with this...
> we could keep a better track of events here, but again, we shall never receive
> an ACK for something we never sent in the context of synthetic events. wdyt?
> 
>
https://codereview.chromium.org/11858007/diff/35002/content/common/view_messa...
> File content/common/view_messages.h (right):
> 
>
https://codereview.chromium.org/11858007/diff/35002/content/common/view_messa...
> content/common/view_messages.h:850:
> IPC_MESSAGE_ROUTED0(ViewMsg_SmoothScrollCompleted)
> On 2013/02/21 19:26:33, rjkroege wrote:
> > using a RenderViewHostObserver could save you needing to add this here too?
> 
> hmm.. this is just being changed, not added, but anyways.. :)
> I can move this to a separate file, say "smooth_scroll_messages.h" if you
> prefer?

no. seems fine as it is. It was more of a "might be worth considering idea". I
had been hoping that the observer would watch sends but it doesn't.

> however, as for making the |gesture controller| a RenderViewHostObserver, I'm
> not quite clear if that would be worth it: my understanding is that this needs
> to be hooked at RenderWidgetHostImpl, which is a parallel hierarchy to
> RenderViewHost, so it wouldn't be possible to instantiate the |gesture
> controller| at construction time... I mean, my understanding is that it'd be
> only possible after the bottom of the diamond, |RenderViewHostImpl|, is
> instantiated.
> 
> however, I may have missed some "view x host x impl" combination here, please
> let me know if this is simpler than I'm seeing :)

[rjkroege, 2013-02-22 17:24:36]

https://codereview.chromium.org/11858007/diff/44001/content/browser/renderer_...
File content/browser/renderer_host/render_widget_host_impl.cc (right):

https://codereview.chromium.org/11858007/diff/44001/content/browser/renderer_...
content/browser/renderer_host/render_widget_host_impl.cc:1169:
SendInputEvent(coalesced_mouse_wheel_events_[i],
you need it before this one too yes?

[sadrul, 2013-02-22 17:26:45]

https://codereview.chromium.org/11858007/diff/44001/content/browser/renderer_...
File content/browser/renderer_host/render_widget_host_impl.cc (right):

https://codereview.chromium.org/11858007/diff/44001/content/browser/renderer_...
content/browser/renderer_host/render_widget_host_impl.cc:1169:
SendInputEvent(coalesced_mouse_wheel_events_[i],
On 2013/02/22 17:24:37, rjkroege wrote:
> you need it before this one too yes?

I think it makes sense to move both in_process_event_types_.push() and
smooth_scroll_gesture_controller_->OnForwardInputEvent() into SendInputEvent.

[rjkroege, 2013-02-22 20:19:15]

content/browser event changes lgtm

[jamesr, 2013-02-22 20:23:39]

content/renderer/ lgtm (but you want Nat to look at it as well)

[bulach, 2013-02-25 09:16:03]

yay, thanks! :)
nat: need your eyes here, specially the telemetry changes..

https://codereview.chromium.org/11858007/diff/44001/content/browser/renderer_...
File content/browser/renderer_host/render_widget_host_impl.cc (right):

https://codereview.chromium.org/11858007/diff/44001/content/browser/renderer_...
content/browser/renderer_host/render_widget_host_impl.cc:1169:
SendInputEvent(coalesced_mouse_wheel_events_[i],
On 2013/02/22 17:26:45, sadrul wrote:
> On 2013/02/22 17:24:37, rjkroege wrote:
> > you need it before this one too yes?
> 
> I think it makes sense to move both in_process_event_types_.push() and
> smooth_scroll_gesture_controller_->OnForwardInputEvent() into SendInputEvent.

yeah, looks better, thanks!

[sadrul, 2013-02-25 15:08:22]

Please make sure you update the CL description before landing!

[bulach, 2013-02-25 15:20:30]

thanks sadrul! updated the description, and also fixed the Mock dtor for clang..

I'll wait for nat's review.

[bulach, 2013-03-19 09:53:32]

nat: mind taking a quick look? :) hopefully we'll be able to untangle this
soon.. (latest patch is just a rebase).

[nduca, 2013-03-20 20:54:52]

lgtm

please note i also just filed a bug about crbug.com/222496 to make the input
queueing stuff either better documented or more explainable.

[bulach, 2013-03-21 12:32:30]

yay, thanks everyone! :) CQing, and then will finally get back to integrate the
android side.

[bulach, 2013-03-21 12:54:30]

nat: I'm very sorry about this... :-/

"Patch Set 7 : Rebased" was *wrong*, I rebased and uploaded based on an old
version, patch set 2, prior to addressing some other comments...


"Patch Set 8 : Rebase from 6" is the right one, which touches telemetry as
well... would you mind taking a look at that one instead?

once again, apologies for the confusion..

[tonyg, 2013-03-28 17:27:24]

On 2013/03/21 12:54:30, bulach wrote:
> nat: I'm very sorry about this... :-/
> 
> "Patch Set 7 : Rebased" was *wrong*, I rebased and uploaded based on an old
> version, patch set 2, prior to addressing some other comments...
> 
> 
> "Patch Set 8 : Rebase from 6" is the right one, which touches telemetry as
> well... would you mind taking a look at that one instead?
> 
> once again, apologies for the confusion..

Telemetry scroll.js changes lgtm (didn't review the entire patch)

[bulach, 2013-03-28 18:33:57]

*sigh*, rebased again, and fixed the test code for aura...

+cevans for messages
tonyg: I would appreciate another look in the tests, I essentially replicated
the gfx::Screen initialization from render_widget_host_unittest.cc...

[Chris Evans, 2013-03-28 18:42:57]

On 2013/03/28 18:33:57, bulach wrote:
> *sigh*, rebased again, and fixed the test code for aura...
> 
> +cevans for messages
> tonyg: I would appreciate another look in the tests, I essentially replicated
> the gfx::Screen initialization from render_widget_host_unittest.cc...

content/common/view_messages.h LGTM

[nduca, 2013-04-01 18:18:29]

who're we waiting on for reviews?

[bulach, 2013-04-02 08:52:17]

nat: tony already looked into the scroll.js and I think we're done. however, you
reviewed patchset 7, which was a bad rebase on my part (sorry!).. that
particular patch did not address your comment:
https://codereview.chromium.org/11858007/#msg23

these were already addressed by patchset 6, and everything after, so it'd be
great if you could look at it to ensure your comments are properly addressed.

again, truly sorry for patchset 7, that caused even more confusion here... :-/

[nduca, 2013-04-02 17:34:42]

lgtm :)

[commit-bot: I haz the power, 2013-04-02 17:35:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bulach@chromium.org/11858007/77001

[commit-bot: I haz the power, 2013-04-02 17:35:16]

Presubmit check for 11858007-77001 failed and returned exit status 1.

INFO:root:Found 12 file(s).

INFO:PRESUBMIT:Skipping pylint: no matching changes.
Running presubmit commit checks ...
Running /b/commit-queue/workdir/chromium/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/tools/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/tools/telemetry/PRESUBMIT.py

** Presubmit ERRORS **
Missing LGTM from an OWNER for these files:
    content/content_tests.gypi
    content/content_browser.gypi

Presubmit checks took 2.2s to calculate.

[bulach, 2013-04-02 18:30:09]

darin: need your OWNERS power for
  content/content_tests.gypi
  content/content_browser.gypi

everything else has been reviewed by various owners.

thanks!

[nduca, 2013-04-02 18:55:39]

piman may be available for content/

[darin (slow to review), 2013-04-03 05:30:08]

LGTM for content/*gypi

[commit-bot: I haz the power, 2013-04-03 07:49:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bulach@chromium.org/11858007/77001

[commit-bot: I haz the power, 2013-04-03 10:48:30]

Change committed as 192041

[phoglund_chromium, 2013-04-03 12:07:50]

On 2013/04/03 10:48:30, I haz the power (commit-bot) wrote:
> Change committed as 192041

I had to revert this one: broke content_unittests at
http://build.chromium.org/p/chromium.win/builders/Vista%20Tests%20%283%29/bui....

[phoglund_chromium, 2013-04-03 12:08:45]

Sorry, I meant
http://build.chromium.org/p/chromium.win/builders/Vista%20Tests%20%283%29/bui....

[phoglund_chromium, 2013-04-03 12:11:36]

Damn it, the page messes up the URL. Try this: http://goo.gl/aGXhJ.