Bug 209853 - Remove FrameLoader::addExtraFieldsToMainResourceRequest
Summary: Remove FrameLoader::addExtraFieldsToMainResourceRequest
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: Safari Technology Preview
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Rob Buis
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-04-01 04:49 PDT by Rob Buis
Modified: 2020-04-02 00:44 PDT (History)
5 users (show)

See Also:


Attachments
Patch (4.33 KB, patch)
2020-04-01 04:58 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (4.23 KB, patch)
2020-04-01 05:35 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (4.22 KB, patch)
2020-04-01 07:25 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (4.35 KB, patch)
2020-04-01 10:09 PDT, Rob Buis
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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>