Closed Bug 731267 Opened 10 years ago Closed 10 years ago

Ignore "All Bookmarks" folder and its badly rooted parent

Categories

(Firefox for Android Graveyard :: General, defect, P1)

ARM
Android
defect

Tracking

(firefox13 verified, firefox14 verified, blocking-fennec1.0 beta+)

VERIFIED FIXED
Firefox 14
Tracking Status
firefox13 --- verified
firefox14 --- verified
blocking-fennec1.0 --- beta+

People

(Reporter: tracy, Assigned: Margaret)

References

Details

Attachments

(3 files)

Select Bookmarks from the Fennec awesomebar. Notice inside there is a folder that has no title.  Inside that folder is an "All Bookmarks" folder.  Inside that folder is just text "All Bookmarks"

I think this came from desktop.  Should this be populated or not synced in the first place?
I wonder if this is the same as Bug 730350….
Blocks: 718154
Possibly, but the untitled folder(with All Bookmarks" inside) that I am seeing is at the same level as the Mobile folder, not inside it.
(In reply to Tracy Walker [:tracy] from comment #2)
> Possibly, but the untitled folder(with All Bookmarks" inside) that I am
> seeing is at the same level as the Mobile folder, not inside it.

We rearranged the database schema, and adjusted how bookmarks are presented, since that bug.
Ok, dupes; probably that one of this since what I'm reporting is current behavior.
Duplicate of this bug: 730350
Priority: -- → P3
Attached image screenshot
I think this should at least be a P2.  Looking at this view gives a lousy user experience of not knowing what's in this folder.
Just to clarify:

Tony, Tracy: do you have STR from a "fresh" (has never synced with Fennec) desktop profile and a fresh (starting from not installed at all) Fennec profile?
(In reply to Richard Newman [:rnewman] from comment #7)
> Just to clarify:
> 
> Tony, Tracy: do you have STR from a "fresh" (has never synced with Fennec)
> desktop profile and a fresh (starting from not installed at all) Fennec
> profile?

Yeah thats what i did for my screenshot.  

STR: 
1) install clean nightly on fennec  (03-05-2012 build)
2) create a clean firefox desktop profile, and bookmark a few sites.  Bookmarked to Bookmarks Menu, Unsorted Bookmarks, create and name a folder within Bookmarks Menu. Even add tags and such.
3) setup a new sync account on desktop, and sync your bookmarked changes.
4) back to fennec, launch Sync > use Jpake, and watch everything sync over
5) Verify empty bookmark folder
Here's what it looks like after you click the titleless folder.   It takes me to an "All Bookmarks" view.  Then if you open that folder, you get a blank entry.
triage: moving to p2, not a sync issue, but bad ux caused by the data from fennec
blocking-fennec1.0: --- → ?
Component: Android Sync → General
Priority: P3 → P2
Product: Mozilla Services → Fennec Native
QA Contact: android-sync → general
(In reply to :Ally Naaktgeboren from comment #10)
> triage: moving to p2, not a sync issue, but bad ux caused by the data from
> fennec

Not sure if that's true; haven't determined a root cause yet. Unless Tony can repro without involving Sync, I'm inclined to point the finger at me...
Marking this as blocking Fennec Native release; we want to try to eliminate any cases of incorrect UX related to Sync even if they are mostly harmless/cosmetic, because we want users to feel that sync is 100% trustworthy.
blocking-fennec1.0: ? → +
See <https://bugzilla.mozilla.org/show_bug.cgi?id=731273#c27>.

We should stop these records from being uploaded, but we have to handle the ones that are there.
Status: NEW → ASSIGNED
blocking-fennec1.0: + → ?
Component: General → Android Sync
Priority: P2 → P1
Product: Fennec Native → Mozilla Services
QA Contact: general → android-sync
Summary: All bookmarks folder is empty inside of an untitled folder → Ignore "All Bookmarks" folder and its badly rooted parent
blocking-fennec1.0: ? → beta+
Whiteboard: [sync]
We'll either do this in Sync or (as gcp and I discussed today) in the Fennec bookmarks UI, by only showing the known special roots. Margaret?
Assignee: nobody → rnewman
I spent a little more time considering this.

If Sync deletes any part of the additional tree structure, we run the risk of dumping subsequent records into Unsorted Bookmarks.

For example, if we delete on-the-fly, you might get "All Bookmarks" show up as a folder in Unsorted Bookmarks, because we deleted its parent before we added it.

If we clean up at the end of a sync, you'll get transient records in the DB (we'll add, then delete), and if the sync is interrupted or new records arrive later, they'll get dumped into Unsorted Bookmarks just as if we deleted on-the-fly.

The only safe thing we can do is store the records in the DB, in order to preserve the tree structure to "catch" subsequent records.

We also don't particularly want to flag these records as deleted; if we do that, then any subsequent upload will attempt to delete them on other machines.

The correct course of action, then, is for Fennec to ignore roots that it doesn't care about.

Over the wall!
Assignee: rnewman → nobody
No longer blocks: 718154
Status: ASSIGNED → NEW
blocking-fennec1.0: beta+ → ?
Component: Android Sync → General
Product: Mozilla Services → Fennec Native
QA Contact: android-sync → general
Whiteboard: [sync]
blocking-fennec1.0: ? → beta+
Assignee: nobody → margaret.leibovic
Attached patch patchSplinter Review
This patch special-cases the FIXED_ROOT_ID and returns only the folders we care about in that case. This also let me remove the code we were using to ignore the root folder and the tags folder in our results.
Attachment #605980 - Flags: review?(lucasr.at.mozilla)
Blocks: 727449
Comment on attachment 605980 [details] [diff] [review]
patch

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

::: mobile/android/base/db/LocalBrowserDB.java
@@ +314,5 @@
> +
> +        if (folderId == Bookmarks.FIXED_ROOT_ID) {
> +            // Because of sync, we can end up with some additional records under
> +            // the root node that we don't want to see. Since sync doesn't 
> +            // want to run into problems deleing these, we can just ignore them

deleting

@@ +321,5 @@
> +                         DEFAULT_BOOKMARK_COLUMNS,
> +                         Bookmarks.GUID + " = ? OR " +
> +                         Bookmarks.GUID + " = ? OR " +
> +                         Bookmarks.GUID + " = ? OR " +
> +                         Bookmarks.GUID + " = ?",

nit: I'd still keep a Bookmarks.PARENT = folderId in the query just to make sure we're still doing the right thing even with the specific set of folders.
Attachment #605980 - Flags: review?(lucasr.at.mozilla) → review+
https://hg.mozilla.org/mozilla-central/rev/2d19579ec69b
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 14
Comment on attachment 605980 [details] [diff] [review]
patch

[Approval Request Comment]
Mobile-only. Blocking fennec-1.0.
Attachment #605980 - Flags: approval-mozilla-aurora?
Comment on attachment 605980 [details] [diff] [review]
patch

[Triage Comment]
Mobile only - approved for Aurora 13.
Attachment #605980 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Verified/fixed on:

Aurora Fennec 13.0a2 (2012-03-26)
Nightly Fennec 14.0a1 (2012-03-26)
Device: Samsung Nexus S
OS: Android 2.3.6
Status: RESOLVED → VERIFIED
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.