Bug 198960 - Introduce LinkLoadParameters
Summary: Introduce LinkLoadParameters
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Page Loading (show other bugs)
Version: Other
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Rob Buis
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-06-18 05:44 PDT by Rob Buis
Modified: 2019-06-25 01:43 PDT (History)
10 users (show)

See Also:


Attachments
Patch (14.23 KB, patch)
2019-06-18 05:48 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (14.33 KB, patch)
2019-06-18 07:55 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (13.44 KB, patch)
2019-06-18 10:27 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (13.44 KB, patch)
2019-06-25 00:26 PDT, Rob Buis
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Rob Buis 2019-06-18 05:44:03 PDT
LinkLoader methods have too many parameters.
Comment 1 Rob Buis 2019-06-18 05:48:46 PDT
Created attachment 372335 [details]
Patch
Comment 2 Rob Buis 2019-06-18 07:55:56 PDT
Created attachment 372338 [details]
Patch
Comment 3 Daniel Bates 2019-06-18 08:56:45 PDT
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.
Comment 4 Rob Buis 2019-06-18 10:26:58 PDT
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.
Comment 5 Rob Buis 2019-06-18 10:27:19 PDT
Created attachment 372346 [details]
Patch
Comment 6 Frédéric Wang (:fredw) 2019-06-25 00:08:35 PDT
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?
Comment 7 Rob Buis 2019-06-25 00:26:07 PDT
Created attachment 372820 [details]
Patch
Comment 8 Rob Buis 2019-06-25 01:37:18 PDT
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.
Comment 9 WebKit Commit Bot 2019-06-25 01:42:36 PDT
Comment on attachment 372820 [details]
Patch

Clearing flags on attachment: 372820

Committed r246786: <https://trac.webkit.org/changeset/246786>
Comment 10 WebKit Commit Bot 2019-06-25 01:42:38 PDT
All reviewed patches have been landed.  Closing bug.
Comment 11 Radar WebKit Bug Importer 2019-06-25 01:43:17 PDT
<rdar://problem/52100030>