Dan Callahan [:callahad] Reporter
	Description • 9 years ago

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

	Kris Maglione [:kmag] Assignee
	Comment 1 • 9 years ago

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.

	Kris Maglione [:kmag] Assignee
	Comment 2 • 9 years ago

Documented here:

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

Be sure to update once this is implemented.

	Kris Maglione [:kmag] Assignee
	Comment 3 • 9 years ago

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.

	Kris Maglione [:kmag] Assignee
	Comment 4 • 9 years ago

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

	Kris Maglione [:kmag] Assignee
	Comment 5 • 9 years ago

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

	Kris Maglione [:kmag] Assignee
	Comment 6 • 9 years ago

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

	Kris Maglione [:kmag] Assignee
	Comment 7 • 9 years ago

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

	Kris Maglione [:kmag] Assignee
	Comment 8 • 9 years ago

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

	Kris Maglione [:kmag] Assignee
	Comment 9 • 9 years ago

(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.

	Bill McCloskey [inactive unless it's an emergency] (:billm)
	Comment 10 • 9 years ago

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.

	Bill McCloskey [inactive unless it's an emergency] (:billm)
	Comment 11 • 9 years ago

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.

	Bill McCloskey [inactive unless it's an emergency] (:billm)
	Comment 12 • 9 years ago

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?

	Bill McCloskey [inactive unless it's an emergency] (:billm)
	Comment 13 • 9 years ago

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.

	Bill McCloskey [inactive unless it's an emergency] (:billm)
	Comment 14 • 9 years ago

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

	Bill McCloskey [inactive unless it's an emergency] (:billm)
	Comment 15 • 9 years ago

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

	Bill McCloskey [inactive unless it's an emergency] (:billm)
	Comment 16 • 9 years ago

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

	Kris Maglione [:kmag] Assignee
	Comment 17 • 9 years ago

(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.

	Kris Maglione [:kmag] Assignee
	Comment 18 • 9 years ago

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/

	Kris Maglione [:kmag] Assignee
	Comment 19 • 9 years ago

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/

	Kris Maglione [:kmag] Assignee
	Comment 20 • 9 years ago

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/

	Kris Maglione [:kmag] Assignee
	Comment 21 • 9 years ago

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/

	Kris Maglione [:kmag] Assignee
	Comment 22 • 9 years ago

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/

	Kris Maglione [:kmag] Assignee
	Comment 23 • 9 years ago

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.

	Kris Maglione [:kmag] Assignee
	Comment 24 • 9 years ago

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.

	Bill McCloskey [inactive unless it's an emergency] (:billm)
	Comment 25 • 9 years ago

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.

	Bill McCloskey [inactive unless it's an emergency] (:billm)
	Comment 26 • 9 years ago

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

	Bill McCloskey [inactive unless it's an emergency] (:billm)
	Comment 27 • 9 years ago

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?

	Kris Maglione [:kmag] Assignee
	Comment 28 • 9 years ago

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/

	Kris Maglione [:kmag] Assignee
	Comment 29 • 9 years ago

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/

	Kris Maglione [:kmag] Assignee
	Comment 30 • 9 years ago

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/

	Kris Maglione [:kmag] Assignee
	Comment 31 • 9 years ago

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/

	Kris Maglione [:kmag] Assignee
	Comment 32 • 9 years ago

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/

	Kris Maglione [:kmag] Assignee
	Comment 33 • 9 years ago

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

	Carsten Book [:Tomcat]
	Comment 34 • 9 years ago
bugherder

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

	Bill McCloskey [inactive unless it's an emergency] (:billm)
	Comment 35 • 9 years ago

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.

	Josh Matthews [:jdm]
	Comment 36 • 9 years ago

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.

	Kris Maglione [:kmag] Assignee
	Comment 37 • 9 years ago

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.

	Kris Maglione [:kmag] Assignee
	Comment 38 • 9 years ago

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

	Phil Ringnalda (:philor)
	Comment 39 • 9 years ago

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

	Kris Maglione [:kmag] Assignee
	Comment 40 • 9 years ago

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

	Pulsebot
	Comment 41 • 9 years ago

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

	Phil Ringnalda (:philor)
	Comment 42 • 9 years ago

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

	Kris Maglione [:kmag] Assignee
	Comment 43 • 9 years ago

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

	Carsten Book [:Tomcat]
	Comment 44 • 9 years ago
bugherder

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

	Will Bamberg [:wbamberg]
	Comment 45 • 8 years ago

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