Bug 188111

Nowadays it fails even earlier:

ERROR:../../Tools/TestWebKitAPI/Tests/WebKitGLib/TestWebKitWebContext.cpp:359:void testWebContextLanguages(WebViewTest*, gconstpointer): assertion failed (mainResourceDataSize == strlen(expectedLanguages)): (5 == 25)

Problem is the language is set to "en-US" rather than the expected "en,ES-es;q=0.90,dE;q=0.80".

(In reply to Michael Catanzaro from comment #1)
> Nowadays it fails even earlier:
> 
> ERROR:../../Tools/TestWebKitAPI/Tests/WebKitGLib/TestWebKitWebContext.cpp:
> 359:void testWebContextLanguages(WebViewTest*, gconstpointer): assertion
> failed (mainResourceDataSize == strlen(expectedLanguages)): (5 == 25)
> 
> Problem is the language is set to "en-US" rather than the expected
> "en,ES-es;q=0.90,dE;q=0.80".

This one is fixed by bug #222347

Created attachment 421392 [details]
Patch

Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See https://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

Comment on attachment 421392 [details]
Patch

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

I was a bit confused how we wound up with a bug from 2018 fixing a regression from 2020. I see you just repurposed this bug to fix the new problem. OK.

> Source/WebKit/ChangeLog:9
> +        are no loner sending the new overrides to the web process. Instead of calling overrideUserPreferredLanguages()

no longer

Hi Chris, could you review this one please?

I didn't know this bug was so old.

Comment on attachment 421392 [details]
Patch

Anyway it looks good to me, but requires owner approval (ideally from Chris).

Comment on attachment 421392 [details]
Patch

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

> Source/WebKit/UIProcess/WebProcessPool.cpp:-308
> -    addLanguageChangeObserver(this, languageChanged);

I don't understand why we are getting rid of the language change observer in the UIProcess. Seems like we would want to know and notify the WebProcesses if the user changed the language, no?

(In reply to Chris Dumez from comment #9)
> Comment on attachment 421392 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=421392&action=review
> 
> > Source/WebKit/UIProcess/WebProcessPool.cpp:-308
> > -    addLanguageChangeObserver(this, languageChanged);
> 
> I don't understand why we are getting rid of the language change observer in
> the UIProcess. Seems like we would want to know and notify the WebProcesses
> if the user changed the language, no?

Because after this patch overrideUserPreferredLanguages() is no longer called from the UI process. Right now is only called from the GLib API, unless I'm missing something.

Chris, are you ok with this then?

(In reply to Carlos Garcia Campos from comment #11)
> Chris, are you ok with this then?

I will look into this today. If this is not used on macOS then how do we detect and apply language change on macOS? macOS definitely has to do it in the UIProcess (and not WebProcess) due to sandboxing reasons). Seems like either macOS uses this or it doesn't and we may have a bug to fix (or there is another mechanism that macOS uses and that I am missing).

Comment on attachment 421392 [details]
Patch

LGTM. The WebProcess is able to listen to change notification on macOS.

Committed r273735: <https://commits.webkit.org/r273735>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 421392 [details].