Bug 209853

Summary:

Remove FrameLoader::addExtraFieldsToMainResourceRequest

Product:

WebKit

Reporter:

Rob Buis <rbuis>

Component:

New Bugs

Assignee:

Rob Buis <rbuis>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

cdumez, darin, ews-watchlist, japhet, webkit-bug-importer

Priority:

Keywords:

InRadar

Version:

Safari Technology Preview

Hardware:

Unspecified

OS:

Unspecified

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	none
Patch	none

Description Rob Buis 2020-04-01 04:49:16 PDT

Remove FrameLoader::addExtraFieldsToMainResourceRequest since the call is
not needed in DocumentLoader and can be inlined in FrameLoader. The call
in DocumentLoader is no longer needed since adding the User-Agent header
is decoupled from addExtraFields functionality and the User-Agent header
will be added in CachedResourceLoader after any custom setting of the
user agent (setCustomUserAgent API).

Comment 1 Rob Buis 2020-04-01 04:58:36 PDT

Created attachment 395158 [details]
Patch

Comment 2 Rob Buis 2020-04-01 05:35:58 PDT

Created attachment 395159 [details]
Patch

Comment 3 Rob Buis 2020-04-01 07:25:43 PDT

Created attachment 395164 [details]
Patch

Comment 4 Darin Adler 2020-04-01 09:54:10 PDT

Comment on attachment 395164 [details]
Patch

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

Code is fine, comments not so good

> Source/WebCore/loader/DocumentLoader.cpp:1822
>      // Make sure we re-apply the user agent to the Document's ResourceRequest upon reload in case the embedding
>      // application has changed it.

The code below *clears* the user agent. The comment says "make sure we re-apply the user agent". Could you reword it to clarify what it means or remove it? Why is it valuable to clear the user agent here? If CachedResourceLoader is going to add it, why do we need to touch it at all here? Do we need to clear it because CachedResourceLoader only overwrites it and doesn’t clear it? Is it valuable to clear it to save memory or something?

> Source/WebCore/loader/FrameLoader.cpp:1525
> +    // FIXME: Using m_loadType seems wrong for some callers.
> +    // If we are only preparing to load the main resource, that is previous load's load type!

The wording of this comment no longer makes sense. It said "some callers" and that meant "some callers of the addExtraFieldsToMainResourceRequest function". Need to rewrite the comment to make logical sense. I like keeping it, but needs rewording.

Comment 5 Rob Buis 2020-04-01 10:09:26 PDT

Created attachment 395182 [details]
Patch

Comment 6 EWS 2020-04-02 00:43:19 PDT

Committed r259379: <https://trac.webkit.org/changeset/259379>

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

Comment 7 Radar WebKit Bug Importer 2020-04-02 00:44:14 PDT

<rdar://problem/61195938>