[webkit-reviews] review requested: [Bug 199261] Align with Origin header changes : [Attachment 393050] Patch

[bugzilla-daemon at webkit.org]

Rob Buis <rbuis at igalia.com> has asked  for review:
Bug 199261: Align with Origin header changes
https://bugs.webkit.org/show_bug.cgi?id=199261

Attachment 393050: Patch

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




--- Comment #13 from Rob Buis <rbuis at igalia.com> ---
Comment on attachment 393050
  --> https://bugs.webkit.org/attachment.cgi?id=393050
Patch

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

>> Source/WebCore/loader/cache/CachedResourceRequest.cpp:230
>>	}
> 
> Remove { }

Done.

>> Source/WebCore/loader/cache/CachedResourceRequest.cpp:231
>> +	if (m_options.mode == FetchOptions::Mode::Cors)
> 
> If mode is CORS, we probably already set the origin header, right?
> So FrameLoader::addHTTPOriginIfNeeded will be a no-op.
> 
> It might be better to do all this processing if the request does not have any
origin.
> Either using 
> if (!m_resourceRequest.httpOrigin().isEmpty())
>     return;
> 
> Or by introducing something like FrameLoader::addHTTPOriginIfiNeeded(Request,
const Function<String()>& computeOriginValue)

So for CORS we do not set the origin header (I added an ASSERT_NOT_REACHED and
it was hit a lot as you can see in the previous upload).
I would really like to centralize the places to where the origin header is set,
ideally to one, so here, but I know it is needed in a few other places like
sync loading.

I made an attempt to return early if the Origin is already set to make things a
bit more effiicient.