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