Closed Bug 1337551 Opened 3 years ago Closed 3 years ago

Migrate Services.jsm to use LocaleService

Categories

(Core :: Internationalization, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox54 --- fixed

People

(Reporter: zbraniecki, Assigned: zbraniecki)

References

Details

Attachments

(1 file)

No description provided.
Depends on: 1308329
Assignee: nobody → gandalf
Status: NEW → ASSIGNED
Comment on attachment 8843118 [details]
Bug 1337551: Migrate Services.jsm to use LocaleService.

triggered around 500 errors. Let's fix them before requesting review.
Attachment #8843118 - Flags: review?(jfkthame)
> 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 on attachment 8843118 [details]
Bug 1337551: Migrate Services.jsm to use LocaleService.

https://reviewboard.mozilla.org/r/116882/#review118870
Attachment #8843118 - Flags: review?(dtownsend) → review+
I had to limit the scope of this bug because of bug 1344445. It's still fortunately possible to migrate the Services.locale.
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...
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.
Pushed by zbraniecki@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/20075fb33fc7
Migrate Services.jsm to use LocaleService. r=mossop
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.
https://hg.mozilla.org/mozilla-central/rev/20075fb33fc7
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
I did introduce a minor regression (in a very obscure codepath - matchOS scenario), that I'm fixing in bug 1344901.
Depends on: 1344978
Depends on: 1344901
No longer depends on: 1344978
Depends on: 1344727
Depends on: 1395355
You need to log in before you can comment on or make changes to this bug.