Closed Bug 1197054 Opened 6 years ago Closed 6 years ago
Build changes to get rid of bookmarks
No description provided.
actually depends on patch in 964946
Attachment #8653781 - Flags: review?(nalexander)
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?
Attachment #8653781 - Flags: review?(nalexander) → review+
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.
(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?
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.
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.
with bookmarks.json.in removed.
https://hg.mozilla.org/integration/fx-team/rev/26b367ff0033d5730f4cd66930a3866eb3c89994 Bug 1197054 - Build changes to get rid of bookmarks.inc.r=nalexander
You need to log in before you can comment on or make changes to this bug.