1209184 - Support localization / i18n message in CSS

Status: RESOLVED FIXED

(WebExtensions :: Untriaged, defect, P1)

Description

[Dan Callahan [:callahad]]

The chrome docs for chrome.i18n suggest using @@extension_id to reference files within an extension, e.g.:

    body {
        background-image:url('chrome-extension://__MSG_@@extension_id__/background.png');
    }

While we support @@extension_id in chrome.i18n.getMessage(), we do not currently translate it in CSS. This limitation is documented at https://developer.mozilla.org/en-US/Add-ons/WebExtensions/Chrome_incompatibilities#i18n

Without support for automatically replacing __MSG_@@extension_id__ in injected CSS, add-on authors can't reference bundled images from their CSS.

Tests that make use of this are available at https://github.com/callahad/webextension-tests/tree/master/background_image

For this bug to be of practical use, we also need to resolve Bug 1208763 and Bug 1208756

Comment 1

[Kris Maglione [:kmag]]

This should be implemented as a simple stream filter on .css files in the moz-extension: protocol, using the Extension.localize method.

I'm willing to mentor anyone who wants to work on this.

Incidentally, I think that @@extension_id is the least interesting use for this. I'd rather extension CSS files reference images using relative URLs rather than hacks like this.

Comment 2

[Kris Maglione [:kmag]]

Documented here:

https://developer.mozilla.org/en-US/Add-ons/WebExtensions/Chrome_incompatibilities#Localized_String_Interpolation

Be sure to update once this is implemented.

Comment 3

[Kris Maglione [:kmag]]

On second thought, this probably isn't a good first bug. The stream filter portion is, but integrating it with the protocol handler will be a bit tricky.

Comment 4

[Kris Maglione [:kmag]]

Attachment: MozReview Request: Bug 1209184: Part 1a - [webext] Make localization work in content processes. r=billm

Bug 1209184: Part 1a - [webext] Make localization work in content processes. r?billm

Comment 5

[Kris Maglione [:kmag]]

Attachment: MozReview Request: Bug 1209184: Part 1b - [webext] Create a stream converter for localization placeholders. r=billm

Bug 1209184: Part 1b - [webext] Create a stream converter for localization placeholders. r?billm

Comment 6

[Kris Maglione [:kmag]]

Attachment: MozReview Request: Bug 1209184: Part 2 - [webext] Localize CSS files served from moz-extension: channels. r=billm r?jdm

Bug 1209184: Part 2 - [webext] Localize CSS files served from moz-extension: channels. r?billm

Comment 7

[Kris Maglione [:kmag]]

Attachment: MozReview Request: Bug 1209184: Part 3 - [webext] Add tests for locale stream converter. r=billm

Bug 1209184: Part 3 - [webext] Add tests for locale stream converter. r?billm

Comment 8

[Kris Maglione [:kmag]]

Attachment: MozReview Request: Bug 1209184: Part 4 - [webext] Add tests for CSS localization filters. r=billm

Bug 1209184: Part 4 - [webext] Add tests for CSS localization filters. r?billm

Comment 9

[Kris Maglione [:kmag]]

(In reply to Kris Maglione [:kmag] from comment #4)
> Created attachment 8689176 [details]
> MozReview Request: Bug 1209184: Part 1a - [webext] Make localization work in
> content processes. r?billm
> 
> Bug 1209184: Part 1a - [webext] Make localization work in content processes.
> r?billm

I didn't realize this was going to be necessary until I ran e10s tests, or I would have done it as a separate bug. I was hoping ReviewBoard would mark the ExtensionData move as an unchanged code move, but alas... In any case, there are no changes to the moved code.

Comment 10

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

Comment on attachment 8689176 [details]
MozReview Request: Bug 1209184: Part 1a - [webext] Make localization work in content processes. r=billm

https://reviewboard.mozilla.org/r/25563/#review23225

I think this would be nicer if we didn't have to move so much code to ExtensionUtils. Most of it will never be used by the content process. As we do more schema checking of the manifest, I worry it will get more awkward if this code doesn't stay in Extension.jsm. Could we just move localize/localizeMessage to ExtensionUtils and pass the data they need as parameters?

::: toolkit/components/extensions/ExtensionUtils.jsm:210
(Diff revision 1)
> +      return Locale.getLocale();

Just noticed this: we need this to be a Chrome-style locale. And we might want to use selectedLocale here, though I'm not sure.

Comment 11

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

Comment on attachment 8689177 [details]
MozReview Request: Bug 1209184: Part 1b - [webext] Create a stream converter for localization placeholders. r=billm

https://reviewboard.mozilla.org/r/25565/#review23227

::: toolkit/components/utils/simpleServices.js:175
(Diff revision 1)
> +  getAddonId(aContext) {

It might be nice to comment that we expect aContext to be a nsIURI.

::: toolkit/components/utils/simpleServices.js:195
(Diff revision 1)
> +  convert(aStream, aFromType, aToType, aContext) {

Can we throw an exception if the from/to types aren't as we expect? Same for the async version.

Comment 12

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

Comment on attachment 8689177 [details]
MozReview Request: Bug 1209184: Part 1b - [webext] Create a stream converter for localization placeholders. r=billm

https://reviewboard.mozilla.org/r/25565/#review23229

::: toolkit/components/utils/simpleServices.js:165
(Diff revision 1)
> -this.NSGetFactory = XPCOMUtils.generateNSGetFactory([RemoteTagServiceService, AddonPolicyService]);
> +function AddonLocalizationConverter()

Can you also refer people to the code in ExtensionProtocolHandler and say that these two are closely tied together?

Comment 13

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

Comment on attachment 8689177 [details]
MozReview Request: Bug 1209184: Part 1b - [webext] Create a stream converter for localization placeholders. r=billm

Reviewboard canceled my r+ for some reason. I guess I shouldn't have commented twice.

Comment 14

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

Comment on attachment 8689178 [details]
MozReview Request: Bug 1209184: Part 2 - [webext] Localize CSS files served from moz-extension: channels. r=billm r?jdm

https://reviewboard.mozilla.org/r/25567/#review23231

Comment 15

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

Comment on attachment 8689179 [details]
MozReview Request: Bug 1209184: Part 3 - [webext] Add tests for locale stream converter. r=billm

https://reviewboard.mozilla.org/r/25569/#review23233

Comment 16

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

Comment on attachment 8689180 [details]
MozReview Request: Bug 1209184: Part 4 - [webext] Add tests for CSS localization filters. r=billm

https://reviewboard.mozilla.org/r/25571/#review23235

Comment 17

[Kris Maglione [:kmag]]

(In reply to Bill McCloskey (:billm) from comment #10)
> Just noticed this: we need this to be a Chrome-style locale. And we might
> want to use selectedLocale here, though I'm not sure.

Chromium appears to use the browser locale.

(In reply to Bill McCloskey (:billm) from comment #11)
> ::: toolkit/components/utils/simpleServices.js:195
> (Diff revision 1)
> > +  convert(aStream, aFromType, aToType, aContext) {
> 
> Can we throw an exception if the from/to types aren't as we expect? Same for
> the async version.

I guess. I thought about checking initially, but I couldn't think of any reason it should matter.

Comment 18

[Kris Maglione [:kmag]]

Comment on attachment 8689176 [details]
MozReview Request: Bug 1209184: Part 1a - [webext] Make localization work in content processes. r=billm

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

Comment 19

[Kris Maglione [:kmag]]

Comment on attachment 8689177 [details]
MozReview Request: Bug 1209184: Part 1b - [webext] Create a stream converter for localization placeholders. r=billm

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

Comment 20

[Kris Maglione [:kmag]]

Comment on attachment 8689178 [details]
MozReview Request: Bug 1209184: Part 2 - [webext] Localize CSS files served from moz-extension: channels. r=billm r?jdm

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

Comment 21

[Kris Maglione [:kmag]]

Comment on attachment 8689179 [details]
MozReview Request: Bug 1209184: Part 3 - [webext] Add tests for locale stream converter. r=billm

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

Comment 22

[Kris Maglione [:kmag]]

Comment on attachment 8689180 [details]
MozReview Request: Bug 1209184: Part 4 - [webext] Add tests for CSS localization filters. r=billm

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

Comment 23

[Kris Maglione [:kmag]]

https://reviewboard.mozilla.org/r/25567/#review23231

Re-requesting review on this one, since I realized the pipe was being held open until the JS wrapper for the listener was GCed. I just added a callback to close it when the request completes.

Comment 24

[Kris Maglione [:kmag]]

https://reviewboard.mozilla.org/r/25563/#review23225

> Just noticed this: we need this to be a Chrome-style locale. And we might want to use selectedLocale here, though I'm not sure.

Since I was fixing this anyway, I also fixed the bidi strings. I was going to do it in another bug, but they're arguably the main use case for CSS localization.

Comment 25

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

Comment on attachment 8689176 [details]
MozReview Request: Bug 1209184: Part 1a - [webext] Make localization work in content processes. r=billm

https://reviewboard.mozilla.org/r/25563/#review23251

This looks great. Thanks!

::: toolkit/components/extensions/Extension.jsm:351
(Diff revision 2)
> -  // Map(locale-name -> Map(message-key -> localized-strings))
> +  this.locales = null;

I think it would be clearer to call this localeData.

Comment 26

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

Comment on attachment 8689177 [details]
MozReview Request: Bug 1209184: Part 1b - [webext] Create a stream converter for localization placeholders. r=billm

https://reviewboard.mozilla.org/r/25565/#review23253

Comment 27

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

Comment on attachment 8689178 [details]
MozReview Request: Bug 1209184: Part 2 - [webext] Localize CSS files served from moz-extension: channels. r=billm r?jdm

Someone who understand the networking code better should look at this. Josh, do you mind?

Comment 28

[Kris Maglione [:kmag]]

Comment on attachment 8689176 [details]
MozReview Request: Bug 1209184: Part 1a - [webext] Make localization work in content processes. r=billm

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

Comment 29

[Kris Maglione [:kmag]]

Comment on attachment 8689177 [details]
MozReview Request: Bug 1209184: Part 1b - [webext] Create a stream converter for localization placeholders. r=billm

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

Comment 30

[Kris Maglione [:kmag]]

Comment on attachment 8689178 [details]
MozReview Request: Bug 1209184: Part 2 - [webext] Localize CSS files served from moz-extension: channels. r=billm r?jdm

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

Comment 31

[Kris Maglione [:kmag]]

Comment on attachment 8689179 [details]
MozReview Request: Bug 1209184: Part 3 - [webext] Add tests for locale stream converter. r=billm

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

Comment 32

[Kris Maglione [:kmag]]

Comment on attachment 8689180 [details]
MozReview Request: Bug 1209184: Part 4 - [webext] Add tests for CSS localization filters. r=billm

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

Comment 33

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/fx-team/rev/eb7962bf88a8517d92c21263f3982fe95ff80eb4
Bug 1209184: Part 1a - [webext] Make localization work in content processes. r=billm

Comment 34

[Carsten Book [:Tomcat]]

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

Comment 35

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

Comment on attachment 8689178 [details]
MozReview Request: Bug 1209184: Part 2 - [webext] Localize CSS files served from moz-extension: channels. r=billm r?jdm

Josh's review is sufficient.

Comment 36

[Josh Matthews [:jdm]]

Comment on attachment 8689178 [details]
MozReview Request: Bug 1209184: Part 2 - [webext] Localize CSS files served from moz-extension: channels. r=billm r?jdm

https://reviewboard.mozilla.org/r/25567/#review23809

::: netwerk/protocol/res/ExtensionProtocolHandler.cpp:51
(Diff revision 3)
> +  PipeCloser(nsIOutputStream* aOutputStream) :

explicit

::: netwerk/protocol/res/ExtensionProtocolHandler.cpp:63
(Diff revision 3)
> +    return mOutputStream->Close();

Let's null out the pointer here, too.

::: netwerk/protocol/res/ExtensionProtocolHandler.cpp:88
(Diff revision 3)
> +  if (ext.LowerCaseEqualsLiteral("css")) {

Could we invert this condition and return early instead?

::: netwerk/protocol/res/ExtensionProtocolHandler.cpp:141
(Diff revision 3)
> +  return rv;

Let's return NS_OK explicitly.

Comment 37

[Kris Maglione [:kmag]]

https://reviewboard.mozilla.org/r/25567/#review23809

Thanks

> Could we invert this condition and return early instead?

I guess we can. I went with the `if` block in case we wanted to add more substitutions later, but that's probably not very likely.

Comment 38

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/fx-team/rev/e75f9f24d0dc7c08131ebc08b0dcfcb4f310269c
Bug 1209184: Part 1b - [webext] Create a stream converter for localization placeholders. r=billm

https://hg.mozilla.org/integration/fx-team/rev/9c63ffd499ebc4ed4f164d4b35e09a7a03c36387
Bug 1209184: Part 2 - [webext] Localize CSS files served from moz-extension: channels. r=jdm

https://hg.mozilla.org/integration/fx-team/rev/f9ab766896106b5718920a274ef4fe3605823b75
Bug 1209184: Part 3 - [webext] Add tests for locale stream converter. r=billm

https://hg.mozilla.org/integration/fx-team/rev/8e692344588a9b0155259d0d3fb1050b5f22230c
Bug 1209184: Part 4 - [webext] Add tests for CSS localization filters. r=billm

Comment 39

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/fx-team/rev/2e446ebafe6c - I don't see why this would be the case, but that broke Android and B2G xpcshell, with a couple of "KeyError: 'tail'" exceptions, https://treeherder.mozilla.org/logviewer.html#?job_id=5951327&repo=fx-team

Comment 40

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/fx-team/rev/a4f1765a6cdf7bca6ed2d22788be9e03e6ed8c24
Bug 1209184: Part 1b - [webext] Create a stream converter for localization placeholders. r=billm

https://hg.mozilla.org/integration/fx-team/rev/a517959befe9b9e426380771c8712e568bb62832
Bug 1209184: Part 2 - [webext] Localize CSS files served from moz-extension: channels. r=jdm

https://hg.mozilla.org/integration/fx-team/rev/399404ff25e40003c620cd1d6193790127a5c121
Bug 1209184: Part 3 - [webext] Add tests for locale stream converter. r=billm

https://hg.mozilla.org/integration/fx-team/rev/24282235336dd3dd3c725bcd6e8025dcf9fe0fb4
Bug 1209184: Part 4 - [webext] Add tests for CSS localization filters. r=billm

Comment 41

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/d0404928473e

Comment 42

[Phil Ringnalda (:philor)]

Rebacked out in https://hg.mozilla.org/integration/fx-team/rev/cbf641f8da0a because I'm pretty sure this is what's causing bug 1228742.

Comment 43

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/fx-team/rev/62ff0d59cd62619bede132718cf524f2a142646d
Bug 1209184: Part 1b - [webext] Create a stream converter for localization placeholders. r=billm

https://hg.mozilla.org/integration/fx-team/rev/ceef1da728bf078fe100f35d80ff237ac7f3b7ba
Bug 1209184: Part 2 - [webext] Localize CSS files served from moz-extension: channels. r=jdm

https://hg.mozilla.org/integration/fx-team/rev/b8fe968c1775a69b166b5c4fae1684d06efec74a
Bug 1209184: Part 3 - [webext] Add tests for locale stream converter. r=billm

https://hg.mozilla.org/integration/fx-team/rev/af59d2160097bfd49aefe71e0ee786111c7ea7a2
Bug 1209184: Part 4 - [webext] Add tests for CSS localization filters. r=billm

Comment 44

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/62ff0d59cd62
https://hg.mozilla.org/mozilla-central/rev/ceef1da728bf
https://hg.mozilla.org/mozilla-central/rev/b8fe968c1775
https://hg.mozilla.org/mozilla-central/rev/af59d2160097

Comment 45

[Will Bamberg [:wbamberg]]

Updated https://developer.mozilla.org/en-US/Add-ons/WebExtensions/Chrome_incompatibilities.