1197054 - Build changes to get rid of bookmarks.inc

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Data Providers, defect)

Comment 1

[Allison Naaktgeboren :ally]

Attachment: moveBookmarkStringsFromINCtoDtd-part2

actually depends on patch in 964946

Comment 2

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8653781 [details] [diff] [review]
moveBookmarkStringsFromINCtoDtd-part2

Review of attachment 8653781 [details] [diff] [review]:
-----------------------------------------------------------------

Wow, such code deletion, reduced l10n.  I can't think of an issue with this (folded with DTD patch), but a green try run would improve my confidence.  I imagine there will be some manual outreach to l10n communities to move bookmarks.inc localizations into android_strings.dtd?

Comment 3

[Axel Hecht [:Pike]]

We should really do both bug 964946 and this in one go, keeping them separate is just confusing for those folks that use version control.

Also, It seems that we're loosing bookmarks.json completely, but AFAICT that's still used in https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/distribution/Distribution.java#380.

Probably need to test this on a fresh profile to see the effects, if this works anything like desktop does.

Comment 4

[Nick Alexander :nalexander [he/him]]

(In reply to Axel Hecht [:Pike] from comment #3)
> We should really do both bug 964946 and this in one go, keeping them
> separate is just confusing for those folks that use version control.
> 
> Also, It seems that we're loosing bookmarks.json completely, but AFAICT
> that's still used in
> https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/
> distribution/Distribution.java#380.
> 
> Probably need to test this on a fresh profile to see the effects, if this
> works anything like desktop does.

Axel, thanks for your careful consideration.  I was not aware of this bookmarks.json, but looking at https://dxr.mozilla.org/mozilla-central/search?tree=mozilla-central&q=bookmarks.json&redirect=true it appears that the bookmarks.json in question is /only/ used for distributions, and that it would remain.  (Unless ally removes it in Bug 964946.)  That is, Desktop Places and the tests for Places use it, but that code is not relevant to mobile.  Do you agree, Axel?  ally?

Comment 5

[Allison Naaktgeboren :ally]

I was aware of bookmarks.json, which is used for bookmarks for distribution builds. To the best of my knowledge that is supposed to remain as-is. I defer to nalexander as the resident expert on the android build system. 

https://bugzilla.mozilla.org/show_bug.cgi?id=964946#c6 provides the rationale for keeping the two patches split. I'm not overly committed to it.

Comment 6

[Richard Newman [:rnewman]]

The reference to bookmarks.json in Distribution is about the file included by distros, not as part of the build:

https://wiki.mozilla.org/Mobile/Distribution_Files#Bookmarks

That's not related to:

https://dxr.mozilla.org/mozilla-central/source/mobile/locales/generic/profile/bookmarks.json.in

which this patch removes from the build step. That file should probably be removed here, also. I don't know why it still exists.

Comment 7

[Allison Naaktgeboren :ally]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=cf2ce2991b97

Comment 8

[Allison Naaktgeboren :ally]

with bookmarks.json.in removed.

Comment 9

[Allison Naaktgeboren :ally]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9fbb444d7320

Comment 10

[Allison Naaktgeboren :ally]

https://hg.mozilla.org/integration/fx-team/rev/26b367ff0033d5730f4cd66930a3866eb3c89994
Bug 1197054 - Build changes to get rid of bookmarks.inc.r=nalexander

Comment 11

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/223abf855cf4

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/26b367ff0033
https://hg.mozilla.org/mozilla-central/rev/223abf855cf4