Bug 148671 – [Cocoa] Allow testing with the system language

• Component: New Bugs
• Status: RESOLVED FIXED

[Myles C. Maxfield]

[Cocoa] Allow setting the system font

[Myles C. Maxfield]

Created attachment 260354 [details]
Patch

[Myles C. Maxfield]

Created attachment 260355 [details]
Patch

[WebKit Commit Bot]

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 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 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 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 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 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 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 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]

> 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 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 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]

Created attachment 260397 [details]
Patch

[WebKit Commit Bot]

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 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 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]

Please see comment 11.

[Build Bot]

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]

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]

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

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

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

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]

Created attachment 260491 [details]
Patch

[Myles C. Maxfield]

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]

Created attachment 260511 [details]
Patch

[WebKit Commit Bot]

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]

Created attachment 260542 [details]
Patch

[Myles C. Maxfield]

Created attachment 260547 [details]
Patch

[Alexey Proskuryakov]

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]

https://bugs.webkit.org/show_bug.cgi?id=148775 adds tests which use this mechanism.

[Myles C. Maxfield]

Created attachment 260558 [details]
WIP

[Myles C. Maxfield]

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 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]

Created attachment 260642 [details]
Patch

[Anders Carlsson]

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]

Created attachment 260660 [details]
Patch

[Anders Carlsson]

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]

Created attachment 260830 [details]
Patch

[Myles C. Maxfield]

Committed r189669: <http://trac.webkit.org/changeset/189669>