Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
11038007:
Minor cleanups to take advantage of lazy initialization (Closed)
|
Created:
8 years, 2 months ago by Alan Knight Modified:
8 years, 2 months ago Reviewers:
Emily Fortuna CC:
reviews_dartlang.org Visibility:
Public. |
DescriptionMinor cleanups to take advantage of lazy initialization
Committed: https://code.google.com/p/dart/source/detail?r=13137
Patch Set 1 #
Total comments: 8
Patch Set 2 : #Messages
Total messages: 8 (0 generated)
A couple of minor cleanups I had pending from before I went away.
https://codereview.chromium.org/11038007/diff/1/pkg/intl/lib/intl.dart File pkg/intl/lib/intl.dart (right): https://codereview.chromium.org/11038007/diff/1/pkg/intl/lib/intl.dart#newcode28 pkg/intl/lib/intl.dart:28: static String _defaultLocale = systemLocale; sorry, why are the changes in this file preferable? https://codereview.chromium.org/11038007/diff/1/pkg/intl/lib/intl.dart#newcod... pkg/intl/lib/intl.dart:170: var oldLocale = _defaultLocale; alert alert spurious whitespace
https://codereview.chromium.org/11038007/diff/1/pkg/intl/lib/intl.dart File pkg/intl/lib/intl.dart (right): https://codereview.chromium.org/11038007/diff/1/pkg/intl/lib/intl.dart#newcode28 pkg/intl/lib/intl.dart:28: static String _defaultLocale = systemLocale; On 2012/10/02 00:17:19, Emily Fortuna wrote: > sorry, why are the changes in this file preferable? It sets the systemLocale on creation so it can avoid a couple of lines with null checks that then set it to that value. That seemed cleaner to me. https://codereview.chromium.org/11038007/diff/1/pkg/intl/lib/intl.dart#newcod... pkg/intl/lib/intl.dart:170: var oldLocale = _defaultLocale; On 2012/10/02 00:17:19, Emily Fortuna wrote: > alert alert spurious whitespace Done.
lgtm, with response to question below https://codereview.chromium.org/11038007/diff/1/pkg/intl/lib/intl.dart File pkg/intl/lib/intl.dart (right): https://codereview.chromium.org/11038007/diff/1/pkg/intl/lib/intl.dart#newcode28 pkg/intl/lib/intl.dart:28: static String _defaultLocale = systemLocale; On 2012/10/02 00:24:29, Alan Knight wrote: > On 2012/10/02 00:17:19, Emily Fortuna wrote: > > sorry, why are the changes in this file preferable? > > It sets the systemLocale on creation so it can avoid a couple of lines with null > checks that then set it to that value. That seemed cleaner to me. Seems good if we can initialize systemLocale and _defaultLocale when they are declared: static String systemLocale = getSystemLocale() <-- which depends on the browser or the os depending on environment can we actually do that? If not, it's probably better to set _defaultLocale after some initialization has occurred so it actually gets set when we have a real value for it we want to use.
https://codereview.chromium.org/11038007/diff/1/pkg/intl/lib/intl.dart File pkg/intl/lib/intl.dart (right): https://codereview.chromium.org/11038007/diff/1/pkg/intl/lib/intl.dart#newcode28 pkg/intl/lib/intl.dart:28: static String _defaultLocale = systemLocale; On 2012/10/02 00:48:35, Emily Fortuna wrote: > On 2012/10/02 00:24:29, Alan Knight wrote: > > On 2012/10/02 00:17:19, Emily Fortuna wrote: > > > sorry, why are the changes in this file preferable? > > > > It sets the systemLocale on creation so it can avoid a couple of lines with > null > > checks that then set it to that value. That seemed cleaner to me. > > Seems good if we can initialize systemLocale and _defaultLocale when they are > declared: > > static String systemLocale = getSystemLocale() <-- which depends on the browser > or the os depending on environment > > can we actually do that? If not, it's probably better to set _defaultLocale > after some initialization has occurred so it actually gets set when we have a > real value for it we want to use. No, we can't, because getting the real value may involve an async call, and also may be set by the user, who knows better than we do. For example, if we set it to the browser's default, but the application knows a more appropriate value by looking at the user account once they've logged in and sets the default explicitly. My assumption was that if someone was going to set the default locale they would be unlikely to create an Intl object until they had done so. But it isn't a big deal either way. If you think the previous setup was preferable, I'm happy enough to leave it that way.
https://codereview.chromium.org/11038007/diff/1/pkg/intl/lib/intl.dart File pkg/intl/lib/intl.dart (right): https://codereview.chromium.org/11038007/diff/1/pkg/intl/lib/intl.dart#newcode28 pkg/intl/lib/intl.dart:28: static String _defaultLocale = systemLocale; On 2012/10/02 16:21:42, Alan Knight wrote: > On 2012/10/02 00:48:35, Emily Fortuna wrote: > > On 2012/10/02 00:24:29, Alan Knight wrote: > > > On 2012/10/02 00:17:19, Emily Fortuna wrote: > > > > sorry, why are the changes in this file preferable? > > > > > > It sets the systemLocale on creation so it can avoid a couple of lines with > > null > > > checks that then set it to that value. That seemed cleaner to me. > > > > Seems good if we can initialize systemLocale and _defaultLocale when they are > > declared: > > > > static String systemLocale = getSystemLocale() <-- which depends on the > browser > > or the os depending on environment > > > > can we actually do that? If not, it's probably better to set _defaultLocale > > after some initialization has occurred so it actually gets set when we have a > > real value for it we want to use. > > No, we can't, because getting the real value may involve an async call, and also > may be set by the user, who knows better than we do. For example, if we set it > to the browser's default, but the application knows a more appropriate value by > looking at the user account once they've logged in and sets the default > explicitly. > > My assumption was that if someone was going to set the default locale they would > be unlikely to create an Intl object until they had done so. But it isn't a big > deal either way. If you think the previous setup was preferable, I'm happy > enough to leave it that way. Right, that's what I figured. So are we going to make withLocale and the others async because we might need to pull down locale info in that step? I just don't like setting it to some value that shouldn't really ever be used because it may trick the user later into thinking things are initialized when they're not.
https://codereview.chromium.org/11038007/diff/1/pkg/intl/lib/intl.dart File pkg/intl/lib/intl.dart (right): https://codereview.chromium.org/11038007/diff/1/pkg/intl/lib/intl.dart#newcode28 pkg/intl/lib/intl.dart:28: static String _defaultLocale = systemLocale; On 2012/10/02 17:11:37, Emily Fortuna wrote: > On 2012/10/02 16:21:42, Alan Knight wrote: > > On 2012/10/02 00:48:35, Emily Fortuna wrote: > > > On 2012/10/02 00:24:29, Alan Knight wrote: > > > > On 2012/10/02 00:17:19, Emily Fortuna wrote: > > > > > sorry, why are the changes in this file preferable? > > > > > > > > It sets the systemLocale on creation so it can avoid a couple of lines > with > > > null > > > > checks that then set it to that value. That seemed cleaner to me. > > > > > > Seems good if we can initialize systemLocale and _defaultLocale when they > are > > > declared: > > > > > > static String systemLocale = getSystemLocale() <-- which depends on the > > browser > > > or the os depending on environment > > > > > > can we actually do that? If not, it's probably better to set _defaultLocale > > > after some initialization has occurred so it actually gets set when we have > a > > > real value for it we want to use. > > > > No, we can't, because getting the real value may involve an async call, and > also > > may be set by the user, who knows better than we do. For example, if we set it > > to the browser's default, but the application knows a more appropriate value > by > > looking at the user account once they've logged in and sets the default > > explicitly. > > > > My assumption was that if someone was going to set the default locale they > would > > be unlikely to create an Intl object until they had done so. But it isn't a > big > > deal either way. If you think the previous setup was preferable, I'm happy > > enough to leave it that way. > > Right, that's what I figured. So are we going to make withLocale and the others > async because we might need to pull down locale info in that step? I just don't > like setting it to some value that shouldn't really ever be used because it may > trick the user later into thinking things are initialized when they're not. I don't want to make withLocale async. The only operation that's async is the findSystemLocale, and the user has to explicitly invoke that, because they may have a better answer and not want to call that at all. So I'd rather just revert the initialization here back to the way it was. So did that.
okay. lgtm! |
