Prepare to add more initiator info to CachedResource(Request)

1) Add CachedResourceInitiatorInfo to CachedResourceRequest
2) Explicitly supply all CachedResourceRequest with initiator
3) Remove initiatorElement from CachedResourceRequest

BUG=

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

[abarth-chromium, 2013-05-15 22:05:24]

LGTM!  Thanks for the CL.

The initiatorElement API isn't used in Chromium?

[Nate Chapin, 2013-05-15 22:14:57]

https://codereview.chromium.org/14672042/diff/1/Source/core/loader/cache/Cach...
File Source/core/loader/cache/CachedResource.h (right):

https://codereview.chromium.org/14672042/diff/1/Source/core/loader/cache/Cach...
Source/core/loader/cache/CachedResource.h:255: void setInitiatorInfo(const
CachedResourceInitiatorInfo& initiatorInfo) { m_initiatorInfo = initiatorInfo; }
What is the rationale for moving this here from CachedResourceLoader? Not
opposed, just want to make sure I understand.

[abarth-chromium, 2013-05-15 22:21:25]

https://codereview.chromium.org/14672042/diff/1/Source/core/loader/cache/Cach...
File Source/core/loader/cache/CachedResource.h (right):

https://codereview.chromium.org/14672042/diff/1/Source/core/loader/cache/Cach...
Source/core/loader/cache/CachedResource.h:255: void setInitiatorInfo(const
CachedResourceInitiatorInfo& initiatorInfo) { m_initiatorInfo = initiatorInfo; }
On 2013/05/15 22:14:58, Nate Chapin wrote:
> What is the rationale for moving this here from CachedResourceLoader? Not
> opposed, just want to make sure I understand.

Previously CachedResourceLoader kept a HashMap keyed off CachedResource.  Given
that all CachedResources are initiated by something, that meant we had a HashMap
entry for all of them.  It's more efficient and more sensible to just store the
information in the CachedResource itself instead of in a look-aside HashMap.

[James Simonsen, 2013-05-16 13:28:44]

https://codereview.chromium.org/14672042/diff/8001/Source/core/loader/cache/C...
File Source/core/loader/cache/CachedResourceLoader.cpp (right):

https://codereview.chromium.org/14672042/diff/8001/Source/core/loader/cache/C...
Source/core/loader/cache/CachedResourceLoader.cpp:582:
resource->setInitiatorInfo(info);
The initiator info is different depending on which document is requesting the
resource. I think it'll be awkward to keep track of that inside the resource.
That's why we put the map on the CachedResourceLoader.

[eustas, 2013-05-16 14:26:53]

https://codereview.chromium.org/14672042/diff/8001/Source/core/loader/cache/C...
File Source/core/loader/cache/CachedResourceLoader.cpp (right):

https://codereview.chromium.org/14672042/diff/8001/Source/core/loader/cache/C...
Source/core/loader/cache/CachedResourceLoader.cpp:582:
resource->setInitiatorInfo(info);
On 2013/05/16 13:28:54, James Simonsen wrote:
> The initiator info is different depending on which document is requesting the
> resource. I think it'll be awkward to keep track of that inside the resource.
> That's why we put the map on the CachedResourceLoader.

I see that CachedResource initiatorInfo is set once per load and used only once
for performance timing.

Perhaps initiatorInfo should be removed when CachedResource is loaded.

Performance timing is reported only for loaded entries, not for taken from
cache.

So there is no need to keep initiatorInfo in CachedResourceLoader.

[James Simonsen, 2013-05-16 14:34:56]

We need to handle the case where two documents request the same resource at
around the same time and the resource is loaded from network.
On May 16, 2013 7:26 AM, <eustas@chromium.org> wrote:

>
> https://codereview.chromium.**org/14672042/diff/8001/Source/**
>
core/loader/cache/**CachedResourceLoader.cpp<https://codereview.chromium.org/14672042/diff/8001/Source/core/loader/cache/CachedResourceLoader.cpp>
> File Source/core/loader/cache/**CachedResourceLoader.cpp (right):
>
> https://codereview.chromium.**org/14672042/diff/8001/Source/**
>
core/loader/cache/**CachedResourceLoader.cpp#**newcode582<https://codereview.chromium.org/14672042/diff/8001/Source/core/loader/cache/CachedResourceLoader.cpp#newcode582>
> Source/core/loader/cache/**CachedResourceLoader.cpp:582:
> resource->setInitiatorInfo(**info);
> On 2013/05/16 13:28:54, James Simonsen wrote:
>
>> The initiator info is different depending on which document is
>>
> requesting the
>
>> resource. I think it'll be awkward to keep track of that inside the
>>
> resource.
>
>> That's why we put the map on the CachedResourceLoader.
>>
>
> I see that CachedResource initiatorInfo is set once per load and used
> only once for performance timing.
>
> Perhaps initiatorInfo should be removed when CachedResource is loaded.
>
> Performance timing is reported only for loaded entries, not for taken
> from cache.
>
> So there is no need to keep initiatorInfo in CachedResourceLoader.
>
>
https://codereview.chromium.**org/14672042/<https://codereview.chromium.org/1...
>

[eustas, 2013-05-16 16:08:11]

On 2013/05/16 14:34:56, James Simonsen wrote:
> We need to handle the case where two documents request the same resource at
> around the same time and the resource is loaded from network.

That is not a problem: we could modify "notifyLoadedFromMemoryCache"
to report performance timing with appropriate intiatorInfo.

[James Simonsen, 2013-05-16 16:18:14]

That's fine, but you need to keep track of the separate initiator
information for each of the documents.


On Thu, May 16, 2013 at 9:08 AM, <eustas@chromium.org> wrote:

> On 2013/05/16 14:34:56, James Simonsen wrote:
>
>> We need to handle the case where two documents request the same resource
>> at
>> around the same time and the resource is loaded from network.
>>
>
> That is not a problem: we could modify "notifyLoadedFromMemoryCache"
> to report performance timing with appropriate intiatorInfo.
>
>
https://codereview.chromium.**org/14672042/<https://codereview.chromium.org/1...
>

[abarth-chromium, 2013-05-16 16:52:47]

On 2013/05/16 16:18:14, James Simonsen wrote:
> That's fine, but you need to keep track of the separate initiator
> information for each of the documents.

I see, you're saying that the map is useful because it's per-Document.

[abarth-chromium, 2013-05-16 17:01:41]

Ok, sounds like we should keep the storage in CachedResourceLoader then, but I
like the rest of the CL.  :)

[James Simonsen, 2013-05-16 17:06:01]

Yes, exactly.


On Thu, May 16, 2013 at 9:52 AM, <abarth@chromium.org> wrote:

> On 2013/05/16 16:18:14, James Simonsen wrote:
>
>> That's fine, but you need to keep track of the separate initiator
>> information for each of the documents.
>>
>
> I see, you're saying that the map is useful because it's per-Document.
>
>
https://codereview.chromium.**org/14672042/<https://codereview.chromium.org/1...
>

[eustas, 2013-05-17 08:26:46]

As I see there are 3 scenarios:
1) resource request initiates load
2) resource has already been requested
3) resource has already been loaded

Currently, only scenario (1) leads to resource timing reporting.

I have an idea to use CachedResourceClient for that purpose. This will make
reporting uniform and this way we do not need to hold initiator map in
CachedResourceLoader.

What do you think about it?

On 2013/05/16 16:18:14, James Simonsen wrote:
> That's fine, but you need to keep track of the separate initiator
> information for each of the documents.

[James Simonsen, 2013-05-17 13:59:34]

It might be worth reading the latest draft of the Resource Timing spec.
AIUI, the scenarios are:

1. The resource is loaded from network. Show all timing data.

2. The resource has already been requested in another document and this
document's request is waiting for that to complete. Show the timing data
starting at the point where this document made the request. So if the other
document's request has already made the DNS lookup, don't report any DNS
time in this document's request.

3. The resource is loaded from cache and it's the first time this resource
is used in this document. Show timing data for the cache lookup.

4. The resource is loaded from cache and we've already reported it in
scenarios 1-3. Don't report timing data.

In all cases, the initiator time and element must be from the current
document. We shouldn't leak that another document requested the same
resource at a specific time or as a different type.

I don't think we match this latest version of the spec. If you can come up
with something that brings us up to date, that'd be great.


On Fri, May 17, 2013 at 1:26 AM, <eustas@chromium.org> wrote:

> As I see there are 3 scenarios:
> 1) resource request initiates load
> 2) resource has already been requested
> 3) resource has already been loaded
>
> Currently, only scenario (1) leads to resource timing reporting.
>
> I have an idea to use CachedResourceClient for that purpose. This will make
> reporting uniform and this way we do not need to hold initiator map in
> CachedResourceLoader.
>
> What do you think about it?
>
>
> On 2013/05/16 16:18:14, James Simonsen wrote:
>
>> That's fine, but you need to keep track of the separate initiator
>> information for each of the documents.
>>
>
>
https://codereview.chromium.**org/14672042/<https://codereview.chromium.org/1...
>

[commit-bot: I haz the power, 2013-05-20 06:03:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/eustas@chromium.org/14672042/27001

[commit-bot: I haz the power, 2013-05-20 06:48:55]

Change committed as 150651