Bug 192735

Summary: Allow clients to set the navigator platform
Product: WebKit Reporter: Megan Gardner <megan_gardner>
Component: New BugsAssignee: Megan Gardner <megan_gardner>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, bdakin, beidson, cdumez, commit-queue, dbates, esprehn+autocc, ews, japhet, kangil.han, rniwa, thorton, webkit-bug-importer, wenson_hsieh
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Archive of layout-test-results from ews103 for mac-sierra
none
Archive of layout-test-results from ews104 for mac-sierra-wk2
none
Archive of layout-test-results from ews122 for ios-simulator-wk2
none
Archive of layout-test-results from ews114 for mac-sierra
none
Patch
none
Archive of layout-test-results from ews101 for mac-sierra
none
Archive of layout-test-results from ews106 for mac-sierra-wk2
none
Archive of layout-test-results from ews113 for mac-sierra
none
Patch
none
Patch for landing none

Description Megan Gardner 2018-12-14 19:25:16 PST
Allow clients to set the navigator platform
Comment 1 Megan Gardner 2018-12-14 19:35:25 PST
Created attachment 357380 [details]
Patch
Comment 2 Build Bot 2018-12-14 19:37:32 PST Comment hidden (obsolete)
Comment 3 Build Bot 2018-12-14 20:49:53 PST Comment hidden (obsolete)
Comment 4 Build Bot 2018-12-14 20:50:01 PST Comment hidden (obsolete)
Comment 5 Build Bot 2018-12-14 20:57:08 PST Comment hidden (obsolete)
Comment 6 Build Bot 2018-12-14 20:57:10 PST Comment hidden (obsolete)
Comment 7 Tim Horton 2018-12-14 21:29:25 PST
Comment on attachment 357380 [details]
Patch

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

> Source/WebCore/dom/Document.cpp:3290
> +String Document::navigatorPlatform() const

userAgent() isn't navigatorUserAgent(), so I don't think this /has/ to be navigatorPlatform(), but I also admit that platform() is pretty generic and confusing, so maybe it's best this way.

> Source/WebCore/page/Navigator.cpp:61
> +#elif OS(MAC_OS_X) && (CPU(PPC) || CPU(PPC64))
> +#define WEBCORE_NAVIGATOR_PLATFORM "MacPPC"_s

You might as well get rid of this case, WebKit certainly doesn't build for PPC macOS anymore.

> Source/WebCore/page/Navigator.cpp:138
> +    if (m_platform.isNull())

Humorous that we check isNull twice in the happy (cached) path.

> Source/WebCore/page/WorkerNavigator.cpp:47
> +    return m_navigatorPlatform;

Why m_navigatorPlatform here instead of just m_platform? This *is* a Navigator

> Source/WebCore/workers/WorkerThread.cpp:75
> -    WorkerThreadStartupData(const URL& scriptURL, const String& name, const String& identifier, const String& userAgent, bool isOnline, const String& sourceCode, WorkerThreadStartMode, const ContentSecurityPolicyResponseHeaders&, bool shouldBypassMainWorldContentSecurityPolicy, const SecurityOrigin& topOrigin, MonotonicTime timeOrigin, PAL::SessionID);
> +    WorkerThreadStartupData(const URL& scriptURL, const String& name, const String& identifier, const String& userAgent, bool isOnline, const String& sourceCode, WorkerThreadStartMode, const ContentSecurityPolicyResponseHeaders&, bool shouldBypassMainWorldContentSecurityPolicy, const SecurityOrigin& topOrigin, MonotonicTime timeOrigin, PAL::SessionID, const String& navigatorPlatform);

Wowzers you should try to convince someone to structify some of this.

> Source/WebKit/WebProcess/InjectedBundle/API/c/WKBundlePageLoaderClient.h:66
> +typedef WKDictionaryRef (*WKBundleNavigatorPropertiesForURLCallback)(WKBundleFrameRef frame, WKURLRef url, const void *clientInfo);

Why is this still here? I think you gave up on this approach.

> Source/WebKit/WebProcess/WebPage/WebPage.cpp:3109
> +    String platform = platformNavigatorPlatform();

platform navigator platform!

> Source/WebKit/WebProcess/WebPage/WebPage.cpp:3123
> +    if (m_page)
> +        m_page->userAgentChanged();

Did it?

> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:3130
> +    return String();

What is happening here

> Source/WebKit/WebProcess/WebPage/mac/WebPageMac.mm:831
> +    return String();

What is happening here

> Source/WebKitLegacy/mac/WebCoreSupport/WebFrameLoaderClient.mm:1551
> +String WebFrameLoaderClient::navigatorPlatform()

What is this for

> Tools/TestWebKitAPI/Tests/WebKitCocoa/WebsitePolicies.mm:1009
> +}

Nice simple happy test.
Comment 8 Build Bot 2018-12-14 21:36:42 PST Comment hidden (obsolete)
Comment 9 Build Bot 2018-12-14 21:36:44 PST Comment hidden (obsolete)
Comment 10 Build Bot 2018-12-14 21:37:17 PST Comment hidden (obsolete)
Comment 11 Build Bot 2018-12-14 21:37:19 PST Comment hidden (obsolete)
Comment 12 Megan Gardner 2018-12-18 14:59:26 PST
Created attachment 357618 [details]
Patch
Comment 13 Build Bot 2018-12-18 15:02:35 PST Comment hidden (obsolete)
Comment 14 Alex Christensen 2018-12-18 15:12:07 PST
Comment on attachment 357618 [details]
Patch

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

LGTM

> Source/WebCore/ChangeLog:10
> +        Lots of piping to allow the setting of a custom navigator platform.

We should probably make a class for all the parameters of making a worker,  but not in this patch.

> Source/WebCore/page/Navigator.cpp:123
> +    if (!defaultPlatform.isEmpty())

This line doesn't compile on linux.

> Source/WebKit/WebProcess/InjectedBundle/API/c/WKBundlePageLoaderClient.h:531
> +

This space doesn't need to be added.
Comment 15 Build Bot 2018-12-18 16:17:19 PST Comment hidden (obsolete)
Comment 16 Build Bot 2018-12-18 16:17:21 PST Comment hidden (obsolete)
Comment 17 Tim Horton 2018-12-18 16:21:47 PST
Comment on attachment 357618 [details]
Patch

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

Also you broke some WPTs.

> Tools/TestWebKitAPI/Tests/WebKitCocoa/WebsitePolicies.mm:1014
> +    auto delegate = adoptNS([[CustomNavigatorPlatformDelegate alloc] init]);

You need to actually load something in the WKWebView :D

Perhaps use loadHTMLString with an empty string.

Also need to put back the wait for finishedNavigation.

Then you'll be good.
Comment 18 Build Bot 2018-12-18 16:51:27 PST Comment hidden (obsolete)
Comment 19 Build Bot 2018-12-18 16:51:29 PST Comment hidden (obsolete)
Comment 20 Build Bot 2018-12-18 17:05:48 PST Comment hidden (obsolete)
Comment 21 Build Bot 2018-12-18 17:05:59 PST Comment hidden (obsolete)
Comment 22 Megan Gardner 2018-12-18 19:38:39 PST
Created attachment 357646 [details]
Patch
Comment 23 Tim Horton 2018-12-19 11:36:39 PST
Comment on attachment 357646 [details]
Patch

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

> Source/WebCore/page/Navigator.cpp:101
> +#if !OS(LINUX)

You shouldn't need this ifdef. You should just have the if !OS(LINUX) path everywhere; now that it's on FrameLoader, it will just always be the null String for Linux and fall back to NavigatorBase. No need for platform checks for no reason :)

> Tools/TestWebKitAPI/Tests/WebKitCocoa/WebsitePolicies.mm:996
> +        [websitePolicies setCustomNavigatorPlatform:@"Test Custom Platform2"];

Why the 2!
Comment 24 Alex Christensen 2018-12-19 12:00:09 PST
It might be worth testing the user agent and platform from a worker to make sure that is propagated correctly.
Comment 25 Megan Gardner 2018-12-19 12:02:10 PST
Created attachment 357706 [details]
Patch for landing
Comment 26 Tim Horton 2018-12-19 12:08:25 PST
(In reply to Alex Christensen from comment #24)
> It might be worth testing the user agent and platform from a worker to make
> sure that is propagated correctly.

We know that it is not, and that is an existing problem.
Comment 27 Chris Dumez 2018-12-19 12:20:32 PST
(In reply to Tim Horton from comment #26)
> (In reply to Alex Christensen from comment #24)
> > It might be worth testing the user agent and platform from a worker to make
> > sure that is propagated correctly.
> 
> We know that it is not, and that is an existing problem.

I do not think this is accurate, it is supposed to be working for dedicated workers.
It may not work for service workers though and we should probably file an issue about that.
Comment 28 WebKit Commit Bot 2018-12-19 12:21:40 PST
Comment on attachment 357706 [details]
Patch for landing

Clearing flags on attachment: 357706

Committed r239384: <https://trac.webkit.org/changeset/239384>
Comment 29 WebKit Commit Bot 2018-12-19 12:21:42 PST
All reviewed patches have been landed.  Closing bug.
Comment 30 Radar WebKit Bug Importer 2018-12-19 12:22:31 PST
<rdar://problem/46848776>