Bug 198960 – Introduce LinkLoadParameters

• Component: Page Loading
• Status: RESOLVED FIXED

[Rob Buis]

LinkLoader methods have too many parameters.

[Rob Buis]

Created attachment 372335 [details]
Patch

[Rob Buis]

Created attachment 372338 [details]
Patch

[Daniel Bates]

Comment on attachment 372338 [details]
Patch

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

> Source/WebCore/html/HTMLLinkElement.cpp:265
> +    const LinkLoadParameters params(

No need for const. Use uniform initializer syntax.

> Source/WebCore/loader/LinkLoader.cpp:64
> +LinkLoadParameters::LinkLoadParameters(const LinkRelAttribute& relAttribute, const URL& href, const String& as, const String& media, const String& mimeType, const String& crossOrigin, const String& imageSrcSet, const String& imageSizes)

Remove.

> Source/WebCore/loader/LinkLoader.cpp:126
> +        const LinkLoadParameters params(relAttribute, url, header.as(), header.media(), header.mimeType(), header.crossOrigin(), header.imageSrcSet(), header.imageSizes());

Ditto.

> Source/WebCore/loader/LinkLoader.cpp:320
> +    m_cachedLinkResource = document.cachedResourceLoader().requestLinkResource(type, CachedResourceRequest(ResourceRequest(document.completeURL(params.href)), options, priority)).value_or(nullptr);

Use uniform initializer syntax.

> Source/WebCore/loader/LinkLoader.cpp:331
> +bool LinkLoader::loadLink(const LinkLoadParameters& params, Document& document)

params could be rvalue reference I think.

> Source/WebCore/loader/LinkLoader.h:48
> +    LinkLoadParameters(const LinkRelAttribute&,

Remove. Use aggregate initialization.

[Rob Buis]

Comment on attachment 372338 [details]
Patch

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

>> Source/WebCore/html/HTMLLinkElement.cpp:265
>> +    const LinkLoadParameters params(
> 
> No need for const. Use uniform initializer syntax.

Done.

>> Source/WebCore/loader/LinkLoader.cpp:64
>> +LinkLoadParameters::LinkLoadParameters(const LinkRelAttribute& relAttribute, const URL& href, const String& as, const String& media, const String& mimeType, const String& crossOrigin, const String& imageSrcSet, const String& imageSizes)
> 
> Remove.

Done.

>> Source/WebCore/loader/LinkLoader.cpp:126
>> +        const LinkLoadParameters params(relAttribute, url, header.as(), header.media(), header.mimeType(), header.crossOrigin(), header.imageSrcSet(), header.imageSizes());
> 
> Ditto.

Done.

>> Source/WebCore/loader/LinkLoader.cpp:320
>> +    m_cachedLinkResource = document.cachedResourceLoader().requestLinkResource(type, CachedResourceRequest(ResourceRequest(document.completeURL(params.href)), options, priority)).value_or(nullptr);
> 
> Use uniform initializer syntax.

I assume you meant ResourceRequest here.

>> Source/WebCore/loader/LinkLoader.cpp:331
>> +bool LinkLoader::loadLink(const LinkLoadParameters& params, Document& document)
> 
> params could be rvalue reference I think.

Since. we don't store the params in LinkLoader but just pass it around, I don't think that is needed.

>> Source/WebCore/loader/LinkLoader.h:48
>> +    LinkLoadParameters(const LinkRelAttribute&,
> 
> Remove. Use aggregate initialization.

Done.

[Rob Buis]

Created attachment 372346 [details]
Patch

[Frédéric Wang (:fredw)]

Comment on attachment 372346 [details]
Patch

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

> Source/WebCore/html/HTMLLinkElement.cpp:273
> +        attributeWithoutSynchronization(imagesizesAttr) };

I would put the closing "};" on a new line. Not sure if check-webkit-style likes it or not...

> Source/WebCore/loader/LinkLoader.cpp:218
> +    if (!params.relAttribute.isLinkPreconnect || !href.isValid() || !params.href.protocolIsInHTTPFamily() || !document.frame())

I see you are using href.isValid(), params.href.protocolIsInHTTPFamily() and SecurityOrigin::create(href)

Is introducing this local href variable actually needed?

[Rob Buis]

Created attachment 372820 [details]
Patch

[Rob Buis]

Comment on attachment 372346 [details]
Patch

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

>> Source/WebCore/html/HTMLLinkElement.cpp:273
>> +        attributeWithoutSynchronization(imagesizesAttr) };
> 
> I would put the closing "};" on a new line. Not sure if check-webkit-style likes it or not...

Done.

>> Source/WebCore/loader/LinkLoader.cpp:218
>> +    if (!params.relAttribute.isLinkPreconnect || !href.isValid() || !params.href.protocolIsInHTTPFamily() || !document.frame())
> 
> I see you are using href.isValid(), params.href.protocolIsInHTTPFamily() and SecurityOrigin::create(href)
> 
> Is introducing this local href variable actually needed?

It is not needed, but I think it makes things a bit easier to read, and it matches the old situation.

[WebKit Commit Bot]

Comment on attachment 372820 [details]
Patch

Clearing flags on attachment: 372820

Committed r246786: <https://trac.webkit.org/changeset/246786>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/52100030>