Bug 117065 – Clean up CachedResourceRequest initiators

• Component: New Bugs
• Status: NEW

[Zan Dobersek]

Clean up CachedResourceRequest initiators

[Zan Dobersek]

Created attachment 203592 [details]
Patch

[Alexey Proskuryakov]

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".

[Zan Dobersek]

Hmm. I was looking at this one, which is marked as a Candidate Recommendation.
http://www.w3.org/TR/resource-timing/

[Darin Adler]

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?

[Alexey Proskuryakov]

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.

[Zan Dobersek]

Agreed, I'll produce a patch based on the editor's draft.

[Zan Dobersek]

Created attachment 204120 [details]
Patch

[WebKit Commit Bot]

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.

[Early Warning System Bot]

Comment on attachment 204120 [details]
Patch

Attachment 204120 [details] did not pass qt-ews (qt):
Output: http://webkit-queues.appspot.com/results/786220

[Early Warning System Bot]

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

[Brent Fulgham]

What's happening with this patch? Has the draft been finalized? Should we take this patch? (Alexey? Darin?)

[Darin Adler]

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.

[Brent Fulgham]

It seems like this (approved) patch got abandoned. Can someone at Igalia clean it up and land it?

[Zan Dobersek]

Created attachment 277861 [details]
WIP

[Zan Dobersek]

(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.