Closed Bug 753534 Opened 12 years ago Closed 12 years ago

Desktop Bookmarks folder is created after migration from XUL to Native


(Firefox for Android Graveyard :: General, defect)

Not set


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

Firefox 15
Tracking Status
firefox14 --- fixed
firefox15 --- verified
blocking-fennec1.0 --- betaN+


(Reporter: gcp, Assigned: Margaret)



(2 files)

Should have responded here, but I talked about how to fix this in bug 752444 comment 13.
Assignee: nobody → margaret.leibovic
Attached 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?(
nom for triage, release blocker?   

It may not be destructive, but 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]

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/
@@ +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?( → review+
Attachment #622787 - Flags: review?(
Comment on attachment 622787 [details] [diff] [review]
part 2: get rid of getMobileBookmarksFolderId

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

Attachment #622787 - Flags: review?( → review+
Comment on attachment 622532 [details] [diff] [review]

[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
Comment on attachment 622532 [details] [diff] [review]

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+

(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.
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.