[Cocoa] Refactor some helper functions for building UserAgent strings

• Component: WebKit2
• Status: RESOLVED FIXED

[Wenson Hsieh]

Work towards <rdar://problem/47228232>

[Wenson Hsieh]

Work towards refactoring some codepaths for the "Request Desktop Site" feature in Safari.

[Wenson Hsieh]

Created attachment 365320 [details]
Patch

[Brent Fulgham]

Comment on attachment 365320 [details]
Patch

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

I think this all looks fine, bug please consider whether the two methods could be combined into one Cocoa variant.

> Source/WebCore/platform/UserAgent.h:34
> +enum class UserAgentType { Default, Desktop };

Do we also need to codify the concept of a Mobile site?

> Source/WebCore/platform/ios/UserAgentIOS.mm:79
> +String standardUserAgentWithApplicationName(const String& applicationName, UserAgentType type)

It seems weird that we have a Mac and an iOS version of this, rather than just a Cocoa version. Maybe if it we did, we could have a UserAgentType::Mobile to give a mobile UA, and UserAgentType::Desktop to force Desktop UA, and UserAgentType::Default to do the right thing based on build type?

> Source/WebCore/platform/ios/UserAgentIOS.mm:83
> +        return makeString("Mozilla/5.0 (Macintosh; Intel Mac OS X 10_14) AppleWebKit/605.1.15 (KHTML, like Gecko)", appNameSuffix);

I wonder if we could make the 10_14 somehow pick up the current macOS revision for the current source target, since we have TARGET_MAC_OS_X_VERSION_MAJOR. But obviously not for this patch.

[Brent Fulgham]

Comment on attachment 365320 [details]
Patch

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

> Source/WebCore/platform/ios/UserAgentIOS.mm:88
>      CFPropertyListRef override = CFPreferencesCopyAppValue(CFSTR("UserAgent"), CFSTR("com.apple.WebFoundation"));

Aside: I do wonder what this is for, and whether it's still needed.

[Wenson Hsieh]

Comment on attachment 365320 [details]
Patch

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

Thanks for taking a look!

>> Source/WebCore/platform/ios/UserAgentIOS.mm:79
>> +String standardUserAgentWithApplicationName(const String& applicationName, UserAgentType type)
> 
> It seems weird that we have a Mac and an iOS version of this, rather than just a Cocoa version. Maybe if it we did, we could have a UserAgentType::Mobile to give a mobile UA, and UserAgentType::Desktop to force Desktop UA, and UserAgentType::Default to do the right thing based on build type?

Constructing the iOS UA requires a bit more information about the current device model and whether it's running in a simulator, which seems quite iOS-specific, so I left it in this logic in the -IOS file. What do you think we would report as the device version for an iPhone UA on macOS? I suppose on the context of a feature like responsive design mode, we would know whether we're attempting to simulate iPad or iPhone, so we could pass these in as additional arguments (or even different values for UserAgentType)...

But since there's no need for grabbing an equivalent mobile UA on macOS, I don't think we need to worry about UserAgentType::Mobile until we need it.

>> Source/WebCore/platform/ios/UserAgentIOS.mm:83
>> +        return makeString("Mozilla/5.0 (Macintosh; Intel Mac OS X 10_14) AppleWebKit/605.1.15 (KHTML, like Gecko)", appNameSuffix);
> 
> I wonder if we could make the 10_14 somehow pick up the current macOS revision for the current source target, since we have TARGET_MAC_OS_X_VERSION_MAJOR. But obviously not for this patch.

I considered ways of trying to do this, but couldn't come up with any mechanism to make this work :(

I'm very open to ideas!

>> Source/WebCore/platform/ios/UserAgentIOS.mm:88
>>      CFPropertyListRef override = CFPreferencesCopyAppValue(CFSTR("UserAgent"), CFSTR("com.apple.WebFoundation"));
> 
> Aside: I do wonder what this is for, and whether it's still needed.

...indeed.

[Wenson Hsieh]

Created attachment 366012 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 366012 [details]
Patch for landing

Clearing flags on attachment: 366012

Committed r243525: <https://trac.webkit.org/changeset/243525>