WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
195990
[Cocoa] Refactor some helper functions for building UserAgent strings
https://bugs.webkit.org/show_bug.cgi?id=195990
Summary
[Cocoa] Refactor some helper functions for building UserAgent strings
Wenson Hsieh
Reported
2019-03-19 22:04:31 PDT
Comment hidden (obsolete)
Work towards <
rdar://problem/47228232
>
Attachments
Patch
(3.60 KB, patch)
2019-03-19 23:03 PDT
,
Wenson Hsieh
bfulgham
: review+
Details
Formatted Diff
Diff
Patch for landing
(3.77 KB, patch)
2019-03-26 15:12 PDT
,
Wenson Hsieh
no flags
Details
Formatted Diff
Diff
View All
Add attachment
proposed patch, testcase, etc.
Wenson Hsieh
Comment 1
2019-03-19 22:24:25 PDT
Work towards refactoring some codepaths for the "Request Desktop Site" feature in Safari.
Wenson Hsieh
Comment 2
2019-03-19 23:03:25 PDT
Created
attachment 365320
[details]
Patch
Brent Fulgham
Comment 3
2019-03-20 12:32:30 PDT
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 4
2019-03-20 12:33:05 PDT
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 5
2019-03-21 08:42:03 PDT
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
Comment 6
2019-03-26 15:12:45 PDT
Created
attachment 366012
[details]
Patch for landing
WebKit Commit Bot
Comment 7
2019-03-26 16:41:47 PDT
Comment on
attachment 366012
[details]
Patch for landing Clearing flags on attachment: 366012 Committed
r243525
: <
https://trac.webkit.org/changeset/243525
>
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug