1488973 - Enable DocumentL10n in non-system-principal

Status: RESOLVED FIXED

(Core :: Internationalization, enhancement, P2)

Description

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

In bug 1455649 we enable DocumentL10n API on nsIDocument but we had to limit the API to work only with system principal because it cannot return the Promise with values from system principal to non-system principal.

This bug will be about extending the API to support that.

Comment 1

[Matthew N. [:MattN]]

Please test with resource://payments/paymentRequest.xhtml to ensure that this meets the needs of web payments now that you closed bug 1407418.

Comment 2

[Dave Townsend [:mossop]]

Attachment: Bug 1488973: Wrap privileged promises in a promise for the document we're returning to.

This is a PoC to make document.l10n work in unprivileged documents. It works by
listening to the promise returned from the privileged code then cloning the
result into the unprivileged context and passing that to a promise generated for
the unprivileged page.

Comment 3

[Dave Townsend [:mossop]]

Comment on attachment 9014620 [details]
Bug 1488973: Wrap privileged promises in a promise for the document we're returning to.

This is a PoC for fixing the issue with promises. It passes tests and seems to work in the about:robots case. What do you think?

Comment 4

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

Comment on attachment 9014620 [details]
Bug 1488973: Wrap privileged promises in a promise for the document we're returning to.

Damn, that looks clean.

Thank you Dave!

Comment 5

[Dave Townsend [:mossop]]

Attachment: Bug 1488973: Register locale sources in all processes.

Comment 6

[Dave Townsend [:mossop]]

Attachment: Bug 1488973: Add tests for localisation of unprivileged content.

Comment 7

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

Attachment: Bug 1488973: Wrap privileged promises in a promise for the document we`re returning to. r=smaug

Comment 8

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

Attachment: Bug 1488973: Register locale sources in all processes. r=mossop

Depends on D7961

Comment 9

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

Dave, I updated your patches to:
 - skip creating the wrapper for system principal
 - only add browser's omni.ja if the app is a browser

I launched a talos test with the changes: https://treeherder.mozilla.org/#/jobs?repo=try&revision=68be1324252b4c48636f8a01a8540ce166083c65

And we should get a perfherder results soon: https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=72eed19d131e&newProject=try&newRevision=68be1324252b

Dave - I could not get the test to work locally at all - I tried `./mach test intl/l10n/test` and it opens a new window for "resource://l10n-test/test.html" and an error: "Firefox can’t find the file at /home/zbraniecki/projects/mozilla-unified/obj-x86_64-pc-linux-gnu-opt/_tests/testing/mochitest/browser/intl/l10n/test.html."

Can you help me workaround it?

Comment 10

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

Hmm, my original patch had an error (B2G ID instead of Firefox ID).

Updated: https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=72eed19d131e&newProject=try&newRevision=921f4ecc001a&framework=1

Some small regression in `cpstartup content-process-startup` but wins in `about_preferences_basic`.

Keeping the NI on Dave for the tests.

Comment 11

[Dave Townsend [:mossop]]

(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #9)
> Dave - I could not get the test to work locally at all - I tried `./mach
> test intl/l10n/test` and it opens a new window for
> "resource://l10n-test/test.html" and an error: "Firefox can’t find the file
> at
> /home/zbraniecki/projects/mozilla-unified/obj-x86_64-pc-linux-gnu-opt/_tests/
> testing/mochitest/browser/intl/l10n/test.html."
> 
> Can you help me workaround it?

You need to add a "/" on the end of the uri at the top of the test file.

Comment 12

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

Attachment: Bug 1488973: Add tests for localisation of unprivileged content. r=mossop

Depends on D7962

Comment 13

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

Tests added!

Comment 14

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

Mossop - I think the performance trade-off is your call. Are you ok landing this patchset with the tradeoff, or would you prefer us to try to optimize it?

On IRC you suggested lazily loading L10nRegistry. I'm not sure if I understand how would that work. Currently, any piece of code (in particular, process bootstrap and AddonManager) can register FileSources.

If we go the naive route and just move the L10nRegistry initialization to mozIDOMLocalization constructor which would load L10nRegistry JSM if it's not loaded, then I'm not sure how it would impact AddonsManager (and overall, will AddonsManager register langpacks per process??)

Alternatively, we could try to rewire L10nRegistry to add/remove sources and fetch/cache I/O only in parent process and create a content process to just fetch data from the parent process but then we'd have to transfer all of the FTL resources (in form of parsed JSON) from parent to content, which make make the content process localization slower. Is that the right tradeoff? And should it block this bug?

Comment 15

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

One more idea, if implementing a full IPC between parent/content process is going to take time, we could keep the l10nregistries separate for now, but align their sources so that only in the parent you can add/remove/update sources, and L10nRegistry constructor in the content process just looks up for the sources registered in the parent one and initializes with them.

This would allow us to initialize L10nRegistry in content process lazily.

Mossop - does it sound like something worth doing, or would you recommend going for full IPC?

Comment 16

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

Ok, I went for the idea from comment 15. I used pmm.sharedData to allow for lazy loading of L10nRegistry in content processes.

I kept the source registration in nsBrowserGlue to keep it per-product and only in parent process.

Here's new talos: https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=c2c98b5b994d&newProject=try&newRevision=d907ff4c6b08&framework=1

I still think we need to go for the good IPC solution, but we should be able to land this right as 65 open without having to wait for bug 1462841.

Comment 17

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

Updated the patches, here are the latest perherder results: https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=fa428c26fb76&newProject=try&newRevision=c0ed6d3335a6&framework=1

Should be ready to review.

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/819df51e3083
Wrap privileged promises in a promise for the document we`re returning to. r=smaug
https://hg.mozilla.org/integration/autoland/rev/c06b2c594294
Register locale sources in all processes. r=mossop
https://hg.mozilla.org/integration/autoland/rev/6eab8a1b17ce
Add tests for localisation of unprivileged content. r=flod,mossop

Comment 19

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/deb666fd309c
Backed out 3 changesets for causing TV failures on browser_resource_uri.js

Comment 20

[Narcis Beleuzu [:NarcisB]]

:gandalf , there were also xpcshell failures on test_webextension_langpack.js

Log link: https://treeherder.mozilla.org/logviewer.html#?job_id=207452385&repo=autoland&lineNumber=7635

Comment 21

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

Thanks!

Fixed the former with a switch to `requestAnimationFrame`.
Fixed the latter by switching to manual loading of `appinfo` in L10nRegistry because extension tests rely on overriding `Services.appinfo`.

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4f1fd54c853f
Wrap privileged promises in a promise for the document we`re returning to. r=smaug
https://hg.mozilla.org/integration/autoland/rev/755d616d0cb9
Register locale sources in all processes. r=mossop
https://hg.mozilla.org/integration/autoland/rev/08b16a40d372
Add tests for localisation of unprivileged content. r=flod,mossop

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/fa8604507541
Followup for eslint failure. CLOSED TREE

Comment 24

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/4f1fd54c853f
https://hg.mozilla.org/mozilla-central/rev/755d616d0cb9
https://hg.mozilla.org/mozilla-central/rev/08b16a40d372
https://hg.mozilla.org/mozilla-central/rev/fa8604507541