[webkit-reviews] review granted: [Bug 209664] Append Upgrade-Insecure-Requests header in CachedResourceLoader : [Attachment 395057] Patch

[bugzilla-daemon at webkit.org]

youenn fablet <youennf at gmail.com> has granted Rob Buis <rbuis at igalia.com>'s
request for review:
Bug 209664: Append Upgrade-Insecure-Requests header in CachedResourceLoader
https://bugs.webkit.org/show_bug.cgi?id=209664

Attachment 395057: Patch

https://bugs.webkit.org/attachment.cgi?id=395057&action=review




--- Comment #10 from youenn fablet <youennf at gmail.com> ---
Comment on attachment 395057
  --> https://bugs.webkit.org/attachment.cgi?id=395057
Patch

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

>>>> Source/WebCore/loader/FrameLoader.cpp:1522
>>>> +	  addExtraFieldsToRequest(r, m_loadType, true);
>>> 
>>> Change log doesn’t explain this change. If
addExtraFieldsToMainResourceRequest still exists, why is this one call site not
using it?
>> 
>> I would like to remove addExtraFieldsToMainResourceRequest and
addExtraFieldsToSubresourceRequest if possible and this is a step in that
direction. However I can remove this part of the patch if you feel it is not
appropriate here.
> 
> I don’t mean to make any statement about whether it’s appropriate. I just
don’t understand why it’s correct, nor does the change log say why.
> 
> I trust you that it’s a good change, but I’d like you to explain why.

Maybe it is better to keep it for now like this and have another patch that
would remove addExtraFieldsToMainResourceRequest.