1337551 - Migrate Services.jsm to use LocaleService

Status: RESOLVED FIXED

(Core :: Internationalization, defect)

Comment 1

[Zibi Braniecki [:zbraniecki][:gandalf]]

Attachment: Bug 1337551: Migrate Services.jsm to use LocaleService.

Review commit: https://reviewboard.mozilla.org/r/116882/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/116882/

Comment 2

[Zibi Braniecki [:zbraniecki][:gandalf]]

Comment on attachment 8843118 [details]
Bug 1337551: Migrate Services.jsm to use LocaleService.

triggered around 500 errors. Let's fix them before requesting review.

Comment 3

[Zibi Braniecki [:zbraniecki][:gandalf]]

Comment on attachment 8843118 [details]
Bug 1337551: Migrate Services.jsm to use LocaleService.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/116882/diff/1-2/

Comment 4

[Zibi Braniecki [:zbraniecki][:gandalf]]

Comment on attachment 8843118 [details]
Bug 1337551: Migrate Services.jsm to use LocaleService.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/116882/diff/2-3/

Comment 5

[Zibi Braniecki [:zbraniecki][:gandalf]]

> triggered around 500 errors.

Update, triggered 500 errors with just a few mistyped characters. (didn't return from a function).

:mossop - this should work now (launched a try) and it moves Services.locale to use LocaleService which seems appropriate and is part of the unifying theme of getting all callsites that are trying to get the app locale call one API.

Comment 6

[Dave Townsend [:mossop]]

Comment on attachment 8843118 [details]
Bug 1337551: Migrate Services.jsm to use LocaleService.

https://reviewboard.mozilla.org/r/116882/#review118870

Comment 7

[Zibi Braniecki [:zbraniecki][:gandalf]]

Comment on attachment 8843118 [details]
Bug 1337551: Migrate Services.jsm to use LocaleService.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/116882/diff/3-4/

Comment 8

[Zibi Braniecki [:zbraniecki][:gandalf]]

Comment on attachment 8843118 [details]
Bug 1337551: Migrate Services.jsm to use LocaleService.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/116882/diff/4-5/

Comment 9

[Zibi Braniecki [:zbraniecki][:gandalf]]

I had to limit the scope of this bug because of bug 1344445. It's still fortunately possible to migrate the Services.locale.

Comment 10

[Jonathan Kew [:jfkthame]]

Comment on attachment 8843118 [details]
Bug 1337551: Migrate Services.jsm to use LocaleService.

https://reviewboard.mozilla.org/r/116882/#review118926

A couple of minor drive-by comments....

::: addon-sdk/source/test/test-l10n-locale.js:113
(Diff revision 5)
> -  let expectedLocale = Services.locale.getLocaleComponentForUserAgent().
> +  let expectedLocale = Services.locale.getAppLocale().
>      toLowerCase();

Might be nice to un-wrap this line now that it's so much shorter.

::: browser/modules/DirectoryLinksProvider.jsm:212
(Diff revision 5)
>            this._setDefaultEnhanced();
>            break;
>  
>          case this._observedPrefs.linksURL:
>            delete this.__linksURL;
>            // fallthrough

There's nothing for this to "fallthrough" to any more... suggest removing this comment, and adding `break` for clarity.

::: browser/modules/DirectoryLinksProvider.jsm:214
(Diff revision 5)
> -        case this._observedPrefs.matchOSLocale:
> -        case this._observedPrefs.prefSelectedLocale:
> -          this._fetchAndCacheLinksIfNecessary(true);
> -          break;
>        }
> +    } else if (aTopic === "intl:app-locales-changed") {

The previous `if (aTopic...` test used `==`, this one uses `===`. Consistency is good...

Comment 11

[Zibi Braniecki [:zbraniecki][:gandalf]]

Comment on attachment 8843118 [details]
Bug 1337551: Migrate Services.jsm to use LocaleService.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/116882/diff/5-6/

Comment 12

[Zibi Braniecki [:zbraniecki][:gandalf]]

Ugh, I had to revert more of the patch because the tests for the behavior are pretty complicated. I want to just move `Services.locale` in this, bug and deal with all callsites to `general.useragent.locale` later.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/20075fb33fc7
Migrate Services.jsm to use LocaleService. r=mossop

Comment 14

[Zibi Braniecki [:zbraniecki][:gandalf]]

Ok, the try is green. It seems like many cleanups I wanted to do in this bug will require bug 1344445 to be resolved, so this is much smaller, but it does transition Services.locale to use mozILocaleService.

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/20075fb33fc7

Comment 16

[Zibi Braniecki [:zbraniecki][:gandalf]]

I did introduce a minor regression (in a very obscure codepath - matchOS scenario), that I'm fixing in bug 1344901.