Closed Bug 1197054 Opened 6 years ago Closed 6 years ago

Build changes to get rid of bookmarks.inc

Categories

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

defect
Not set
normal

Tracking

(firefox43 fixed)

RESOLVED FIXED
Firefox 43
Tracking Status
firefox43 --- fixed

People

(Reporter: ally, Assigned: ally)

References

Details

Attachments

(1 file)

No description provided.
Blocks: 964946, 1196543
Assignee: nobody → ally
actually depends on patch in 964946
Attachment #8653781 - Flags: review?(nalexander)
No longer blocks: 964946
Depends on: 964946
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?
Flags: needinfo?(l10n)
Flags: needinfo?(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.
Flags: needinfo?(ally)
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/mozilla-central/rev/26b367ff0033
https://hg.mozilla.org/mozilla-central/rev/223abf855cf4
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 43
Flags: needinfo?(l10n)
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.