Removed requirement for the renderer to know it's process ID

Removed requirement for the renderer to know it's process ID

Process ID is now patched into the latency info once it reaches the browser process

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

[bajones, 2014-01-16 23:02:05]

PTAL at the respective bits under your ownership. If it seems that I have missed
any significant compositing paths (that is, they do not call
LatencyInfo::AnnotateWithProcessId or
LatencyInfo::AnnotateComponentsWithProcessId) please let me know!

[jamesr, 2014-01-16 23:07:21]

I think you should populate the entire component ID on the browser side, not
just the RenderProcessHost ID (which is not the same as an os-level PID concept
- it's a content generated ID for the browser to use)

https://codereview.chromium.org/140663003/diff/100001/content/renderer/render...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/140663003/diff/100001/content/renderer/render...
content/renderer/render_view_impl.cc:2763: return GetRoutingID() |
(static_cast<int64>(base::kNullProcessId) << 32);
why do we put the routing ID in here?  I think the entire component is replaced
on the browser side, so we really should be able to just put '0' in here

https://codereview.chromium.org/140663003/diff/100001/ui/events/latency_info.h
File ui/events/latency_info.h (right):

https://codereview.chromium.org/140663003/diff/100001/ui/events/latency_info....
ui/events/latency_info.h:128: base::ProcessId process_id);
nit: i don't think that the ID used here is actually a PID

[bajones, 2014-01-16 23:44:33]

On 2014/01/16 23:07:21, jamesr wrote:
> I think you should populate the entire component ID on the browser side, not
> just the RenderProcessHost ID (which is not the same as an os-level PID
concept
> - it's a content generated ID for the browser to use)

That's a pretty obvious idea in retrospect. Thanks, implemented!

[jamesr, 2014-01-16 23:47:02]

cool! content/renderer/ lgtm

[jbauman, 2014-01-17 00:02:28]

https://codereview.chromium.org/140663003/diff/160001/ui/events/latency_info.cc
File ui/events/latency_info.cc (right):

https://codereview.chromium.org/140663003/diff/160001/ui/events/latency_info....
ui/events/latency_info.cc:266: latency_components.erase(eraseIter);
how about "latency_components.erase(lc++);" instead?

https://codereview.chromium.org/140663003/diff/160001/ui/events/latency_info.h
File ui/events/latency_info.h (right):

https://codereview.chromium.org/140663003/diff/160001/ui/events/latency_info....
ui/events/latency_info.h:13: #include "base/process/process_handle.h"
Not necessary anymore.

https://codereview.chromium.org/140663003/diff/160001/ui/events/latency_info....
ui/events/latency_info.h:139: // WINDOW_SNAPSHOT_FRAME_NUMBER_COMPONENTs,
require a ProcessId to route
Not a ProcessId anymore.

https://codereview.chromium.org/140663003/diff/160001/ui/events/latency_info....
ui/events/latency_info.h:142: void FixMissingComponentIds(int64 component_id);
Could you put this on RenderWidgetHostImpl instead? ui/events ideally shouldn't
have to worry about which components are created in the renderer or the browser
process.

[bajones, 2014-01-17 00:20:25]

Thanks jbauman@! Addressed everything you mentioned.

[jbauman, 2014-01-17 00:26:13]

lgtm with one nit.

https://codereview.chromium.org/140663003/diff/240001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_impl.h (right):

https://codereview.chromium.org/140663003/diff/240001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_impl.h:556: void
FixMissingLatencyInfoComponentIds(ui::LatencyInfo& latency_info);
Technically, this should be a pointer, not a reference.

[jam, 2014-01-17 01:27:26]

thanks a lot for the quick turnaround, just some minor comments

https://codereview.chromium.org/140663003/diff/240001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_impl.cc (right):

https://codereview.chromium.org/140663003/diff/240001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_impl.cc:2535: if
(lc->first.second == 0) {
i think we should always set it if it's coming from the renderer. that way a
renderer can't put a non-zero value and then we'd trust it.

https://codereview.chromium.org/140663003/diff/240001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_impl.h (right):

https://codereview.chromium.org/140663003/diff/240001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_impl.h:556: void
FixMissingLatencyInfoComponentIds(ui::LatencyInfo& latency_info);
On 2014/01/17 00:26:13, jbauman wrote:
> Technically, this should be a pointer, not a reference.

yep according to style guide it has to be a pointer.

nit: i would name this method AddLatencyInfoComponenttIds since it's not
"missing" as the renderer's job isn't to add it. just nitting because we
wouldn't want someone to see "fix" in the method name and then fix it by
changing the renderer

[bajones, 2014-01-17 17:56:34]

Thanks for the feedback! CL updated accordingly.

[jam, 2014-01-17 21:59:50]

lgtm

[bajones, 2014-01-18 00:02:35]

Thanks jam@!

Can I get an owners review for view_messages.h, please? It'll be a fast one, I
promise! :)

[Ken Russell (switch to Gerrit), 2014-01-18 00:30:46]

Thanks for cleaning this up. LGTM

[Cris Neckar, 2014-01-21 18:45:10]

On 2014/01/18 00:30:46, Ken Russell wrote:
> Thanks for cleaning this up. LGTM

IPC changes LGTM

[commit-bot: I haz the power, 2014-01-21 19:53:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bajones@chromium.org/140663003/440001

[commit-bot: I haz the power, 2014-01-21 23:01:27]

Change committed as 246149