Bug 210407

Created attachment 396231 [details]
Patch

Comment on attachment 396231 [details]
Patch

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

> Source/WebCore/ChangeLog:11
> +        Remove addExtraFieldsToSubresourceRequest since it can be replaced by
> +        calling addExtraFieldsToRequest. The loadType parameter is not taken
> +        into account by defaultRequestCachingPolicy so FrameLoadType::Standard
> +        rather than m_loadType is passed.

I’m not completely comfortable with passing a "wrong" load type, knowing that the load type doesn’t matter. Is there some better way to resolve this.

> Source/WebCore/loader/FrameLoader.cpp:3059
> +    addExtraFieldsToRequest(initialRequest, FrameLoadType::Standard, false);

I’m not thrilled with spreading more mysterious "false" arguments in even more places in the code.

Comment on attachment 396231 [details]
Patch

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

>> Source/WebCore/ChangeLog:11
>> +        rather than m_loadType is passed.
> 
> I’m not completely comfortable with passing a "wrong" load type, knowing that the load type doesn’t matter. Is there some better way to resolve this.

I gave the LoadType parameter a default value so addExtraFieldsToRequest calls for subresources do not need to specify anything for it.

>> Source/WebCore/loader/FrameLoader.cpp:3059
>> +    addExtraFieldsToRequest(initialRequest, FrameLoadType::Standard, false);
> 
> I’m not thrilled with spreading more mysterious "false" arguments in even more places in the code.

Fair enough, I added an enum

Created attachment 396239 [details]
Patch

Comment on attachment 396239 [details]
Patch

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

OK

> Source/WebCore/loader/FrameLoader.h:94
> +enum class MainResource : bool { Yes, No };

I think the name should be either ForMainResource or IsMainResource. A type named MainResource sounds more like a resource than an indicator of the nature of a resource.

Also, I suggest { No, Yes } so we have the traditional representation of 0 for false and 1 for true.

> Source/WebCore/loader/FrameLoader.h:332
> +    void addExtraFieldsToRequest(ResourceRequest&, MainResource, FrameLoadType = FrameLoadType::Standard);

How should a caller know when it’s important to pass in a load type, and when it’s not important?

Created attachment 396259 [details]
Patch

Comment on attachment 396239 [details]
Patch

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

>> Source/WebCore/loader/FrameLoader.h:94
>> +enum class MainResource : bool { Yes, No };
> 
> I think the name should be either ForMainResource or IsMainResource. A type named MainResource sounds more like a resource than an indicator of the nature of a resource.
> 
> Also, I suggest { No, Yes } so we have the traditional representation of 0 for false and 1 for true.

That is better indeed, fixed.

>> Source/WebCore/loader/FrameLoader.h:332
>> +    void addExtraFieldsToRequest(ResourceRequest&, MainResource, FrameLoadType = FrameLoadType::Standard);
> 
> How should a caller know when it’s important to pass in a load type, and when it’s not important?

It is hard to make it clear but I added a comment for it.

Committed r259997: <https://trac.webkit.org/changeset/259997>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 396259 [details].

<rdar://problem/61703487>