Inject renderer recieve message time into resource notifications.

Inject renderer receive message time into resource notifications.

Previously time calculations based on "now"-time taken when renderer processed resource notification messages.

Renderer time is calculated by interpolation based on taken values.
Because of long JS evaluation or heavy rendering, "renderer" time range might be skewed.

To fix situation, "renderer" time should be taken when IPC messages are processed by renderer IO thread.

This patch adds message filter that supplies aforementioned messages with timestamps.

BUG=223836

[James Simonsen, 2013-04-26 23:02:28]

Thanks for taking this on!

https://codereview.chromium.org/14359004/diff/3001/content/common/child_resou...
File content/common/child_resource_message_filter.cc (right):

https://codereview.chromium.org/14359004/diff/3001/content/common/child_resou...
content/common/child_resource_message_filter.cc:15: :
task_runner_(base::ThreadTaskRunnerHandle::Get()),
I believe this ends up being the main thread's task runner. That's good. But,
can you make it clear by calling it main_thread_task_runner_?

https://codereview.chromium.org/14359004/diff/3001/content/common/resource_di...
File content/common/resource_dispatcher.h (right):

https://codereview.chromium.org/14359004/diff/3001/content/common/resource_di...
content/common/resource_dispatcher.h:76: io_timestamp_ = io_timestamp;
What happens if there are many resource messages queued? Admittedly it's better
than what we have today, but it's still not exactly what we want. Could we add a
field to the messages themselves instead?

[eustas, 2013-04-29 08:59:03]

https://codereview.chromium.org/14359004/diff/3001/content/common/child_resou...
File content/common/child_resource_message_filter.cc (right):

https://codereview.chromium.org/14359004/diff/3001/content/common/child_resou...
content/common/child_resource_message_filter.cc:15: :
task_runner_(base::ThreadTaskRunnerHandle::Get()),
On 2013/04/26 23:02:28, James Simonsen wrote:
> I believe this ends up being the main thread's task runner. That's good. But,
> can you make it clear by calling it main_thread_task_runner_?

Done.

https://codereview.chromium.org/14359004/diff/3001/content/common/resource_di...
File content/common/resource_dispatcher.h (right):

https://codereview.chromium.org/14359004/diff/3001/content/common/resource_di...
content/common/resource_dispatcher.h:76: io_timestamp_ = io_timestamp;
On 2013/04/26 23:02:28, James Simonsen wrote:
> What happens if there are many resource messages queued? Admittedly it's
better
> than what we have today, but it's still not exactly what we want. Could we add
a
> field to the messages themselves instead?

I've got exactly the same idea first (see Patch Set 1).
I've added fields to messages and tampered them in filter.
But this looks too hacky, because (conceptually) filters should not change
messages (they get const IPC::Message).

If I'm not mistaken, setting timestamps and receiving messages tasks will be
interleaved, so each message will correspond to its own timestamp.

[James Simonsen, 2013-04-29 21:12:56]

lgtm (from a non-OWNER)

On 2013/04/29 08:59:03, eustas.ru wrote:
>
https://codereview.chromium.org/14359004/diff/3001/content/common/child_resou...
> File content/common/child_resource_message_filter.cc (right):
> 
>
https://codereview.chromium.org/14359004/diff/3001/content/common/child_resou...
> content/common/child_resource_message_filter.cc:15: :
> task_runner_(base::ThreadTaskRunnerHandle::Get()),
> On 2013/04/26 23:02:28, James Simonsen wrote:
> > I believe this ends up being the main thread's task runner. That's good.
But,
> > can you make it clear by calling it main_thread_task_runner_?
> 
> Done.
> 
>
https://codereview.chromium.org/14359004/diff/3001/content/common/resource_di...
> File content/common/resource_dispatcher.h (right):
> 
>
https://codereview.chromium.org/14359004/diff/3001/content/common/resource_di...
> content/common/resource_dispatcher.h:76: io_timestamp_ = io_timestamp;
> On 2013/04/26 23:02:28, James Simonsen wrote:
> > What happens if there are many resource messages queued? Admittedly it's
> better
> > than what we have today, but it's still not exactly what we want. Could we
add
> a
> > field to the messages themselves instead?
> 
> I've got exactly the same idea first (see Patch Set 1).
> I've added fields to messages and tampered them in filter.
> But this looks too hacky, because (conceptually) filters should not change
> messages (they get const IPC::Message).
> 
> If I'm not mistaken, setting timestamps and receiving messages tasks will be
> interleaved, so each message will correspond to its own timestamp.

OIC. It's using the same task runner. Duh. Yeah, you're right.

Can you add a comment to the top of child_resource_message_filter.h explaining
how it works? You may also want to reference that comment from set_io_timestamp.
We're much more generous with comments in Chrome code. :) It's important
everyone understands how this works and that they don't break it in the future.

[eustas, 2013-04-30 13:07:39]

On 2013/04/29 21:12:56, James Simonsen wrote:
> lgtm (from a non-OWNER)
> 
> On 2013/04/29 08:59:03, eustas.ru wrote:
> >
>
https://codereview.chromium.org/14359004/diff/3001/content/common/child_resou...
> > File content/common/child_resource_message_filter.cc (right):
> > 
> >
>
https://codereview.chromium.org/14359004/diff/3001/content/common/child_resou...
> > content/common/child_resource_message_filter.cc:15: :
> > task_runner_(base::ThreadTaskRunnerHandle::Get()),
> > On 2013/04/26 23:02:28, James Simonsen wrote:
> > > I believe this ends up being the main thread's task runner. That's good.
> But,
> > > can you make it clear by calling it main_thread_task_runner_?
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/14359004/diff/3001/content/common/resource_di...
> > File content/common/resource_dispatcher.h (right):
> > 
> >
>
https://codereview.chromium.org/14359004/diff/3001/content/common/resource_di...
> > content/common/resource_dispatcher.h:76: io_timestamp_ = io_timestamp;
> > On 2013/04/26 23:02:28, James Simonsen wrote:
> > > What happens if there are many resource messages queued? Admittedly it's
> > better
> > > than what we have today, but it's still not exactly what we want. Could we
> add
> > a
> > > field to the messages themselves instead?
> > 
> > I've got exactly the same idea first (see Patch Set 1).
> > I've added fields to messages and tampered them in filter.
> > But this looks too hacky, because (conceptually) filters should not change
> > messages (they get const IPC::Message).
> > 
> > If I'm not mistaken, setting timestamps and receiving messages tasks will be
> > interleaved, so each message will correspond to its own timestamp.
> 
> OIC. It's using the same task runner. Duh. Yeah, you're right.
> 
> Can you add a comment to the top of child_resource_message_filter.h explaining
> how it works? You may also want to reference that comment from
set_io_timestamp.
> We're much more generous with comments in Chrome code. :) It's important
> everyone understands how this works and that they don't break it in the
future.

Done.

[Charlie Reis, 2013-04-30 16:27:06]

I assume you're looking for an owners review?  I'm not very familiar with the
message filter design and whether adding this to ChildThread is reasonable, so
I'll have to defer to John.

nit: Please fix typos in the CL description (recieve, intepolation).

https://codereview.chromium.org/14359004/diff/13001/content/common/child_reso...
File content/common/child_resource_message_filter.h (right):

https://codereview.chromium.org/14359004/diff/13001/content/common/child_reso...
content/common/child_resource_message_filter.h:32:
ChildResourceMessageFilter(ResourceDispatcher* resource_dispatcher);
nit: explicit

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Explic...

[pfeldman, 2013-04-30 16:29:51]

I suggested using devtools_agent_filter offline (in order to limit amount of
filters). And there is a straw man for that here:
https://codereview.chromium.org/14646006/.

@eustas: which one should we be looking at?

[eustas, 2013-04-30 16:44:04]

On 2013/04/30 16:29:51, pfeldman wrote:
> I suggested using devtools_agent_filter offline (in order to limit amount of
> filters). And there is a straw man for that here:
> https://codereview.chromium.org/14646006/.
> 
> @eustas: which one should we be looking at?

I think that we should look at https://codereview.chromium.org/14646006/

I'm going to update its description now.