Closed Bug 836450 Opened 7 years ago Closed 7 years ago

Add default bookmark support for distributions

Categories

(Firefox for Android :: General, defect)

ARM
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 21

People

(Reporter: Margaret, Assigned: Margaret)

References

(Blocks 1 open bug)

Details

Attachments

(3 files, 3 obsolete files)

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
Attached patch WIP (obsolete) — Splinter Review
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)
Attachment #709269 - Flags: feedback?(mark.finkle)
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.
Attachment #709269 - Flags: feedback?(mark.finkle) → feedback+
(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.
Attached patch patch (obsolete) — Splinter Review
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.
Attachment #709269 - Attachment is obsolete: true
Attachment #711574 - Flags: review?(wjohnston)
Attachment #711574 - Flags: review?(mark.finkle)
Attached file test bookmarks.json
Put this at /objdir/dist/bin/distribution/bookmarks.json then package/install to test.
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.
Attachment #711574 - Flags: review?(mark.finkle) → review+
(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 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
}
Attachment #711574 - Flags: review?(wjohnston) → review-
(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.
Attached patch patch v2 (obsolete) — Splinter Review
This patch addresses wesj's comments.
Attachment #711574 - Attachment is obsolete: true
Attachment #712644 - Flags: review?(wjohnston)
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()
Attachment #712644 - Flags: review?(wjohnston) → review+
Blocks: 841255
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
Looks like the test failure culprit was a missing method on ContextProviderMockContext.
Attachment #714008 - Flags: review?(gbrown)
Attached patch patch v3Splinter Review
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
Attachment #712644 - Attachment is obsolete: true
Attachment #714011 - Flags: review?(mark.finkle)
Attachment #714011 - Flags: review?(mark.finkle) → review+
Comment on attachment 714008 [details] [diff] [review]
Add getPackageName method to ContextProviderMockContext

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

Good find - thanks!
Attachment #714008 - Flags: review?(gbrown) → review+
https://hg.mozilla.org/mozilla-central/rev/3be58b112a33
https://hg.mozilla.org/mozilla-central/rev/458969f6b369
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 21
No longer depends on: 849211
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.
(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.
(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?
(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.
You need to log in before you can comment on or make changes to this bug.