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.
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)
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.
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?
I think this generally looks fine. The comments above are all somewhat minor.
Created attachment 173895 [details] Patch
Created attachment 173897 [details] Patch
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.
Created attachment 173900 [details] Patch
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.
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..
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.
Created attachment 173917 [details] Patch
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.
Comment on attachment 173917 [details] Patch Thanks for iterating on this patch.
Comment on attachment 173917 [details] Patch Clearing flags on attachment: 173917 Committed r134442: <http://trac.webkit.org/changeset/134442>
All reviewed patches have been landed. Closing bug.
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.
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.
I think this patch has broken the world. CachedResourceRequestInitiators.h doesn't appear to be where it's expected to be.
(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
Re-opened since this is blocked by bug 102111
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.
> Okay, problems in the xcode project: It's probably worth rolling out anyway given that there are some comments above about the patch.
Most probably the error is because of my manual xcodeproj additions -> will fix that in the new version of the patch.
Created attachment 174135 [details] Patch
Created attachment 174182 [details] Patch
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.)
Comment on attachment 174182 [details] Patch Attachment 174182 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/14846055
Comment on attachment 174182 [details] Patch Attachment 174182 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/14832627
Created attachment 174440 [details] Patch
Comment on attachment 174440 [details] Patch The bots are green now.
Comment on attachment 174440 [details] Patch Clearing flags on attachment: 174440 Committed r134930: <http://trac.webkit.org/changeset/134930>
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?
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).