Package localized prerendered files

RESOLVED FIXED in Firefox 58

Status

()

defect
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: Mardak, Assigned: Mardak)

Tracking

57 Branch
Firefox 58
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox57 wontfix, firefox58 fixed)

Details

Attachments

(1 attachment)

Assignee

Description

2 years ago
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 4

2 years ago
mozreview-review
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?
Assignee

Comment 5

2 years ago
mozreview-review-reply
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee

Comment 9

2 years ago
mozreview-review-reply
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
Assignee

Comment 10

2 years ago
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 hidden (mozreview-request)

Comment 12

2 years ago
mozreview-review
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

2 years ago
mozreview-review-reply
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?
Assignee

Comment 14

2 years ago
(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

2 years ago
mozreview-review
Comment on attachment 8923637 [details]
Bug 1412930 - Package localized prerendered files.

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

Awesome, thanks!
Attachment #8923637 - Flags: review?(khudson) → review+

Comment 16

2 years ago
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 hidden (mozreview-request)

Comment 18

2 years ago
Pushed by edilee@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/f7af48305978
Package localized prerendered files. r=k88hudson
https://hg.mozilla.org/mozilla-central/rev/f7af48305978
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58
Assignee

Updated

2 years ago
Blocks: 1419601
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?
Flags: needinfo?(edilee)
Assignee

Comment 21

2 years ago
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.
Flags: needinfo?(edilee)
See Also: → 1423703
You need to log in before you can comment on or make changes to this bug.