1412930 - Package localized prerendered files

Status: RESOLVED FIXED

(Firefox :: New Tab Page, defect)

Description

[Ed Lee :Mardak]

The #if checks in jar.mn are only evaluated with AB_CD = en_US because repacks don't build that jar.mn with other locales. Can't expose js/html to localizations, see bug 1411452. So reverting to old behavior of packaging all locales but will allow for aboutNewTabService to directly point to the appropriate file.

Comment 1

[Ed Lee :Mardak]

Attachment: Bug 1412930 - Package localized prerendered files.

Review commit: https://reviewboard.mozilla.org/r/194760/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/194760/

Comment 2

[Ed Lee :Mardak]

Comment on attachment 8923637 [details]
Bug 1412930 - Package localized prerendered files.

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

Comment 3

[Ed Lee :Mardak]

Comment on attachment 8923637 [details]
Bug 1412930 - Package localized prerendered files.

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

Comment 4

[Kate Hudson :k88hudson]

Comment on attachment 8923637 [details]
Bug 1412930 - Package localized prerendered files.

https://reviewboard.mozilla.org/r/194760/#review200514

::: browser/components/newtab/aboutNewTabService.js:197
(Diff revision 1)
> -      return ACTIVITY_STREAM_URLS[prerender + debug];
> +      // resource://activity-stream/prerendered/en-US/activity-stream-prerendered.html
> +      // resource://activity-stream/prerendered/activity-stream-debug.html
> +      return [
> +        "resource://activity-stream/prerendered/",
> +        // Only use localized path for non-debug pages (actually localized)
> +        this.activityStreamDebug ? "" : getLocalizedPath(),

Since this is re-calculated every time .defaultURL is referenced I wonder if caching the result of getLocalizedPath would be worth doing (especially since locale won't change)? It's possible the list of locales might also grow

::: browser/extensions/activity-stream/jar.mn:26
(Diff revision 1)
> -  content/data/content/activity-stream-debug.html (./prerendered/en-US/activity-stream-debug.html)
> -  content/data/content/activity-stream-prerendered-debug.html (./prerendered/en-US/activity-stream-prerendered-debug.html)
> +  content/prerendered/activity-stream-debug.html (./prerendered/en-US/activity-stream-debug.html)
> +  content/prerendered/activity-stream-prerendered-debug.html (./prerendered/en-US/activity-stream-prerendered-debug.html)
>  #endif
>  
> -# These defines and ifs below are generated by https://github.com/mozilla/activity-stream/blob/master/bin/render-activity-stream-html.js
> -#define bn_BD bn-BD
> +# These package lines below are generated by https://github.com/mozilla/activity-stream/blob/master/bin/render-activity-stream-html.js
> +  content/prerendered/en-US/activity-stream-initial-state.js (./prerendered/en-US/activity-stream-initial-state.js)

Is it not possible to add everything with prerendered/* or something?

Comment 5

[Ed Lee :Mardak]

Comment on attachment 8923637 [details]
Bug 1412930 - Package localized prerendered files.

https://reviewboard.mozilla.org/r/194760/#review200514

> Since this is re-calculated every time .defaultURL is referenced I wonder if caching the result of getLocalizedPath would be worth doing (especially since locale won't change)? It's possible the list of locales might also grow

We could… I wasn't sure if we wanted to add a pref / event listener for locale change vs just getting the current locale.

> Is it not possible to add everything with prerendered/* or something?

There's the debug files (and state when strings are moved out) that exist directly inside the prerendered directory, and the debug ones are conditionally packaged already, so a `prerendered/*` would either unnecessarily package or re-package them. Unless we want to move those out of the prerendered directory. Or maybe we create another sub directory, e.g.,

prerendered/static/{debug}
prerendered/locales/{en-US,ar,…}

?

Comment 6

[Ed Lee :Mardak]

Comment on attachment 8923637 [details]
Bug 1412930 - Package localized prerendered files.

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

Comment 7

[Ed Lee :Mardak]

Comment on attachment 8923637 [details]
Bug 1412930 - Package localized prerendered files.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/194760/diff/4-5/

Comment 8

[Ed Lee :Mardak]

Comment on attachment 8923637 [details]
Bug 1412930 - Package localized prerendered files.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/194760/diff/5-6/

Comment 9

[Ed Lee :Mardak]

Comment on attachment 8923637 [details]
Bug 1412930 - Package localized prerendered files.

https://reviewboard.mozilla.org/r/194760/#review200514

> We could… I wasn't sure if we wanted to add a pref / event listener for locale change vs just getting the current locale.

Cached via `this._activityStreamPath`

> There's the debug files (and state when strings are moved out) that exist directly inside the prerendered directory, and the debug ones are conditionally packaged already, so a `prerendered/*` would either unnecessarily package or re-package them. Unless we want to move those out of the prerendered directory. Or maybe we create another sub directory, e.g.,
> 
> prerendered/static/{debug}
> prerendered/locales/{en-US,ar,…}
> 
> ?

Packaging everything at `prerendered/*` for now meaning the debug files will be doubly-packaged at `prerendered/static` and `prerendered/en-US` for now, but I'll fix that when we move/delete various files as part of https://github.com/mozilla/activity-stream/pull/3755 to avoid CLOBBERing twice

Comment 10

[Ed Lee :Mardak]

Eh nevermind. `./mach package` doesn't allow the duplicate the debug files being in both static and en-US. Hrmmm... Do you think it'll be okay to temporarily break -debug versions by not packaging them in static/, but will get fixed as part of the next export that contains the CLOBBER?

Comment 11

[Ed Lee :Mardak]

Comment on attachment 8923637 [details]
Bug 1412930 - Package localized prerendered files.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/194760/diff/6-7/

Comment 12

[Kate Hudson :k88hudson]

Comment on attachment 8923637 [details]
Bug 1412930 - Package localized prerendered files.

https://reviewboard.mozilla.org/r/194760/#review201068

::: browser/extensions/activity-stream/prerendered/en-US/activity-stream-prerendered-debug.html:1
(Diff revision 7)
>  <!doctype html>

Isn't this supposed to be in prerendered/static not prerendered/en-US?

Comment 13

[Kate Hudson :k88hudson]

Comment on attachment 8923637 [details]
Bug 1412930 - Package localized prerendered files.

https://reviewboard.mozilla.org/r/194760/#review201068

> Isn't this supposed to be in prerendered/static not prerendered/en-US?

Also, I can't seem to find activity-stream-debug.html either, is it missing?

Comment 14

[Ed Lee :Mardak]

(In reply to Kate Hudson :k88hudson from comment #12)
> browser/extensions/activity-stream/prerendered/en-US/activity-stream-
> prerendered-debug.html:1
> Isn't this supposed to be in prerendered/static not prerendered/en-US?
Both debug files are in en-US for now to avoid a CLOBBER for this patch. And with the jar.mn lines commented out for the debug files, they won't work either. The next export in bug 1413550 can probably happen today that will fix things.

Comment 15

[Kate Hudson :k88hudson]

Comment on attachment 8923637 [details]
Bug 1412930 - Package localized prerendered files.

https://reviewboard.mozilla.org/r/194760/#review201454

Awesome, thanks!

Comment 16

[Mozilla Autoland]

We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 6f2531fcfa81 -d f62bd9c2b794: rebasing 431976:6f2531fcfa81 "Bug 1412930 - Package localized prerendered files. r=k88hudson" (tip)
merging browser/base/content/test/static/browser_all_files_referenced.js
merging browser/components/newtab/tests/browser/browser.ini
warning: conflicts while merging browser/components/newtab/tests/browser/browser.ini! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)

Comment 17

[Ed Lee :Mardak]

Comment on attachment 8923637 [details]
Bug 1412930 - Package localized prerendered files.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/194760/diff/7-8/

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f7af48305978
Package localized prerendered files. r=k88hudson

Comment 19

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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

Comment 20

[Florian Quèze [:florian]]

What's the plan to remove the very broad whitelist entry at https://searchfox.org/mozilla-central/rev/f6f1731b1b7fec332f86b55fa40e2c9ae67ac39b/browser/base/content/test/static/browser_all_files_referenced.js#31 and stop shipping files in locales the user will never see?

Comment 21

[Ed Lee :Mardak]

The files are referenceable dynamically via aboutNewTabService for all users for all locales, but generally, most users won't be using the files that aren't of their default locale. Bug 1423703 will actually make it less likely that users can use those other files as Activity Stream would use the same locale as the Firefox UI instead of the user's requested locale, but the discussion there seems to indicate that either through Firefox UI or WebExtensions, the user will be more easily able to change the locale, which would mean a higher likelihood of these files getting referenced by more users.