Ensure Platform.localeName isn't '_'

[zanderso]

Another attempt to fix flutter/flutter#13748 that only modifies what is directly fed into Platform.localeName.

[tvolkert]

?. wasn't used before - seems like we could do without them here. If we do need then, then you should check for null below.

[tvolkert]

Why is it _ in the first place versus just being null or the empty string? Are the tests that failed valid in wanting it to start out as _?

[zanderso]

The Locale is initialized from the engine with empty strings for the language and the country codes. Some logic in the tests and/or the framework appears to rely on that behavior. Whether that makes sense or not, I wouldn't venture to guess.

[tvolkert]

Looking at the test failures (pulled from the failing Travis job), my guess is that the test assumptions are wrong -- and that we should probably go with your initial fix, along with updating the test in flutter_localizations.

It also doesn't explain why tests started failing inside Google only once #4450 was rolled into Google.

@Hixie @HansMuller

[zanderso]

Before the closure was set in #4450, Platform.localeName would always return "en_US" even if that was wrong. Setting up the closure makes it return window.locale.toString() instead. The tests failed in Google because window.locale has empty strings for the language code and country code unless set to something else, which the tests in Google didn't do, which made Platform.localeName come back as '_'.

[tvolkert]

On Android, window.locale is set by FlutterView, and on iOS, it's set by FlutterViewController. Maybe the right solution is just to explicitly initialize window.locale to en_US in flutter_tester?

With this PR, future platforms will be ensured to always get a window.locale even if the platform-specific code fails to initialize it. Whether that's a feature or a bug I'm not sure.

I'm ok with this change if @Hixie is.

[tvolkert]

ping @Hixie for thoughts -- see discussion above.

[Hixie]

I don't understand what this code is doing. Why is the engine reading window.locale?

[Hixie]

Ah, I see. This is an attempt to implement _localeName in platform_impl.dart?

(Side note: looking at platform_impl.dart (the dart:io Platform implementation), I noticed a bug: localeName caches the locale, which is invalid because the locale can change during app execution.)

[Hixie]

The documentation for Platform.localeName doesn't say what the format will be so I don't really know what the correct fix is.

[Hixie]

I don't really understand why the existing implementations of Platform::LocaleName in the Dart SDK aren't sufficient. Is it just that the platform_android.cc implementation is wrong? (It tries to read an environment variable, which seems likely to be bogus.)

[zanderso]

localeName used to cache, but doesn't anymore: https://github.com/dart-lang/sdk/blob/master/sdk/lib/io/platform_impl.dart#L45

Yes, Platfrom::LocaleName on Android is wrong, and I don't think there's a good way to fix it right there.

[Hixie]

Ah, my local copy must be behind.

I would expect Platform.localeName to return the same as window.locale (but in whatever format we document Platform.localeName should return), so I agree with Todd that it seems weird to add a mention of en_US here. Also, we shouldn't parse the value (e.g. comparing to _), we should just look at the real data. Feel free to add a member to Locale that you can query if you need to check for an "empty" Locale, or some such. But really the engine should never be providing the empty string, that's a bug. We might as well fix that bug now as part of fixing this. The bug is flutter/flutter#1865.

[Hixie]

@zanderso Can you clarify your intent for this PR? Thanks. (I'm just cleaning out old PRs on the review queue.)

[zanderso]

Yeah, this is not the right fix. I'll close this.