[webkit-reviews] review granted: [Bug 117065] Clean up CachedResourceRequest initiators : [Attachment 204120] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Zan Dobersek
<zandobersek at gmail.com>'s request for review:
Bug 117065: Clean up CachedResourceRequest initiators
https://bugs.webkit.org/show_bug.cgi?id=117065

Attachment 204120: Patch
https://bugs.webkit.org/attachment.cgi?id=204120&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
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.