Closed Bug 1411452 Opened 7 years ago Closed 7 years ago

Use prerendered localized html/js in repacks

Categories

(Firefox :: New Tab Page, defect)

Product:
Firefox
Component:
New Tab Page
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED INVALID
Tracking Flags:
Tracking Status
firefox58 --- affected

People

(Reporter: Mardak, Assigned: Mardak)

References

Details

Attachments

(3 files)

Bug 1411452 - Use prerendered localized html/js in repacks.
(deleted), text/x-review-board-request
Pike
: review-
Details
Bug 1411452 - Use chrome: instead of resource: for localized files. (Not really part 2 - to be squashed into other patch).
(deleted), text/x-review-board-request
Details
screenshot of "ar" repack
(deleted), image/png
Details 
Activity Stream exports various html and js files that are React prerendered (generated files that include locale, strings, text direction and other state):
https://searchfox.org/mozilla-central/source/browser/extensions/activity-stream/prerendered/ar/activity-stream.html#2

This would allow for locales like "ar" to be first loaded in the expected rtl direction with its strings without needing to wait for the main process to send data to the content process.

These files would be packaged by jar.mn with the appropriate AB_CD set:
https://searchfox.org/mozilla-central/source/browser/extensions/activity-stream/jar.mn#53-56

With some initial poking around browser/locales/Makefile.in, the xpi-stage/locale-* version of the file does get the expected file. Notice that locale-ar has the appropriate lang and rtl direction below:

```
obj$ find dist/xpi-stage -name activity-stream.html -print -exec grep lang= {} \;
dist/xpi-stage/locale-ar/browser/features/activity-stream@mozilla.org/chrome/content/data/content/activity-stream.html
<html lang="ar" dir="rtl">
dist/xpi-stage/locale-en-US/browser/features/activity-stream@mozilla.org/chrome/content/data/content/activity-stream.html
<html lang="en-US" dir="ltr">
dist/xpi-stage/locale-zh-TW/browser/features/activity-stream@mozilla.org/chrome/content/data/content/activity-stream.html
<html lang="zh-TW" dir="ltr">
```

Do I need to get some activity-stream entry into manifest.json? I skipped the `else` case of parse_chrome_manifest for now just so that at least it doesn't blow up because activity-stream's jar.mn doesn't actually result in a ManifestLocale (no `% locale` entry).

```diff
diff --git a/browser/locales/Makefile.in b/browser/locales/Makefile.in
--- a/browser/locales/Makefile.in
+++ b/browser/locales/Makefile.in
@@ -91,16 +91,17 @@ DEFINES += -DBOOKMARKS_INCLUDE_DIR=$(dir $(call MERGE_FILE,profile/bookmarks.inc
 
 libs-%: AB_CD=$*
 libs-%:
 	$(if $(filter en-US,$(AB_CD)),, @$(MAKE) merge-$*)
 	$(NSINSTALL) -D $(DIST)/install
 	@$(MAKE) -C ../../toolkit/locales libs-$* XPI_ROOT_APPID='$(XPI_ROOT_APPID)'
 	@$(MAKE) -C ../../services/sync/locales AB_CD=$* XPI_NAME=locale-$*
 	@$(MAKE) -C ../../extensions/spellcheck/locales AB_CD=$* XPI_NAME=locale-$*
+	@$(MAKE) -C ../extensions/activity-stream AB_CD=$* XPI_NAME=locale-$*
 ifneq (,$(wildcard ../extensions/formautofill/locales))
 	@$(MAKE) -C ../extensions/formautofill/locales AB_CD=$* XPI_NAME=locale-$*
 endif
 	@$(MAKE) -C ../extensions/onboarding/locales AB_CD=$* XPI_NAME=locale-$*
 	@$(MAKE) -C ../extensions/pocket/locale AB_CD=$* XPI_NAME=locale-$*
 ifndef RELEASE_OR_BETA
 	@$(MAKE) -C ../extensions/presentation/locale AB_CD=$* XPI_NAME=locale-$*
 endif
diff --git a/python/mozbuild/mozbuild/action/langpack_manifest.py b/python/mozbuild/mozbuild/action/langpack_manifest.py
--- a/python/mozbuild/mozbuild/action/langpack_manifest.py
+++ b/python/mozbuild/mozbuild/action/langpack_manifest.py
@@ -275,18 +275,16 @@ def parse_chrome_manifest(path, base_path, chrome_entries):
                 'path': os.path.join(
                     os.path.relpath(
                         os.path.dirname(path),
                         base_path
                     ),
                     entry.relpath
                 )
             })
-        else:
-            raise Exception('Unknown type {0}'.format(entry.name))
 
 
 ###
 # Generates a new web manifest dict with values specific for a language pack.
 #
 # Args:
 #    locstr         (str)  - A string with a comma separated list of locales
 #                            for which resources are embedded in the
```
Flags: needinfo?(l10n)
For some more context, activity stream uses aboutNewTabService to have about:newtab internally pick the appropriate file to load based on various prefs:

  "": "resource://activity-stream/data/content/activity-stream.html",
  "debug": "resource://activity-stream/data/content/activity-stream-debug.html",
  "prerender": "resource://activity-stream/data/content/activity-stream-prerendered.html",
  "prerenderdebug": "resource://activity-stream/data/content/activity-stream-prerendered-debug.html",
https://searchfox.org/mozilla-central/source/browser/components/newtab/aboutNewTabService.js#21-25

For nightly, there's a debug version that is prerendered with en-US:
https://searchfox.org/mozilla-central/source/browser/extensions/activity-stream/jar.mn#20-22

I suppose potentially we could have aboutNewTabService load chrome://activity-stream/locale/activity-stream.html or resource://activity-stream/data/content/activity-stream-debug.html as appropriate (i.e., packaging some files via locales/jar.mn and others via the current resource way).
Assignee: nobody → edilee
I have a couple of doubts here, and for Firefox proper, the whole stack should go through a throrogh l10n review.

For the system addon, please package all your addon data in the addon, and don't try to use the Firefox build system to optimize things.

Bundling all locales and using chrome://activity-stream/locale/activity-stream.html sounds like an OK path forward.
Flags: needinfo?(l10n)
Attached image screenshot of "ar" repack (deleted) — 
Tested with *not* artifact build:

./mach build && ./mach package && ./mach build installers-ar
./obj/dist/l10n-stage/firefox/Nightly.app/Contents/MacOS/firefox-bin --profile ./obj/tmp/scratch_user --foreground about:home view-source:about:home

Notice the lang/dir/title/chrome:…initial-state.
Ah hrm… turns out the localized scripts are failing to load:

<script> source URI is not allowed in this document: “chrome://activity-stream/locale/activity-stream-initial-state.js”.
<script> source URI is not allowed in this document: “chrome://activity-stream/locale/activity-stream-strings.js”.

It's not that noticeable with the patches here because we get the data from main right away. But with a separate PR that I additionally remove the strings data from main, it's clear because we end up with "search_web_placeholder" replacing the prerendered "Search the Web", etc.
Ah. Well turns out chrome://browser/content/* is allowed because `% content … contentaccessible=yes` although that flag does not apply to `% locale` entries:

https://searchfox.org/mozilla-central/source/xpcom/components/ManifestParser.cpp#59

That's why we can load chrome://browser/content/contentSearchUI.js but not chrome://activity-stream/locale/activity-stream-initial-state.js
Depends on: 1412151
Without bug 1412151, I suppose we could just package all locales' state in a resource: .js file, e.g.,

resource://activity-stream/data/content/activity-stream-initial-state.js

window.gActivityStreamPrerenderedState = ({
  "ar": {"TopSites": …},
  "en-US": {…},
  …
})[document.documentElement.lang];
Flags: needinfo?(khudson)
Comment on attachment 8922539 [details]
Bug 1411452 - Use prerendered localized html/js in repacks.

https://reviewboard.mozilla.org/r/193616/#review198978

Even more so than before, I'm concerned about the blend between add-on and firefox core here. An optimization of a single add-on shouldn't tie so deeply into what the rest of Firefox does architecturally, I think.

::: browser/extensions/activity-stream/locales/jar.mn:40
(Diff revision 1)
> +  locale/@AB_CD@/activity-stream.html (../prerendered/@AB_CD@/activity-stream.html)
> +#else
> +  locale/@AB_CD@/activity-stream-initial-state.js (../prerendered/en-US/activity-stream-initial-state.js)
> +  locale/@AB_CD@/activity-stream-prerendered.html (../prerendered/en-US/activity-stream-prerendered.html)
> +  locale/@AB_CD@/activity-stream.html (../prerendered/en-US/activity-stream.html)
> +#endif

I'm sorry, but this is way too much logic.

Quite a few folks in build and l10n want to change how l10n repacks work under the hood, and I don't want to commit anyone to support this particular usage.
Attachment #8922539 - Flags: review?(l10n) → review-
Comment on attachment 8922539 [details]
Bug 1411452 - Use prerendered localized html/js in repacks.

Putting this on Dan's radar, too. Took me a while to get a glimpse on what we're doing here.

AFAICT, we're taking localization files, and then build js files from them, and then load those. Mardak, is that right?

Wanna make sure that Dan is OK with this before I go in and r+ a patch.
Attachment #8922539 - Flags: feedback?(dveditz)
(In reply to Axel Hecht [:Pike] from comment #9)
> I'm sorry, but this is way too much logic.
What's the concern here? Is it the large condition? It's the same thing being used elsewhere in the tree, e.g., https://searchfox.org/mozilla-central/source/browser/extensions/pocket/locale/jar.mn#35

(In reply to Axel Hecht [:Pike] from comment #10)
> AFAICT, we're taking localization files, and then build js files from them,
> and then load those. Mardak, is that right?
Pretty much yes. You can think of it as converting the pontoon data into a JS object (and adding in other activity stream data):
https://searchfox.org/mozilla-central/source/browser/extensions/activity-stream/prerendered/ar/activity-stream-initial-state.js#13-15

Should we be doing things without a locale manifest and keep with our resource: usage? I can WONTFIX this bug if so.
Flags: needinfo?(l10n)
The dependency on bug 1412151 is only if we include .js files in the locale manifest. If it's just .html files, we won't need contentaccessible=yes.
I think going for resource: and doing language negotiation in the script that loads the data is the best way to do this, and should get you across the chrome/content boundary, too.

As for security concerns, best to handle l10n as data. We had issues with localizations having <script> tags in them in the past.
Flags: needinfo?(l10n)
Pike, just to be clear, is it okay to package localized .html files? Attachment 8922545 [details] shows us packaging a dir="rtl" for the "ar" version of the .html file. If we don't do that, we then need to dynamically set the text direction causing a brief flash of ltr then rtl.
Flags: needinfo?(l10n)
You're asking me for something that's effectively a security review. I'd have to defer to the folks in of that.

I suspect that we had solutions to this in the b2g days, but I don't remember what those were, and how they intermingled with other things. Also, they had a low-end js stack back then. gandalf or stas might remember.
Flags: needinfo?(l10n)
Oh. I suppose is the security issue that some malicious repack could put in whatever html/js it wanted getting access to more data? I suppose if that's the case, then I guess we don't want to expose this activity stream stuff via locale manifest.
Sounds like there would be security and privacy issues of the approach, so I'll just close this.
Status: NEW → RESOLVED
Closed: 7 years ago
Flags: needinfo?(khudson)
Resolution: --- → INVALID
Attachment #8922539 - Flags: feedback?(dveditz)
Attachment #8922540 - Flags: review?(khudson)
Component: Activity Streams: Newtab → New Tab Page
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: