Clean up CachedResourceRequest initiators
Created attachment 203592 [details] Patch
Comment on attachment 203592 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=203592&action=review > Source/WebCore/ChangeLog:22 > + the request's initiator, as compared to the previous possibility of defining the initiator through an AtomicString or an Element > + object's name (which is not in line with the specification and therefor removed). Oh, I didn't realize that this could be a name. Looking at the current editor's draft <http://www.w3c-test.org/webperf/specs/ResourceTiming/>, the feature is there: initiatorType attribute If the initiator is an element, on getting, the initiatorType attribute must return a DOMString with the same value as the localName of that element. If the initiator is a CSS resource downloaded by the url() syntax, such as @import url() or background: url(), on getting, the initiatorType attribute must return the DOMString "css". If the initiator is an XMLHttpRequest object, on getting, the initiatorType attribute must return the DOMString "xmlhttprequest".
Hmm. I was looking at this one, which is marked as a Candidate Recommendation. http://www.w3.org/TR/resource-timing/
Comment on attachment 203592 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=203592&action=review I’m OK with this change, but think it could be better. > Source/WebCore/html/parser/HTMLPreloadScanner.cpp:102 > + default: Adding this default case gets rid of the compile time checking that we have all the tag values covered. Could you undo that change? > Source/WebCore/loader/cache/CachedResourceRequestInitiator.h:30 > +#include "ThreadGlobalData.h" > +#include <wtf/text/AtomicString.h> This file should not be including these headers. > Source/WebCore/loader/cache/CachedResourceRequestInitiator.h:34 > +enum CachedResourceRequestInitiator { Can we use a shorter name for this class and this source file? I suggest CacheRequestInitiator or LoadInitiator or LoadInitiatorType. > Source/WebCore/page/PerformanceResourceTiming.cpp:105 > + DEFINE_STATIC_LOCAL(const String, cssName, (ASCIILiteral("css"))); > + DEFINE_STATIC_LOCAL(const String, embedName, (ASCIILiteral("embed"))); > + DEFINE_STATIC_LOCAL(const String, imageName, (ASCIILiteral("img"))); > + DEFINE_STATIC_LOCAL(const String, linkName, (ASCIILiteral("link"))); > + DEFINE_STATIC_LOCAL(const String, objectName, (ASCIILiteral("object"))); > + DEFINE_STATIC_LOCAL(const String, scriptName, (ASCIILiteral("script"))); > + DEFINE_STATIC_LOCAL(const String, subdocumentName, (ASCIILiteral("subdocument"))); > + DEFINE_STATIC_LOCAL(const String, svgName, (ASCIILiteral("svg"))); > + DEFINE_STATIC_LOCAL(const String, xmlhttprequestName, (ASCIILiteral("xmlhttprequest"))); > + DEFINE_STATIC_LOCAL(const String, otherName, (ASCIILiteral("other"))); Is having a global for each of these an important optimization?
Comment on attachment 203592 [details] Patch My comment was confusing, and I really should have r-'ed the patch. Unless there is a specific reason why we disagree with current editor's draft, and are trying to get it fixed, this is really the version we should be targeting. So, removing support for something that's in the draft would be a step in the wrong direction.
Agreed, I'll produce a patch based on the editor's draft.
Created attachment 204120 [details] Patch
Attachment 204120 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/http/tests/webperf/resource-timing/initiator-type-css-expected.txt', u'LayoutTests/http/tests/webperf/resource-timing/initiator-type-css.html', u'LayoutTests/http/tests/webperf/resource-timing/initiator-type-iframe-expected.txt', u'LayoutTests/http/tests/webperf/resource-timing/initiator-type-iframe.html', u'LayoutTests/http/tests/webperf/resource-timing/initiator-type-img-expected.txt', u'LayoutTests/http/tests/webperf/resource-timing/initiator-type-img.html', u'LayoutTests/http/tests/webperf/resource-timing/initiator-type-link-expected.txt', u'LayoutTests/http/tests/webperf/resource-timing/initiator-type-link.html', u'LayoutTests/http/tests/webperf/resource-timing/initiator-type-script-expected.txt', u'LayoutTests/http/tests/webperf/resource-timing/initiator-type-script.html', u'LayoutTests/http/tests/webperf/resource-timing/initiator-type-xmlhttprequest-expected.txt', u'LayoutTests/http/tests/webperf/resource-timing/initiator-type-xmlhttprequest.html', u'LayoutTests/http/tests/webperf/resources/abe.png', u'LayoutTests/http/tests/webperf/resources/iframe.html', u'LayoutTests/platform/efl/TestExpectations', u'LayoutTests/platform/mac/TestExpectations', u'LayoutTests/platform/qt/TestExpectations', u'LayoutTests/platform/win/TestExpectations', u'Source/WebCore/CMakeLists.txt', u'Source/WebCore/ChangeLog', u'Source/WebCore/GNUmakefile.list.am', u'Source/WebCore/Target.pri', u'Source/WebCore/WebCore.vcxproj/WebCore.vcxproj', u'Source/WebCore/WebCore.vcxproj/WebCore.vcxproj.filters', u'Source/WebCore/WebCore.xcodeproj/project.pbxproj', u'Source/WebCore/css/CSSFontFaceSrcValue.cpp', u'Source/WebCore/css/CSSImageSetValue.cpp', u'Source/WebCore/css/CSSImageValue.cpp', u'Source/WebCore/css/CSSImageValue.h', u'Source/WebCore/css/StyleRuleImport.cpp', u'Source/WebCore/css/WebKitCSSSVGDocumentValue.cpp', u'Source/WebCore/css/WebKitCSSShaderValue.cpp', u'Source/WebCore/dom/ScriptElement.cpp', u'Source/WebCore/html/HTMLBodyElement.cpp', u'Source/WebCore/html/HTMLLinkElement.cpp', u'Source/WebCore/html/parser/CSSPreloadScanner.cpp', u'Source/WebCore/html/parser/HTMLPreloadScanner.cpp', u'Source/WebCore/html/parser/HTMLResourcePreloader.h', u'Source/WebCore/loader/DocumentThreadableLoader.cpp', u'Source/WebCore/loader/ImageLoader.cpp', u'Source/WebCore/loader/LinkLoader.cpp', u'Source/WebCore/loader/ThreadableLoader.cpp', u'Source/WebCore/loader/ThreadableLoader.h', u'Source/WebCore/loader/cache/CachedResourceLoader.cpp', u'Source/WebCore/loader/cache/CachedResourceLoader.h', u'Source/WebCore/loader/cache/CachedResourceRequest.cpp', u'Source/WebCore/loader/cache/CachedResourceRequest.h', u'Source/WebCore/loader/cache/CachedResourceRequestInitiator.h', u'Source/WebCore/loader/cache/CachedResourceRequestInitiators.cpp', u'Source/WebCore/loader/cache/CachedResourceRequestInitiators.h', u'Source/WebCore/loader/icon/IconLoader.cpp', u'Source/WebCore/page/Performance.cpp', u'Source/WebCore/page/Performance.h', u'Source/WebCore/page/PerformanceResourceTiming.cpp', u'Source/WebCore/page/PerformanceResourceTiming.h', u'Source/WebCore/platform/ThreadGlobalData.cpp', u'Source/WebCore/platform/ThreadGlobalData.h', u'Source/WebCore/svg/SVGFEImageElement.cpp', u'Source/WebCore/svg/SVGFontFaceUriElement.cpp', u'Source/WebCore/svg/SVGUseElement.cpp', u'Source/WebCore/xml/XMLHttpRequest.cpp']" exit_code: 1 Source/WebCore/loader/cache/CachedResourceLoader.h:183: Missing space inside { }. [whitespace/braces] [5] Source/WebCore/loader/cache/CachedResourceRequestInitiator.h:38: enum members should use InterCaps with an initial capital letter. [readability/enum_casing] [4] Total errors found: 2 in 55 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 204120 [details] Patch Attachment 204120 [details] did not pass qt-ews (qt): Output: http://webkit-queues.appspot.com/results/786220
Comment on attachment 204120 [details] Patch Attachment 204120 [details] did not pass qt-wk2-ews (qt-wk2): Output: http://webkit-queues.appspot.com/results/697909
What's happening with this patch? Has the draft been finalized? Should we take this patch? (Alexey? Darin?)
Comment on attachment 204120 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=204120&action=review Generally looks good, but I’m not sure this patch is a big improvement. Replacing the concept "initiator type" with a new concept "initiator" is not so great, since the data is actually the type of initiator, not the initiator itself. And moving the code to extract the local name from a central place and instead doing it at each call site isn’t an obvious improvement either. Using hard-coded strings for "icon" and "link" at the call site also isn’t really an improvement. > Source/WebCore/dom/ScriptElement.cpp:271 > + request.setInitiator(String(m_element->localName())); This is not the preferred syntax. Instead we should use localName().string(). > Source/WebCore/html/HTMLLinkElement.cpp:221 > + request.setInitiator(String(localName())); Ditto. > Source/WebCore/html/parser/HTMLPreloadScanner.cpp:102 > - ASSERT_NOT_REACHED(); > - return "unknown"; > + break; Why this change? > Source/WebCore/html/parser/HTMLPreloadScanner.cpp:104 > + Why this change? I don’t think we need to touch this file in this patch. > Source/WebCore/html/parser/HTMLResourcePreloader.h:31 > +#include <wtf/WeakPtr.h> Why are we adding this include? I don’t see any new use of WeakPtr in this header. > Source/WebCore/loader/ImageLoader.cpp:184 > + request.setInitiator(String(m_element->localName())); Please use localName().string() here instead as above. > Source/WebCore/loader/LinkLoader.cpp:119 > + // FIXME: Should use the HTMLLinkElement::localName. Not sure calling through HTMLLinkElement is really important. We can use HTMLNames::linkTag.string() here instead of writing it as a literal. > Source/WebCore/loader/LinkLoader.cpp:120 > + linkRequest.setInitiator(String("link")); This should either be just "link" or ASCIILiteral("link"). Not String("link"). > Source/WebCore/loader/cache/CachedResourceLoader.cpp:559 > + InitiatorInfo info(String(frame()->ownerElement()->localName()), monotonicallyIncreasingTime()); Again, should use localName().string(). Why change to the old function-style initialization instead of the modern C++ style? I like the new style better; it looks more like initialization to me and is stricter about type conversions. > Source/WebCore/loader/cache/CachedResourceLoader.cpp:563 > - InitiatorInfo info = { request.initiatorName(), monotonicallyIncreasingTime() }; > + InitiatorInfo info(request.initiator(), monotonicallyIncreasingTime()); Why change to the old function-style initialization instead of the modern C++ style? I like the new style better; it looks more like initialization to me and is stricter about type conversions. > Source/WebCore/loader/cache/CachedResourceLoader.h:181 > - struct InitiatorInfo { > - AtomicString name; > - double startTime; > + class InitiatorInfo { Why change this from a struct to a class? > Source/WebCore/loader/cache/CachedResourceRequestInitiator.h:53 > + CachedResourceRequestInitiator(const String& initiatorName) > + : m_type(Element) > + , m_initiatorName(initiatorName) > + { > + ASSERT(!m_initiatorName.isEmpty()); > + } It’s not necessarily good to store element local names as a String rather than an AtomicString. I guess the reason is wanting to send things across threads? > Source/WebCore/loader/icon/IconLoader.cpp:70 > + request.setInitiator(String("icon")); Should be ASCIILiteral("icon"). > Source/WebCore/page/PerformanceResourceTiming.cpp:81 > + : PerformanceEntry(request.url().string(), "resource", monotonicTimeToDocumentMilliseconds(requestingDocument, initiationTime), Should use ASCIILiteral("resource"). > Source/WebCore/page/PerformanceResourceTiming.cpp:100 > + return "unknown"; Should be return ASCIILiteral("unknown"). > Source/WebCore/page/PerformanceResourceTiming.cpp:104 > + return "css"; Should also use ACIILiteral. > Source/WebCore/page/PerformanceResourceTiming.cpp:106 > + return "xmlhttprequest"; Ditto. > Source/WebCore/page/PerformanceResourceTiming.cpp:110 > + return "unknown"; Ditto. > Source/WebCore/svg/SVGFEImageElement.cpp:85 > + request.setInitiator(String(localName())); Again, localName().string(). > Source/WebCore/svg/SVGFontFaceUriElement.cpp:100 > + request.setInitiator(String(localName())); Ditto. > Source/WebCore/svg/SVGUseElement.cpp:254 > + request.setInitiator(String(localName())); Ditto.
It seems like this (approved) patch got abandoned. Can someone at Igalia clean it up and land it?
Created attachment 277861 [details] WIP
(In reply to comment #14) > Created attachment 277861 [details] > WIP Work in progress. Needs: * xcode build rules, * passing layout tests, * initiator for the DocumentLoader case.