Closed Bug 1295894 Opened 3 years ago Closed 3 years ago

Show localized strings for browserAction/pageAction title

Categories

(WebExtensions :: Untriaged, defect)

47 Branch
defect
Not set

Tracking

(firefox48 affected, firefox49 fixed, firefox50 fixed, firefox51 verified)

VERIFIED FIXED
mozilla51
Tracking Status
firefox48 --- affected
firefox49 --- fixed
firefox50 --- fixed
firefox51 --- verified

People

(Reporter: hectorz, Assigned: hectorz)

References

Details

(Whiteboard: triaged)

Attachments

(1 file)

Instead of the strings from default locale.

The bug comes from [1], where the normalization context is passed to (LocaleData).localize as the second argument, when it is expected to be an available locale name.

[1]: https://hg.mozilla.org/mozilla-central/rev/4c924e5c2749#l8.33
Assignee: nobody → bzhao
Comment on attachment 8781863 [details]
Bug 1295894 - Show localized strings for browserAction/pageAction title.

https://reviewboard.mozilla.org/r/72202/#review70392

::: browser/components/extensions/test/browser/browser_ext_pageAction_context.js:138
(Diff revision 1)
>          resolve();
>        }
>      });
>    });
>  
> +  SpecialPowers.setCharPref("general.useragent.locale", "es-ES");

`SpecialPowers.pushPrefEnv` please.

::: browser/components/extensions/test/browser/browser_ext_pageAction_context.js:196
(Diff revision 1)
> +          "message": "default.html",
> +          "description": "Popup",
> +        },
> +
> +        "title": {
> +          "message": "title",

It would be nice to use a string that differs by more than case.

::: toolkit/components/extensions/Extension.jsm:998
(Diff revision 1)
>  
>          preprocessors: {},
>        };
>  
>        if (this.localeData) {
> -        context.preprocessors.localize = this.localize.bind(this);
> +        context.preprocessors.localize = (value, context) => this.localize(value);

Why this change?

::: toolkit/components/extensions/Extension.jsm:1709
(Diff revision 1)
>  
>      return this.permissions.has(perm);
>    }
>  
>    get name() {
> -    return this.localize(this.manifest.name);
> +    return this.localize(this.rawManifest.name);

I don't think this is the right approach. We definitely shouldn't be re-localizing the localized name, but we currently localize all other extension strings based on the selected locale at startup, and I don't think this one should be any different.
Attachment #8781863 - Flags: review?(kmaglione+bmo)
Comment on attachment 8781863 [details]
Bug 1295894 - Show localized strings for browserAction/pageAction title.

https://reviewboard.mozilla.org/r/72202/#review70392

> Why this change?

Without this change, the updated test will fail.

If the preprocess `context` is passed to `(ExtensionData).localize` and then `(LocaleData).localize` as the second parameter, this `context` will be passed to `(LocaleData).localizeMessage` as `options.locale` instead of `(LocaleData).selectedLocale`. As a result, the strings in manifest.json will be localized into `(LocaleData).defaultLocale`.
Comment on attachment 8781863 [details]
Bug 1295894 - Show localized strings for browserAction/pageAction title.

https://reviewboard.mozilla.org/r/72202/#review70908

Thanks
Attachment #8781863 - Flags: review?(kmaglione+bmo) → review+
(In reply to Hector Zhao [:hectorz] from comment #6)
> Comment on attachment 8781863 [details]
> Bug 1295894 - Show localized strings for browserAction/pageAction title.
> 
> Review request updated; see interdiff:
> https://reviewboard.mozilla.org/r/72202/diff/2-3/

Updated the test to use "\u00ed" instead of "í", carrying over r+.
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/5aea87afc510
Show localized strings for browserAction/pageAction title. r=kmag
Keywords: checkin-needed
Whiteboard: triaged
https://hg.mozilla.org/mozilla-central/rev/5aea87afc510
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Verified on 51.0a1 (buildid: 20160823072522), fixed!
Status: RESOLVED → VERIFIED
Comment on attachment 8781863 [details]
Bug 1295894 - Show localized strings for browserAction/pageAction title.

Approval Request Comment
[Feature/regressing bug #]: Bug 1214955
[User impact if declined]: Tooltips of toolbar buttons created by WebExtensions are not localized
[Describe test coverage new/current, TreeHerder]: Verified as fixed by QA in Beijing office, and pageAction tests updated for this
[Risks and why]: Low, the fix is minimal and only touches preprocessing of WebExtensions manifest
[String/UUID change made/needed]: None
Attachment #8781863 - Flags: approval-mozilla-beta?
Attachment #8781863 - Flags: approval-mozilla-aurora?
Comment on attachment 8781863 [details]
Bug 1295894 - Show localized strings for browserAction/pageAction title.

This patch fixes a localized string issue and is verified and test is included. Take it in aurora and 49 beta.
Attachment #8781863 - Flags: approval-mozilla-beta?
Attachment #8781863 - Flags: approval-mozilla-beta+
Attachment #8781863 - Flags: approval-mozilla-aurora?
Attachment #8781863 - Flags: approval-mozilla-aurora+
Duplicate of this bug: 1299423
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.