836450 - Add default bookmark support for distributions

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[:Margaret Leibovic]

Spinning this off of bug 834681.

* Default bookmarks
  * Title, URL and Favicon
  * Maybe add support for pre-pinned bookmarks
  * Someday we need to add support for a pre-thumbnail, but for now that is a separate bug

Comment 1

[:Margaret Leibovic]

Attachment: WIP

I found that Distribution.init() runs before createDefaultBookmarks, but the pref was still STATE_UNKNOWN, so there's definitely a race going on in here waiting for copyFiles() to finish. I need to figure out how to test the path where the pref is STATE_SET, since I never ended up in there on my test runs. But this was definitely working for the STATE_UNKNOWN case.

I decided to make bookmarks.json a JSON array. The test file I was using was:

[
  {
    "title": "Queso",
    "url": "http://queso.com/"
  },
  {
    "title": "Mozilla",
    "url": "http://mozilla.org"
  },
  {
    "title": "Missing URL"
  },
  {
    "url": "http://test.com"
  }
]

(The last two don't work, but we log the error as expected)

Comment 2

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 709269 [details] [diff] [review]
WIP

* Another TODO is handling l10n (different locales)
* You might be able to test STATE_SET by doing a first-run (which will use STATE_UNKNOWN) then using ANdroid "App managment" to "Clear Data" for the app. That should remove the profile, but I don't know if it removes the sharedPrefs and/or the distribution/ files you extracted.

Comment 3

[:Margaret Leibovic]

(In reply to Mark Finkle (:mfinkle) from comment #2)

> * You might be able to test STATE_SET by doing a first-run (which will use
> STATE_UNKNOWN) then using ANdroid "App managment" to "Clear Data" for the
> app. That should remove the profile, but I don't know if it removes the
> sharedPrefs and/or the distribution/ files you extracted.

Yeah, this also removes the sharedPrefs, so that puts us back at STATE_UNKNOWN. Maybe I can just hard-code something to test.

It would also be nice to write some automated tests for this. Maybe wesj has some ideas based on what he did to test the current default bookmarks stuff.

Comment 4

[:Margaret Leibovic]

Attachment: patch

Also asking wesj to take a look at this, since it moves around some of the default bookmarks code he wrote.

I wanted to reuse as much of the current code as possible, so I factored out a common createBookmark method that takes the various bookmark bits, including a Bitmap to set the favicon. I also did a bit of cleaning up in here, like getting rid of unused parameters and fixing some indentation. Hopefully nothing is too out of scope to land as part of this bug.

Comment 5

[:Margaret Leibovic]

Attachment: test bookmarks.json

Put this at /objdir/dist/bin/distribution/bookmarks.json then package/install to test.

Comment 6

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 711574 [details] [diff] [review]
patch

Looks good to me. I assume you tested that the Firefox bookmarks are created OK in a fresh profile? Even without a distribution bookmarks file.

Comment 7

[:Margaret Leibovic]

(In reply to Mark Finkle (:mfinkle) from comment #6)

> Looks good to me. I assume you tested that the Firefox bookmarks are created
> OK in a fresh profile? Even without a distribution bookmarks file.

Yes.

Comment 8

[Wesley Johnston (:wesj)]

Comment on attachment 711574 [details] [diff] [review]
patch

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

Good cleanup of my code! Maybe I'm going a bit overboard with the locales stuff, but wanted to know if you'd looked at it.

::: mobile/android/base/db/BrowserProvider.java.in
@@ +998,5 @@
> +        private JSONArray inputStreamToJSONArray(InputStream inputStream) throws IOException, JSONException {
> +            BufferedReader reader = new BufferedReader(new InputStreamReader(inputStream));
> +            StringBuilder stringBuilder = new StringBuilder();
> +            String s;
> +            while ((s = reader.readLine()) != null)

I see my code didn't do it at all, but I think we use single line braces in java.

@@ +1018,5 @@
> +                    File dataDir = new File(mContext.getApplicationInfo().dataDir);
> +                    File file = new File(dataDir, "distribution/bookmarks.json");
> +                    InputStream inputStream = new FileInputStream(file);
> +                    JSONArray bookmarks = inputStreamToJSONArray(inputStream);
> +                    inputStream.close();

I think to do this properly, we should probably do this close in a finally block.

@@ +1050,5 @@
> +            if (bookmarks == null)
> +                return 0;
> +
> +            Locale defaultLocale = Locale.getDefault();
> +            String locale = defaultLocale.getLanguage() + "-" + defaultLocale.getCountry();

I don't love this. Locale.toString() should return language_country_variant, so we should just need to replace "_" with "-" to match our syntax.

@@ +1060,5 @@
> +
> +                    // Look for localized strings, but use the default if they don't exist
> +                    String title;
> +                    try {
> +                        title = bookmark.getString("title." + locale);

This won't correctly fall back either. i.e. if you're using EN_GB and this locale has EN set, we won't pick it up. Maybe we can pull this bit out into something like

private String getLocalizedString(JSONObject, String, Locale) {
  // first look for the full locale
  // if this has a variant, try again without it
  // if this has a country code, try again without it
  // give up and use the default
}

Comment 9

[:Margaret Leibovic]

(In reply to Wesley Johnston (:wesj) from comment #8)

> @@ +1018,5 @@
> > +                    File dataDir = new File(mContext.getApplicationInfo().dataDir);
> > +                    File file = new File(dataDir, "distribution/bookmarks.json");
> > +                    InputStream inputStream = new FileInputStream(file);
> > +                    JSONArray bookmarks = inputStreamToJSONArray(inputStream);
> > +                    inputStream.close();
> 
> I think to do this properly, we should probably do this close in a finally
> block.

Hm, I did basically the same thing in Distribution.java, so maybe we should take a look at that to see if there are things that need fixing in there.

> @@ +1050,5 @@
> > +            if (bookmarks == null)
> > +                return 0;
> > +
> > +            Locale defaultLocale = Locale.getDefault();
> > +            String locale = defaultLocale.getLanguage() + "-" + defaultLocale.getCountry();
> 
> I don't love this. Locale.toString() should return language_country_variant,
> so we should just need to replace "_" with "-" to match our syntax.

I considered this but thought it was safer to be explicit about constructing the string because Locale.toString() will also add a variant code if it exists, and we don't handle that:
http://developer.android.com/reference/java/util/Locale.html#toString%28%29

However, I realize with your comment below that this won't work well if we try to handle language without country code. Maybe I'll just find a way to explicitly ignore the variant. And thinking about it more, this is ugly. I'll fix it :)

> @@ +1060,5 @@
> > +
> > +                    // Look for localized strings, but use the default if they don't exist
> > +                    String title;
> > +                    try {
> > +                        title = bookmark.getString("title." + locale);
> 
> This won't correctly fall back either. i.e. if you're using EN_GB and this
> locale has EN set, we won't pick it up. Maybe we can pull this bit out into
> something like
> 
> private String getLocalizedString(JSONObject, String, Locale) {
>   // first look for the full locale
>   // if this has a variant, try again without it
>   // if this has a country code, try again without it
>   // give up and use the default
> }

I agree this is smarter. In my defense, the desktop code doesn't handle this either :) Also, we could update the distribution prefs code to do this as well, since my prefs patch didn't handle this case.

Comment 10

[:Margaret Leibovic]

Attachment: patch v2

This patch addresses wesj's comments.

Comment 11

[Wesley Johnston (:wesj)]

Comment on attachment 712644 [details] [diff] [review]
patch v2

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

Seems good

::: mobile/android/base/db/BrowserProvider.java.in
@@ +1058,5 @@
> +                return bookmark.getString(fullLocale);
> +            }
> +
> +            // Try without a variant
> +            if (locale.getVariant() != "") {

TextUtils.isEmpty()

Comment 12

[:Margaret Leibovic]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4f22708024f9

Comment 13

[Ryan VanderMeulen [:RyanVM]]

Backed out for robocop failures.
https://hg.mozilla.org/integration/mozilla-inbound/rev/b9c5b66bb94f

https://tbpl.mozilla.org/php/getParsedLog.php?id=19722363&tree=Mozilla-Inbound

0 INFO SimpleTest START
1 INFO TEST-START | testMigration
2 INFO TEST-PASS | testMigration | places.sqlite.zip found in assets: README images places.sqlite.zip sounds testcheck2-motionevents webkit  - true should equal true
3 INFO TEST-PASS | testMigration | Filename match: places.sqlite - places.sqlite should equal places.sqlite
4 INFO TEST-PASS | testMigration | OK old file exists: /mnt/sdcard/tests/profile/places.sqlite - true should equal true
5 INFO TEST-UNEXPECTED-FAIL | testMigration | Exception clearing history:java.lang.reflect.InvocationTargetException
	at org.mozilla.gecko.db.BrowserDB.clearHistory(BrowserDB.java:181)
	at java.lang.reflect.Method.invokeNative(Native Method)
	at java.lang.reflect.Method.invoke(Method.java:521)
	at org.mozilla.fennec.tests.testMigration.clearHistory(testMigration.java:185)
	at org.mozilla.fennec.tests.testMigration.testMigration(testMigration.java:150)
	at java.lang.reflect.Method.invokeNative(Native Method)
	at java.lang.reflect.Method.invoke(Method.java:521)
	at android.test.InstrumentationTestCase.runMethod(InstrumentationTestCase.java:204)
	at android.test.InstrumentationTestCase.runTest(InstrumentationTestCase.java:194)
	at android.test.ActivityInstrumentationTestCase2.runTest(ActivityInstrumentationTestCase2.java:186)
	at org.mozilla.fennec.tests.BaseTest.runTest(BaseTest.java:130)
	at junit.framework.TestCase.runBare(TestCase.java:127)
	at junit.framework.TestResult$1.protect(TestResult.java:106)
	at junit.framework.TestResult.runProtected(TestResult.java:124)
	at junit.framework.TestResult.run(TestResult.java:109)
	at junit.framework.TestCase.run(TestCase.java:118)
	at android.test.AndroidTestRunner.runTest(AndroidTestRunner.java:169)
	at android.test.AndroidTestRunner.runTest(AndroidTestRunner.java:154)
	at android.test.InstrumentationTestRunner.onStart(InstrumentationTestRunner.java:520)
	at android.app.Instrumentation$InstrumentationThread.run(Instrumentation.java:1447)
Caused by: java.lang.UnsupportedOperationException
	at android.test.mock.MockContext.getPackageName(MockContext.java:101)
	at android.content.ContextWrapper.getPackageName(ContextWrapper.java:120)
	at android.content.ContextWrapper.getPackageName(ContextWrapper.java:120)
	at org.mozilla.fennec.db.BrowserProvider$DatabaseHelper.getDistributionBookmarks(BrowserProvider.java:1010)
	at org.mozilla.fennec.db.BrowserProvider$DatabaseHelper.createDistributionBookmarks(BrowserProvider.java:1078)
	at org.mozilla.fennec.db.BrowserProvider$DatabaseHelper.onCreate(BrowserProvider.java:994)
	at android.database.sqlite.SQLiteOpenHelper.getWritableDatabase(SQLiteOpenHelper.java:106)
	at org.mozilla.gecko.db.DBUtils.ensureDatabaseIsNotLocked(DBUtils.java:76)
	at org.mozilla.fennec.db.BrowserProvider.getDatabaseHelperForProfile(BrowserProvider.java:1872)
	at org.mozilla.fennec.db.BrowserProvider.getWritableDatabase(BrowserProvider.java:1917)
	at org.mozilla.fennec.db.BrowserProvider.delete(BrowserProvider.java:2110)
	at org.mozilla.fennec.tests.ContentProviderTest$DelegatingTestContentProvider.delete(ContentProviderTest.java:126)
	at android.content.ContentProvider$Transport.delete(ContentProvider.java:198)
	at android.content.ContentResolver.delete(ContentResolver.java:675)
	at org.mozilla.gecko.db.LocalBrowserDB.clearHistory(LocalBrowserDB.java:349)
	... 20 more
 - got true, expected false
Exception caught during test!
junit.framework.AssertionFailedError: 5 INFO TEST-UNEXPECTED-FAIL | testMigration | Exception clearing history:java.lang.reflect.InvocationTargetException
	at org.mozilla.gecko.db.BrowserDB.clearHistory(BrowserDB.java:181)
	at java.lang.reflect.Method.invokeNative(Native Method)
	at java.lang.reflect.Method.invoke(Method.java:521)
	at org.mozilla.fennec.tests.testMigration.clearHistory(testMigration.java:185)
	at org.mozilla.fennec.tests.testMigration.testMigration(testMigration.java:150)
	at java.lang.reflect.Method.invokeNative(Native Method)
	at java.lang.reflect.Method.invoke(Method.java:521)
	at android.test.InstrumentationTestCase.runMethod(InstrumentationTestCase.java:204)
	at android.test.InstrumentationTestCase.runTest(InstrumentationTestCase.java:194)
	at android.test.ActivityInstrumentationTestCase2.runTest(ActivityInstrumentationTestCase2.java:186)
	at org.mozilla.fennec.tests.BaseTest.runTest(BaseTest.java:130)
	at junit.framework.TestCase.runBare(TestCase.java:127)
	at junit.framework.TestResult$1.protect(TestResult.java:106)
	at junit.framework.TestResult.runProtected(TestResult.java:124)
	at junit.framework.TestResult.run(TestResult.java:109)
	at junit.framework.TestCase.run(TestCase.java:118)
	at android.test.AndroidTestRunner.runTest(AndroidTestRunner.java:169)
	at android.test.AndroidTestRunner.runTest(AndroidTestRunner.java:154)
	at android.test.InstrumentationTestRunner.onStart(InstrumentationTestRunner.java:520)
	at android.app.Instrumentation$InstrumentationThread.run(Instrumentation.java:1447)
Caused by: java.lang.UnsupportedOperationException
	at android.test.mock.MockContext.getPackageName(MockContext.java:101)
	at android.content.ContextWrapper.getPackageName(ContextWrapper.java:120)
	at android.content.ContextWrapper.getPackageName(ContextWrapper.java:120)
	at org.mozilla.fennec.db.BrowserProvider$DatabaseHelper.getDistributionBookmarks(BrowserProvider.java:1010)
	at org.mozilla.fennec.db.BrowserProvider$DatabaseHelper.createDistributionBookmarks(BrowserProvider.java:1078)
	at org.mozilla.fennec.db.BrowserProvider$DatabaseHelper.onCreate(BrowserProvider.java:994)
	at android.database.sqlite.SQLiteOpenHelper.getWritableDatabase(SQLiteOpenHelper.java:106)
	at org.mozilla.gecko.db.DBUtils.ensureDatabaseIsNotLocked(DBUtils.java:76)
	at org.mozilla.fennec.db.BrowserProvider.getDatabaseHelperForProfile(BrowserProvider.java:1872)
	at org.mozilla.fennec.db.BrowserProvider.getWritableDatabase(BrowserProvider.java:1917)
	at org.mozilla.fennec.db.BrowserProvider.delete(BrowserProvider.java:2110)
	at org.mozilla.fennec.tests.ContentProviderTest$DelegatingTestContentProvider.delete(ContentProviderTest.java:126)
	at android.content.ContentProvider$Transport.delete(ContentProvider.java:198)
	at android.content.ContentResolver.delete(ContentResolver.java:675)
	at org.mozilla.gecko.db.LocalBrowserDB.clearHistory(LocalBrowserDB.java:349)
	... 20 more
 - got true, expected false
	at junit.framework.Assert.fail(Assert.java:47)
	at org.mozilla.fennec.FennecMochitestAssert._logMochitestResult(FennecMochitestAssert.java:107)
	at org.mozilla.fennec.FennecMochitestAssert.ok(FennecMochitestAssert.java:136)
	at org.mozilla.fennec.FennecMochitestAssert.is(FennecMochitestAssert.java:142)
	at org.mozilla.fennec.tests.testMigration.clearHistory(testMigration.java:188)
	at org.mozilla.fennec.tests.testMigration.testMigration(testMigration.java:150)
	at java.lang.reflect.Method.invokeNative(Native Method)
	at java.lang.reflect.Method.invoke(Method.java:521)
	at android.test.InstrumentationTestCase.runMethod(InstrumentationTestCase.java:204)
	at android.test.InstrumentationTestCase.runTest(InstrumentationTestCase.java:194)
	at android.test.ActivityInstrumentationTestCase2.runTest(ActivityInstrumentationTestCase2.java:186)
	at org.mozilla.fennec.tests.BaseTest.runTest(BaseTest.java:130)
	at junit.framework.TestCase.runBare(TestCase.java:127)
	at junit.framework.TestResult$1.protect(TestResult.java:106)
	at junit.framework.TestResult.runProtected(TestResult.java:124)
	at junit.framework.TestResult.run(TestResult.java:109)
	at junit.framework.TestCase.run(TestCase.java:118)
	at android.test.AndroidTestRunner.runTest(AndroidTestRunner.java:169)
	at android.test.AndroidTestRunner.runTest(AndroidTestRunner.java:154)
	at android.test.InstrumentationTestRunner.onStart(InstrumentationTestRunner.java:520)
	at android.app.Instrumentation$InstrumentationThread.run(Instrumentation.java:1447)
6 INFO TEST-UNEXPECTED-FAIL | testMigration | Exception caught - junit.framework.AssertionFailedError: 5 INFO TEST-UNEXPECTED-FAIL | testMigration | Exception clearing history:java.lang.reflect.InvocationTargetException
	at org.mozilla.gecko.db.BrowserDB.clearHistory(BrowserDB.java:181)
	at java.lang.reflect.Method.invokeNative(Native Method)
	at java.lang.reflect.Method.invoke(Method.java:521)
	at org.mozilla.fennec.tests.testMigration.clearHistory(testMigration.java:185)
	at org.mozilla.fennec.tests.testMigration.testMigration(testMigration.java:150)
	at java.lang.reflect.Method.invokeNative(Native Method)
	at java.lang.reflect.Method.invoke(Method.java:521)
	at android.test.InstrumentationTestCase.runMethod(InstrumentationTestCase.java:204)
	at android.test.InstrumentationTestCase.runTest(InstrumentationTestCase.java:194)
	at android.test.ActivityInstrumentationTestCase2.runTest(ActivityInstrumentationTestCase2.java:186)
	at org.mozilla.fennec.tests.BaseTest.runTest(BaseTest.java:130)
	at junit.framework.TestCase.runBare(TestCase.java:127)
	at junit.framework.TestResult$1.protect(TestResult.java:106)
	at junit.framework.TestResult.runProtected(TestResult.java:124)
	at junit.framework.TestResult.run(TestResult.java:109)
	at junit.framework.TestCase.run(TestCase.java:118)
	at android.test.AndroidTestRunner.runTest(AndroidTestRunner.java:169)
	at android.test.AndroidTestRunner.runTest(AndroidTestRunner.java:154)
	at android.test.InstrumentationTestRunner.onStart(InstrumentationTestRunner.java:520)
	at android.app.Instrumentation$InstrumentationThread.run(Instrumentation.java:1447)
Caused by: java.lang.UnsupportedOperationException
	at android.test.mock.MockContext.getPackageName(MockContext.java:101)
	at android.content.ContextWrapper.getPackageName(ContextWrapper.java:120)
	at android.content.ContextWrapper.getPackageName(ContextWrapper.java:120)
	at org.mozilla.fennec.db.BrowserProvider$DatabaseHelper.getDistributionBookmarks(BrowserProvider.java:1010)
	at org.mozilla.fennec.db.BrowserProvider$DatabaseHelper.createDistributionBookmarks(BrowserProvider.java:1078)
	at org.mozilla.fennec.db.BrowserProvider$DatabaseHelper.onCreate(BrowserProvider.java:994)
	at android.database.sqlite.SQLiteOpenHelper.getWritableDatabase(SQLiteOpenHelper.java:106)
	at org.mozilla.gecko.db.DBUtils.ensureDatabaseIsNotLocked(DBUtils.java:76)
	at org.mozilla.fennec.db.BrowserProvider.getDatabaseHelperForProfile(BrowserProvider.java:1872)
	at org.mozilla.fennec.db.BrowserProvider.getWritableDatabase(BrowserProvider.java:1917)
	at org.mozilla.fennec.db.BrowserProvider.delete(BrowserProvider.java:2110)
	at org.mozilla.fennec.tests.ContentProviderTest$DelegatingTestContentProvider.delete(ContentProviderTest.java:126)
	at android.content.ContentProvider$Transport.delete(ContentProvider.java:198)
	at android.content.ContentResolver.delete(ContentResolver.java:675)
	at org.mozilla.gecko.db.LocalBrowserDB.clearHistory(LocalBrowserDB.java:349)
	... 20 more
 - got true, expected false
tearDown deleted /mnt/sdcard/tests/profile/places.sqlite
7 INFO TEST-END | testMigration | finished in 678ms
8 INFO TEST-START | Shutdown
9 INFO Passed: 3
10 INFO Failed: 2
11 INFO Todo: 0
12 INFO SimpleTest FINISHED

Comment 14

[Ryan VanderMeulen [:RyanVM]]

This was also causing troboprovider failures.

https://tbpl.mozilla.org/php/getParsedLog.php?id=19723972&tree=Mozilla-Inbound

Comment 15

[:Margaret Leibovic]

Attachment: Add getPackageName method to ContextProviderMockContext

Looks like the test failure culprit was a missing method on ContextProviderMockContext.

Comment 16

[:Margaret Leibovic]

Attachment: patch v3

While investigating the test failure, I decided that the distribution-specific logic should really go in Distribution.java.

I also updated the Distribution methods to take a Context rather than an Activity, because we only have a Context to work with in BrowserProvider.

With these two patches, testMigration started passing for me locally, so I think this should fix the failures. I pushed to try, let's see what happens:
https://tbpl.mozilla.org/?tree=Try&rev=81a8138f740d

Comment 17

[Geoff Brown [:gbrown]]

Comment on attachment 714008 [details] [diff] [review]
Add getPackageName method to ContextProviderMockContext

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

Good find - thanks!

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/3be58b112a33
https://hg.mozilla.org/mozilla-central/rev/458969f6b369

Comment 19

[Peipei Cheng (needinfo if you need my action)]

Tested build: Fennec V21.0 release, Fennec V21 Beta 8

Reproduced steps:
1. Install Fennec V21
2. Create "bookmarks.json" under /system/org.mozilla.firefox/distribution (for beta version it is /system/org.mozilla.firefox_beta/distribution) 
3. start Fennec
[issue] bookmarks are not added 

If we put distribution/bookmarks.json under \data\data\org.mozilla.firefox after installing Fennec V21, and then launch Fennec, bookmarks can be loaded.

Comment 20

[:Margaret Leibovic]

(In reply to pcheng from comment #19)
> Tested build: Fennec V21.0 release, Fennec V21 Beta 8
> 
> Reproduced steps:
> 1. Install Fennec V21
> 2. Create "bookmarks.json" under /system/org.mozilla.firefox/distribution
> (for beta version it is /system/org.mozilla.firefox_beta/distribution) 
> 3. start Fennec
> [issue] bookmarks are not added 

Currently bookmarks are only supported when the distribution in included in the APK. We should file a follow-up to bug 843821 to add that support.

> If we put distribution/bookmarks.json under \data\data\org.mozilla.firefox
> after installing Fennec V21, and then launch Fennec, bookmarks can be loaded.

Yes, this is where they would be copied to if they were included in the APK.

Comment 21

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to :Margaret Leibovic from comment #20)
> (In reply to pcheng from comment #19)
> > Tested build: Fennec V21.0 release, Fennec V21 Beta 8
> > 
> > Reproduced steps:
> > 1. Install Fennec V21
> > 2. Create "bookmarks.json" under /system/org.mozilla.firefox/distribution
> > (for beta version it is /system/org.mozilla.firefox_beta/distribution) 
> > 3. start Fennec
> > [issue] bookmarks are not added 
> 
> Currently bookmarks are only supported when the distribution in included in
> the APK. We should file a follow-up to bug 843821 to add that support.

I think we need to have parity for both styles of distributions. Can you file a bug for it?

Comment 22

[:Margaret Leibovic]

(In reply to Mark Finkle (:mfinkle) from comment #21)
> (In reply to :Margaret Leibovic from comment #20)
> > (In reply to pcheng from comment #19)
> > > Tested build: Fennec V21.0 release, Fennec V21 Beta 8
> > > 
> > > Reproduced steps:
> > > 1. Install Fennec V21
> > > 2. Create "bookmarks.json" under /system/org.mozilla.firefox/distribution
> > > (for beta version it is /system/org.mozilla.firefox_beta/distribution) 
> > > 3. start Fennec
> > > [issue] bookmarks are not added 
> > 
> > Currently bookmarks are only supported when the distribution in included in
> > the APK. We should file a follow-up to bug 843821 to add that support.
> 
> I think we need to have parity for both styles of distributions. Can you
> file a bug for it?

Filed bug 872737.