Bug 157756

Created attachment 279047 [details] something like this maybe

Created attachment 279081 [details] or maybe this

Created attachment 279084 [details] less wrong

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

Created attachment 279102 [details] the patch

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.

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

Is something motivating this change?

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?

(In reply to comment #8) > Is something motivating this change? https://bugs.webkit.org/show_bug.cgi?id=157755

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

Comment on attachment 279102 [details] the patch r=me

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.

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

Created attachment 279142 [details] patch for landing

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.

Comment on attachment 279142 [details] patch for landing I'll let CQ worry about this beast.

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

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

Comment on attachment 279142 [details] patch for landing Clearing flags on attachment: 279142 Committed r201038: <http://trac.webkit.org/changeset/201038>

All reviewed patches have been landed. Closing bug.

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

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