RESOLVED FIXED 148671
[Cocoa] Allow testing with the system language
https://bugs.webkit.org/show_bug.cgi?id=148671
Summary [Cocoa] Allow testing with the system language
Myles C. Maxfield
Reported 2015-08-31 22:54:05 PDT
[Cocoa] Allow setting the system font
Attachments
Patch (19.22 KB, patch)
2015-08-31 23:16 PDT, Myles C. Maxfield
no flags
Patch (19.29 KB, patch)
2015-08-31 23:18 PDT, Myles C. Maxfield
no flags
Patch (25.24 KB, patch)
2015-09-01 17:49 PDT, Myles C. Maxfield
no flags
Archive of layout-test-results from ews107 for mac-mavericks-wk2 (1.38 MB, application/zip)
2015-09-01 19:20 PDT, Build Bot
no flags
Patch (23.15 KB, patch)
2015-09-02 23:54 PDT, Myles C. Maxfield
no flags
Patch (27.71 KB, patch)
2015-09-03 12:46 PDT, Myles C. Maxfield
no flags
Patch (23.10 KB, patch)
2015-09-03 17:46 PDT, Myles C. Maxfield
no flags
Patch (24.00 KB, patch)
2015-09-03 18:22 PDT, Myles C. Maxfield
ap: review+
WIP (24.10 KB, patch)
2015-09-03 19:58 PDT, Myles C. Maxfield
no flags
Patch (26.02 KB, patch)
2015-09-04 16:16 PDT, Myles C. Maxfield
no flags
Patch (16.01 KB, patch)
2015-09-04 17:40 PDT, Myles C. Maxfield
no flags
Patch (19.85 KB, patch)
2015-09-08 21:41 PDT, Myles C. Maxfield
no flags
Myles C. Maxfield
Comment 1 2015-08-31 23:16:13 PDT
Myles C. Maxfield
Comment 2 2015-08-31 23:18:22 PDT
WebKit Commit Bot
Comment 3 2015-08-31 23:19:21 PDT
Attachment 260355 [details] did not pass style-queue: ERROR: Tools/WebKitTestRunner/TestController.cpp:57: Streams are highly discouraged. [readability/streams] [3] Total errors found: 1 in 12 files If any of these errors are false positives, please file a bug against check-webkit-style.
Alexey Proskuryakov
Comment 4 2015-09-01 09:58:50 PDT
Comment on attachment 260355 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=260355&action=review r- since this break tests. The general approach looks fine to me, although I think that bundle initialization is not the right place to set the language. > Tools/ChangeLog:15 > + This patch sets the system language early in the lifetime of the web process, inside the > + initialization of the injected bundle. This is early enough for Core Text, but too late for > + AppKit. As such, the solution isn't perfect, but it is enough to test recent text breakages > + which were not found due to the system language being non-English. What prevents us from doing it the right way? WebContent language is sent from UI process in a bootstrap message already, we just need to send the fake one. > Tools/WebKitTestRunner/InjectedBundle/mac/InjectedBundleMac.mm:-58 > - // FIXME (<rdar://problem/13396515>): It is too late to set AppleLanguages here, as loaded frameworks localizations cannot be changed. > - // This breaks some accessibility tests on machines with non-English user language. Wy remove the FIXME, given that it's still true? > Tools/WebKitTestRunner/TestController.cpp:800 > +static String testFilename(const WKURLRef url) Looks like this returns a path, not a filename. > Tools/WebKitTestRunner/TestController.cpp:813 > +static void updateViewOptionsFromTestItself(ViewOptions& viewOptions, const TestInvocation& test) We need a better name to use for these comments than "test itself".
Myles C. Maxfield
Comment 5 2015-09-01 12:53:15 PDT
Comment on attachment 260355 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=260355&action=review >> Tools/ChangeLog:15 >> + which were not found due to the system language being non-English. > > What prevents us from doing it the right way? WebContent language is sent from UI process in a bootstrap message already, we just need to send the fake one. Note to self: ProcessLauncherMac is the one who seems to do this.
Myles C. Maxfield
Comment 6 2015-09-01 13:12:23 PDT
Comment on attachment 260355 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=260355&action=review >>> Tools/ChangeLog:15 >>> + which were not found due to the system language being non-English. >> >> What prevents us from doing it the right way? WebContent language is sent from UI process in a bootstrap message already, we just need to send the fake one. > > Note to self: ProcessLauncherMac is the one who seems to do this. It appears that we never do anything with the -AppleLanguages command line argument. Maybe CF is doing some magic?
Myles C. Maxfield
Comment 7 2015-09-01 13:13:31 PDT
Comment on attachment 260355 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=260355&action=review > Tools/WebKitTestRunner/InjectedBundle/mac/InjectedBundleMac.mm:74 > + @"AppleLanguages": static_cast<NSArray *>(languages.get()), Mavericks doesn't like this static_cast.
Myles C. Maxfield
Comment 8 2015-09-01 13:15:46 PDT
Comment on attachment 260355 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=260355&action=review >>>> Tools/ChangeLog:15 >>>> + which were not found due to the system language being non-English. >>> >>> What prevents us from doing it the right way? WebContent language is sent from UI process in a bootstrap message already, we just need to send the fake one. >> >> Note to self: ProcessLauncherMac is the one who seems to do this. > > It appears that we never do anything with the -AppleLanguages command line argument. Maybe CF is doing some magic? WebProcess::initializeWebProcess() also sets some language stuff.
Myles C. Maxfield
Comment 9 2015-09-01 13:23:45 PDT
Comment on attachment 260355 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=260355&action=review >>>>> Tools/ChangeLog:15 >>>>> + which were not found due to the system language being non-English. >>>> >>>> What prevents us from doing it the right way? WebContent language is sent from UI process in a bootstrap message already, we just need to send the fake one. >>> >>> Note to self: ProcessLauncherMac is the one who seems to do this. >> >> It appears that we never do anything with the -AppleLanguages command line argument. Maybe CF is doing some magic? > > WebProcess::initializeWebProcess() also sets some language stuff. The UI process asks WebCore's userPreferredLanguages() for the current language, and sends that inside the WebProcessCreationParameters, where WebCore will just override the language setting with overrideUserPreferredLanguages(). The UI process listens for changes to the system language (using WebCore's Language.cpp stuff), and broadcasts a message to all web processes with the new information. When the web process receives this information, it will do the same WebCore-based language overriding.
Myles C. Maxfield
Comment 10 2015-09-01 15:12:26 PDT
Comment on attachment 260355 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=260355&action=review >>>>>> Tools/ChangeLog:15 >>>>>> + which were not found due to the system language being non-English. >>>>> >>>>> What prevents us from doing it the right way? WebContent language is sent from UI process in a bootstrap message already, we just need to send the fake one. >>>> >>>> Note to self: ProcessLauncherMac is the one who seems to do this. >>> >>> It appears that we never do anything with the -AppleLanguages command line argument. Maybe CF is doing some magic? >> >> WebProcess::initializeWebProcess() also sets some language stuff. > > The UI process asks WebCore's userPreferredLanguages() for the current language, and sends that inside the WebProcessCreationParameters, where WebCore will just override the language setting with overrideUserPreferredLanguages(). > > The UI process listens for changes to the system language (using WebCore's Language.cpp stuff), and broadcasts a message to all web processes with the new information. When the web process receives this information, it will do the same WebCore-based language overriding. CF is doing magic. It calls _NSGetArgv() in its initialization routine.
Alexey Proskuryakov
Comment 11 2015-09-01 15:43:46 PDT
> CF is doing magic. Correct. But also, -AppleLanguages is for the old WebProcess code path, the XPC service uses _CFBundleSetupXPCBootstrap (and we can add to the bootstrap message if needed).
Myles C. Maxfield
Comment 12 2015-09-01 15:46:10 PDT
Comment on attachment 260355 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=260355&action=review >>>>>>> Tools/ChangeLog:15 >>>>>>> + which were not found due to the system language being non-English. >>>>>> >>>>>> What prevents us from doing it the right way? WebContent language is sent from UI process in a bootstrap message already, we just need to send the fake one. >>>>> >>>>> Note to self: ProcessLauncherMac is the one who seems to do this. >>>> >>>> It appears that we never do anything with the -AppleLanguages command line argument. Maybe CF is doing some magic? >>> >>> WebProcess::initializeWebProcess() also sets some language stuff. >> >> The UI process asks WebCore's userPreferredLanguages() for the current language, and sends that inside the WebProcessCreationParameters, where WebCore will just override the language setting with overrideUserPreferredLanguages(). >> >> The UI process listens for changes to the system language (using WebCore's Language.cpp stuff), and broadcasts a message to all web processes with the new information. When the web process receives this information, it will do the same WebCore-based language overriding. > > CF is doing magic. It calls _NSGetArgv() in its initialization routine. By default, WebKitTestRunner will use XPC to start the service, thereby short-circuiting the AppleLanguages command line argument. See shouldUseXPC() in WebProcessProxymac.mm. You can still use posix_spawn if you set the WebKit2UseXPCServiceForWebProcess default to false
Myles C. Maxfield
Comment 13 2015-09-01 17:22:18 PDT
Comment on attachment 260355 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=260355&action=review >>>>>>>> Tools/ChangeLog:15 >>>>>>>> + which were not found due to the system language being non-English. >>>>>>> >>>>>>> What prevents us from doing it the right way? WebContent language is sent from UI process in a bootstrap message already, we just need to send the fake one. >>>>>> >>>>>> Note to self: ProcessLauncherMac is the one who seems to do this. >>>>> >>>>> It appears that we never do anything with the -AppleLanguages command line argument. Maybe CF is doing some magic? >>>> >>>> WebProcess::initializeWebProcess() also sets some language stuff. >>> >>> The UI process asks WebCore's userPreferredLanguages() for the current language, and sends that inside the WebProcessCreationParameters, where WebCore will just override the language setting with overrideUserPreferredLanguages(). >>> >>> The UI process listens for changes to the system language (using WebCore's Language.cpp stuff), and broadcasts a message to all web processes with the new information. When the web process receives this information, it will do the same WebCore-based language overriding. >> >> CF is doing magic. It calls _NSGetArgv() in its initialization routine. > > By default, WebKitTestRunner will use XPC to start the service, thereby short-circuiting the AppleLanguages command line argument. See shouldUseXPC() in WebProcessProxymac.mm. You can still use posix_spawn if you set the WebKit2UseXPCServiceForWebProcess default to false InjectedBundle's platformInitialize() gets run before WebProcess::initializeWebProcess().
Myles C. Maxfield
Comment 14 2015-09-01 17:49:52 PDT
WebKit Commit Bot
Comment 15 2015-09-01 17:51:43 PDT
Attachment 260397 [details] did not pass style-queue: ERROR: Source/WebCore/ChangeLog:8: You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible. [changelog/nonewtests] [5] ERROR: Tools/WebKitTestRunner/TestController.cpp:57: Streams are highly discouraged. [readability/streams] [3] Total errors found: 2 in 17 files If any of these errors are false positives, please file a bug against check-webkit-style.
Alexey Proskuryakov
Comment 16 2015-09-01 18:40:41 PDT
Comment on attachment 260397 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=260397&action=review > Source/WebCore/ChangeLog:16 > + This patch sets the system language early in the lifetime of the web process, inside the > + initialization of the injected bundle. This is early enough for Core Text, but too late for > + AppKit. As such, the solution isn't perfect, but it is enough to test recent text breakages Still unsure why this cannot use bootstrap.
Myles C. Maxfield
Comment 17 2015-09-01 18:55:10 PDT
Comment on attachment 260397 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=260397&action=review >> Source/WebCore/ChangeLog:16 >> + AppKit. As such, the solution isn't perfect, but it is enough to test recent text breakages > > Still unsure why this cannot use bootstrap. What do you mean by "bootstrap?"
Alexey Proskuryakov
Comment 18 2015-09-01 19:08:29 PDT
Please see comment 11.
Build Bot
Comment 19 2015-09-01 19:20:36 PDT
Comment on attachment 260397 [details] Patch Attachment 260397 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/131455 Number of test failures exceeded the failure limit.
Build Bot
Comment 20 2015-09-01 19:20:39 PDT
Created attachment 260400 [details] Archive of layout-test-results from ews107 for mac-mavericks-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews107 Port: mac-mavericks-wk2 Platform: Mac OS X 10.9.5
Myles C. Maxfield
Comment 21 2015-09-01 19:22:08 PDT
(In reply to comment #19) > Comment on attachment 260397 [details] > Patch > > Attachment 260397 [details] did not pass mac-wk2-ews (mac-wk2): > Output: http://webkit-queues.webkit.org/results/131455 > > Number of test failures exceeded the failure limit. Ah, you're talking about my comment in preferredBundleLocalizationName()
Myles C. Maxfield
Comment 22 2015-09-01 20:21:29 PDT
(In reply to comment #21) > (In reply to comment #19) > > Comment on attachment 260397 [details] > > Patch > > > > Attachment 260397 [details] did not pass mac-wk2-ews (mac-wk2): > > Output: http://webkit-queues.webkit.org/results/131455 > > > > Number of test failures exceeded the failure limit. > > Ah, you're talking about my comment in preferredBundleLocalizationName() It turns out that there are two different notions of system language: one for CFBundle and one for CFLocale. Command line arguments and XPC bootstrap arguments affect the CFBundle languages. The code that I'm trying to test consults with CFLocale languages.
Myles C. Maxfield
Comment 23 2015-09-01 20:23:16 PDT
(In reply to comment #22) > (In reply to comment #21) > > (In reply to comment #19) > > > Comment on attachment 260397 [details] > > > Patch > > > > > > Attachment 260397 [details] did not pass mac-wk2-ews (mac-wk2): > > > Output: http://webkit-queues.webkit.org/results/131455 > > > > > > Number of test failures exceeded the failure limit. > > > > Ah, you're talking about my comment in preferredBundleLocalizationName() > > It turns out that there are two different notions of system language: one > for CFBundle and one for CFLocale. Command line arguments and XPC bootstrap > arguments affect the CFBundle languages. The code that I'm trying to test > consults with CFLocale languages. In particular, the "Language & Region" pane in System Preferences seems to have no effect on CFBundle languages.
Alexey Proskuryakov
Comment 24 2015-09-01 22:02:19 PDT
This is not correct. CFBundle does have its own language, but it directly depends on AppleLanguages. Also, while CFBundle bootstrap handling only affects bundle language, nothing stops us from passing and handling AppleLanguages as a separate bootstrap dictionary key.
Myles C. Maxfield
Comment 25 2015-09-02 23:54:12 PDT
Myles C. Maxfield
Comment 26 2015-09-02 23:54:58 PDT
Comment on attachment 260491 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=260491&action=review > Tools/WebKitTestRunner/TestController.cpp:856 > +static std::string testPath(const WKURLRef url) > +{ > + auto scheme = adoptWK(WKURLCopyScheme(url)); > + if (WKStringIsEqualToUTF8CStringIgnoringCase(scheme.get(), "file")) { > + auto path = adoptWK(WKURLCopyPath(url)); > + auto buffer = std::vector<char>(WKStringGetMaximumUTF8CStringSize(path.get())); > + auto length = WKStringGetUTF8CString(path.get(), buffer.data(), buffer.size()); > + return std::string(buffer.data(), length); > + } > + return std::string(); > +} > + > +static void updateViewOptionsFromTestHeader(ViewOptions& viewOptions, const TestInvocation& test) > +{ > + std::string filename = testPath(test.url()); > + if (filename.empty()) > + return; > + > + std::string options; > + std::ifstream testFile(filename.data()); > + if (!testFile.good()) > + return; > + getline(testFile, options); > + std::string beginString("webkit-test-runner [ "); > + std::string endString(" ]"); > + size_t beginLocation = options.find(beginString); > + if (beginLocation == std::string::npos) > + return; > + size_t endLocation = options.find(endString, beginLocation); > + if (endLocation == std::string::npos) { > + LOG_ERROR("Could not find end of test header in %s", filename.c_str()); > + return; > + } > + std::string pairString = options.substr(beginLocation + beginString.size(), endLocation - (beginLocation + beginString.size())); > + size_t pairStart = 0; > + while (pairStart < pairString.size()) { > + size_t pairEnd = pairString.find(" ", pairStart); > + if (pairEnd == std::string::npos) > + pairEnd = pairString.size(); > + size_t equalsLocation = pairString.find("=", pairStart); > + if (equalsLocation == std::string::npos) { > + LOG_ERROR("Malformed option in test header (could not find '=' character) in %s", filename.c_str()); > + break; > + } > + auto key = pairString.substr(pairStart, equalsLocation - pairStart); > + auto value = pairString.substr(equalsLocation + 1, pairEnd - (equalsLocation + 1)); > + if (key == "language") > + String(value.c_str()).split(",", false, viewOptions.overrideLanguages); > + // Other options processing goes here. > + pairStart = pairEnd + 1; > + } > +} These functions are a part of https://bugs.webkit.org/show_bug.cgi?id=148723
Myles C. Maxfield
Comment 27 2015-09-03 12:46:41 PDT
WebKit Commit Bot
Comment 28 2015-09-03 12:49:02 PDT
Attachment 260511 [details] did not pass style-queue: ERROR: Tools/WebKitTestRunner/TestController.cpp:57: Streams are highly discouraged. [readability/streams] [3] Total errors found: 1 in 21 files If any of these errors are false positives, please file a bug against check-webkit-style.
Myles C. Maxfield
Comment 29 2015-09-03 17:46:50 PDT
Myles C. Maxfield
Comment 30 2015-09-03 18:22:48 PDT
Alexey Proskuryakov
Comment 31 2015-09-03 19:36:33 PDT
Comment on attachment 260547 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=260547&action=review > Source/WebKit2/Shared/EntryPointUtilities/mac/XPCService/XPCServiceMain.Development.mm:170 > + [newLanguages.get() addObject:[NSString stringWithCString:xpc_string_get_string_ptr(value) encoding:NSUTF8StringEncoding]]; Is this .get() needed? > Source/WebKit2/UIProcess/API/C/WKContext.cpp:608 > +void WKContextOverrideSystemLanguage(WKContextRef contextRef, WKArrayRef arrayRef) I was under the impression that we were supposed to use Objective-C API. I may be wrong, could you please check with Anders before landing? > Source/WebKit2/UIProcess/WebProcessProxy.cpp:139 > + languageString.append(String(ASCIILiteral(","))); Can be simply: languageString.append(','); > LayoutTests/platform/mac-wk1/TestExpectations:149 > +# Testing the system language decaratively only makes sense in WK1. Typo: declaratively. Does this comment need to say "...in WK2"? I'd even add ", because it's implemented in WebKitTestRunner by launching a new WebContent process". > LayoutTests/platform/mac-wk1/TestExpectations:150 > +[ Mavericks Yosemite ElCapitan ] fast/text/international/system-language [ Failure ] If this is supposed to be a failure on all OS X versions, then there shouldn't be any qualifiers: fast/text/international/system-language [ Failure ] Otherwise, we'd have to revisit this for every future OS X version, and every time we drop support for the previous one.
Myles C. Maxfield
Comment 32 2015-09-03 19:40:00 PDT
https://bugs.webkit.org/show_bug.cgi?id=148775 adds tests which use this mechanism.
Myles C. Maxfield
Comment 33 2015-09-03 19:58:13 PDT
Myles C. Maxfield
Comment 34 2015-09-04 13:41:03 PDT
Comment on attachment 260558 [details] WIP View in context: https://bugs.webkit.org/attachment.cgi?id=260558&action=review > Tools/WebKitTestRunner/TestController.cpp:874 > + updateViewOptionsFromTestHeader(viewOptions, test); Nope.
Myles C. Maxfield
Comment 35 2015-09-04 13:47:36 PDT
Comment on attachment 260558 [details] WIP View in context: https://bugs.webkit.org/attachment.cgi?id=260558&action=review > Source/WebKit2/UIProcess/WebProcessPool.cpp:1461 > + WebCore::overrideUserPreferredLanguages(result); We shouldn't call into WebCore from the UIProcess.
Myles C. Maxfield
Comment 36 2015-09-04 16:16:56 PDT
Anders Carlsson
Comment 37 2015-09-04 16:31:42 PDT
Comment on attachment 260642 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=260642&action=review > Source/WebKit2/ChangeLog:8 > + This patch creates a new API function, WKContextOverrideSystemLanguage(), which Please update this ChangeLog. > Source/WebKit2/UIProcess/API/C/WKContextConfigurationPrivate.h:35 > +WK_EXPORT void WKContextConfigurationSetOverrideLanguages(WKContextConfigurationRef configuration, WKArrayRef overrideLanguages); You should add a getter as well. Also, I don't think this needs to be in a separate file - it can just go into WKContextConfigurationRef (since all of the WebKit C SPI is private). > Source/WebKit2/UIProcess/WebProcessProxy.cpp:109 > + , m_initialLaunchOptions(initialLaunchOptions) I don't like storing launch options here - the WebProcessProxy knows its WebContext which knows its configuration which knows the override languages so it should be able to get them from there.
Myles C. Maxfield
Comment 38 2015-09-04 17:40:45 PDT
Anders Carlsson
Comment 39 2015-09-08 11:28:00 PDT
Comment on attachment 260660 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=260660&action=review > Source/WebKit2/UIProcess/API/C/WKContextConfigurationRef.cpp:140 > + Vector<String> result; > + API::Array* overrideLanguagesImpl = toImpl(overrideLanguages); > + if (overrideLanguagesImpl) { > + for (size_t i = 0; i < overrideLanguagesImpl->size(); ++i) { > + if (API::String* font = overrideLanguagesImpl->at<API::String>(i)) > + result.append(font->string()); > + } > + } > + toImpl(configuration)->setOverrideLanguages(WTF::move(result)); I think you can use API::Array::toStringVector() here.
Myles C. Maxfield
Comment 40 2015-09-08 21:41:25 PDT
Myles C. Maxfield
Comment 41 2015-09-12 20:48:09 PDT
Note You need to log in before you can comment on or make changes to this bug.