Send input event's LatencyInfo back from renderer to browser when acked

Send input event's LatencyInfo back from renderer to browser when acked

Currently input event and its associated LatencyInfo are sent from
browser to renderer together. But the LatencyInfo is not sent back to
browser with individual input event, and only at frame swap time the
browser will get the aggregated LatencyInfo of all input events during
that frame. This makes it difficulte for browser side to track the
individual event's latency info if it is modified at renderer side.

In this CL we send the input event's LatencyInfo back from renderer
to browser when the input event is acked, so that if any update happens
to the event's LatencyInfo at renderer side, we can access the update
also in browser side.

BUG=261388
TEST=Added some latency component in renderer, and check that they can
     be accessed from browser side.

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

[jamesr, 2013-07-19 19:42:26]

I think John should do the primary review on this when he's back.

I don't follow the keyboard shortcut related code in content/renderer/gpu/ and I
can't find anything about it in the patch description.  What are these changes
for?

[Yufeng Shen (Slow to review), 2013-07-19 19:49:22]

On 2013/07/19 19:42:26, jamesr wrote:
> I think John should do the primary review on this when he's back.
> 
> I don't follow the keyboard shortcut related code in content/renderer/gpu/ and
I
> can't find anything about it in the patch description.  What are these changes
> for?

It is because we want to unpack the whole message, add new latency component to
the
LatencyInfo if needed, and pack the message back. So we extract every member in
the
message (including the is_keyboard_shortcut) so to put it back.

If there is way to modify the member of IPC::Message inplace, then this
re-packing
could be avoided.

[jamesr, 2013-07-19 19:57:37]

But I don't see a new field added to a message or the latency info struct in
this patch, so I don't get why we have to start packing this differently.

[miletus1, 2013-07-19 20:08:49]

Right,  I should have made this more clear.

The motivation of this CL is to have new latency component added in
the renderer, and we pass the LatencyInfo back to browser and
can still access these renderer side latency component. I made this
preparing CL which only facilitates the passing
back of LatencyInfo, but leave adding new latency component to future
CLs. With those future CLs in mind, I sort of already setup it
in the CL the way that unpacking the latency info, and repacking it
when forwarded to main thread. And the new latency components
will be filled in later. But I might have overdone it.


On Fri, Jul 19, 2013 at 3:57 PM,  <jamesr@chromium.org> wrote:
> But I don't see a new field added to a message or the latency info struct in
> this patch, so I don't get why we have to start packing this differently.
>
> https://codereview.chromium.org/19670007/

[jamesr, 2013-07-20 02:20:35]

Can't we use a generic cracker interface mechanism?  Having to handle individual
fields specially like this in the input filter is not going to scale.

[jbauman, 2013-07-30 22:21:21]

lgtm

[Yufeng Shen (Slow to review), 2013-07-31 21:35:05]

On 2013/07/20 02:20:35, jamesr wrote:
> Can't we use a generic cracker interface mechanism?  Having to handle
individual
> fields specially like this in the input filter is not going to scale.

hmmm, so I replace the CrackMessage() function with the IPC message's own read
function

InputMsg_HandleInputEvent::Read(
    &message, &event, &latency_info, &is_keyboard_shortcut))

If the message structure ever changes, this function will not compile. Not sure
if this
is what you want for scalability, but it does seem cleaner for me.

PTAL.

[jamesr, 2013-08-06 17:52:42]

content/renderer/gpu lgtm. this doesn't seem super clean, but at least it won't
fail silently

[sadrul, 2013-08-06 19:08:27]

/cc jdduke@

https://codereview.chromium.org/19670007/diff/8001/content/browser/renderer_h...
File content/browser/renderer_host/input/touch_event_queue.cc (right):

https://codereview.chromium.org/19670007/diff/8001/content/browser/renderer_h...
content/browser/renderer_host/input/touch_event_queue.cc:137:
touch_queue_.front()->SetLatencyInfoForCoalescedEvent(latency_info);
Instead of doing this, can you send |latency_info| to PopTouchEventWithAck (and
perhaps rename it to PopTouchEventToClient, and send ui::LatencyInfo() in the
other calls to PopTouchEvent...)

[Yufeng Shen (Slow to review), 2013-08-06 19:36:13]

https://chromiumcodereview.appspot.com/19670007/diff/8001/content/browser/ren...
File content/browser/renderer_host/input/touch_event_queue.cc (right):

https://chromiumcodereview.appspot.com/19670007/diff/8001/content/browser/ren...
content/browser/renderer_host/input/touch_event_queue.cc:137:
touch_queue_.front()->SetLatencyInfoForCoalescedEvent(latency_info);
On 2013/08/06 19:08:27, sadrul wrote:
> Instead of doing this, can you send |latency_info| to PopTouchEventWithAck
(and
> perhaps rename it to PopTouchEventToClient, and send ui::LatencyInfo() in the
> other calls to PopTouchEvent...)

Done.

[sadrul, 2013-08-06 19:49:30]

lgtm

https://chromiumcodereview.appspot.com/19670007/diff/16001/ui/base/latency_in...
File ui/base/latency_info.cc (right):

https://chromiumcodereview.appspot.com/19670007/diff/16001/ui/base/latency_in...
ui/base/latency_info.cc:30: for (LatencyMap::const_iterator it =
other.latency_components.begin();
Fix indentation

[jdduke (slow), 2013-08-06 19:50:38]

I'd really prefer it if the returned ui::LatencyInfo contained only additional
entries that were made in the renderer, rather than all aggregated entries and
performing a diff.  Is there a clean way of doing this?

[Yufeng Shen (Slow to review), 2013-08-06 20:00:57]

On 2013/08/06 19:50:38, jdduke wrote:
> I'd really prefer it if the returned ui::LatencyInfo contained only additional
> entries that were made in the renderer, rather than all aggregated entries and
> performing a diff.  Is there a clean way of doing this?

hmmm, not sure how to do it cleanly, especially when passing the latency_info
from
impl-thread to main-thread, you will have to carry 2 latency_info: one for
original
latency_info comes with the event, and one for the additional entries that
happened
in the impl-thread (and will be sent back to browser from main-thread).

[Yufeng Shen (Slow to review), 2013-08-06 20:01:04]

https://chromiumcodereview.appspot.com/19670007/diff/16001/ui/base/latency_in...
File ui/base/latency_info.cc (right):

https://chromiumcodereview.appspot.com/19670007/diff/16001/ui/base/latency_in...
ui/base/latency_info.cc:30: for (LatencyMap::const_iterator it =
other.latency_components.begin();
On 2013/08/06 19:49:31, sadrul wrote:
> Fix indentation

Done.

[Yufeng Shen (Slow to review), 2013-08-06 20:11:02]

+ jln@ for OWNER of content/common/input_messages.hq

[jln (very slow on Chromium), 2013-08-06 20:30:08]

On 2013/08/06 20:11:02, Yufeng Shen wrote:
> + jln@ for OWNER of content/common/input_messages.hq

content/common/input_messages.h lgtm

[commit-bot: I haz the power, 2013-08-06 20:55:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/miletus@chromium.org/19670007/15002

[commit-bot: I haz the power, 2013-08-06 21:23:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/miletus@chromium.org/19670007/57001

[commit-bot: I haz the power, 2013-08-07 00:27:10]

Change committed as 216043

[jdduke (slow), 2013-08-08 15:51:28]

On 2013/08/07 00:27:10, I haz the power (commit-bot) wrote:
> Change committed as 216043

... Unless I'm missing something, this patch does absolutely nothing.  The
LatencyInfo returned by InputEventFilter has had no entries added to it.  The
LatencyInfo returned by RenderWidget is const, and again has had no entries
added to it.  So this is just returning the original LatencyInfo... to what end?

[miletus1, 2013-08-08 16:42:59]

You are right, this patch is just a preparation one and future patches need
to fill in the actual LatencyInfo entries. Currently we have 2 such entries
in
mind:
1) crbug.com/261388     adding an entry to track when a touch move event
is forwarded from impl to main thread and then in browser we check how often
these touch move become ScrollUpdate.
2) https://chromiumcodereview.appspot.com/16213002/
There is a long debate on this CL about whether we should calculate the UMA
metric as soon as the needed timestamp is available or should we have a
central
place to compute all the UMA metric. Now we tend to take the latter
approach, and
to do that, we will need to add the LatencyInfo entry in renderer for touch
 & gesture
event, let the latencyinfo come back to browser side and then compute the
UMA
metric from there.

And you are also right about the const-ness. When the actual entry is
added, the
const qualifier will need to be removed.

Hope this make things clear.




On Thu, Aug 8, 2013 at 11:51 AM, <jdduke@chromium.org> wrote:

> On 2013/08/07 00:27:10, I haz the power (commit-bot) wrote:
>
>> Change committed as 216043
>>
>
> ... Unless I'm missing something, this patch does absolutely nothing.  The
> LatencyInfo returned by InputEventFilter has had no entries added to it.
>  The
> LatencyInfo returned by RenderWidget is const, and again has had no entries
> added to it.  So this is just returning the original LatencyInfo... to
> what end?
>
>
>
https://chromiumcodereview.**appspot.com/19670007/<https://chromiumcodereview...
>

[nduca, 2013-08-09 16:25:55]

owners: this should have been lg'd by jbauman at minimuum and gotten buyoff by
jared. these two people work on this system, bypassing them during review is not
appropriate.

[sadrul, 2013-08-09 16:28:42]

On 2013/08/09 16:25:55, nduca wrote:
> owners: this should have been lg'd by jbauman at minimuum and gotten buyoff by
> jared. these two people work on this system, bypassing them during review is
not
> appropriate.

jbauman did review: https://codereview.chromium.org/19670007/#msg7