Bug 157756 – WTF should know about Language

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Template Framework

[Filip Pizlo]

Patch forthcoming.

[Filip Pizlo]

Created attachment 279047 [details]
something like this maybe

[Filip Pizlo]

Created attachment 279081 [details]
or maybe this

[Filip Pizlo]

Created attachment 279084 [details]
less wrong

[Filip Pizlo]

Created attachment 279100 [details]
more of it compiles than ever before

[Filip Pizlo]

Created attachment 279102 [details]
the patch

[WebKit Commit Bot]

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


ERROR: Source/WebKit2/UIProcess/Launcher/mac/ProcessLauncherMac.mm:43:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WTF/wtf/PlatformGTK.cmake:10:  Alphabetical sorting problem. "PlatformUserPreferredLanguagesUnix.cpp" should be before "glib/RunLoopGLib.cpp".  [list/order] [5]
ERROR: Source/WTF/wtf/spi/cf/CFBundleSPI.h:40:  The parameter name "stringEncoding" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WTF/wtf/spi/cf/CFBundleSPI.h:41:  The parameter name "stringEncoding" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WebCore/platform/graphics/avfoundation/objc/SourceBufferPrivateAVFObjC.mm:52:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 5 in 65 files


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

[Filip Pizlo]

I CC'd a bunch of people because this touches port-specific things in pretty much every port.

[Alex Christensen]

Is something motivating this change?

[Alex Christensen]

Comment on attachment 279102 [details]
the patch

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

> Source/WebCore/platform/Language.cpp:41
> +static void registerLanguageDidChangeCallbackIfNecessary()

What's this all about?

[Filip Pizlo]

(In reply to comment #8)
> Is something motivating this change?

https://bugs.webkit.org/show_bug.cgi?id=157755

[Filip Pizlo]

(In reply to comment #9)
> Comment on attachment 279102 [details]
> the patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=279102&action=review
> 
> > Source/WebCore/platform/Language.cpp:41
> > +static void registerLanguageDidChangeCallbackIfNecessary()
> 
> What's this all about?

The changelog has more details but basically, when the platform preferred language was detected in WebCore, languageDidChange would get called by that code. Now that code is in WTF but languageDidChane is still in WebCore so we need a callback mechanism.

[Geoffrey Garen]

Comment on attachment 279102 [details]
the patch

r=me

[Alexey Proskuryakov]

Comment on attachment 279102 [details]
the patch

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

Looks fine to me.

> Source/WTF/ChangeLog:13
> +        - Move WebCore::platformUserPreferredLanguages() to WTF::platformUserPreferredLanguages().

One question that I'm not sure how to answer is whether sandboxed apps using JSC still work. AppSandboxed ones hopefully do (still could be worth testing), but Apple ones with custom sandboxes are more concerning.

> Source/WTF/ChangeLog:20
> +        Moving platformUserPreferredLanguages() is mostly easy except for the weird callback.
> +        That function would call languageDidChange(), which needs to stay in WebCore. So, this
> +        gives WebCore the ability to register a languageDidChange callback. Other than this new

So other JSC clients will not dynamically adapt to system language change?

Note that I'm not entirely sure if they should, or even if Safari should.

> Source/WTF/wtf/BlockObjCExceptions.mm:34
> +    NSLog(@"*** WebKit discarding exception: <%@> %@", [exception name], [exception reason]);

I guess we can still say WebKit here, although it would confuse pure JSC users.

[Filip Pizlo]

(In reply to comment #13)
> Comment on attachment 279102 [details]
> the patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=279102&action=review
> 
> Looks fine to me.
> 
> > Source/WTF/ChangeLog:13
> > +        - Move WebCore::platformUserPreferredLanguages() to WTF::platformUserPreferredLanguages().
> 
> One question that I'm not sure how to answer is whether sandboxed apps using
> JSC still work. AppSandboxed ones hopefully do (still could be worth
> testing), but Apple ones with custom sandboxes are more concerning.

Is there a way to defend against sandboxing so that this returns some sensible default (heck, even en_us) when the sandbox doesn't allow us to do anything smarter?

> 
> > Source/WTF/ChangeLog:20
> > +        Moving platformUserPreferredLanguages() is mostly easy except for the weird callback.
> > +        That function would call languageDidChange(), which needs to stay in WebCore. So, this
> > +        gives WebCore the ability to register a languageDidChange callback. Other than this new
> 
> So other JSC clients will not dynamically adapt to system language change?

They will, but they don't need a callback to do it.

The current JSC implementation in trunk will ask WebCore for the "default language" every time a global object's code asks for the default locale for the first time.  So, if you change the platform settings, they will take effect no later than the next time JSC creates a global object.

Once I finish https://bugs.webkit.org/show_bug.cgi?id=157755, it will ask either WTF (via platformUserPreferredLanguages()) or WebCore (the same way it does now).  It will still ask WebCore when the global object is a DOM global object, since WebCore adds its own abilities to override this setting.

> 
> Note that I'm not entirely sure if they should, or even if Safari should.

Yeah, it's all a bit funky.  I wonder if removing that functionality would break anything at all.

> 
> > Source/WTF/wtf/BlockObjCExceptions.mm:34
> > +    NSLog(@"*** WebKit discarding exception: <%@> %@", [exception name], [exception reason]);
> 
> I guess we can still say WebKit here, although it would confuse pure JSC
> users.

Oh, that's funny.  I added a FIXME and filed https://bugs.webkit.org/show_bug.cgi?id=157804.

[Filip Pizlo]

Created attachment 279142 [details]
patch for landing

[WebKit Commit Bot]

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


ERROR: Source/WebKit2/UIProcess/Launcher/mac/ProcessLauncherMac.mm:43:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WTF/wtf/PlatformGTK.cmake:10:  Alphabetical sorting problem. "PlatformUserPreferredLanguagesUnix.cpp" should be before "glib/RunLoopGLib.cpp".  [list/order] [5]
ERROR: Source/WTF/wtf/spi/cf/CFBundleSPI.h:40:  The parameter name "stringEncoding" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WTF/wtf/spi/cf/CFBundleSPI.h:41:  The parameter name "stringEncoding" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WebCore/platform/graphics/avfoundation/objc/SourceBufferPrivateAVFObjC.mm:52:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 5 in 65 files


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

[Filip Pizlo]

Comment on attachment 279142 [details]
patch for landing

I'll let CQ worry about this beast.

[Alexey Proskuryakov]

> Is there a way to defend against sandboxing so that this returns some sensible default (heck, even en_us) when the sandbox doesn't allow us to do anything smarter?

It may happen to work that way - library calls below us will fail (e.g. CFPreferences), and ideally, we'll just hit all the right fallback paths.

The cost will of course be that sandbox violation logs will be generated, which is far from free.

[Filip Pizlo]

(In reply to comment #18)
> > Is there a way to defend against sandboxing so that this returns some sensible default (heck, even en_us) when the sandbox doesn't allow us to do anything smarter?
> 
> It may happen to work that way - library calls below us will fail (e.g.
> CFPreferences), and ideally, we'll just hit all the right fallback paths.
> 
> The cost will of course be that sandbox violation logs will be generated,
> which is far from free.

That's probably pretty good.  Hopefully our caching causes us to remember the failure, so that we do this only once per process.

[WebKit Commit Bot]

Comment on attachment 279142 [details]
patch for landing

Clearing flags on attachment: 279142

Committed r201038: <http://trac.webkit.org/changeset/201038>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Alexey Proskuryakov]

> > Note that I'm not entirely sure if they should, or even if Safari should.

> Yeah, it's all a bit funky.  I wonder if removing that functionality would break anything at all.

Well, it would definitely change the behavior - once you change the language, new tabs will pick it, but old tabs will keep the old one.

What I don't know is what the expectation for user experience when changing languages is. The pref pane says that running applications will need to be restarted, but in personal experience, that may not be quite right, and may not match actual user expectations. I may have the wrong impression though.

[Filip Pizlo]

(In reply to comment #22)
> > > Note that I'm not entirely sure if they should, or even if Safari should.
> 
> > Yeah, it's all a bit funky.  I wonder if removing that functionality would break anything at all.
> 
> Well, it would definitely change the behavior - once you change the
> language, new tabs will pick it, but old tabs will keep the old one.

Right.  It's a behavior change for sure, but I'm not sure if it's a bad one or a benign one.

> 
> What I don't know is what the expectation for user experience when changing
> languages is. The pref pane says that running applications will need to be
> restarted, but in personal experience, that may not be quite right, and may
> not match actual user expectations. I may have the wrong impression though.

My impression has always been that you need to restart an app for the changes to fully take effect, but that the changes may take partial effect in some apps immediately.