| Summary: | Allow clients to set the navigator platform | ||
|---|---|---|---|
| Product: | WebKit | Reporter: | Megan Gardner <megan_gardner> |
| Component: | New Bugs | Assignee: | Megan Gardner <megan_gardner> |
| Status: | RESOLVED FIXED | ||
| Severity: | Normal | CC: | achristensen, bdakin, beidson, cdumez, commit-queue, dbates, esprehn+autocc, ews-watchlist, japhet, kangil.han, rniwa, thorton, webkit-bug-importer, wenson_hsieh |
| Priority: | P2 | Keywords: | InRadar |
| Version: | WebKit Nightly Build | ||
| Hardware: | Unspecified | ||
| OS: | Unspecified | ||
| Attachments: | |||
|
Description
Megan Gardner
2018-12-14 19:25:16 PST
Created attachment 357380 [details]
Patch
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.
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 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
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 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
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 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 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
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 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
Created attachment 357618 [details]
Patch
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.
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 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 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
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 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 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
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 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
Created attachment 357646 [details]
Patch
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! It might be worth testing the user agent and platform from a worker to make sure that is propagated correctly. Created attachment 357706 [details]
Patch for landing
(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. (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 on attachment 357706 [details] Patch for landing Clearing flags on attachment: 357706 Committed r239384: <https://trac.webkit.org/changeset/239384> All reviewed patches have been landed. Closing bug. |