Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Add support for using local DBs for Bookmarks and History

RESOLVED FIXED

Status

()

Firefox for Android
General
P1
normal
RESOLVED FIXED
6 years ago
a year ago

People

(Reporter: mfinkle, Assigned: lucasr)

Tracking

unspecified
x86
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox11 fixed, firefox-esr10 wontfix, status1.9.2 unaffected, fennec11+)

Details

(Whiteboard: [sg:want])

Attachments

(3 attachments, 4 obsolete attachments)

Currently Fennec uses the Android system DBs for storing bookmarks and history. We'd like to add support for using a local (in Fennec profile) DBs as well. The DBs can have the same schema as the system DBs so switching between them is easy.
(Reporter)

Updated

6 years ago
Assignee: nobody → lucasr.at.mozilla
Priority: -- → P1
mfinkle: do you have a timeline for this? And do you intend to do it seamlessly through a separate content provider?

I don't have a good answer for how Sync (which isn't aware of Fennec profiles) could decide which DB to use, or how to make that behave correctly. (Which is not to say I don't think offering non-Google storage is a good idea.)
Seems like we should expose the DBs as content providers, so Sync can access and update the data even while Firefox is not running. We can also assume that we'll only have a single profile for now, but we should put some kind of "what is the current profile" mechanism into the system. I don't know exactly how that would work yet.
(Reporter)

Updated

6 years ago
Blocks: 695463

Updated

6 years ago
Whiteboard: [sg:want]
A few more thoughts around profiles and intermediation:

* Sync would presumably store some reference to the profile in its Sync account record on the device. The string name should suffice.

* During setup, we'd need a way to ask for a list of profiles, in order to decide which ones to offer in the UI. Alternatively, if Fennec invokes our setup activity, it could include the destination profile in the URI. (This would be the most likely flow, I think.)

* During sync, we'd need an interface which takes a profile as input and returns a collection of content providers to which we can sync. We'd call this on every sync.

* This information hand-off would presumably also include just enough tracking information for us to figure out when the user has switched databases, so we know to do a full sync and clean up our tracking metadata.
(Assignee)

Comment 4

6 years ago
Created attachment 577989 [details] [diff] [review]
(1/2) Abstract all bookmark/history access behing a common API
Attachment #577989 - Flags: review?(blassey.bugs)
(Assignee)

Comment 5

6 years ago
Created attachment 577990 [details] [diff] [review]
(2/2) Introduce new local bookmarks/history database
Attachment #577990 - Flags: review?(blassey.bugs)
(Assignee)

Comment 6

6 years ago
I have Fennec fully working using local DB with those 2 patches. No obvious crashes AFAIK. The only piece that is still missing is how we're going to switch a profile between Android's and Fennec's DB. My initial idea is to use a simple Android settings key per profile. Any other ideas? We don't want to rely on Gecko prefs for that because the correct DB has to be immediately usable even before Gecko is fully loaded.

A possible follow-up patch is to create default bookmarks when DB is first created. Android uses a simple array resource for that. We could do the same.
(Assignee)

Updated

6 years ago
Attachment #577990 - Flags: feedback?(rnewman)
Attachment #577990 - Flags: feedback?(mark.finkle)
Attachment #577990 - Flags: feedback?(kgupta)
(Assignee)

Updated

6 years ago
Attachment #577989 - Flags: feedback?(rnewman)
Attachment #577989 - Flags: feedback?(mark.finkle)
Attachment #577989 - Flags: feedback?(kgupta)
Comment on attachment 577989 [details] [diff] [review]
(1/2) Abstract all bookmark/history access behing a common API

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

Code-wise, this looks good to me. Now, on to the meat!


Further to initial conversation on IRC: in general, Sync wants a lastModified and a GUID column for all data sources that it needs to sync. Having now looked at some context for that pre-coffee conversation (i.e., this patch!), I need to give you a little more of a nuanced opinion :D


For the built-in Browser DB we can't add columns, so we're implementing a two-stage sync as described here: <https://wiki.mozilla.org/Services/NativeSync/Bookmarks_and_History>.


For Fennec's own hybrid favicons (which we'll want to sync eventually, and thus should plan for now) this has a slight complication. We care about when the favicon data changed (favicon modified time) and when the bookmark changed to point to a different favicon (bookmark modified time). You have a separate table for the latter, and the data for the former is jammed into the built-in Browser table. We obviously don't want to re-upload a favicon every time a new *.foo.com page points to the same ol' sequence of bytes.

(This is a very different schema to Places, which has favicons as a first-class entity: it's inverted.)

I'm not sure what proposal to make on this front. The Android design is bass-ackward. What do you think about having another table for favicons, reifying them, on which we can hang URL (the real primary key), guid, change time?


For bookmarks and history we have two choices.

On one hand, you could just behave exactly like the built-in Browser store, and we'll apply exactly the same kinds of awful workarounds that we do for them. This is arguably simpler, but has tradeoffs. We'll just use your translation interface instead of Browser.

On the other hand, you could provide a rich schema: every record having a GUID and a lastModified time. We're then free to use those to implement a better scheme, eventually allowing us to rely on ICS and Fennec's improved schema to discard our duplicate DB.

The question then becomes how to layer that on top of the built-in stuff, given that this is an abstraction layer, and it might not do to have some rows come back simply without a GUID or lastModified!


This might need some more pondering.

::: mobile/android/base/Tab.java
@@ +335,5 @@
>      private class AddBookmarkTask extends GeckoAsyncTask<Void, Void, Void> {
>          @Override
>          protected Void doInBackground(Void... unused) {
>              ContentResolver resolver = Tabs.getInstance().getContentResolver();
> +            BrowserDB.addBookmark(resolver, getTitle(), getURL());

Doesn't this schema just make ya cry?
Attachment #577989 - Flags: feedback?(rnewman) → feedback+
One more thing I forgot to mention in my comments for Part 1: of course, for this to be useful for Sync it has to be accessible without being built-in to Fennec. We need to be able to ask the system for an appropriate instance that can run queries on our behalf.

That means you need to implement a ContentProvider/ContentResolver. sriram is the source of knowledge here :)
Comment on attachment 577990 [details] [diff] [review]
(2/2) Introduce new local bookmarks/history database

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

Clearing review flag on this until we decide on a direction for Sync support.
Attachment #577990 - Flags: feedback?(rnewman)
Comment on attachment 577989 [details] [diff] [review]
(1/2) Abstract all bookmark/history access behing a common API

I assume you tested as much of the application with this patch applied as you could, looking for any breakage.
Attachment #577989 - Flags: feedback?(mark.finkle) → feedback+
(Assignee)

Comment 11

6 years ago
Created attachment 578228 [details] [diff] [review]
(1/2) Abstract all bookmark/history access behing a common API

Updated patch to also cover the new native about:home.
Attachment #577989 - Attachment is obsolete: true
Attachment #577989 - Flags: review?(blassey.bugs)
Attachment #577989 - Flags: feedback?(kgupta)
Attachment #578228 - Flags: review?(blassey.bugs)
(Assignee)

Comment 12

6 years ago
Created attachment 578229 [details] [diff] [review]
(2/2) Introduce new local bookmarks/history database

Updated patch to also cover the new native about:home.
Attachment #577990 - Attachment is obsolete: true
Attachment #577990 - Flags: review?(blassey.bugs)
Attachment #577990 - Flags: feedback?(mark.finkle)
Attachment #577990 - Flags: feedback?(kgupta)
Attachment #578229 - Flags: review?(blassey.bugs)
(Assignee)

Comment 13

6 years ago
After approval, I suggest us to land these patches with local DB always disabled. I'll then write the switch mechanism (through an Android setting key) with its respective prefs UI on a follow-up patch to allow users to enable/disable it.

It would be great to land those patches asap because they touch many parts of the app and it will be a PITA to keep rebasing them all the time.
(Reporter)

Updated

6 years ago
Duplicate of this bug: 706823
Comment on attachment 578228 [details] [diff] [review]
(1/2) Abstract all bookmark/history access behing a common API

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

::: mobile/android/base/db/AndroidBrowserDB.java
@@ +95,5 @@
> +    }
> +
> +    public Cursor getRecentHistory(ContentResolver cr, int limit) {
> +        Cursor c = cr.query(Browser.BOOKMARKS_URI,
> +                            null,

use a projection

@@ +111,5 @@
> +    }
> +
> +    public Cursor getAllBookmarks(ContentResolver cr) {
> +        Cursor c = cr.query(Browser.BOOKMARKS_URI,
> +                            null,

use a projection

@@ +125,5 @@
> +    }
> +
> +    public boolean isBookmark(ContentResolver cr, String uri) {
> +        Cursor cursor = cr.query(Browser.BOOKMARKS_URI,
> +                                 null,

use a projection

@@ +138,5 @@
> +    }
> +
> +    public void addBookmark(ContentResolver cr, String title, String uri) {
> +        Cursor cursor = cr.query(Browser.BOOKMARKS_URI,
> +                                 null,

use a projection

@@ +218,5 @@
> +            BitmapDrawable thumbnail) {
> +        Bitmap bitmap = thumbnail.getBitmap();
> +
> +        Cursor cursor = cr.query(Browser.BOOKMARKS_URI,
> +                                 null,

use a projection
Attachment #578228 - Flags: review?(blassey.bugs) → review+
(Assignee)

Comment 16

6 years ago
(In reply to Brad Lassey [:blassey] from comment #15)
> Comment on attachment 578228 [details] [diff] [review] [diff] [details] [review]
> (1/2) Abstract all bookmark/history access behing a common API
> 
> Review of attachment 578228 [details] [diff] [review] [diff] [details] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/base/db/AndroidBrowserDB.java
> @@ +95,5 @@
> > +    }
> > +
> > +    public Cursor getRecentHistory(ContentResolver cr, int limit) {
> > +        Cursor c = cr.query(Browser.BOOKMARKS_URI,
> > +                            null,
> 
> use a projection
> 
> @@ +111,5 @@
> > +    }
> > +
> > +    public Cursor getAllBookmarks(ContentResolver cr) {
> > +        Cursor c = cr.query(Browser.BOOKMARKS_URI,
> > +                            null,
> 
> use a projection
> 
> @@ +125,5 @@
> > +    }
> > +
> > +    public boolean isBookmark(ContentResolver cr, String uri) {
> > +        Cursor cursor = cr.query(Browser.BOOKMARKS_URI,
> > +                                 null,
> 
> use a projection
> 
> @@ +138,5 @@
> > +    }
> > +
> > +    public void addBookmark(ContentResolver cr, String title, String uri) {
> > +        Cursor cursor = cr.query(Browser.BOOKMARKS_URI,
> > +                                 null,
> 
> use a projection
> 
> @@ +218,5 @@
> > +            BitmapDrawable thumbnail) {
> > +        Bitmap bitmap = thumbnail.getBitmap();
> > +
> > +        Cursor cursor = cr.query(Browser.BOOKMARKS_URI,
> > +                                 null,
> 
> use a projection

Added all projections.
(Assignee)

Comment 17

6 years ago
Yesterday rnewman and I discussed what needs to be in the local DB schema from a sync perspective. Here are the changes I'll make to the schema before landing those patches:

1. Add a "GUID" column to "History" and "Bookmarks" tables
2. Add "GUID" and "Last modified" columns "Images" table
3. Add "favicon url" column to Images table.

About 3, currently we have a separate database to store favicon URLs. However, we need to have this information in the local DB for syncing. So, my plan to move the favicon URL DB to be used only when profile is using Android's DB. On local DB, the favicon URL will be available directly in the "Images" table. I can do this on a follow-up patch (bug 707119) but I want to have the favicon-url in the local DB schema from day 1 to avoid having to write DB migration just for that later.

Other necessary follow-up changes:
1. Provide a special query returning the schema version (bug 707124)
2. Create a ContentProvider to provide profile information (list of profiles, which DB a profile is using, etc) (bug 707123)

Will be sending an updated 2/2 patch with the schema changes above in the next hour or so.
(Assignee)

Comment 18

6 years ago
Created attachment 578541 [details] [diff] [review]
(1/2) Abstract all bookmark/history access behing a common API

Updated patch with changes suggested by blassey. Keeping the r+.
Attachment #578228 - Attachment is obsolete: true
Attachment #578541 - Flags: review+
(Assignee)

Comment 19

6 years ago
FYI: Filed bug 707150 as a follow-up bug to add mechanism to enable/disable local bookmarks/history DB.
(Assignee)

Comment 20

6 years ago
Created attachment 578564 [details] [diff] [review]
(2/2) Introduce new local bookmarks/history database

Updated patch with all the schema changes to make local DB "sync-ready". All follow-up bugs are now files. Just waiting for review on 2/2 now.
Attachment #578229 - Attachment is obsolete: true
Attachment #578229 - Flags: review?(blassey.bugs)
Attachment #578564 - Flags: review?(blassey.bugs)
(Assignee)

Comment 21

6 years ago
Comment on attachment 578564 [details] [diff] [review]
(2/2) Introduce new local bookmarks/history database

rnewman, I've added all the bits we agreed on yesterday. Could you have a look at the schema and let me know if there's anything missing?
Attachment #578564 - Flags: feedback?(rnewman)
(Assignee)

Comment 22

6 years ago
FYI: I pushed patch 1/2 to avoid constant rebase headaches: http://hg.mozilla.org/projects/birch/rev/839f36123bce
(Assignee)

Comment 23

6 years ago
FYI: backed out patch 1/2 due to Try failure: http://hg.mozilla.org/projects/birch/rev/6bc702e26354
(In reply to Lucas Rocha (:lucasr) from comment #17)
> 
> About 3, currently we have a separate database to store favicon URLs.
> However, we need to have this information in the local DB for syncing. So,
> my plan to move the favicon URL DB to be used only when profile is using
> Android's DB. On local DB, the favicon URL will be available directly in the
> "Images" table. I can do this on a follow-up patch (bug 707119) but I want
> to have the favicon-url in the local DB schema from day 1 to avoid having to
> write DB migration just for that later.

We also need _some_ way to get a GUID for a favicon. It doesn't matter to me where this lives, so long as we're at some point able to assemble the following:

  * GUID
  * Favicon URL
  * Timestamp favicon itself was last modified

and for each bookmark and history item, figure out (indirectly is fine) which favicon GUID applies.

If you can't do this, it's not a massive deal; we can keep a local DB to track these things. But it'd help having it all in one place.

> Will be sending an updated 2/2 patch with the schema changes above in the
> next hour or so.

Fantastic!
Comment on attachment 578564 [details] [diff] [review]
(2/2) Introduce new local bookmarks/history database

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

Ah, I see this:

  map.put(Images.GUID, Images.GUID);

so I think this all makes sense. Now let's see if I can access the CP from another app :D
Attachment #578564 - Flags: feedback?(rnewman) → feedback+
(Assignee)

Comment 26

6 years ago
(In reply to Richard Newman [:rnewman] from comment #25)
> Comment on attachment 578564 [details] [diff] [review] [diff] [details] [review]
> (2/2) Introduce new local bookmarks/history database
> 
> Review of attachment 578564 [details] [diff] [review] [diff] [details] [review]:
> -----------------------------------------------------------------
> 
> Ah, I see this:
> 
>   map.put(Images.GUID, Images.GUID);
> 
> so I think this all makes sense. Now let's see if I can access the CP from
> another app :D

I'll be landing the CP with no way to access from outside Fennec (i.e. android:exported="false"). I filed a separate bug to figure out the right/safe way to expose the CP to Sync (see bug 707636).
(Assignee)

Updated

6 years ago
Blocks: 701835
(Assignee)

Updated

6 years ago
Blocks: 698828
(Assignee)

Updated

6 years ago
Blocks: 701913
(Assignee)

Updated

6 years ago
Blocks: 707732
Comment on attachment 578564 [details] [diff] [review]
(2/2) Introduce new local bookmarks/history database

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

::: mobile/android/base/db/BrowserProvider.java
@@ +284,5 @@
> +
> +        if (dbHelper == null) {
> +            synchronized (this) {
> +                dbHelper = new DatabaseHelper(getContext(), getDatabasePath(profile));
> +                mDatabasePerProfile.put(profile, dbHelper);

So, is it possible to operate on more than one profile at a time? This shouldn't happen in normal use of the browser, but I wonder if it is a requirement for sync?

@@ +327,5 @@
> +
> +        if (profileDir == null) {
> +            Log.d(LOGTAG, "Couldn't find directory for profile: " + profile);
> +            return null;
> +        }

There is a getProfileDir() function in GeckoApp now. Use it here and improve it if need be (there are two XXX ToDo's in the impl
(Assignee)

Comment 28

6 years ago
(In reply to Brad Lassey [:blassey] from comment #27)
> Comment on attachment 578564 [details] [diff] [review] [diff] [details] [review]
> (2/2) Introduce new local bookmarks/history database
> 
> Review of attachment 578564 [details] [diff] [review] [diff] [details] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/base/db/BrowserProvider.java
> @@ +284,5 @@
> > +
> > +        if (dbHelper == null) {
> > +            synchronized (this) {
> > +                dbHelper = new DatabaseHelper(getContext(), getDatabasePath(profile));
> > +                mDatabasePerProfile.put(profile, dbHelper);
> 
> So, is it possible to operate on more than one profile at a time? This
> shouldn't happen in normal use of the browser, but I wonder if it is a
> requirement for sync?

Yep. The idea is be able to deliver commands to the profile databases at any given time. The ContentProvider will keep separate database connections for each database accordingly.

> @@ +327,5 @@
> > +
> > +        if (profileDir == null) {
> > +            Log.d(LOGTAG, "Couldn't find directory for profile: " + profile);
> > +            return null;
> > +        }
> 
> There is a getProfileDir() function in GeckoApp now. Use it here and improve
> it if need be (there are two XXX ToDo's in the impl

Ok, will have a look into that.
Lucas: could you un-bitrot these? They don't apply on current m-c.

I was all set to do the following and try throwing a sketch patch at you…

• For "deletion", update lastModified and clear other columns
• Add a "uri IS NOT NULL" clause to retrieval operations
• Add a cleanup operation a la

  DELETE FROM bookmarks WHERE uri IS NULL and lastModified < (30 days ago)

Then Sync will do the following:

• Retrieve all items, counting those without a URI as deleted
• Purge flagged items on sync completion.

I would probably also have attempted part of Bug 708149, namely adding a couple of extra columns for Sync to reduce the impedance mismatch.

Thoughts on this?
Depends on: 708151
Comment on attachment 578564 [details] [diff] [review]
(2/2) Introduce new local bookmarks/history database

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

::: mobile/android/base/db/BrowserContract.java
@@ +94,5 @@
> +        public static final String IS_FOLDER = "folder";
> +
> +        public static final String PARENT = "parent";
> +
> +        public static final String POSITION = "position";

what is a bookmark position?

::: mobile/android/base/db/BrowserProvider.java
@@ +103,5 @@
> +
> +    private HashMap<String, DatabaseHelper> mDatabasePerProfile;
> +
> +    static {
> +        final UriMatcher matcher = URI_MATCHER;

why not just use URI_MATCHER directly?

@@ +104,5 @@
> +    private HashMap<String, DatabaseHelper> mDatabasePerProfile;
> +
> +    static {
> +        final UriMatcher matcher = URI_MATCHER;
> +        final String authority = BrowserContract.AUTHORITY;

why not use BrowserContract.AUTHORITY directly?

@@ +105,5 @@
> +
> +    static {
> +        final UriMatcher matcher = URI_MATCHER;
> +        final String authority = BrowserContract.AUTHORITY;
> +        HashMap<String, String> map;

why not use the *_PROJECTION_MAPs directly?
Attachment #578564 - Flags: review?(blassey.bugs) → review+

Updated

6 years ago
Depends on: 708331

Updated

6 years ago
Depends on: 708485
(Assignee)

Comment 31

6 years ago
(In reply to Brad Lassey [:blassey] from comment #30)
> Comment on attachment 578564 [details] [diff] [review] [diff] [details] [review]
> (2/2) Introduce new local bookmarks/history database
> 
> Review of attachment 578564 [details] [diff] [review] [diff] [details] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/base/db/BrowserContract.java
> @@ +94,5 @@
> > +        public static final String IS_FOLDER = "folder";
> > +
> > +        public static final String PARENT = "parent";
> > +
> > +        public static final String POSITION = "position";
> 
> what is a bookmark position?

Sync needs it.
 
> ::: mobile/android/base/db/BrowserProvider.java
> @@ +103,5 @@
> > +
> > +    private HashMap<String, DatabaseHelper> mDatabasePerProfile;
> > +
> > +    static {
> > +        final UriMatcher matcher = URI_MATCHER;
> 
> why not just use URI_MATCHER directly?

I thought it was cleaner. No big deal. Changed it to use URI_MATCHER directly.

> @@ +104,5 @@
> > +    private HashMap<String, DatabaseHelper> mDatabasePerProfile;
> > +
> > +    static {
> > +        final UriMatcher matcher = URI_MATCHER;
> > +        final String authority = BrowserContract.AUTHORITY;
> 
> why not use BrowserContract.AUTHORITY directly?

Same reason. Fixed.
 
> @@ +105,5 @@
> > +
> > +    static {
> > +        final UriMatcher matcher = URI_MATCHER;
> > +        final String authority = BrowserContract.AUTHORITY;
> > +        HashMap<String, String> map;
> 
> why not use the *_PROJECTION_MAPs directly?

I'll keep this one as it makes the code more readable.
(Assignee)

Comment 32

6 years ago
Created attachment 580029 [details] [diff] [review]
Add API to get directory for any given profile

Needed changes to use getProfileDir() from GeckoApp.
Attachment #580029 - Flags: review?(blassey.bugs)
(Assignee)

Updated

6 years ago
Duplicate of this bug: 708610
(Assignee)

Comment 34

6 years ago
Pushed the first patch: http://hg.mozilla.org/integration/mozilla-inbound/rev/af13b9729453

Just waiting for review on the getProfileDir() patch to land the second.
Comment on attachment 580029 [details] [diff] [review]
Add API to get directory for any given profile

Looks good to me
Attachment #580029 - Flags: review?(blassey.bugs) → review+
(Assignee)

Comment 36

6 years ago
Pushed:
http://hg.mozilla.org/integration/mozilla-inbound/rev/ebc46471b041
http://hg.mozilla.org/integration/mozilla-inbound/rev/bb39bae574f8

Note that local DB is still disabled (as I proposed on comment 13). I'll enable it once we decide on how to expose this feature (Android DB vs Local DB) to users (bug 707150).
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Blocks: 708651
(Assignee)

Comment 37

6 years ago
Oops, shouldn't have resolved it just yet, sorry.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(In reply to Lucas Rocha (:lucasr) from comment #31)
> (In reply to Brad Lassey [:blassey] from comment #30)
> > Comment on attachment 578564 [details] [diff] [review]
> > (2/2) Introduce new local bookmarks/history database
> > 
> > Review of attachment 578564 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: mobile/android/base/db/BrowserContract.java
> > @@ +94,5 @@
> > > +        public static final String IS_FOLDER = "folder";
> > > +
> > > +        public static final String PARENT = "parent";
> > > +
> > > +        public static final String POSITION = "position";
> > 
> > what is a bookmark position?
> 
> Sync needs it.

what is a bookmark position? why does sync need it?
(In reply to Brad Lassey [:blassey] from comment #38)
> (In reply to Lucas Rocha (:lucasr) from comment #31)
> > (In reply to Brad Lassey [:blassey] from comment #30)

> > > what is a bookmark position?
> > 
> > Sync needs it.
> 
> what is a bookmark position? why does sync need it?

It's the position of the bookmark in the menu or toolbar where it is being shown. If we don't support it, like the other bits, then when the bookmarks are synced back to desktop, the order of you bookmarks will be screwed.
(In reply to Mark Finkle (:mfinkle) from comment #39)

> It's the position of the bookmark in the menu or toolbar where it is being
> shown. If we don't support it, like the other bits, then when the bookmarks
> are synced back to desktop, the order of you bookmarks will be screwed.

Over the past 18 months, a bookmarks engine bug that caused bookmarks to be reordered or moved between folders was the single biggest source of user anger at Sync that I can remember.

(Bug 621584.)

Apparently users really care about this!

Note that without any kind of positional information, Fennec's own bookmark display would be unpredictable after a sync operation: this is how you can tell Sync not to permute the contents of a container. We'll respect position as best we can.
https://hg.mozilla.org/mozilla-central/rev/bb39bae574f8
Status: REOPENED → RESOLVED
Last Resolved: 6 years ago6 years ago
Resolution: --- → FIXED
Hit submit too soon; full list:
https://hg.mozilla.org/mozilla-central/rev/af13b9729453
https://hg.mozilla.org/mozilla-central/rev/ebc46471b041
https://hg.mozilla.org/mozilla-central/rev/bb39bae574f8
Depends on: 709655
(Reporter)

Updated

6 years ago
Duplicate of this bug: 714054
(Reporter)

Updated

6 years ago
Blocks: 713623
tracking-fennec: --- → 11+
status-firefox11: --- → fixed

Updated

6 years ago
Depends on: 722184
status-firefox-esr10: --- → wontfix
status1.9.2: --- → unaffected
You need to log in before you can comment on or make changes to this bug.