Fennec Bookmark Management: Allow users to move bookmarks and folders into folders

RESOLVED FIXED in Firefox 55

Status

()

Firefox for Android
General
P1
normal
RESOLVED FIXED
a year ago
4 months ago

People

(Reporter: jcheng, Assigned: jwu)

Tracking

(Blocks: 1 bug)

unspecified
Firefox 55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed, firefox58 verified)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments)

As a user, I'd like to be able to move both bookmarks and folders into folders without problems
(Reporter)

Updated

a year ago
Blocks: 1329127
(Assignee)

Updated

a year ago
Assignee: nobody → topwu.tw
Priority: -- → P1
(Assignee)

Updated

11 months ago
Depends on: 1232439
(Assignee)

Updated

11 months ago
Blocks: 1329128
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 3

11 months ago
I'm not sure if you have considered this, but you need to keep in mind that there's a limit on how many bookmark record any particular folder might have.

When we're syncing this data, a folder is, very roughly, serialized something like this: 
{
  "id": "-F_Szdjg3GzY",
  "modified": 1388635807.41,
  "sortindex": 140,
  "payload": "... children:[..list of guids...]..."
}

See https://docs.services.mozilla.com/storage/apis-1.5.html#basic-storage-object

The payload object is limited in size, and sync servers won't accept records whose payloads exceed a certain byte limit. If, during a sync, server refuses one of the records - we won't be able to sync that collections. In other words, if you have too many bookmarks, our bookmark sync will stop working!

Furthermore, high-level mobile bookmark records and folders will be under the mobile root folder. So ideally your bookmark management UI should not allow users to either create too many top-level bookmarks, or add too many bookmarks to a particular folder.

Bug 1343726 might be of interest, which improves how we handle payload_byte limits during a sync.
(Assignee)

Updated

10 months ago
Attachment #8848972 - Attachment is obsolete: true
Attachment #8848972 - Flags: review?(gkruglov)
(Assignee)

Updated

10 months ago
Attachment #8848973 - Attachment is obsolete: true
Attachment #8848973 - Flags: review?(gkruglov)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 6

10 months ago
mozreview-review
Comment on attachment 8848972 [details]
Bug 1329131 - Part 1: Implement the functionality of listing all bookmark folders except special folders.

https://reviewboard.mozilla.org/r/121818/#review135954

::: mobile/android/base/java/org/mozilla/gecko/db/BrowserDB.java:114
(Diff revision 2)
>      public abstract boolean addBookmark(ContentResolver cr, String title, String uri);
>      public abstract Uri addBookmarkFolder(ContentResolver cr, String title, long parentId);
>      public abstract Cursor getBookmarkForUrl(ContentResolver cr, String url);
>      public abstract Cursor getBookmarksForPartialUrl(ContentResolver cr, String partialUrl);
>      public abstract Cursor getBookmarkById(ContentResolver cr, long id);
> +    public abstract Cursor getBookmarkByGuid(ContentResolver cr, String guid);

If you stick with `null`, mark as @Nullable

::: mobile/android/base/java/org/mozilla/gecko/db/LocalBrowserDB.java:1840
(Diff revision 2)
>  
>          return c;
>      }
>  
>      @Override
> +    public Cursor getBookmarkByGuid(ContentResolver cr, String guid) {

If you want to return a `null` and have it as a legit scenario, mark method as @Nullable so that the consumers know they should check, and IDEs make programmer errors less likely.

::: mobile/android/base/java/org/mozilla/gecko/db/LocalBrowserDB.java:1849
(Diff revision 2)
> +                                  new String[] { guid },
> +                                  null);
> +
> +        if (c != null && c.getCount() == 0) {
> +            c.close();
> +            return null;

Perhaps consider just returning an empty cursor. Nulls which are floating around are a PITA to deal with.

::: mobile/android/base/java/org/mozilla/gecko/db/LocalBrowserDB.java:1856
(Diff revision 2)
> +
> +        return c;
> +    }
> +
> +    @Override
> +    public Cursor getBookmarkFoldersWithParent(ContentResolver cr, long parentId) {

Can you explain how this is different from `getBookmarksInFolder`? I see that this function clearly returns _less_ folders, but no explanation as to why. At a glance this seems like a copy/paste with a few omissions.

What's the logic here?

::: mobile/android/base/java/org/mozilla/gecko/db/LocalBrowserDB.java:1873
(Diff revision 2)
> +
> +        // We always want to show mobile bookmarks in the root view.
> +        if (parentId == Bookmarks.FIXED_ROOT_ID) {
> +            c = cr.query(mBookmarksUriWithProfile,
> +                         DEFAULT_BOOKMARK_COLUMNS,
> +                         Bookmarks.GUID + " = ? AND " + Bookmarks.TYPE + " = ?",

A bookmark with guid="mobile" that isn't a folder means that something got _really_ screwed up.
Attachment #8848972 - Flags: review?(gkruglov) → review-
(Assignee)

Comment 7

10 months ago
mozreview-review-reply
Comment on attachment 8848972 [details]
Bug 1329131 - Part 1: Implement the functionality of listing all bookmark folders except special folders.

https://reviewboard.mozilla.org/r/121818/#review135954

> Perhaps consider just returning an empty cursor. Nulls which are floating around are a PITA to deal with.

I prefer to add @Nullable because changing to empty cursor might affect caller behaviour.

> Can you explain how this is different from `getBookmarksInFolder`? I see that this function clearly returns _less_ folders, but no explanation as to why. At a glance this seems like a copy/paste with a few omissions.
> 
> What's the logic here?

`getBookmarkFoldersWithParent` is used to query child folders by a given parent ID. 

There are some differences between these two methods:
1. `getBookmarkFoldersWithParent` only queries folders, but `getBookmarksInFolder` queries both bookmarks and folders.
2. If parentId is 'FIXED_ROOT_ID', `getBookmarkFoldersWithParent` queries 'mobile bookmark', but `getBookmarksInFolder` queries children inside 'mobile bookmark'.

If I reuse `getBookmarksInFolder`, the logic in AsyncTaskLoader might be a little more complex.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 10

10 months ago
mozreview-review
Comment on attachment 8848973 [details]
Bug 1329131 - Part 2: Implement selecting bookmark folder page.

https://reviewboard.mozilla.org/r/121820/#review136650

The UI parts look good! I'm concerned about the way we load the folder tree, since it performs 1 DB query per folder. I don't know whether or not we need to care (grisha might have more useful opinions on that), but if we do: I have an unused implementation that builds the folder tree based on one DB query.

::: mobile/android/base/java/org/mozilla/gecko/bookmarks/SelectFolderFragment.java:112
(Diff revision 3)
> +
> +    @Override
> +    public void onResume() {
> +        super.onResume();
> +
> +        getLoaderManager().initLoader(0, null, new FoldersLoaderCallbacks());

I know we only have one loader here - but it would be LOADER_ID_?????? just in case we ever extend the loaders, and also to ensure this never breaks.

::: mobile/android/base/java/org/mozilla/gecko/bookmarks/SelectFolderFragment.java:124
(Diff revision 3)
> +        getLoaderManager().destroyLoader(0);
> +    }
> +
> +    public void onFolderSelected(final Folder folder) {
> +        // Ignore fake folders.
> +        if (folder.id == BrowserContract.Bookmarks.FAKE_DESKTOP_FOLDER_ID) {

Do we do anything in the UI to make sure its obvious that the desktop folder can't be selected? It might be unintuitive if we simply do nothing, maybe we can show a snackbar or some other form of feedback (this might need UX input?).

::: mobile/android/base/java/org/mozilla/gecko/bookmarks/SelectFolderFragment.java:211
(Diff revision 3)
> +            if (cursor == null) {
> +                return children;
> +            }
> +
> +            try {
> +                while (cursor.moveToNext()) {

IIUC this will perform a DB query for every single folder. That's OK for a small number of folders, but isn't great if someone has many folders. (I don't think that happens much in real life, but there might be some such users. Maybe we have telemetry for that?)

It might be more efficient to query all folders at one time, and then build the tree. I actually had an unlanded/unreviewed WIP patch, see:
https://reviewboard.mozilla.org/r/43897/diff/1#index_header
(Assignee)

Comment 11

10 months ago
mozreview-review-reply
Comment on attachment 8848973 [details]
Bug 1329131 - Part 2: Implement selecting bookmark folder page.

https://reviewboard.mozilla.org/r/121820/#review136650

> Do we do anything in the UI to make sure its obvious that the desktop folder can't be selected? It might be unintuitive if we simply do nothing, maybe we can show a snackbar or some other form of feedback (this might need UX input?).

I'll ask Tori for help to check if we should apply different style for desktop folder.

> IIUC this will perform a DB query for every single folder. That's OK for a small number of folders, but isn't great if someone has many folders. (I don't think that happens much in real life, but there might be some such users. Maybe we have telemetry for that?)
> 
> It might be more efficient to query all folders at one time, and then build the tree. I actually had an unlanded/unreviewed WIP patch, see:
> https://reviewboard.mozilla.org/r/43897/diff/1#index_header

I'll send another patch based on your suggestion.

The metohd `getAllBookmarkFolders` in LocalBrowserDB is similar to your patch but with a little difference: I prefer it just simply returns a cursor instead of a data model. So that each AsyncTaskLoader calling this method can decide how to compose the data from the cursor.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(In reply to Andrzej Hunt :ahunt from comment #10)

> IIUC this will perform a DB query for every single folder. That's OK for a
> small number of folders, but isn't great if someone has many folders. (I
> don't think that happens much in real life, but there might be some such
> users. Maybe we have telemetry for that?)

It absolutely does; by the time you get to the 1% you're into the users with 50,000 bookmarks meticulously arranged into folders.

I don't use folders much, and I have 128 in places.sqlite.


> It might be more efficient to query all folders at one time, and then build
> the tree.

Definitely do so.


>      @Override
> +    public Cursor getBookmarkByGuid(ContentResolver cr, String guid) {

If this is returning a single value or none, why return a `Cursor` at all? Return some richer structure. `Cursor` requires the caller to understand the columns you're populating, which is implicit and fragile.

More broadly, I strongly discourage the use of data access methods that work with a single value via external iteration. They lead to behavior that tests fine with small test data, but is very expensive for real-world data.

There's no use of this method in either Part 1 or Part 2, so I can't tell if that's what it's for. If you're listing folder contents do not use this approach. If you're listing a single record (e.g., to populate an edit UI), then this is fine.

Comment 15

10 months ago
mozreview-review
Comment on attachment 8848972 [details]
Bug 1329131 - Part 1: Implement the functionality of listing all bookmark folders except special folders.

https://reviewboard.mozilla.org/r/121818/#review138148

::: mobile/android/base/java/org/mozilla/gecko/db/LocalBrowserDB.java:1845
(Diff revision 4)
>          return c;
>      }
>  
>      @Override
> +    @Nullable
> +    public Cursor getBookmarkByGuid(ContentResolver cr, String guid) {

I think you meant to include this change as part of Bug 1329128. You seem to be using this method to fetch a single mobile root folder record, so I think this is fine.

Please do move this change to the other bug's patch though.

::: mobile/android/tests/background/junit4/src/org/mozilla/gecko/db/LocalBrowserDBTest.java:365
(Diff revision 4)
> +
> +        // Create 2 folders into mobile
> +        db.addBookmarkFolder(cr, "child-1", parentId);
> +        db.addBookmarkFolder(cr, "child-2", parentId);
> +
> +        // Create a bookmark, add bookmark should not increase folder count.

"adding a bookmarks"

::: mobile/android/tests/background/junit4/src/org/mozilla/gecko/db/LocalBrowserDBTest.java:372
(Diff revision 4)
> +
> +        final Cursor cursor = db.getAllBookmarkFolders(cr);
> +        assertNotNull(cursor);
> +
> +        try {
> +            // Current folder count should equal (folderCount + 2) since we add 2 folders.

"added"

::: mobile/android/tests/background/junit4/src/org/mozilla/gecko/db/LocalBrowserDBTest.java:374
(Diff revision 4)
> +        assertNotNull(cursor);
> +
> +        try {
> +            // Current folder count should equal (folderCount + 2) since we add 2 folders.
> +            final int currentFolderCount = cursor.getCount();
> +            assertEquals(folderCount + 2, currentFolderCount);

You might also want to check that you're getting back all of the rows that you're expecting.

Also, since you're going to rely on the cursor's projection in the code calling this function, it would be good to add assertions that the columns are also what you expect them to be.
Attachment #8848972 - Flags: review?(gkruglov) → review+

Comment 16

10 months ago
mozreview-review
Comment on attachment 8848973 [details]
Bug 1329131 - Part 2: Implement selecting bookmark folder page.

https://reviewboard.mozilla.org/r/121820/#review138166

::: mobile/android/base/java/org/mozilla/gecko/bookmarks/BookmarkEditFragment.java:237
(Diff revision 4)
>              locationLayout.setVisibility(View.VISIBLE);
>          }
>          locationText.setText(bookmark.url);
>  
>          if (Bookmarks.MOBILE_FOLDER_GUID.equals(bookmark.guid)) {
> -            folderText.setText(R.string.bookmarks_folder_mobile);
> +            folderText.setText(R.string.bookmarks_folder_none);

I wonder if "None" is the right choice here. On one hand, once synced to desktop these bookmarks will be displayed under "Mobile Bookmarks" folder. On the other hand, we just display them as top level on Android, and so a user might perceive them as not being in a folder at all.

To make matters worse, on iOS android's bookmarks will be displayed in the Desktop Bookmarks -> Mobile Bookmarks folder.

What a mess!

I'm inclined to say "let's aim for consistency with desktop" - and so just say "Mobile Bookmarks" here. But this is a good place to get input from UX.

Comment 17

10 months ago
mozreview-review
Comment on attachment 8848973 [details]
Bug 1329131 - Part 2: Implement selecting bookmark folder page.

https://reviewboard.mozilla.org/r/121820/#review138170

::: mobile/android/base/java/org/mozilla/gecko/bookmarks/SelectFolderFragment.java:132
(Diff revision 4)
> +    }
> +
> +    public void onFolderSelected(final Folder folder) {
> +        // Ignore fake folders.
> +        if (folder.id == BrowserContract.Bookmarks.FAKE_DESKTOP_FOLDER_ID) {
> +            return;

IIUC, you're waiting on UX feedback for this?

::: mobile/android/base/java/org/mozilla/gecko/bookmarks/SelectFolderFragment.java:208
(Diff revision 4)
> +            final Cursor cursor = db.getAllBookmarkFolders(cr);
> +            if (cursor == null) {
> +                return null;
> +            }
> +
> +            final LongSparseArray<List<Folder>> array = new LongSparseArray<>();

Why did you pick this data structure vs a basic HashMap?

Docs mention that it's something like 50% as performant as a HashMap when dealing with hundreds of items, and the implication is that it's much less performant when dealing with thousands. Each `array.get` call below will perform a binary search, which will get keep getting more expensive as you're building up your tree.

Benefit seems to be a smaller memory footprint. However, it doesn't really matter for small number of folders - and the performance might be much worse when we'll be dealing with thousands of folders.

::: mobile/android/base/java/org/mozilla/gecko/bookmarks/SelectFolderFragment.java:211
(Diff revision 4)
> +            }
> +
> +            final LongSparseArray<List<Folder>> array = new LongSparseArray<>();
> +            try {
> +                while (cursor.moveToNext()) {
> +                    final long id = cursor.getLong(cursor.getColumnIndex(BrowserContract.Bookmarks._ID));

Our convention is to use `getColumnIndexOrThrow`. The strange crashes that will happen in case of missing columns will be more obvious.

::: mobile/android/base/java/org/mozilla/gecko/bookmarks/SelectFolderFragment.java:232
(Diff revision 4)
> +
> +                        default:
> +                            parentId = cursor.getLong(cursor.getColumnIndex(BrowserContract.Bookmarks.PARENT));
> +                    }
> +
> +                    final List<Folder> folders = array.get(parentId, new ArrayList<Folder>());

This will needlessly create an empty list for every iteration.

::: mobile/android/base/java/org/mozilla/gecko/bookmarks/SelectFolderFragment.java:246
(Diff revision 4)
> +            folders = lookupChildFolders(array, BrowserContract.Bookmarks.FIXED_ROOT_ID, 0);
> +            return folders;
> +        }
> +
> +        /**
> +         * Look up child folders recursively with given parent ID.

Java doesn't have tail call elimination, and I bet this will StackOverflow given a deep enough nesting of folders - or simply with large enough folders! Remember that you might be dealing with potentially broken bookmark trees, or with folders that will have thousands of bookmarks. This might even fail with seemingly innocent trees depending on restricted the platform is and how small of a stack your background thread will get alloted.

It will be safer and will probably save us a lot of grief in the future if you re-wrote this in a non-recursive fashion.
Attachment #8848973 - Flags: review?(gkruglov) → review-
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 20

10 months ago
mozreview-review-reply
Comment on attachment 8848973 [details]
Bug 1329131 - Part 2: Implement selecting bookmark folder page.

https://reviewboard.mozilla.org/r/121820/#review138170

> IIUC, you're waiting on UX feedback for this?

Yeah, but the change might just add a new UI style for special folder(e.g. 'Desktop Bookmarks'). It won't change a lot of code here. Maybe we should file a new bug instead of let this midification as a blocker?
(Assignee)

Comment 21

10 months ago
mozreview-review-reply
Comment on attachment 8848973 [details]
Bug 1329131 - Part 2: Implement selecting bookmark folder page.

https://reviewboard.mozilla.org/r/121820/#review138166

> I wonder if "None" is the right choice here. On one hand, once synced to desktop these bookmarks will be displayed under "Mobile Bookmarks" folder. On the other hand, we just display them as top level on Android, and so a user might perceive them as not being in a folder at all.
> 
> To make matters worse, on iOS android's bookmarks will be displayed in the Desktop Bookmarks -> Mobile Bookmarks folder.
> 
> What a mess!
> 
> I'm inclined to say "let's aim for consistency with desktop" - and so just say "Mobile Bookmarks" here. But this is a good place to get input from UX.

Actually the spec in Bug 1347095 covers the strings for new bookmark management and it uses 'None' instead of 'Mobile Bookmarks'. I'll ask UX to help double confirm, especially the consistency between different platforms.

Maybe we can leave string modification to Bug 1347095 and file another bug if any modification needed to keep the consistency between different platforms?
(Assignee)

Comment 22

10 months ago
mozreview-review-reply
Comment on attachment 8848973 [details]
Bug 1329131 - Part 2: Implement selecting bookmark folder page.

https://reviewboard.mozilla.org/r/121820/#review138170

Comment 23

10 months ago
(In reply to Jing-wei Wu [:jwu] from comment #20)
> Yeah, but the change might just add a new UI style for special folder(e.g.
> 'Desktop Bookmarks'). It won't change a lot of code here. Maybe we should
> file a new bug instead of let this midification as a blocker?

Sounds good, just make sure it doesn't get lost in the ether :-)

Comment 24

10 months ago
(In reply to Jing-wei Wu [:jwu] from comment #21)
> Actually the spec in Bug 1347095 covers the strings for new bookmark
> management and it uses 'None' instead of 'Mobile Bookmarks'. I'll ask UX to
> help double confirm, especially the consistency between different platforms.
> 
> Maybe we can leave string modification to Bug 1347095 and file another bug
> if any modification needed to keep the consistency between different
> platforms?

Follow-up bug sounds good to me, as this requires some cross-platform consideration.
(Assignee)

Comment 25

10 months ago
(In reply to :Grisha Kruglov from comment #24)
> (In reply to Jing-wei Wu [:jwu] from comment #21)
> > Actually the spec in Bug 1347095 covers the strings for new bookmark
> > management and it uses 'None' instead of 'Mobile Bookmarks'. I'll ask UX to
> > help double confirm, especially the consistency between different platforms.
> > 
> > Maybe we can leave string modification to Bug 1347095 and file another bug
> > if any modification needed to keep the consistency between different
> > platforms?
> 
> Follow-up bug sounds good to me, as this requires some cross-platform
> consideration.

After discussion with UX, we'll use 'Mobile Bookmarks' instead of 'None'. I'll send a new patch for this string modification.

For keep bookmarks order consistent between different platforms, I've created a Bug 1361678 to help track this issue.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 28

10 months ago
mozreview-review
Comment on attachment 8848973 [details]
Bug 1329131 - Part 2: Implement selecting bookmark folder page.

https://reviewboard.mozilla.org/r/121820/#review138984

Looks good to me, with some nits.

::: mobile/android/base/java/org/mozilla/gecko/bookmarks/SelectFolderFragment.java:206
(Diff revision 6)
> +            this.bookmarkId = bookmarkId;
> +        }
> +
> +        @Override
> +        public List<Folder> loadInBackground() {
> +            final Cursor cursor = db.getAllBookmarkFolders(cr);

You might want to note here that we're relying on a stable order of this cursor (don't want your tree to shuffle every time you look at it!).

Seems like in this case we're going to use the default ordering supplied by BrowserProvider, e.g. `ASC TYPE, ASC POSITION, ASC ID`, which I think should be fine here.

::: mobile/android/base/java/org/mozilla/gecko/bookmarks/SelectFolderFragment.java:211
(Diff revision 6)
> +            final Cursor cursor = db.getAllBookmarkFolders(cr);
> +            if (cursor == null) {
> +                return null;
> +            }
> +
> +            @SuppressLint("UseSparseArrays") final Map<Long, List<Folder>> folderMap = new HashMap<>();

Suppressing warning, eh. I hope this was a good call :-)

I suggest experimenting with different data and see if you can measure a perf difference between using a sparselist vs a hashmap. I wonder what impact GC churn will have when using an int-keyed HashMap.

::: mobile/android/base/java/org/mozilla/gecko/bookmarks/SelectFolderFragment.java:248
(Diff revision 6)
> +                }
> +            } finally {
> +                cursor.close();
> +            }
> +
> +            folders = flattenChildFolders(folderMap);

Just `return flattenChildFolders(folderMap)`

::: mobile/android/base/java/org/mozilla/gecko/bookmarks/SelectFolderFragment.java:254
(Diff revision 6)
> +            return folders;
> +        }
> +
> +        private List<Folder> flattenChildFolders(Map<Long, List<Folder>> map) {
> +            final List<Folder> result = new ArrayList<>();
> +            final ArrayDeque<Folder> folderQueue = new ArrayDeque<>();

You're using this as a stack, for performance reasons I presume? If so, add a note here saying so.

::: mobile/android/base/java/org/mozilla/gecko/bookmarks/SelectFolderFragment.java:254
(Diff revision 6)
> +            return folders;
> +        }
> +
> +        private List<Folder> flattenChildFolders(Map<Long, List<Folder>> map) {
> +            final List<Folder> result = new ArrayList<>();
> +            final ArrayDeque<Folder> folderQueue = new ArrayDeque<>();

You'd want to initialize both of these structures with a sufficient initial capacity to avoid resizing. A multiple of your map size should approximate this, if you want to guess an average number of folders in a folder.

Or you can just pass in size of the cursor from the method above, which should be the exact size you'll use.
Attachment #8848973 - Flags: review?(gkruglov) → review+

Comment 29

10 months ago
> Do we do anything in the UI to make sure its obvious that the desktop folder
> can't be selected? It might be unintuitive if we simply do nothing, maybe we
> can show a snackbar or some other form of feedback (this might need UX
> input?).

Thank you for pointing this! I think it's a bit unintuitive but I wonder showing a snack bar or a different UI might cost extra effort and makes things more complex. The chance that having desktop bookmark synced and selecting desktop folder for a mobile bookmark is very small, so I'm fine with current design.

In the end bookmark structure should be more consistent across desktop/Android/iOS so hopefully we'll fix it in bug 1361678 in the future.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

10 months ago
Keywords: checkin-needed

Comment 32

10 months ago
mozreview-review
Comment on attachment 8848972 [details]
Bug 1329131 - Part 1: Implement the functionality of listing all bookmark folders except special folders.

https://reviewboard.mozilla.org/r/121818/#review140928
Attachment #8848972 - Flags: review?(ahunt) → review+

Comment 33

10 months ago
mozreview-review
Comment on attachment 8848973 [details]
Bug 1329131 - Part 2: Implement selecting bookmark folder page.

https://reviewboard.mozilla.org/r/121820/#review140932
Attachment #8848973 - Flags: review?(ahunt) → review+

Comment 34

10 months ago
Pushed by ihsiao@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e407c1b7d3ea
Part 1: Implement the functionality of listing all bookmark folders except special folders. r=Grisha
https://hg.mozilla.org/integration/autoland/rev/142b90327e7c
Part 2: Implement selecting bookmark folder page. r=Grisha
Keywords: checkin-needed

Comment 35

10 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/e407c1b7d3ea
https://hg.mozilla.org/mozilla-central/rev/142b90327e7c
Status: NEW → RESOLVED
Last Resolved: 10 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55

Updated

9 months ago
Duplicate of this bug: 972193
Depends on: 1401871
Verified as fixed in build 58.0a1 (10/23).
Devices: Motorola Nexus 6 (Android 7.1.1), Google Pixel (Android 8.0), Sony Xperia Z5 Premium (Android 6.0.1).

I was able to move folders into folders, bookmarks from one folder to another, also editing and moving at the same step.
status-firefox58: --- → verified
You need to log in before you can comment on or make changes to this bug.