Bug 192509 – Add SPI to allow the client to set the user-agent at main frame level, from the UIProcess

• Component: WebKit2
• Status: RESOLVED FIXED

[Chris Dumez]

Add SPI to allow the client to set the user-agent at frame level, from the UIProcess instead of doing it at resource-level from the injected bundle.

[Chris Dumez]

<rdar://problem/46500832>

[Chris Dumez]

Created attachment 356829 [details]
Patch

[EWS Watchlist]

Attachment 356829 [details] did not pass style-queue:


ERROR: Tools/TestWebKitAPI/Tests/WebKitCocoa/WebsitePolicies.mm:923:  Multi-line string ("...") found.  This lint script doesn't do well with such strings, and may give bogus warnings.  They're ugly and unnecessary, and you should use concatenation instead".  [readability/multiline_string] [5]
ERROR: Tools/TestWebKitAPI/Tests/WebKitCocoa/WebsitePolicies.mm:934:  Multi-line string ("...") found.  This lint script doesn't do well with such strings, and may give bogus warnings.  They're ugly and unnecessary, and you should use concatenation instead".  [readability/multiline_string] [5]
ERROR: Tools/TestWebKitAPI/Tests/WebKitCocoa/WebsitePolicies.mm:936:  Multi-line string ("...") found.  This lint script doesn't do well with such strings, and may give bogus warnings.  They're ugly and unnecessary, and you should use concatenation instead".  [readability/multiline_string] [5]
ERROR: Tools/TestWebKitAPI/Tests/WebKitCocoa/WebsitePolicies.mm:947:  Multi-line string ("...") found.  This lint script doesn't do well with such strings, and may give bogus warnings.  They're ugly and unnecessary, and you should use concatenation instead".  [readability/multiline_string] [5]
Total errors found: 4 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Alex Christensen]

Comment on attachment 356829 [details]
Patch

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

> Source/WebCore/loader/FrameLoader.cpp:2667
>  String FrameLoader::userAgent(const URL& url) const
>  {
> +    if (auto* documentLoader = activeDocumentLoader()) {

I think we will want subframes to have the same user agent as their parent?  I think this should go to the main frame's active document loader and have a test.

[Chris Dumez]

(In reply to Alex Christensen from comment #4)
> Comment on attachment 356829 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=356829&action=review
> 
> > Source/WebCore/loader/FrameLoader.cpp:2667
> >  String FrameLoader::userAgent(const URL& url) const
> >  {
> > +    if (auto* documentLoader = activeDocumentLoader()) {
> 
> I think we will want subframes to have the same user agent as their parent? 
> I think this should go to the main frame's active document loader and have a
> test.

decidePolicyForNavigationAction is per-frame? So why does it make sense for the userAgent provided by decidePolicyForNavigationAction handler to impact the whole page?

[Chris Dumez]

(In reply to Chris Dumez from comment #5)
> (In reply to Alex Christensen from comment #4)
> > Comment on attachment 356829 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=356829&action=review
> > 
> > > Source/WebCore/loader/FrameLoader.cpp:2667
> > >  String FrameLoader::userAgent(const URL& url) const
> > >  {
> > > +    if (auto* documentLoader = activeDocumentLoader()) {
> > 
> > I think we will want subframes to have the same user agent as their parent? 
> > I think this should go to the main frame's active document loader and have a
> > test.
> 
> decidePolicyForNavigationAction is per-frame? So why does it make sense for
> the userAgent provided by decidePolicyForNavigationAction handler to impact
> the whole page?

I think the current design makes sense from an API point-of-view since decidePolicyForNavigationAction is per-frame. If the client want the user-agent to be the same for all frames, my current design still allows them to do so.

[Alex Christensen]

Comment on attachment 356829 [details]
Patch

If that's what the client of this SPI wants we should make the test have different a user agent for the iframe and verify the correct one is sent.  I'm curious what the user agent on the main resource of the iframe will be.

[Chris Dumez]

(In reply to Alex Christensen from comment #7)
> Comment on attachment 356829 [details]
> Patch
> 
> If that's what the client of this SPI wants we should make the test have
> different a user agent for the iframe and verify the correct one is sent. 
> I'm curious what the user agent on the main resource of the iframe will be.

To be clear, per-frame user-agent is not what Safari wants. However, this is what seem to make sense from an API point of view, if we add this to WebsitePolicies and thus to decidePolicyForNavigationAction.

So we do not need this flexibility, I however do not know how to make the API not confusing if we to it only for main frame.

If we keep the API as-is, I'd be happy to write an extra API test coverage different user agents for different frames.

[Chris Dumez]

Created attachment 356962 [details]
Patch

[EWS Watchlist]

Attachment 356962 [details] did not pass style-queue:


ERROR: Tools/TestWebKitAPI/Tests/WebKitCocoa/WebsitePolicies.mm:929:  Multi-line string ("...") found.  This lint script doesn't do well with such strings, and may give bogus warnings.  They're ugly and unnecessary, and you should use concatenation instead".  [readability/multiline_string] [5]
ERROR: Tools/TestWebKitAPI/Tests/WebKitCocoa/WebsitePolicies.mm:940:  Multi-line string ("...") found.  This lint script doesn't do well with such strings, and may give bogus warnings.  They're ugly and unnecessary, and you should use concatenation instead".  [readability/multiline_string] [5]
ERROR: Tools/TestWebKitAPI/Tests/WebKitCocoa/WebsitePolicies.mm:942:  Multi-line string ("...") found.  This lint script doesn't do well with such strings, and may give bogus warnings.  They're ugly and unnecessary, and you should use concatenation instead".  [readability/multiline_string] [5]
ERROR: Tools/TestWebKitAPI/Tests/WebKitCocoa/WebsitePolicies.mm:953:  Multi-line string ("...") found.  This lint script doesn't do well with such strings, and may give bogus warnings.  They're ugly and unnecessary, and you should use concatenation instead".  [readability/multiline_string] [5]
Total errors found: 4 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Alex Christensen]

Comment on attachment 356962 [details]
Patch

In NavigationState::NavigationClient::decidePolicyForNavigationAction we raise an ObjC exception if the websiteDataStore was set on a non-mainframe navigation, because that is similarly nonsensical.  We could do something like that with the user agent.

[Chris Dumez]

(In reply to Alex Christensen from comment #11)
> Comment on attachment 356962 [details]
> Patch
> 
> In NavigationState::NavigationClient::decidePolicyForNavigationAction we
> raise an ObjC exception if the websiteDataStore was set on a non-mainframe
> navigation, because that is similarly nonsensical.  We could do something
> like that with the user agent.

Fine by me. I'll look into doing this since it would make the SPI a little bit easier to adopt.

[Chris Dumez]

Created attachment 356966 [details]
Patch

[EWS Watchlist]

Attachment 356966 [details] did not pass style-queue:


ERROR: Tools/TestWebKitAPI/Tests/WebKitCocoa/WebsitePolicies.mm:925:  Multi-line string ("...") found.  This lint script doesn't do well with such strings, and may give bogus warnings.  They're ugly and unnecessary, and you should use concatenation instead".  [readability/multiline_string] [5]
ERROR: Tools/TestWebKitAPI/Tests/WebKitCocoa/WebsitePolicies.mm:936:  Multi-line string ("...") found.  This lint script doesn't do well with such strings, and may give bogus warnings.  They're ugly and unnecessary, and you should use concatenation instead".  [readability/multiline_string] [5]
ERROR: Tools/TestWebKitAPI/Tests/WebKitCocoa/WebsitePolicies.mm:938:  Multi-line string ("...") found.  This lint script doesn't do well with such strings, and may give bogus warnings.  They're ugly and unnecessary, and you should use concatenation instead".  [readability/multiline_string] [5]
ERROR: Tools/TestWebKitAPI/Tests/WebKitCocoa/WebsitePolicies.mm:949:  Multi-line string ("...") found.  This lint script doesn't do well with such strings, and may give bogus warnings.  They're ugly and unnecessary, and you should use concatenation instead".  [readability/multiline_string] [5]
Total errors found: 4 in 13 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Commit Bot]

Comment on attachment 356966 [details]
Patch

Clearing flags on attachment: 356966

Committed r239046: <https://trac.webkit.org/changeset/239046>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.