Bug 101935 – Add initiator to CachedResourceRequest.

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Marja Hölttä]

This is needed for bug 92761 and for bug 84883.

This differs from simonjam's approach in bug 101300: We don't store a CachedResourceRequest anywhere, so it doesn't store references to the initiator objects.

[Marja Hölttä]

Created attachment 173641 [details]
Patch

Comment on attachment 173641 [details]
Patch

What are you gonna do about CachedResourceRequests created by the ThreadableLoader?

View in context: https://bugs.webkit.org/attachment.cgi?id=173641&action=review

> Source/WebCore/ChangeLog:8
> +        Motivation: at least one embedder needs to know which elements request a

uh, which embedder might that be?

> Source/WebCore/css/CSSFontFaceSrcValue.cpp:100
> +        request.setInitiator("css", document);

should stuff like "css" be an AtomicString (see e.g. dom/EventNames.h)

[Adam Barth]

Comment on attachment 173641 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=173641&action=review

> Source/WebCore/css/CSSImageValue.h:70
> +    RefPtr<Element> m_initiatorElement;

Are we worried about the amount of extra storage this requires?  A document might have many CSSImageValues...

> Source/WebCore/html/parser/HTMLPreloadScanner.cpp:146
> +    RefPtr<Document> m_document;

I don't understand why you made these changes.  It seems like we already had document in preload().  There doesn't seem to be a reason to ref the document here.

[Adam Barth]

Comment on attachment 173641 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=173641&action=review

Should we add an ASSERT somewhere that we always set an initiator?

> Source/WebCore/loader/cache/CachedResourceRequest.h:74
> +    RefPtr<Element> m_initiatorElement;
> +    RefPtr<Document> m_initiatorDocument;

Can both of these be non-0 at the same time?

[Adam Barth]

I think this generally looks fine.  The comments above are all somewhat minor.

[Marja Hölttä]

Created attachment 173895 [details]
Patch

[Marja Hölttä]

Created attachment 173897 [details]
Patch

[WebKit Review Bot]

Attachment 173897 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/CMakeLists.txt', u'Source/W..." exit_code: 1
Source/WebCore/platform/ThreadGlobalData.h:44:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1 in 35 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Marja Hölttä]

Created attachment 173900 [details]
Patch

[Marja Hölttä]

Comment on attachment 173641 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=173641&action=review

>> Source/WebCore/ChangeLog:8
>> +        Motivation: at least one embedder needs to know which elements request a
> 
> uh, which embedder might that be?

Changed to "Chromium needs to know".

>> Source/WebCore/css/CSSFontFaceSrcValue.cpp:100
>> +        request.setInitiator("css", document);
> 
> should stuff like "css" be an AtomicString (see e.g. dom/EventNames.h)

Done.

>> Source/WebCore/css/CSSImageValue.h:70
>> +    RefPtr<Element> m_initiatorElement;
> 
> Are we worried about the amount of extra storage this requires?  A document might have many CSSImageValues...

Removed m_initiatorElement here; passing it to CSSImageValue::cachedImage as a parameter instead. (CSSCrossFaderValue & CSSCursorImageValue pass 0.)

>> Source/WebCore/html/parser/HTMLPreloadScanner.cpp:146
>> +    RefPtr<Document> m_document;
> 
> I don't understand why you made these changes.  It seems like we already had document in preload().  There doesn't seem to be a reason to ref the document here.

Yes, this was wrong -> cancelled the changes to add m_document.

>> Source/WebCore/loader/cache/CachedResourceRequest.h:74
>> +    RefPtr<Document> m_initiatorDocument;
> 
> Can both of these be non-0 at the same time?

There are asserts in the setInitiator functions which make sure that setInitiator (either version) is called only once, and that only one of these will be non-0.

[Marja Hölttä]

Thanks for comments!

abarth, can you have a look at the new patch?

I created a meta bug, bug 102064 for tracking the different phases of this work. I can split this CL further if you'd like..

[Adam Barth]

Comment on attachment 173900 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=173900&action=review

This looks good.  Some nits below.  My main concern here is about maintenance.  We want to make sure this code is well-tested so that the initiator doesn't become copy pasta.  I appreciate that you've broken this work up into small patches, but I would like to get to the point where we can test these changes sooner rather than later.  :)

> Source/WebCore/ChangeLog:5
> +

You should leave the Reviewed by NODBODY (OPPS!) line in the ChangeLog entry.  The bots use that to know where to insert the name of the person who actually reviewed the patch.

> Source/WebCore/css/CSSCursorImageValue.cpp:142
> -    return CSSImageValue::cachedImage(loader, url());
> +    return CSSImageValue::cachedImage(loader, url(), 0);

In these cases, we won't have initiators?  Is that a problem?

> Source/WebCore/css/CSSImageValue.cpp:92
> +            request.setInitiator(cachedResourceRequestInitiators().css, loader->document());

Oh, I see.  It defaults to the document.  In that case, I might add a default argument of 0 so that the callers don't have this funny 0 hanging around.

> Source/WebCore/css/WebKitCSSSVGDocumentValue.cpp:57
> +        request.setInitiator("css", loader->document());

"css" -> cachedResourceRequestInitiators().css

> Source/WebCore/loader/cache/CachedResourceRequest.cpp:73
> +void CachedResourceRequest::setInitiator(AtomicString name, PassRefPtr<Document> document)

AtomicString -> const AtomicString& (that avoid churning the reference count)

> Source/WebCore/platform/ThreadGlobalData.cpp:32
> +#include "CachedResourceRequestInitiators.h"
>  #include "DOMImplementation.h"
>  #include "EventNames.h"

Technically these are all layering violations, but I'm not sure we have a plan for solving this problem without this layering violation.

[Marja Hölttä]

Created attachment 173917 [details]
Patch

[Marja Hölttä]

Comment on attachment 173900 [details]
Patch

Thanks for comments; the new patch set addresses them.

View in context: https://bugs.webkit.org/attachment.cgi?id=173900&action=review

>> Source/WebCore/ChangeLog:5
>> +
> 
> You should leave the Reviewed by NODBODY (OPPS!) line in the ChangeLog entry.  The bots use that to know where to insert the name of the person who actually reviewed the patch.

Done (oops).

>> Source/WebCore/css/CSSImageValue.cpp:92
>> +            request.setInitiator(cachedResourceRequestInitiators().css, loader->document());
> 
> Oh, I see.  It defaults to the document.  In that case, I might add a default argument of 0 so that the callers don't have this funny 0 hanging around.

Done.

>> Source/WebCore/css/WebKitCSSSVGDocumentValue.cpp:57
>> +        request.setInitiator("css", loader->document());
> 
> "css" -> cachedResourceRequestInitiators().css

Done.

>> Source/WebCore/loader/cache/CachedResourceRequest.cpp:73
>> +void CachedResourceRequest::setInitiator(AtomicString name, PassRefPtr<Document> document)
> 
> AtomicString -> const AtomicString& (that avoid churning the reference count)

Done.

[Adam Barth]

Comment on attachment 173917 [details]
Patch

Thanks for iterating on this patch.

[WebKit Review Bot]

Comment on attachment 173917 [details]
Patch

Clearing flags on attachment: 173917

Committed r134442: <http://trac.webkit.org/changeset/134442>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

Comment on attachment 173917 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=173917&action=review

Architecturally, I’m surprised that tag names are a suitable way to represent the initiator.

> Source/WebCore/loader/cache/CachedResourceRequestInitiators.h:34
> +class CachedResourceRequestInitiators {

I don’t understand why this class is named “initiators”. Do objects of this class represent multiple “initiators”? Could you explain further?

> Source/WebCore/loader/cache/CachedResourceRequestInitiators.h:38
> +public:
> +    AtomicString css;
> +    AtomicString icon;

Normally a class with public data members would be defined as a “struct” rather than “class” in WebKit.

If these data members are initialized in the constructor then maybe they should be const AtomicString instead of AtomicString?

> Source/WebCore/loader/cache/CachedResourceRequestInitiators.h:44
> +inline CachedResourceRequestInitiators& cachedResourceRequestInitiators()

Why does this return a non-const references? I don’t understand why this object is both global and mutable. This seems a different approach than what we’ve done for other global string constants such as HTML attribute names, and I don’t understand the reason for a different idiom.

[Marja Hölttä]

Comment on attachment 173917 [details]
Patch

If you'd like me to do the changes below, I could create a follow-up patch tomorrow (UTC...).

View in context: https://bugs.webkit.org/attachment.cgi?id=173917&action=review

>> Source/WebCore/loader/cache/CachedResourceRequestInitiators.h:34
>> +class CachedResourceRequestInitiators {
> 
> I don’t understand why this class is named “initiators”. Do objects of this class represent multiple “initiators”? Could you explain further?

It could've also been called CachedResourceRequestInitiatorNames (like EventNames), but it's quite long.. Do you mean the plural (why not CachedResourceRequestInitiator instead of CachedResourceRequestInitiators) or the name in general?

>> Source/WebCore/loader/cache/CachedResourceRequestInitiators.h:38
>> +    AtomicString icon;
> 
> Normally a class with public data members would be defined as a “struct” rather than “class” in WebKit.
> 
> If these data members are initialized in the constructor then maybe they should be const AtomicString instead of AtomicString?

Copied from EventNames, but I can change them to be const AtomicString.

>> Source/WebCore/loader/cache/CachedResourceRequestInitiators.h:44
>> +inline CachedResourceRequestInitiators& cachedResourceRequestInitiators()
> 
> Why does this return a non-const references? I don’t understand why this object is both global and mutable. This seems a different approach than what we’ve done for other global string constants such as HTML attribute names, and I don’t understand the reason for a different idiom.

Also copied from EventNames, but I can change this, too.

[Oliver Hunt]

I think this patch has broken the world. CachedResourceRequestInitiators.h doesn't appear to be where it's expected to be.

[Adam Barth]

(In reply to comment #20)
> I think this patch has broken the world. CachedResourceRequestInitiators.h doesn't appear to be where it's expected to be.

rolling out now

[WebKit Review Bot]

Re-opened since this is blocked by bug 102111

[Oliver Hunt]

Okay, problems in the xcode project:
CachedScript.cpp was dropped from the project
CachedResourceRequestInitiators.{h,cpp} -- I have no idea what happened here, occasionally the hashes are identical, occasionally they mismatch.

[Adam Barth]

> Okay, problems in the xcode project:

It's probably worth rolling out anyway given that there are some comments above about the patch.

[Marja Hölttä]

Most probably the error is because of my manual xcodeproj additions -> will fix that in the new version of the patch.

[Marja Hölttä]

Created attachment 174135 [details]
Patch

[Marja Hölttä]

Created attachment 174182 [details]
Patch

[Marja Hölttä]

Comment on attachment 174182 [details]
Patch

This patch fixes the xcode build.

Also, changed the CachedResourceRequestInitiators according to Darin's suggestions: it's a struct now, ThreadGlobalData returns a const reference, and the AtomicStrings are const. (Kept the name the same; not sure what do to there.)

[Build Bot]

Comment on attachment 174182 [details]
Patch

Attachment 174182 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/14846055

[Build Bot]

Comment on attachment 174182 [details]
Patch

Attachment 174182 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14832627

[Marja Hölttä]

Created attachment 174440 [details]
Patch

[Marja Hölttä]

Comment on attachment 174440 [details]
Patch

The bots are green now.

[WebKit Review Bot]

Comment on attachment 174440 [details]
Patch

Clearing flags on attachment: 174440

Committed r134930: <http://trac.webkit.org/changeset/134930>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[James Simonsen]

Comment on attachment 174440 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=174440&action=review

> Source/WebCore/css/CSSImageValue.cpp:90
> +            request.setInitiator(initiatorElement);

For Resource Timing, the spec says the initiator should be "css" here. Do you need it to be the element instead for some reason?

[Marja Hölttä]

Comment on attachment 174440 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=174440&action=review

>> Source/WebCore/css/CSSImageValue.cpp:90
>> +            request.setInitiator(initiatorElement);
> 
> For Resource Timing, the spec says the initiator should be "css" here. Do you need it to be the element instead for some reason?

Afaics not. This behavior was just copied from your patch (bug 101300).