1429964 - Re-enable browser_packaged_as_locales.js with dynamically testing app locales

Status: RESOLVED FIXED

(Firefox :: New Tab Page, enhancement)

Description

[Ed Lee :Mardak]

Bug 1423703 disabled browser_packaged_as_locales.js because there isn't a good way to dynamically change the app locale for the test. See 1423703 comment 20.

gandalf, do you have a bug already that we can set blocking this bug?

Comment 1

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

Let's block on bug 1358653.

Comment 2

[Ed Lee :Mardak]

Attachment: Bug 1429964 - Re-enable browser_packaged_as_locales.js with dynamically testing app locales.

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

Comment 3

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

Comment on attachment 8957268 [details]
Bug 1429964 - Re-enable browser_packaged_as_locales.js with dynamically testing app locales.

https://reviewboard.mozilla.org/r/226188/#review232134

::: browser/components/newtab/aboutNewTabService.js:20
(Diff revision 1)
>  
>  // Dummy references to the files that this service no longer allows loading.
>  // Bug 1409054 to remove "chrome://browser/content/abouthome/aboutHome.xhtml"
>  
>  const TOPIC_APP_QUIT = "quit-application-granted";
> -const TOPIC_LOCALES_CHANGE = "intl:requested-locales-changed";
> +const TOPIC_LOCALES_CHANGE = "intl:app-locales-changed";

Yay! Was planning to poke you about it. Glad to see that!

::: browser/components/newtab/tests/browser/browser_packaged_as_locales.js:10
(Diff revision 1)
>  
>  const DEFAULT_URL = "resource://activity-stream/prerendered/en-US/activity-stream-prerendered.html";
>  async function getUrlForLocale(locale) {
> +  const origAvailable = Services.locale.getAvailableLocales();
> +  const origRequested = Services.locale.getRequestedLocales();
> +  try {

Why do you wrap it in try/catch? 

The only scenario in which any of those LocaleService methods throw is if you pass it an incorrectly formatted BCP47 tag.

And in this case, I think it would be better to blow the test rather than silently pass. :)

Comment 4

[Ed Lee :Mardak]

(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #3)
> Why do you wrap it in try/catch? 
Technically, there's no `catch` ;) ;) The finally is so that code can run after the return statement. But.. I guess I can just make it store the return url value and return that after restoring the locale.

Comment 5

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

oh! That got me, I'm not used to use try/finally for that - neat trick.

Up to you if you want to change that. Maybe just a docstring explaining the technique for folks like me will be enough? :)

Comment 6

[Ed Lee :Mardak]

Comment on attachment 8957268 [details]
Bug 1429964 - Re-enable browser_packaged_as_locales.js with dynamically testing app locales.

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

Comment 7

[Ed Lee :Mardak]

Comment on attachment 8957268 [details]
Bug 1429964 - Re-enable browser_packaged_as_locales.js with dynamically testing app locales.

https://reviewboard.mozilla.org/r/226188/#review232134

> Why do you wrap it in try/catch? 
> 
> The only scenario in which any of those LocaleService methods throw is if you pass it an incorrectly formatted BCP47 tag.
> 
> And in this case, I think it would be better to blow the test rather than silently pass. :)

Added comments before for the function and inline

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/543721b84a36
Re-enable browser_packaged_as_locales.js with dynamically testing app locales. r=gandalf

Comment 9

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/543721b84a36