Replace use of WKCopyCFLocalizationPreferredName with NSLocale public API
Created attachment 236820 [details] Patch
Attachment 236820 [details] did not pass style-queue: ERROR: Source/WebKit/mac/Plugins/Hosted/NetscapePluginHostManager.mm:119: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Total errors found: 1 in 7 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 236820 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=236820&action=review This is a very welcome improvement. It's quite difficult to verify that behavior is correct in all code paths, but we should definitely stop using script manager, it's just crazy at this point. > Source/WebKit2/UIProcess/Launcher/mac/ProcessLauncherMac.mm:214 > - RetainPtr<CFStringRef> localization = adoptCF(WKCopyCFLocalizationPreferredName(0)); > + NSString *localization = [[NSLocale currentLocale] localeIdentifier]; > if (localization && _CFBundleSetupXPCBootstrapPtr()) { This null check is a mistake from adopting _CFBundleSetupXPCBootstrap, the code for getting localization should be simply removed. > Source/WebKit2/UIProcess/Launcher/mac/ProcessLauncherMac.mm:460 > - CString appleLanguagesArgument = String("('" + String(adoptCF(WKCopyCFLocalizationPreferredName(0)).get()) + "')").utf8(); > + CString appleLanguagesArgument = String("('" + String([[NSLocale currentLocale] localeIdentifier]) + "')").utf8(); I think that this is wrong. The goal here is to get main bundle localization, not user locale. Also, locale is not language. This code only runs for legacy process (not XPC), so it's a little tricky to test. I would just add these lines in some regular code path and rebuild to verify what they do. The way to test this is to set system language to anything but English, re-login, and then to run your local build of Safari. It is not localized, so WKCopyCFLocalizationPreferredName will return English, and [NSLocale currentLocale] will return the locale as set up in system preferences. I'm not 100% sure about what we need instead, probably _CFBundleCopyLanguageSearchListInBundle(CFBundleGetMainBundle()). > Source/WebKit/mac/Plugins/WebBasePluginPackage.mm:194 > + if (![localizationName isEqualToString:[[NSLocale currentLocale] localeIdentifier]]) I have never seen this code before, but looks like we need main process localization language here as well, not current locale. > Source/WebKit/mac/Plugins/Hosted/NetscapePluginHostManager.mm:119 > - (NSString *)localization.get(), @"localization", > + [[NSLocale currentLocale] localeIdentifier], @"localization", Ditto.
Created attachment 236825 [details] Patch
Attachment 236825 [details] did not pass style-queue: ERROR: Source/WebKit/mac/Plugins/Hosted/NetscapePluginHostManager.mm:119: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Total errors found: 1 in 7 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 236825 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=236825&action=review > Source/WebKit2/UIProcess/Launcher/mac/ProcessLauncherMac.mm:213 > + if (_CFBundleSetupXPCBootstrapPtr()) { This is not necessary any more, the code can be made nicer by replacing SOFT_LINK_OPTIONAL with SOFT_LINK. > Source/WebKit2/UIProcess/Launcher/mac/ProcessLauncherMac.mm:461 > + RetainPtr<CFArrayRef> preferredLocalizations = adoptCF(CFBundleCopyPreferredLocalizationsFromArray(bundleLocalizations.get())); > + CFStringRef localization = (CFStringRef)CFArrayGetValueAtIndex(preferredLocalizations.get(), 0); Does something guarantee that CFBundleCopyPreferredLocalizationsFromArray returns an non-empty array? If not, it would be safe to check the length, and fall back to "en". > Source/WebKit/mac/Plugins/WebBasePluginPackage.mm:194 > - if (![localizationName isEqualToString:[[self class] preferredLocalizationName]]) > + if (![localizationName isEqualToString:[[NSLocale currentLocale] localeIdentifier]]) Was it intentional that you didn't make the same fix here? > Source/WebKit/mac/Plugins/Hosted/NetscapePluginHostManager.mm:119 > - (NSString *)localization.get(), @"localization", > + [[NSLocale currentLocale] localeIdentifier], @"localization", And here?
1. WebBasePluginPackage.mm appears to cache MIME type information from a plug-in. Since type descriptions can be localized, it stores the current localization, and if that doesn't match, it re-creates the cache. It seems OK to change the values here, as the cache will be re-created anyway. I'm not sure if we want system primary language or UI process language - the latter would be better for UI, but then we'd thrash the cache file every time an unlocalized process uses the plug-in after Safari. But I think that we should make a language code out of the locale code for consistency. 2. NetscapePluginHostManager.mm just passes the localization down to plug-in process. This seems like the same thing that you fixed for WebKit2, so maybe we should share and reuse the code.
Comment on attachment 236825 [details] Patch Attachment 236825 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/6456667931672576 New failing tests: js/dom/navigator-language.html
Created attachment 236834 [details] Archive of layout-test-results from webkit-ews-01 for mac-mountainlion The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: webkit-ews-01 Port: mac-mountainlion Platform: Mac OS X 10.8.5
Comment on attachment 236825 [details] Patch Attachment 236825 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/4739110241566720 New failing tests: js/dom/navigator-language.html
Created attachment 236838 [details] Archive of layout-test-results from webkit-ews-06 for mac-mountainlion The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: webkit-ews-06 Port: mac-mountainlion Platform: Mac OS X 10.8.5
Comment on attachment 236825 [details] Patch Attachment 236825 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/6302671610839040 New failing tests: compositing/checkerboard.html compositing/bounds-in-flipped-writing-mode.html animations/3d/transform-origin-vs-functions.html http/tests/appcache/abort-cache-onchecking.html http/tests/appcache/abort-cache-onchecking-resource-404.html http/tests/appcache/abort-cache-onchecking-manifest-404.html canvas/philip/tests/2d.clearRect+fillRect.basic.html animations/animation-controller-drt-api.html canvas/philip/tests/2d.canvas.reference.html animations/3d/state-at-end-event-transform.html canvas/philip/tests/2d.clearRect+fillRect.alpha0.5.html accessibility/alt-tag-on-image-with-nonimage-role.html canvas/philip/tests/2d.clearRect+fillRect.alpha0.html
Created attachment 236851 [details] Archive of layout-test-results from webkit-ews-12 for mac-mountainlion-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: webkit-ews-12 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.5
Created attachment 236859 [details] New version; fixes correctness issues in last patch
The latest patch more closely replicates what we used to do with SPI, but using only public API.
Attachment 236859 [details] did not pass style-queue: ERROR: Source/WebKit/mac/Plugins/Hosted/NetscapePluginHostManager.mm:119: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Total errors found: 1 in 10 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 236860 [details] With proper includes this time.
Attachment 236860 [details] did not pass style-queue: ERROR: Source/WebKit/mac/Plugins/Hosted/NetscapePluginHostManager.mm:120: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Total errors found: 1 in 10 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 236861 [details] With actually proper includes this time.
Attachment 236861 [details] did not pass style-queue: ERROR: Source/WebKit/mac/Plugins/Hosted/NetscapePluginHostManager.mm:120: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/WebKit/mac/Plugins/WebBasePluginPackage.mm:29: Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] Total errors found: 2 in 10 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 236861 [details] With actually proper includes this time. Attachment 236861 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/6671474815401984 New failing tests: media/video-trackmenu-selection.html
Created attachment 236863 [details] Archive of layout-test-results from webkit-ews-11 for mac-mountainlion-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: webkit-ews-11 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.5
(In reply to comment #21) > (From update of attachment 236861 [details]) > Attachment 236861 [details] did not pass mac-wk2-ews (mac-wk2): > Output: http://webkit-queues.appspot.com/results/6671474815401984 > > New failing tests: > media/video-trackmenu-selection.html The details of this failure totally look like something that could have been caused by my patch somehow, but I cannot reproduce locally :-(
Comment on attachment 236861 [details] With actually proper includes this time. View in context: https://bugs.webkit.org/attachment.cgi?id=236861&action=review r- because of the broken test, and because of canonicalization in preferredBundleLocalizationName. > Source/WebCore/ChangeLog:12 > + * platform/mac/WebCoreNSStringExtras.mm: Replacements for the aspects of > + WKCopyCFLocalizationPreferredName. I would prefer to have a meaningful name for the header, and probably have the functions work with WTF Strings. This functionality has nothing to do with NSString. This technique used to be more common in WebKit early years, but I don't see a lot of upsides to it. The downsides are: 1. Internal code leaking into clients such as Safari, which start to use it and thus complicate WebKit refactoring. 2. More difficulty figuring out where functions are declared and implemented. 3. Potential for name conflicts with Foundation and other frameworks. > Source/WebCore/ChangeLog:17 > + * WebCore.order: Remove mention of WKCopyCFLocalizationPreferredName. We generally don't update WebCore.order even when removing functions, there is no need to. > Source/WebCore/platform/mac/Language.mm:35 > +#import <wtf/text/CString.h> This added include seems unnecessary. > Source/WebCore/platform/mac/WebCoreNSStringExtras.mm:126 > +NSString *preferredBundleLocalizationName() This is the localization name, so the function name is good. I would personally have named it "effectiveBundleLocalizationName". I'd also add a comment explaining that we only look at the first localization here. This may cause complications when NSBundle uses two localizations at once, e.g. to get some strings from en.lproj, and others form en_GB.lproj. It's not a serious issue in practice, so it's not even a FIXME. I just find that explaining such things saves a lot of time and confusion later. But then again, perhaps we can simply do this at each call site, and not expose a function? > Source/WebCore/platform/mac/WebCoreNSStringExtras.mm:128 > + return canonicalLocalizationName([[[NSBundle mainBundle] preferredLocalizations] objectAtIndex:0]); Nice find using -[NSBundle preferredLocalizations] instead of the longer CF equivalent. But we shouldn't be canonicalizing these. The result is provided by NS/CFBundle, and is used by it, so it should be good as is. > Source/WebCore/platform/mac/WebCoreNSStringExtras.mm:131 > +NSString *canonicalLocalizationName(NSString *language) This function name is incorrect. It returns a locale identifier, which (if we look the other way for a moment) can be used as language identifier, but it has nothing to do with CFBundle and localizations. I'd name the function canonicalWebLanguageName() for now, and at some point later, we may find out that different web technologies use different canonicalizations. > Source/WebCore/platform/mac/WebCoreNSStringExtras.mm:134 > + LangCode languageCode; > + RegionCode regionCode; This needs a FIXME, explaining that Script Manager codes cannot represent all languages that are supported by the OS, so we should move away from them. > Source/WebCore/platform/mac/WebCoreNSStringExtras.mm:138 > + return @""; Are callers prepared to deal with an empty string? I'd use @"en" here.
(In reply to comment #24) > (From update of attachment 236861 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=236861&action=review > > r- because of the broken test, and because of canonicalization in preferredBundleLocalizationName. I'll probably have to build on Mountain Lion to see what's going on there. > > > Source/WebCore/ChangeLog:12 > > + * platform/mac/WebCoreNSStringExtras.mm: Replacements for the aspects of > > + WKCopyCFLocalizationPreferredName. > > I would prefer to have a meaningful name for the header, and probably have the functions work with WTF Strings. This functionality has nothing to do with NSString. > > This technique used to be more common in WebKit early years, but I don't see a lot of upsides to it. The downsides are: > > 1. Internal code leaking into clients such as Safari, which start to use it and thus complicate WebKit refactoring. > 2. More difficulty figuring out where functions are declared and implemented. > 3. Potential for name conflicts with Foundation and other frameworks. Not sure what you mean by "this technique" in this context. What alternative technique are you suggesting that would not have these problems? > > > Source/WebCore/ChangeLog:17 > > + * WebCore.order: Remove mention of WKCopyCFLocalizationPreferredName. > > We generally don't update WebCore.order even when removing functions, there is no need to. I feel it's cleaner to do so even if there isn't a material upside - this way there's no accidental search hits for the no-longer-used function. > > > Source/WebCore/platform/mac/Language.mm:35 > > +#import <wtf/text/CString.h> > > This added include seems unnecessary. One version of the patch used CString, but I changed that, so I'll take it out. > > > Source/WebCore/platform/mac/WebCoreNSStringExtras.mm:126 > > +NSString *preferredBundleLocalizationName() > > This is the localization name, so the function name is good. I would personally have named it "effectiveBundleLocalizationName". I'd also add a comment explaining that we only look at the first localization here. This may cause complications when NSBundle uses two localizations at once, e.g. to get some strings from en.lproj, and others form en_GB.lproj. > > It's not a serious issue in practice, so it's not even a FIXME. I just find that explaining such things saves a lot of time and confusion later. I'm not sure I understand enough about why we do it, when it would come up, and what problems it would cause, to write a meaningful comment. Also old code had presumably the same issues and no FIXME. I can add a comment if you suggest good text, though. > > But then again, perhaps we can simply do this at each call site, and not expose a function? > > > Source/WebCore/platform/mac/WebCoreNSStringExtras.mm:128 > > + return canonicalLocalizationName([[[NSBundle mainBundle] preferredLocalizations] objectAtIndex:0]); > > Nice find using -[NSBundle preferredLocalizations] instead of the longer CF equivalent. > > But we shouldn't be canonicalizing these. The result is provided by NS/CFBundle, and is used by it, so it should be good as is. The old code using CF SPI would canonicalize when used to get the most preferred localization. I can't tell at the call sites if the canonicalization would matter or not. In the case covered by the test suite, default canonicalization did matter. So I'm hesitant to change behavior unless I can prove it doesn't matter, and I'm not sure how to do that. > > > Source/WebCore/platform/mac/WebCoreNSStringExtras.mm:131 > > +NSString *canonicalLocalizationName(NSString *language) > > This function name is incorrect. It returns a locale identifier, which (if we look the other way for a moment) can be used as language identifier, but it has nothing to do with CFBundle and localizations. > > I'd name the function canonicalWebLanguageName() for now, and at some point later, we may find out that different web technologies use different canonicalizations. I don't think that would be quite right. This returns language names with a format like "en_US", which is what the old SPI-based code would do. However, the web uses names like "en-us". The code in Language.mm converts one to the other. If you want to be picky, canonicalLocaleName() would probably be technically correct. > > > Source/WebCore/platform/mac/WebCoreNSStringExtras.mm:134 > > + LangCode languageCode; > > + RegionCode regionCode; > > This needs a FIXME, explaining that Script Manager codes cannot represent all languages that are supported by the OS, so we should move away from them. OK. If I do that, I will also need to file a request for an API way to do what we're doing (i.e. add a default country when providing just a language) without use of Script Manager codes. Afaict there's no code in the OS that can do this without use of a region code. > > > Source/WebCore/platform/mac/WebCoreNSStringExtras.mm:138 > > + return @""; > > Are callers prepared to deal with an empty string? I'd use @"en" here. Sounds reasonable. Though probably it should be @"en_US" to match what the function does otherwise.
Comment on attachment 236861 [details] With actually proper includes this time. View in context: https://bugs.webkit.org/attachment.cgi?id=236861&action=review >>> Source/WebCore/ChangeLog:12 >>> + WKCopyCFLocalizationPreferredName. >> >> I would prefer to have a meaningful name for the header, and probably have the functions work with WTF Strings. This functionality has nothing to do with NSString. >> >> This technique used to be more common in WebKit early years, but I don't see a lot of upsides to it. The downsides are: >> >> 1. Internal code leaking into clients such as Safari, which start to use it and thus complicate WebKit refactoring. >> 2. More difficulty figuring out where functions are declared and implemented. >> 3. Potential for name conflicts with Foundation and other frameworks. > > Not sure what you mean by "this technique" in this context. What alternative technique are you suggesting that would not have these problems? Sorry for being unclear, I meant putting arbitrary WebKit functionality on NSString to share code across frameworks. The alternative is to export a C/C++ function from WebCore. For preferredBundleLocalizationName(), another way to do it is to not have a separate function at all, and just inline the code. >>> Source/WebCore/platform/mac/WebCoreNSStringExtras.mm:126 >>> +NSString *preferredBundleLocalizationName() >> >> This is the localization name, so the function name is good. I would personally have named it "effectiveBundleLocalizationName". I'd also add a comment explaining that we only look at the first localization here. This may cause complications when NSBundle uses two localizations at once, e.g. to get some strings from en.lproj, and others form en_GB.lproj. >> >> It's not a serious issue in practice, so it's not even a FIXME. I just find that explaining such things saves a lot of time and confusion later. >> >> But then again, perhaps we can simply do this at each call site, and not expose a function? > > I'm not sure I understand enough about why we do it, when it would come up, and what problems it would cause, to write a meaningful comment. Also old code had presumably the same issues and no FIXME. I can add a comment if you suggest good text, though. How about something like: // We only return the highest priority localization, which does not fully capture effective localization. NSBundle can combine two localization folders for a language and its regional variant (e.g. take some strings from en.lproj, and some from en_GB.lproj). // Also, localization for other bundles will not follow localization of main bundle if CFBundleAllowMixedLocalizations plist key is used. In clients that use this key, main bundle effective localization should not affect WebKit. This is only relevant if you choose to keep this as a separate function. >>> Source/WebCore/platform/mac/WebCoreNSStringExtras.mm:128 >>> + return canonicalLocalizationName([[[NSBundle mainBundle] preferredLocalizations] objectAtIndex:0]); >> >> Nice find using -[NSBundle preferredLocalizations] instead of the longer CF equivalent. >> >> But we shouldn't be canonicalizing these. The result is provided by NS/CFBundle, and is used by it, so it should be good as is. > > The old code using CF SPI would canonicalize when used to get the most preferred localization. I can't tell at the call sites if the canonicalization would matter or not. In the case covered by the test suite, default canonicalization did matter. So I'm hesitant to change behavior unless I can prove it doesn't matter, and I'm not sure how to do that. OK, let's keep it as is. >>> Source/WebCore/platform/mac/WebCoreNSStringExtras.mm:131 >>> +NSString *canonicalLocalizationName(NSString *language) >> >> This function name is incorrect. It returns a locale identifier, which (if we look the other way for a moment) can be used as language identifier, but it has nothing to do with CFBundle and localizations. >> >> I'd name the function canonicalWebLanguageName() for now, and at some point later, we may find out that different web technologies use different canonicalizations. > > I don't think that would be quite right. This returns language names with a format like "en_US", which is what the old SPI-based code would do. However, the web uses names like "en-us". The code in Language.mm converts one to the other. If you want to be picky, canonicalLocaleName() would probably be technically correct. OK. >>> Source/WebCore/platform/mac/WebCoreNSStringExtras.mm:134 >>> + RegionCode regionCode; >> >> This needs a FIXME, explaining that Script Manager codes cannot represent all languages that are supported by the OS, so we should move away from them. > > OK. If I do that, I will also need to file a request for an API way to do what we're doing (i.e. add a default country when providing just a language) without use of Script Manager codes. Afaict there's no code in the OS that can do this without use of a region code. We may need to have a table of our own in WebKit. There seems to be little logic in how the languages are named in browsers - e.g. English is "en-us", but Russian is "ru", and there used to be web sites that broke if Accept-Language header contained "ru-ru". Talking about this example, the old code did generate "ru-ru", and CFNetwork actually has a special case for it. We should likely do the same in WebKit, but not in this patch. There are likely many more languages where we don't use the same language codes as other browsers. >>> Source/WebCore/platform/mac/WebCoreNSStringExtras.mm:138 >>> + return @""; >> >> Are callers prepared to deal with an empty string? I'd use @"en" here. > > Sounds reasonable. Though probably it should be @"en_US" to match what the function does otherwise. OK.
(In reply to comment #26) > (From update of attachment 236861 [details]) > > > > > Not sure what you mean by "this technique" in this context. What alternative technique are you suggesting that would not have these problems? > > Sorry for being unclear, I meant putting arbitrary WebKit functionality on NSString to share code across frameworks. > > The alternative is to export a C/C++ function from WebCore. For preferredBundleLocalizationName(), another way to do it is to not have a separate function at all, and just inline the code. My patch does export a C/C++ function from WebCore. For preferredBundleLocalizationName, I agree inlining makes sense if the code is simple enough. It was definitely not simple enough when using CF. I guess whether it is simple enough now depends on whether it canonicalizes. > How about something like: Thanks! Good suggestion. Manager codes. Afaict there's no code in the OS that can do this without use of a region code. > > We may need to have a table of our own in WebKit. There seems to be little logic in how the languages are named in browsers - e.g. English is "en-us", but Russian is "ru", and there used to be web sites that broke if Accept-Language header contained "ru-ru". > > Talking about this example, the old code did generate "ru-ru", and CFNetwork actually has a special case for it. We should likely do the same in WebKit, but not in this patch. There are likely many more languages where we don't use the same language codes as other browsers. OK. It probably makes sense long-term to have our own table to add appropriate country codes, but agreed, not in this patch. I'll file a follow-up Radar for that and add a FIXME.
> My patch does export a C/C++ function from WebCore. Ouch! I don't know how I could misread it so badly, and repeatedly. It would still be nice to have this functionality in a dedicated header (perhaps Language.h?). Most existing functions in WebCoreNSStringExtras.h header are pure string manipulation.
> media/video-trackmenu-selection.html I could reproduce the failure by running the test individually on Mountain Lion. In WebKit1 mode, I'm getting yet different results (the strings are localized), but I don't know if it's a change from this patch. Not even sure if it's a regression, or an OK change.
(In reply to comment #28) > > My patch does export a C/C++ function from WebCore. > > Ouch! I don't know how I could misread it so badly, and repeatedly. > > It would still be nice to have this functionality in a dedicated header (perhaps Language.h?). Most existing functions in WebCoreNSStringExtras.h header are pure string manipulation. I thought that too, but it seems in line with stringEncodingForResource() in the same file (even down to also using LocaleStringToLangAndRegionCodes!)
IIRC stringEncodingForResource() is only used in plug-in code, and should be moved closer to where it's used. Or maybe there are no functional plug-ins with resource forks any more. HomeResFile and FSGetForkCBInfo bring back bad memories :) Not a huge deal either way.
(In reply to comment #29) > > media/video-trackmenu-selection.html > > I could reproduce the failure by running the test individually on Mountain Lion. In WebKit1 mode, I'm getting yet different results (the strings are localized), but I don't know if it's a change from this patch. > > Not even sure if it's a regression, or an OK change. I finally managed to get a working Mountain Lion dev environment and build, and I can't reproduce this failure. I am testing a slightly different patch but the difference should not be relevant. Might need debugging help (in the meantime I'll upload the new patch).
Created attachment 236910 [details] With some changes addressing comments
Attachment 236910 [details] did not pass style-queue: ERROR: Source/WebKit/mac/Plugins/Hosted/NetscapePluginHostManager.mm:120: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/WebKit/mac/Plugins/WebBasePluginPackage.mm:29: Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] Total errors found: 2 in 10 files If any of these errors are false positives, please file a bug against check-webkit-style.
I don't really get why, but it looks like the new patch doesn't have the same issue, so it should be ready for final review.
Curious. The only real change is to return @"en_US" instead of @"", right? Also, the new patch omits changes to WebCoreSystemInterfaceIOS.mm - why are these not necessary?
(In reply to comment #36) > Curious. The only real change is to return @"en_US" instead of @"", right? That seems like the only thing that could make a difference. I can't test for sure until tomorrow though. > Also, the new patch omits changes to WebCoreSystemInterfaceIOS.mm - why are these not necessary? They are totally necessary. I collided with WebCoreExport changes, and webkit-patch just failed to upload that part til I resolved the conflict. :-( New patch coming.
Created attachment 236915 [details] Patch
Attachment 236915 [details] did not pass style-queue: ERROR: Source/WebKit/mac/Plugins/Hosted/NetscapePluginHostManager.mm:120: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/WebKit/mac/Plugins/WebBasePluginPackage.mm:29: Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] Total errors found: 2 in 10 files If any of these errors are false positives, please file a bug against check-webkit-style.
Verified that this is the change that fixes it. It's almost easy to explain now - providing a valid localization is what makes localized track language names available in the first place. The part that is harder to explain is why we even go into this fallback code path, and whether this patch breaks all non-English track language names (and more) by effectively hardcoding en_US.
canonicalLocalizationName() is called multiple times while running this test: +LocaleStringToLangAndRegionCodes (null) -> -128, -128, error -50 +LocaleStringToLangAndRegionCodes en -> 0, 0, error 0 +LocaleStringToLangAndRegionCodes ru -> 32, 49, error 0 +LocaleStringToLangAndRegionCodes ar -> 12, 16, error 0 +LocaleStringToLangAndRegionCodes cs -> 38, 56, error 0 +LocaleStringToLangAndRegionCodes hu -> 26, 43, error 0 +LocaleStringToLangAndRegionCodes tr -> 17, 24, error 0 +LocaleStringToLangAndRegionCodes ca -> 130, 73, error 0 +LocaleStringToLangAndRegionCodes el -> 14, 20, error 0 +LocaleStringToLangAndRegionCodes he -> 10, 13, error 0 +LocaleStringToLangAndRegionCodes hr -> 18, 68, error 0 +LocaleStringToLangAndRegionCodes ro -> 37, 39, error 0 +LocaleStringToLangAndRegionCodes sk -> 39, 57, error 0 +LocaleStringToLangAndRegionCodes th -> 22, 54, error 0 +LocaleStringToLangAndRegionCodes uk -> 45, 62, error 0 The first call is obviously problematic, and it is made in UI process here: Thread 1 Crashed:: Dispatch queue: com.apple.WebKit.ProcessLauncher 0 com.apple.JavaScriptCore 0x00000001057412ca WTFCrash + 42 (Assertions.cpp:329) 1 com.apple.WebCore 0x000000010b6fbb16 canonicalLocalizationName + 102 (WebCoreNSStringExtras.mm:138) 2 com.apple.WebCore 0x000000010b6fba9f preferredBundleLocalizationName + 95 (WebCoreNSStringExtras.mm:128) 3 com.apple.WebKit 0x000000010707fbd2 WebKit::createProcess(WebKit::ProcessLauncher::LaunchOptions const&, bool, WebKit::ProcessLauncher*, void (WebKit::ProcessLauncher::*)(int, IPC::Connection::Identifier)) + 1106 (ProcessLauncherMac.mm:459) 4 com.apple.WebKit 0x000000010707f0d2 WebKit::ProcessLauncher::launchProcess() + 338 (ProcessLauncherMac.mm:575)
(lldb) po [[NSBundle mainBundle] preferredLocalizations] nil Seems like we need to be prepared to the result of this function being nil. It's surprising that you were not seeing the problem - did you invoke run-webkit-tests with "-2" argument to run in WebKit2 mode?
CFBundleGetLocalizationInfoForLocalization(null, …) successfully returns 0/0/0 in WebKitTestRunner (which is langEnglish/verUS/smRoman), so the fix to add a fallback to en_US in preferredBundleLocalizationName() when the array is nil or empty.
(In reply to comment #42) > (lldb) po [[NSBundle mainBundle] preferredLocalizations] > nil > > Seems like we need to be prepared to the result of this function being nil. It's surprising that you were not seeing the problem - did you invoke run-webkit-tests with "-2" argument to run in WebKit2 mode? I just tested a patch that already had the fix to fall back to "en_US" instead of "". It probably would have reproduced without that change. Uploading an additional patch for bad preferredLocalizations values.
Created attachment 236936 [details] Patch
Attachment 236936 [details] did not pass style-queue: ERROR: Source/WebKit/mac/Plugins/Hosted/NetscapePluginHostManager.mm:120: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/WebKit/mac/Plugins/WebBasePluginPackage.mm:29: Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] Total errors found: 2 in 10 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 236936 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=236936&action=review > Source/WebCore/platform/mac/WebCoreNSStringExtras.mm:132 > + if (!preferredLocalizations || ![preferredLocalizations count]) Since it's Objective-C, "[preferredLocalizations count]" would be sufficient to check for. > Source/WebCore/platform/mac/WebCoreNSStringExtras.mm:135 > + return canonicalLocaleName([ objectAtIndex:0]); This won't build, missing message receive.
Created attachment 236945 [details] Final patch (uploading for EWS check)
Attachment 236945 [details] did not pass style-queue: ERROR: Source/WebKit/mac/Plugins/Hosted/NetscapePluginHostManager.mm:120: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/WebCore/WebCore.exp.in:0: Source/WebCore/WebCore.exp.in should be sorted, use Tools/Scripts/sort-export-file script [list/order] [5] ERROR: Source/WebKit/mac/Plugins/WebBasePluginPackage.mm:29: Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] Total errors found: 3 in 10 files If any of these errors are false positives, please file a bug against check-webkit-style.
Committed r172866: <http://trac.webkit.org/changeset/172866>
(In reply to comment #50) > Committed r172866: <http://trac.webkit.org/changeset/172866> This broke the iOS build. In particular, WebCore fails with the linker error: [[ ... Ld WebCore Undefined symbols for architecture x86_64: "_canonicalLocaleName", referenced from: __ZN7WebCoreL21httpStyleLanguageCodeEP8NSString in Language-2662140F3DCB43.o "_preferredBundleLocalizationName", referenced from: -exported_symbol[s_list] command line option ld: symbol(s) not found for architecture x86_64 clang: error: linker command failed with exit code 1 (use -v to see invocation) ]] Notice that the patch only implements the functions preferredBundleLocalizationName canonicalLocaleName for the Mac port (since the implementation for these functions are inside a !PLATFORM(IOS)-guarded section of the fie WebCoreNSStringExtras.mm). I tried moving these functions outside of the !PLATFORM(IOS)-guarded section so that they are defined for both Mac and iOS, but this leads to a compile-time failure because LocaleStringToLangAndRegionCodes() (used canonicalLocaleName()) isn't available on iOS.
Re-opened since this is blocked by bug 136177
Comment on attachment 236945 [details] Final patch (uploading for EWS check) View in context: https://bugs.webkit.org/attachment.cgi?id=236945&action=review > Source/WebCore/platform/mac/WebCoreNSStringExtras.mm:150 > + return [(NSString*)code.get() autorelease]; This looks like an overrelease to me. The RetainPtr will release, and the autorelease will release a second time. I believe the idiomatic way to write this in current WebKit code is: return CFBridgingRelease(code.leakRef()); or: return CFBridgingRelease(CFLocaleCreateCanonicalLocaleIdentifierFromScriptManagerCodes(0, languageCode, regionCode)); I think we were also contemplating more elegant ways of writing this.
(In reply to comment #53) > (From update of attachment 236945 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=236945&action=review > > > Source/WebCore/platform/mac/WebCoreNSStringExtras.mm:150 > > + return [(NSString*)code.get() autorelease]; > > This looks like an overrelease to me. The RetainPtr will release, and the autorelease will release a second time. I believe the idiomatic way to write this in current WebKit code is: > > return CFBridgingRelease(code.leakRef()); > > or: > > return CFBridgingRelease(CFLocaleCreateCanonicalLocaleIdentifierFromScriptManagerCodes(0, languageCode, regionCode)); > > I think we were also contemplating more elegant ways of writing this. I noticed this while fixing the patch (it needs to be fixed for iOS) and I wrote it as [[(NSString*)code.get() retain] autorelease] That has an extra retain, but Im sure it's safe. Does CFBridgingRelease turn a CF retain into an NS retain, or into an autorelease?
(In reply to comment #54) > (In reply to comment #53) > > (From update of attachment 236945 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=236945&action=review > > > > > Source/WebCore/platform/mac/WebCoreNSStringExtras.mm:150 > > > + return [(NSString*)code.get() autorelease]; > > > > This looks like an overrelease to me. The RetainPtr will release, and the autorelease will release a second time. I believe the idiomatic way to write this in current WebKit code is: > > > > return CFBridgingRelease(code.leakRef()); > > > > or: > > > > return CFBridgingRelease(CFLocaleCreateCanonicalLocaleIdentifierFromScriptManagerCodes(0, languageCode, regionCode)); > > > > I think we were also contemplating more elegant ways of writing this. > > I noticed this while fixing the patch (it needs to be fixed for iOS) and I wrote it as [[(NSString*)code.get() retain] autorelease] > > That has an extra retain, but Im sure it's safe. Does CFBridgingRelease turn a CF retain into an NS retain, or into an autorelease? I looked up CFBridringRelease and it sounds like it would be more likely to be correct in the face of ARC.
Created attachment 237122 [details] Patch
Attachment 237122 [details] did not pass style-queue: ERROR: Source/WebKit/mac/Plugins/Hosted/NetscapePluginHostManager.mm:120: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/WebKit/mac/Plugins/WebBasePluginPackage.mm:29: Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] Total errors found: 2 in 12 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 237124 [details] Fixed refcounting approach
Attachment 237124 [details] did not pass style-queue: ERROR: Source/WebKit/mac/Plugins/Hosted/NetscapePluginHostManager.mm:120: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/WebKit/mac/Plugins/WebBasePluginPackage.mm:29: Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] Total errors found: 2 in 12 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 237124 [details] Fixed refcounting approach View in context: https://bugs.webkit.org/attachment.cgi?id=237124&action=review > Source/WebCore/platform/mac/WebCoreNSStringExtras.mm:158 > + return [CFBridgingRElease(code.leafRef()) autorelease]; This won’t compile. What you want is: return CFBridgingRelease(code.leakRef()); The code here still has an extra autorelease, and it has two typos.
(In reply to comment #60) > (From update of attachment 237124 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=237124&action=review > > > Source/WebCore/platform/mac/WebCoreNSStringExtras.mm:158 > > + return [CFBridgingRElease(code.leafRef()) autorelease]; > > This won’t compile. What you want is: > > return CFBridgingRelease(code.leakRef()); > > The code here still has an extra autorelease, and it has two typos. CFBridgingRelease does an autorelease? I could find no mention of such a thing in the docs. All it says is that it "transfers ownership to ARC". How can I confirm that CFBridgingRelease will do an autorelease?
Looks like the actual best way to do it is RetainPtr::autorelease()
(Which implies that CFBridgingRelease does an autorelease, though I can still find no docs for this).
Created attachment 237131 [details] Really fix the refcounting for real.
Attachment 237131 [details] did not pass style-queue: ERROR: Source/WebKit/mac/Plugins/Hosted/NetscapePluginHostManager.mm:120: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/WebKit/mac/Plugins/WebBasePluginPackage.mm:29: Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] Total errors found: 2 in 12 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 237138 [details] This time for sure!
Attachment 237138 [details] did not pass style-queue: ERROR: Source/WebKit/mac/Plugins/Hosted/NetscapePluginHostManager.mm:120: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/WebKit/mac/Plugins/WebBasePluginPackage.mm:29: Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] Total errors found: 2 in 12 files If any of these errors are false positives, please file a bug against check-webkit-style.
For reference: I successfully built with this patch for iOS using 'make release SDKROOT=iphoneos.internal ARCHS=armv7'.
Committed r172975: <http://trac.webkit.org/changeset/172975>