Desktop Bookmarks folder is created after migration from XUL to Native

VERIFIED FIXED in Firefox 14

Status

()

defect
--
minor
VERIFIED FIXED
7 years ago
7 years ago

People

(Reporter: gcp, Assigned: Margaret)

Tracking

Trunk
Firefox 15
ARM
Android
Points:
---

Firefox Tracking Flags

(firefox14 fixed, firefox15 verified, blocking-fennec1.0 betaN+)

Details

Attachments

(2 attachments)

Should have responded here, but I talked about how to fix this in bug 752444 comment 13.
Assignee: nobody → margaret.leibovic
Posted patch patchSplinter Review
This is a more correct approach, since we're only going to be able to show bookmarks in these three folders in the UI. However, I'm a bit torn about this because it's less efficient to be getting these different IDs, especially the first time this gets called before those IDs are cached.

Alternately, perhaps we should just avoid migrating any bookmarks that aren't in these expected folders, since they don't serve us any use hanging out in the DB. This fix would be more time-sensitive, though.

(I also have a follow-up patch to replace getMobileBookmarksFolderId with getFolderIdFromGuid(cr, Bookmarks.MOBILE_FOLDER_GUID), since this patch would make the caching in getMobileBookmarksFolderId redundant. However, I didn't want to scope creep this fix.)
Attachment #622532 - Flags: review?(lucasr.at.mozilla)
nom for triage, release blocker?   

It may not be destructive, but https://bugzilla.mozilla.org/show_bug.cgi?id=752444#c18 comments more on the perception of dataloss to a migrated user.
blocking-fennec1.0: --- → ?
It's a purely cosmetic issue. Note that the person commenting did in fact have dataloss due to *other* bugs.
Comment on attachment 622532 [details] [diff] [review]
patch

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

You can probably replace getMobileBookmarksFolderId with getFolderIdFromGuid now, right? Follow patch with that would be nice.

::: mobile/android/base/db/LocalBrowserDB.java
@@ +410,5 @@
>          mMobileFolderId = getFolderIdFromGuid(cr, Bookmarks.MOBILE_FOLDER_GUID);
>          return mMobileFolderId;
>      }
>  
>      private long getFolderIdFromGuid(ContentResolver cr, String guid) {

Don't you have to handle concurrent getFolderIdFromGuid() calls? Shouldn't it be synchronized?
Attachment #622532 - Flags: review?(lucasr.at.mozilla) → review+
Attachment #622787 - Flags: review?(lucasr.at.mozilla)
Comment on attachment 622787 [details] [diff] [review]
part 2: get rid of getMobileBookmarksFolderId

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

Nice.
Attachment #622787 - Flags: review?(lucasr.at.mozilla) → review+
Comment on attachment 622532 [details] [diff] [review]
patch

[Approval Request Comment]
Regression caused by (bug #): n/a
User impact if declined: after migration from XUL, the "Desktop Bookmarks" folder can appear, even if there are no desktop bookmarks
Testing completed (on m-c, etc.): just landed on inbound
Risk to taking this patch (and alternatives if risky): mobile-only, only affects the display of bookmarks, not the data itself
String changes made by this patch: n/a
Attachment #622532 - Flags: approval-mozilla-aurora?
blocking-fennec1.0: ? → betaN+
This bug seems to be fixed on the latest Nightly build, after performing the steps from bug 752444.

Marking bug as verified fixed on:

Firefox 15.0a1 (2012-05-11)
Device: Galaxy Nexus
OS: Android 4.0.2
Status: RESOLVED → VERIFIED
Comment on attachment 622532 [details] [diff] [review]
patch

Nom the other patch if you think it'll make things too difficult down the road.
Attachment #622532 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/b726a1552b8a

(In reply to Joe Drew (:JOEDREW!) from comment #12)
> Comment on attachment 622532 [details] [diff] [review]
> patch
> 
> Nom the other patch if you think it'll make things too difficult down the
> road.

I don't see us messing with this code much more for this release, but hey, you never know! I think we can wait to see if it becomes a problem.
You need to log in before you can comment on or make changes to this bug.