1464128 - Migrate bookmarks.html.in to LOCALIZED_GENERATED_FILES

Status: RESOLVED FIXED

(Firefox Build System :: General, enhancement)

Description

[Nick Alexander :nalexander [he/him]]

Right now, bookmarks.html is locale-aware and preprocessed:

https://searchfox.org/mozilla-central/rev/dc6d85680539cb7e5fe2843d38a30a0581bfefe1/browser/locales/jar.mn#15

It all works because of special handling of BOOKMARKS_INCLUDE_DIR, see

https://searchfox.org/mozilla-central/search?q=BOOKMARKS_INCLUDE_DIR&path=

With the new LOCALIZED_GENERATED_FILES, we can avoid a lot of this cruft, bringing more of the l10n ecosystem into moz.build files.

However, I think we have another bridge to cross: this will be the first l10n-aware L_G_F that is installed into a locale-aware location in the omnijar.  Doing that is a little tricky, I think: do we use LOCALIZED_FILES or do we try to keep the existing entry in the jar.mn, and take the content from the object directory?

Comment 1

[Nick Alexander :nalexander [he/him]]

Attachment: Bug 1464128 - Migrate bookmarks.html.in to LOCALIZED_GENERATED_FILES. .mielczarek

This one looks to be pretty straight-forward.  It irritates me that
the jar.mn entry doesn't explicitly say that the result is coming from
the object directory, like

locale/browser/bookmarks.html (!bookmarks.html)

but that's for another day.

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

Comment 2

[Kim Moir [:kmoir] ET]

triaging, assigned to nick since he attached patches

Comment 3

[(not currently active) Ted Mielczarek]

Comment on attachment 8980380 [details]
Bug 1464128 - Migrate bookmarks.html.in to LOCALIZED_GENERATED_FILES. .mielczarek

https://reviewboard.mozilla.org/r/246552/#review252694

Nice bit of cleanup removing all the places that had to copy that define around! It's nice when we can build abstractions that actually let us do the things we need to do in a sensible way. :)

::: browser/locales/generic/profile/bookmarks.html.in:6
(Diff revision 1)
>  #filter substitution
> -#include @BOOKMARKS_INCLUDE_DIR@/bookmarks.inc
> +#include @BOOKMARKS_INCLUDE_PATH@
>  #define ja_jp_mac ja-JP-mac
>  #if AB_CD == ja_jp_mac
>  #define AB_CD ja
>  #endif

You could move this weird define futzing into the generation script while you're at it, if you were so inclined.

::: browser/locales/jar.mn:15
(Diff revision 1)
>  [localization] @AB_CD@.jar:
>    browser                                          (%browser/**/*.ftl)
>  
>  @AB_CD@.jar:
>  % locale browser @AB_CD@ %locale/browser/
> -*   locale/browser/bookmarks.html                  (generic/profile/bookmarks.html.in)
> +    locale/browser/bookmarks.html                  (bookmarks.html)

It would be nice if the source of the file was clearer. Maybe just add a comment on the line above indicating that this is an output of `LOCALIZED_GENERATED_FILES`?

Comment 4

[Nick Alexander :nalexander [he/him]]

Disassembling a few builds in https://treeherder.mozilla.org/#/jobs?repo=try&revision=46a8f9b38762fb4ae75f7a4b1cfea0c6e66052c3 suggests this is working fine: I see German bookmarks.html content in the omnijar and langpacks.

Comment 5

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8980380 [details]
Bug 1464128 - Migrate bookmarks.html.in to LOCALIZED_GENERATED_FILES. .mielczarek

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

Comment 6

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8980380 [details]
Bug 1464128 - Migrate bookmarks.html.in to LOCALIZED_GENERATED_FILES. .mielczarek

https://reviewboard.mozilla.org/r/246552/#review252694

> It would be nice if the source of the file was clearer. Maybe just add a comment on the line above indicating that this is an output of `LOCALIZED_GENERATED_FILES`?

Yep, added a comment.  I started looking at Bug 1407427 as well, which might require really suppporting !objdir_paths properly, so this might even get better.

Comment 7

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8980380 [details]
Bug 1464128 - Migrate bookmarks.html.in to LOCALIZED_GENERATED_FILES. .mielczarek

https://reviewboard.mozilla.org/r/246552/#review252768

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0f0e532db5ec
Migrate bookmarks.html.in to LOCALIZED_GENERATED_FILES. r=ted.mielczarek

Comment 9

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/0f0e532db5ec