Offer Resource Timing in workers

Offer Resource Timing in workers

This patch makes worker's performance timeline populated with resource
timings. Most of the changes are plumbing for ResourceTimingInfo from
ResourceFetcher in main thread to WorkerThreadableLoader in worker
thread.

BUG=465641
TEST=http/tests/workers,http/tests/serviceworker

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=197707

[Kunihiko Sakamoto, 2015-06-19 05:55:59]

Patchset #1 (id:1) has been deleted

[Kunihiko Sakamoto, 2015-06-19 05:56:12]

Patchset #1 (id:20001) has been deleted

[Kunihiko Sakamoto, 2015-06-19 06:33:03]

Patchset #1 (id:40001) has been deleted

[Kunihiko Sakamoto, 2015-06-19 08:08:57]

Patchset #1 (id:60001) has been deleted

[Kunihiko Sakamoto, 2015-06-19 09:34:09]

Patchset #1 (id:80001) has been deleted

[Kunihiko Sakamoto, 2015-06-19 09:40:45]

ksakamoto@chromium.org changed reviewers:
+ japhet@chromium.org, kinuko@chromium.org

[kinuko, 2015-06-22 07:01:01]

Sorry for my slow review.  The current plumbing is a bit unfortunate but this
change looks reasonable. (Also having the same method names on both class Foo
and FooClient feels a bit uncommon, though they need to chain through
anyways...)

https://codereview.chromium.org/1184403003/diff/100001/Source/core/fetch/Reso...
File Source/core/fetch/Resource.h (right):

https://codereview.chromium.org/1184403003/diff/100001/Source/core/fetch/Reso...
Source/core/fetch/Resource.h:200: virtual void reportResourceTiming(const
ResourceTimingInfo&) { }
Can we add a comment? It's not obvious whom this method reports the timing. (At
RawResource layer it's slightly more obvious since its client has corresponding
method but at Resource layer it looks a bit puzzling...)

https://codereview.chromium.org/1184403003/diff/100001/Source/platform/networ...
File Source/platform/network/ResourceTimingInfo.cpp (right):

https://codereview.chromium.org/1184403003/diff/100001/Source/platform/networ...
Source/platform/network/ResourceTimingInfo.cpp:27: data->m_type = m_type;
Looking into AtomicString.h I'm not fully sure which type operator is called.
Wouldn't it use the one that returns internal m_string? Should we explicitly
make an isolated copy if that's the case (e.g. m_type.string().isolatedCopy())?

https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

[Kunihiko Sakamoto, 2015-06-22 09:49:56]

Thanks for the review.

https://codereview.chromium.org/1184403003/diff/100001/Source/core/fetch/Reso...
File Source/core/fetch/Resource.h (right):

https://codereview.chromium.org/1184403003/diff/100001/Source/core/fetch/Reso...
Source/core/fetch/Resource.h:200: virtual void reportResourceTiming(const
ResourceTimingInfo&) { }
On 2015/06/22 07:01:00, kinuko wrote:
> Can we add a comment? It's not obvious whom this method reports the timing.
(At
> RawResource layer it's slightly more obvious since its client has
corresponding
> method but at Resource layer it looks a bit puzzling...)

Added a comment. Also renamed it to more verbose name,
reportResourceTimingToWorker, hoping to make it clearer.

https://codereview.chromium.org/1184403003/diff/100001/Source/platform/networ...
File Source/platform/network/ResourceTimingInfo.cpp (right):

https://codereview.chromium.org/1184403003/diff/100001/Source/platform/networ...
Source/platform/network/ResourceTimingInfo.cpp:27: data->m_type = m_type;
On 2015/06/22 07:01:00, kinuko wrote:
> Looking into AtomicString.h I'm not fully sure which type operator is called.
> Wouldn't it use the one that returns internal m_string? Should we explicitly
> make an isolated copy if that's the case (e.g.
m_type.string().isolatedCopy())?
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

Ah, good catch!
Done.

[kinuko, 2015-06-23 05:09:50]

Looking good. I'm still not 100% comfortable with the current plumbing, but we
could probably call for Nate's voice here.

https://codereview.chromium.org/1184403003/diff/140001/Source/core/fetch/Reso...
File Source/core/fetch/ResourceFetcher.cpp (right):

https://codereview.chromium.org/1184403003/diff/140001/Source/core/fetch/Reso...
Source/core/fetch/ResourceFetcher.cpp:816:
resource->reportResourceTimingToWorker(*info);
I'm wondering if we could rather make it general to both worker and document?
(From Resource's pov it's basically just about exposing resource timing info to
its client, it doesn't seem to need to know about Worker)

Putting aside all gotcha my preference is:

Resource has a method like: reportResourceTimingToClients()
  we could have a default impl which sends timing info to clients
ResourceClient has a method like: didReceiveResourceTiming()
  which could have a default impl for Document case
DocumentThreadableLoader overrides it

Also: please help me understand a bit more: for worker case is RawResource the
only resource we want to care about, or would we need to implement it in other
resource/resourceclient someday?

[Kunihiko Sakamoto, 2015-06-23 06:47:34]

https://codereview.chromium.org/1184403003/diff/140001/Source/core/fetch/Reso...
File Source/core/fetch/ResourceFetcher.cpp (right):

https://codereview.chromium.org/1184403003/diff/140001/Source/core/fetch/Reso...
Source/core/fetch/ResourceFetcher.cpp:816:
resource->reportResourceTimingToWorker(*info);
On 2015/06/23 05:09:50, kinuko wrote:
> I'm wondering if we could rather make it general to both worker and document?
> (From Resource's pov it's basically just about exposing resource timing info
to
> its client, it doesn't seem to need to know about Worker)

Yeah I tried to unify worker / document path, but couldn't come up with an
elegant way to do so.

> Putting aside all gotcha my preference is:
> 
> Resource has a method like: reportResourceTimingToClients()
>   we could have a default impl which sends timing info to clients
> ResourceClient has a method like: didReceiveResourceTiming()
>   which could have a default impl for Document case
> DocumentThreadableLoader overrides it

Thanks for the naming suggestion. Renamed the methods in that manner.
Now Resource doesn't care about worker, so reportResourceTimingToClients is
called unconditionally and only DocumentThreadableLoader handles it.

> Also: please help me understand a bit more: for worker case is RawResource the
> only resource we want to care about, or would we need to implement it in other
> resource/resourceclient someday?

We only need to care about RawResource as long as all worker-initiated requests
use DocumentThreadableLoader, since DocumentThreadableLoader treats every
resource as RawResource.

[kinuko, 2015-06-23 07:48:42]

Thanks, lgtm

https://codereview.chromium.org/1184403003/diff/160001/Source/core/fetch/Reso...
File Source/core/fetch/Resource.h (right):

https://codereview.chromium.org/1184403003/diff/160001/Source/core/fetch/Reso...
Source/core/fetch/Resource.h:200: // Reports resource timing to the worker
thread that initiated the request.
nit: please update the comment too.

[Kunihiko Sakamoto, 2015-06-23 08:05:08]

Nate, could you take a look?

https://codereview.chromium.org/1184403003/diff/160001/Source/core/fetch/Reso...
File Source/core/fetch/Resource.h (right):

https://codereview.chromium.org/1184403003/diff/160001/Source/core/fetch/Reso...
Source/core/fetch/Resource.h:200: // Reports resource timing to the worker
thread that initiated the request.
On 2015/06/23 07:48:41, kinuko wrote:
> nit: please update the comment too.

Removed it because now its name explains itself.

[Nate Chapin, 2015-06-23 18:17:17]

LGTM, though I don't think you have approval for Source/platform/ yet?

https://codereview.chromium.org/1184403003/diff/180001/Source/core/loader/Thr...
File Source/core/loader/ThreadableLoaderClient.h (right):

https://codereview.chromium.org/1184403003/diff/180001/Source/core/loader/Thr...
Source/core/loader/ThreadableLoaderClient.h:60: virtual void
didReceiveResourceTiming(const ResourceTimingInfo&) { }
I don't know the worker logic very well; is there a way to send a message to the
worker without going through ThreadableLoaderClient? This is ok, but the
plumbing does seem a little strange.

https://codereview.chromium.org/1184403003/diff/180001/Source/core/timing/Res...
File Source/core/timing/ResourceTimingInfo.h (left):

https://codereview.chromium.org/1184403003/diff/180001/Source/core/timing/Res...
Source/core/timing/ResourceTimingInfo.h:1: /*
Nit #1: This looks like it's blanking the file, rather than deleting it?

Nit #2: Please remove the rule for core/timing/ResourceTimingInfo.h in
Source/core/fetch/DEPS. Now that it is in platform/network/, it is no longer a
layering violation.

[Kunihiko Sakamoto, 2015-06-24 01:31:31]

ksakamoto@chromium.org changed reviewers:
+ tkent@chromium.org

[Kunihiko Sakamoto, 2015-06-24 01:31:32]

+tkent@

Kent-san, could you take a look for Source/platform?
This patch moves ResourceTimingInfo to platform/ to make it cross-thread
copyable.

https://codereview.chromium.org/1184403003/diff/180001/Source/core/loader/Thr...
File Source/core/loader/ThreadableLoaderClient.h (right):

https://codereview.chromium.org/1184403003/diff/180001/Source/core/loader/Thr...
Source/core/loader/ThreadableLoaderClient.h:60: virtual void
didReceiveResourceTiming(const ResourceTimingInfo&) { }
On 2015/06/23 18:17:17, Nate Chapin wrote:
> I don't know the worker logic very well; is there a way to send a message to
the
> worker without going through ThreadableLoaderClient? This is ok, but the
> plumbing does seem a little strange.

I agree this is a bit strange, but kinuko@ and I think there's no cleaner way...

https://codereview.chromium.org/1184403003/diff/180001/Source/core/timing/Res...
File Source/core/timing/ResourceTimingInfo.h (left):

https://codereview.chromium.org/1184403003/diff/180001/Source/core/timing/Res...
Source/core/timing/ResourceTimingInfo.h:1: /*
On 2015/06/23 18:17:17, Nate Chapin wrote:
> Nit #1: This looks like it's blanking the file, rather than deleting it?

It seems codereview marks a file 'M' when it is moved?
The patch header says "deleted file mode 100644", so I think I'm deleting it.

> Nit #2: Please remove the rule for core/timing/ResourceTimingInfo.h in
> Source/core/fetch/DEPS. Now that it is in platform/network/, it is no longer a
> layering violation.

Done.

[tkent, 2015-06-24 01:51:50]

Looking.

On 2015/06/24 01:31:32, Kunihiko Sakamoto wrote:
> Kent-san, could you take a look for Source/platform?
> This patch moves ResourceTimingInfo to platform/ to make it cross-thread
> copyable.

You don't need to move ResourceTimingInfo only for cross-thread copyable.
But the moving makes sense because it is network-related data.

[tkent, 2015-06-24 02:03:58]

https://codereview.chromium.org/1184403003/diff/200001/Source/platform/CrossT...
File Source/platform/CrossThreadCopier.h (right):

https://codereview.chromium.org/1184403003/diff/200001/Source/platform/CrossT...
Source/platform/CrossThreadCopier.h:140: template<> struct
CrossThreadCopierBase<false, false, false, ResourceTimingInfo> {
Please move this to ResourceTimingInfo.h.

https://codereview.chromium.org/1184403003/diff/200001/Source/platform/networ...
File Source/platform/network/ResourceTimingInfo.h (right):

https://codereview.chromium.org/1184403003/diff/200001/Source/platform/networ...
Source/platform/network/ResourceTimingInfo.h:100:
WTF_MAKE_NONCOPYABLE(CrossThreadResourceTimingInfoData);
WTF_MAKE_FAST_ALLOCATED(CrossThreadResourceTimingInfoData);
Do not write two expressions on one line.

https://codereview.chromium.org/1184403003/diff/200001/Source/platform/networ...
Source/platform/network/ResourceTimingInfo.h:102:
CrossThreadResourceTimingInfoData() { }
nit: Remove a space between { and }.

[Kunihiko Sakamoto, 2015-06-24 02:47:50]

https://codereview.chromium.org/1184403003/diff/200001/Source/platform/CrossT...
File Source/platform/CrossThreadCopier.h (right):

https://codereview.chromium.org/1184403003/diff/200001/Source/platform/CrossT...
Source/platform/CrossThreadCopier.h:140: template<> struct
CrossThreadCopierBase<false, false, false, ResourceTimingInfo> {
On 2015/06/24 02:03:58, tkent wrote:
> Please move this to ResourceTimingInfo.h.

Done.
(Moved definition of copy() to ResourceTimingInfo.cpp, as well)

https://codereview.chromium.org/1184403003/diff/200001/Source/platform/networ...
File Source/platform/network/ResourceTimingInfo.h (right):

https://codereview.chromium.org/1184403003/diff/200001/Source/platform/networ...
Source/platform/network/ResourceTimingInfo.h:100:
WTF_MAKE_NONCOPYABLE(CrossThreadResourceTimingInfoData);
WTF_MAKE_FAST_ALLOCATED(CrossThreadResourceTimingInfoData);
On 2015/06/24 02:03:58, tkent wrote:
> Do not write two expressions on one line.

Done.

https://codereview.chromium.org/1184403003/diff/200001/Source/platform/networ...
Source/platform/network/ResourceTimingInfo.h:102:
CrossThreadResourceTimingInfoData() { }
On 2015/06/24 02:03:58, tkent wrote:
> nit: Remove a space between { and }.

Done.

[Kunihiko Sakamoto, 2015-06-24 02:58:29]

https://codereview.chromium.org/1184403003/diff/200001/Source/platform/CrossT...
File Source/platform/CrossThreadCopier.h (right):

https://codereview.chromium.org/1184403003/diff/200001/Source/platform/CrossT...
Source/platform/CrossThreadCopier.h:140: template<> struct
CrossThreadCopierBase<false, false, false, ResourceTimingInfo> {
On 2015/06/24 02:47:50, Kunihiko Sakamoto wrote:
> (Moved definition of copy() to ResourceTimingInfo.cpp, as well)
...and made it inline.

[tkent, 2015-06-24 03:04:59]

lgtm

https://codereview.chromium.org/1184403003/diff/240001/Source/platform/networ...
File Source/platform/network/ResourceTimingInfo.cpp (right):

https://codereview.chromium.org/1184403003/diff/240001/Source/platform/networ...
Source/platform/network/ResourceTimingInfo.cpp:26:
OwnPtr<CrossThreadResourceTimingInfoData> data =  adoptPtr(new
CrossThreadResourceTimingInfoData);
Remove one space after '='

[Kunihiko Sakamoto, 2015-06-24 04:10:34]

Thanks for the review!

https://codereview.chromium.org/1184403003/diff/240001/Source/platform/networ...
File Source/platform/network/ResourceTimingInfo.cpp (right):

https://codereview.chromium.org/1184403003/diff/240001/Source/platform/networ...
Source/platform/network/ResourceTimingInfo.cpp:26:
OwnPtr<CrossThreadResourceTimingInfoData> data =  adoptPtr(new
CrossThreadResourceTimingInfoData);
On 2015/06/24 03:04:59, tkent wrote:
> Remove one space after '='

Done.

[Kunihiko Sakamoto, 2015-06-24 04:10:46]

The CQ bit was checked by ksakamoto@chromium.org

[Kunihiko Sakamoto, 2015-06-24 04:10:46]

The patchset sent to the CQ was uploaded after l-g-t-m from kinuko@chromium.org,
japhet@chromium.org, tkent@chromium.org
Link to the patchset: https://codereview.chromium.org/1184403003/#ps260001
(title: "style fix")

[commit-bot: I haz the power, 2015-06-24 04:11:16]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1184403003/260001

[commit-bot: I haz the power, 2015-06-24 05:29:10]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-06-24 05:29:11]

Try jobs failed on following builders:
  mac_blink_rel on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/60506)

[Kunihiko Sakamoto, 2015-06-24 05:36:53]

The CQ bit was checked by ksakamoto@chromium.org

[commit-bot: I haz the power, 2015-06-24 05:37:09]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1184403003/260001

[commit-bot: I haz the power, 2015-06-24 06:19:02]

Committed patchset #9 (id:260001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=197707