1192433 - Support localized name and description in Web Extensions

Status: RESOLVED FIXED

(WebExtensions :: Untriaged, defect, P1)

Description

[Dave Townsend [:mossop]]

We need to load localized values from the extensions locales.

Comment 1

[Kris Maglione [:kmag]]

Attachment: MozReview Request: Bug 1192433: Part 1 - [webext] Split Extension class to allow accessing data from non-running extensions. r=billm

Bug 1192433: Part 1 - [webext] Split Extension class to allow accessing data from non-running extensions. r?billm

Comment 2

[Kris Maglione [:kmag]]

Attachment: MozReview Request: Bug 1192433: Part 2 - [webext] Allow loading and querying all available locales. r?billm

Bug 1192433: Part 2 - [webext] Allow loading and querying all available locales.

Comment 3

[Kris Maglione [:kmag]]

Attachment: MozReview Request: Bug 1192433: Part 3 - Support localized names and descriptions in WebExtension manifests. r?Mossop

Bug 1192433: Part 3 - Support localized names and descriptions in WebExtension manifests. r?Mossop

Comment 4

[Kris Maglione [:kmag]]

I'm on the fence about the error handling in this. I'm currently logging any packaging errors, but trying to recover from them, and only fatally failing at the last possible moment. The idea is that we should give developers information about as many errors as possible before aborting, so they don't have to fix them one at a time.

But that may start to get complicated as we add more error checking, so perhaps it would be better to just fail immediately.

I uploaded part 3 for reference, but I'm not flagging for review until I have feedback on the first two.

Comment 5

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Sorry for the delay here. This is taking a long time to review.

Comment 6

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8678413 [details]
MozReview Request: Bug 1192433: Part 1 - [webext] Split Extension class to allow accessing data from non-running extensions. r=billm

https://reviewboard.mozilla.org/r/23207/#review20965

::: toolkit/components/extensions/Extension.jsm:641
(Diff revision 1)
> +  __proto__: ExtensionData.prototype,

__proto__ is a non-standard SpiderMonkey extension. Please don't use it.

Comment 7

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8678414 [details]
MozReview Request: Bug 1192433: Part 2 - [webext] Allow loading and querying all available locales. r?billm

https://reviewboard.mozilla.org/r/23209/#review20967

Generally this looks good. However, I'm not happy with the way the difference between Chrome locale names (which use underscores) and Gecko locale names (with dashes) is handled. We seem to be treating the Gecko name as the preferred, canonical form and translating to the Chrome name when needed. I'm not sure that's the right decision. The only place we actually use the Gecko locale is in findClosest, which we only call twice (once in XPIProvider). So maybe we should go the other way around and use the Chrome locale names throughout and translate when calling findClosest?

Either way, the distinction between these two conventions needs to be documented better in the code.

Let's do one more round on this patch. I promise I'll get to it sooner the next time.

::: toolkit/components/extensions/Extension.jsm:367
(Diff revision 1)
> +    this.logger.error(`Loading extension '${this.id}': ${message}`);

Now that we're using this logger, should we hook it up to something with addAppender? And don't we need to set the log level somehow?

::: toolkit/components/extensions/Extension.jsm:371
(Diff revision 1)
>    localizeMessage(message, substitutions) {

What if we move selectedLocale to Extension and require that people calling localize/localizeMessage on ExtensionData have to pass in a locale name? I don't like how XPIProvider needs to set selectedLocale itself.

Extension would override localize to automatically pass in the selectedLocale.

::: toolkit/components/extensions/Extension.jsm:372
(Diff revision 1)
> -    if (message in this.localeMessages) {
> +    if (this.localeMessages.has(this.selectedLocale)) {

How about just using {} if selectedLocale isn't present? Then we save one level of indent.

::: toolkit/components/extensions/Extension.jsm:410
(Diff revision 1)
> -      return this.id;
> +      if (this.uuid) {

uuid is not part of ExtensionData, so this seems wrong. Does the comment mean that we won't get to this point unless we're an Extension?

::: toolkit/components/extensions/Extension.jsm:444
(Diff revision 1)
> +      let fullPath = uri.QueryInterface(Ci.nsIFileURL).file.path

Missing ; here and above.

::: toolkit/components/extensions/Extension.jsm:495
(Diff revision 1)
> +            isDir: name.endsWith("/")

Trailing comma, even for the last item.

::: toolkit/components/extensions/Extension.jsm:531
(Diff revision 1)
> +        this.id = this.manifest.applications.gecko.id;

Maybe make sure it's not just undefined too.

::: toolkit/components/extensions/Extension.jsm:541
(Diff revision 1)
> -    let dir = locale.replace("-", "_");
> +  // parsed contents in |this.localeMessages[locale]|.

this.localeMessages.get(locale)

::: toolkit/components/extensions/Extension.jsm:560
(Diff revision 1)
> +    if (!this._availableLocales) {

Please set locales and \_availableLocales to null in the constructor. Also, it seems like we have both \_availableLocales and locales, which are the same. Can we just use |locales|?

::: toolkit/components/extensions/Extension.jsm:564
(Diff revision 1)
> +        let entries = yield this.readDirectory("_locales")

Missing ;

::: toolkit/components/extensions/Extension.jsm:588
(Diff revision 1)
> +      if (!this.locales.has(default_locale)) {

This seems wrong. default_locale uses Chrome's format (with underscores) and the keys of this.locales are in Gecko format (with dashes).

I think we should provide a getter for default_locale that would convert the manifest property to Gecko form. Then we should use that in XPIProvider.

::: toolkit/components/extensions/Extension.jsm:611
(Diff revision 1)
>      if (locale) {

I just realized that we should be using default_locale as a fallback here.

::: toolkit/components/extensions/Extension.jsm:612
(Diff revision 1)
> -      return this.readLocaleFile(locale.name).catch(() => {});
> +      let name = locale.locales[0];

This seems a little overcomplicated. As long as you have the |locales| map, you can just use the Gecko locale for everything. So .name can be the Gecko locale and then you can use locale.name as the argument to readLocaleFile.

Comment 8

[Kris Maglione [:kmag]]

(In reply to Bill McCloskey (:billm) from comment #5)
> Sorry for the delay here. This is taking a long time to review.

I'll take the blame for that. I was hoping to do this in smaller
pieces, but I kept running into corner cases I didn't want to
ignore. In any case, I'm traveling this week, and barely
managing to keep up with bugmail.

(In reply to Bill McCloskey (:billm) from comment #6)
> ::: toolkit/components/extensions/Extension.jsm:641
> (Diff revision 1)
> > +  __proto__: ExtensionData.prototype,
> 
> __proto__ is a non-standard SpiderMonkey extension. Please don't use it.

It is standard as of ES6, but is only required when he host is a web browser: http://www.ecma-international.org/ecma-262/6.0/#sec-__proto__-property-names-in-object-initializers

Even so, I was hoping to avoid using it, but I couldn't find any good alternatives, aside from writing a helper or waiting for ES 6 classes. Nearly every other place we use sub-classes in JS, we use __proto__. The only existing helpers I could find were in the SDK, some third-party libs bundled with Loop, or TabView, and using any of them seemed like a bad idea.

(In reply to Bill McCloskey (:billm) from comment #7)
> Generally this looks good. However, I'm not happy with the way the
> difference between Chrome locale names (which use underscores) and Gecko
> locale names (with dashes) is handled. We seem to be treating the Gecko name
> as the preferred, canonical form and translating to the Chrome name when
> needed. I'm not sure that's the right decision. The only place we actually
> use the Gecko locale is in findClosest, which we only call twice (once in
> XPIProvider). So maybe we should go the other way around and use the Chrome
> locale names throughout and translate when calling findClosest?
> 
> Either way, the distinction between these two conventions needs to be
> documented better in the code.

I didn't notice how quite inconsistent/confusing that might be.
I'll clean up the documentation to make sure it's clear what
convention we're using. I think I'd prefer to stick with the
gecko convention internally. Right now, it's necessary for the
add-on manager and findClosestLocale. I think that using the
Chrome convention internally is a lot more likely to trip people
up in the future, when interfacing with other Gecko code, but
the extension API barely exposes locale codes at all.

> ::: toolkit/components/extensions/Extension.jsm:367
> (Diff revision 1)
> > +    this.logger.error(`Loading extension '${this.id}': ${message}`);
> 
> Now that we're using this logger, should we hook it up to something with
> addAppender? And don't we need to set the log level somehow?

It's a sub-logger of the one created by the add-on manager, so
it should use its appenders and log levels by default. I think
at some point we're going to want per-add-on appenders and log
levels (which is why I didn't use that logger directly), but
things should work pretty well for now.

> ::: toolkit/components/extensions/Extension.jsm:371
> (Diff revision 1)
> >    localizeMessage(message, substitutions) {
> 
> What if we move selectedLocale to Extension and require that people calling
> localize/localizeMessage on ExtensionData have to pass in a locale name? I
> don't like how XPIProvider needs to set selectedLocale itself.

I didn't really like that either. I was trying to keep things
as simple as possible for the vast majority of callers who only
ever care about one locale, but it's definitely not ideal.

> Extension would override localize to automatically pass in the
> selectedLocale.

I think maybe it would be better to have an argument that
defaults to `this.selectedLocale`. In ExtensionData, that would
be set to `manifest.default_locale`, and in Extension it would
be set based on the UI locale.

> ::: toolkit/components/extensions/Extension.jsm:372
> (Diff revision 1)
> > -    if (message in this.localeMessages) {
> > +    if (this.localeMessages.has(this.selectedLocale)) {
> 
> How about just using {} if selectedLocale isn't present? Then we save one
> level of indent.

Good idea.

> ::: toolkit/components/extensions/Extension.jsm:410
> (Diff revision 1)
> > -      return this.id;
> > +      if (this.uuid) {
> 
> uuid is not part of ExtensionData, so this seems wrong. Does the comment
> mean that we won't get to this point unless we're an Extension?

We may get to this point, but if we do, that property will be
ignored. I guess it would make more sense to have `if ("uuid" in this)`.
It would probably be cleaner to have a method that Extension
could override to provide additional messages, but that seems
excessive for just one message.

> ::: toolkit/components/extensions/Extension.jsm:560
> (Diff revision 1)
> > +    if (!this._availableLocales) {
> 
> Please set locales and \_availableLocales to null in the constructor. Also,
> it seems like we have both \_availableLocales and locales, which are the
> same. Can we just use |locales|?

Well, `_availableLocales` is a promise and `locales` is a map. I
initially just had `locales` and required that
`readAvailableLocales` be called during initialization, before
any function that needed it, but it felt really fragile.

Now that I look at it again, the only thing that uses `.locales`
now without calling `promiseAvailableLocales` is
`readLocaleFile`, and it can just as easily use the promise
version. So I think the solution is to just drop `.locales`.

> ::: toolkit/components/extensions/Extension.jsm:588
> (Diff revision 1)
> > +      if (!this.locales.has(default_locale)) {
> 
> This seems wrong. default_locale uses Chrome's format (with underscores) and
> the keys of this.locales are in Gecko format (with dashes).
> 
> I think we should provide a getter for default_locale that would convert the
> manifest property to Gecko form. Then we should use that in XPIProvider.

Good idea. Will do.

> ::: toolkit/components/extensions/Extension.jsm:611
> (Diff revision 1)
> >      if (locale) {
> 
> I just realized that we should be using default_locale as a fallback here.

Hm. Yeah. I thought `findClosestLocale` always returned a match
if there were any valid locales in the list, but it seems that
it doesn't.

> ::: toolkit/components/extensions/Extension.jsm:612
> (Diff revision 1)
> > -      return this.readLocaleFile(locale.name).catch(() => {});
> > +      let name = locale.locales[0];
> 
> This seems a little overcomplicated. As long as you have the |locales| map,
> you can just use the Gecko locale for everything. So .name can be the Gecko
> locale and then you can use locale.name as the argument to readLocaleFile.

Yeah, good point.

Comment 9

[Kris Maglione [:kmag]]

Comment on attachment 8678413 [details]
MozReview Request: Bug 1192433: Part 1 - [webext] Split Extension class to allow accessing data from non-running extensions. r=billm

Bug 1192433: Part 1 - [webext] Split Extension class to allow accessing data from non-running extensions. r=billm

Comment 10

[Kris Maglione [:kmag]]

Comment on attachment 8678414 [details]
MozReview Request: Bug 1192433: Part 2 - [webext] Allow loading and querying all available locales. r?billm

Bug 1192433: Part 2 - [webext] Allow loading and querying all available locales. r?billm

Comment 11

[Kris Maglione [:kmag]]

Comment on attachment 8678415 [details]
MozReview Request: Bug 1192433: Part 3 - Support localized names and descriptions in WebExtension manifests. r?Mossop

Bug 1192433: Part 3 - Support localized names and descriptions in WebExtension manifests. r?Mossop

Comment 12

[Kris Maglione [:kmag]]

Reflagging part 1 for review, since the changes to get rid of __proto__ were non-trivial. I think I've addressed everything.

Comment 13

[Dave Townsend [:mossop]]

Comment on attachment 8678415 [details]
MozReview Request: Bug 1192433: Part 3 - Support localized names and descriptions in WebExtension manifests. r?Mossop

https://reviewboard.mozilla.org/r/23211/#review21427

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:927
(Diff revision 2)
> +  addon.locales = [...locales.keys()].map(getLocale);

TIL: Array.from(locales.keys(), getLocale)

Comment 14

[Kris Maglione [:kmag]]

(In reply to Dave Townsend [:mossop] from comment #13)
> ::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:927
> (Diff revision 2)
> > +  addon.locales = [...locales.keys()].map(getLocale);
> 
> TIL: Array.from(locales.keys(), getLocale)

Oh. Handy.

Comment 15

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8678414 [details]
MozReview Request: Bug 1192433: Part 2 - [webext] Allow loading and querying all available locales. r?billm

https://reviewboard.mozilla.org/r/23209/#review21753

Comment 16

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/fx-team/rev/d3d825076a477d6886126e1a7966dc81bda4ec15
Bug 1192433: Part 1 - [webext] Split Extension class to allow accessing data from non-running extensions. r=billm

https://hg.mozilla.org/integration/fx-team/rev/e922469bfe5f3d6b6b93be2c6c3f1525567e32eb
Bug 1192433: Part 2 - [webext] Allow loading and querying all available locales. r=billm

https://hg.mozilla.org/integration/fx-team/rev/d34bfda459c96a520d1a3a168e696d990599ea2e
Bug 1192433: Part 3 - Support localized names and descriptions in WebExtension manifests. r=Mossop

Comment 17

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/fx-team/rev/f103f2fdbe131ec3c4f5423588bafb0294d52bf3
Bug 1192433: Return an empty list rather than throwing an error when attempting to read a directory in an app: URL (fixes M1 bustage). r=trivial

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d3d825076a47
https://hg.mozilla.org/mozilla-central/rev/e922469bfe5f
https://hg.mozilla.org/mozilla-central/rev/d34bfda459c9

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f103f2fdbe13