Closed Bug 1412930 Opened 3 years ago Closed 3 years ago

Package localized prerendered files

Categories

(Firefox :: New Tab Page, defect)

57 Branch
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 58
Tracking Status
firefox57 --- wontfix
firefox58 --- fixed

People

(Reporter: Mardak, Assigned: Mardak)

References

Details

Attachments

(1 file)

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 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 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 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
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 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 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?
(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 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+
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)
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: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58
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)
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
Component: Activity Streams: Newtab → New Tab Page
You need to log in before you can comment on or make changes to this bug.