Bug 192735 – Allow clients to set the navigator platform

• Component: New Bugs
• Status: RESOLVED FIXED

[Megan Gardner]

Allow clients to set the navigator platform

[Megan Gardner]

Created attachment 357380 [details]
Patch

[EWS Watchlist]

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


ERROR: Source/WebCore/loader/EmptyFrameLoaderClient.h:153:  Inline functions should not be in classes annotated with WEBCORE_EXPORT. Remove the macro from the class and apply it to each appropriate method, or move the inline function definition out-of-line.  [build/webcore_export] [4]
ERROR: Source/WebCore/workers/service/context/ServiceWorkerThread.cpp:75:  Missing spaces around :  [whitespace/init] [4]
Total errors found: 2 in 56 files


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

[EWS Watchlist]

Comment on attachment 357380 [details]
Patch

Attachment 357380 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/10407968

New failing tests:
imported/w3c/web-platform-tests/workers/WorkerNavigator_platform.htm
imported/w3c/web-platform-tests/workers/interfaces/WorkerUtils/navigator/004.html

[EWS Watchlist]

Created attachment 357387 [details]
Archive of layout-test-results from ews103 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-sierra  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 357380 [details]
Patch

Attachment 357380 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/10407984

New failing tests:
imported/w3c/web-platform-tests/workers/WorkerNavigator_platform.htm
imported/w3c/web-platform-tests/workers/interfaces/WorkerUtils/navigator/004.html

[EWS Watchlist]

Created attachment 357388 [details]
Archive of layout-test-results from ews104 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6

[Tim Horton]

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.

[EWS Watchlist]

Comment on attachment 357380 [details]
Patch

Attachment 357380 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/10408125

New failing tests:
imported/w3c/web-platform-tests/workers/WorkerNavigator_platform.htm
imported/w3c/web-platform-tests/workers/interfaces/WorkerUtils/navigator/004.html

[EWS Watchlist]

Created attachment 357389 [details]
Archive of layout-test-results from ews122 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews122  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.6

[EWS Watchlist]

Comment on attachment 357380 [details]
Patch

Attachment 357380 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/10408114

New failing tests:
imported/w3c/web-platform-tests/workers/WorkerNavigator_platform.htm
imported/w3c/web-platform-tests/workers/interfaces/WorkerUtils/navigator/004.html

[EWS Watchlist]

Created attachment 357390 [details]
Archive of layout-test-results from ews114 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-sierra  Platform: Mac OS X 10.12.6

[Megan Gardner]

Created attachment 357618 [details]
Patch

[EWS Watchlist]

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


ERROR: Source/WebCore/loader/EmptyFrameLoaderClient.h:153:  Inline functions should not be in classes annotated with WEBCORE_EXPORT. Remove the macro from the class and apply it to each appropriate method, or move the inline function definition out-of-line.  [build/webcore_export] [4]
ERROR: Source/WebCore/workers/service/context/ServiceWorkerThread.cpp:75:  Missing spaces around :  [whitespace/init] [4]
Total errors found: 2 in 58 files


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

[Alex Christensen]

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.

[EWS Watchlist]

Comment on attachment 357618 [details]
Patch

Attachment 357618 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/10464946

New failing tests:
imported/w3c/web-platform-tests/workers/WorkerNavigator_platform.htm
imported/w3c/web-platform-tests/workers/interfaces/WorkerUtils/navigator/004.html

[EWS Watchlist]

Created attachment 357631 [details]
Archive of layout-test-results from ews101 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-sierra  Platform: Mac OS X 10.12.6

[Tim Horton]

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.

[EWS Watchlist]

Comment on attachment 357618 [details]
Patch

Attachment 357618 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/10465411

New failing tests:
imported/w3c/web-platform-tests/workers/WorkerNavigator_platform.htm
imported/w3c/web-platform-tests/workers/interfaces/WorkerUtils/navigator/004.html

[EWS Watchlist]

Created attachment 357634 [details]
Archive of layout-test-results from ews106 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 357618 [details]
Patch

Attachment 357618 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/10465205

New failing tests:
imported/w3c/web-platform-tests/workers/WorkerNavigator_platform.htm
imported/w3c/web-platform-tests/workers/interfaces/WorkerUtils/navigator/004.html

[EWS Watchlist]

Created attachment 357636 [details]
Archive of layout-test-results from ews113 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews113  Port: mac-sierra  Platform: Mac OS X 10.12.6

[Megan Gardner]

Created attachment 357646 [details]
Patch

[Tim Horton]

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!

[Alex Christensen]

It might be worth testing the user agent and platform from a worker to make sure that is propagated correctly.

[Megan Gardner]

Created attachment 357706 [details]
Patch for landing

[Tim Horton]

(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.

[Chris Dumez]

(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.

[WebKit Commit Bot]

Comment on attachment 357706 [details]
Patch for landing

Clearing flags on attachment: 357706

Committed r239384: <https://trac.webkit.org/changeset/239384>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/46848776>