753534 - Desktop Bookmarks folder is created after migration from XUL to Native

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Gian-Carlo Pascutto [:gcp]]

https://bugzilla.mozilla.org/show_bug.cgi?id=752444#c0
https://bugzilla.mozilla.org/show_bug.cgi?id=752444#c7

Comment 1

[:Margaret Leibovic]

Should have responded here, but I talked about how to fix this in bug 752444 comment 13.

Comment 2

[:Margaret Leibovic]

Attachment: patch

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.)

Comment 3

[Tony Chung [:tchung]]

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.

Comment 4

[Gian-Carlo Pascutto [:gcp]]

It's a purely cosmetic issue. Note that the person commenting did in fact have dataloss due to *other* bugs.

Comment 5

[Lucas Rocha (:lucasr)]

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?

Comment 6

[:Margaret Leibovic]

Attachment: part 2: get rid of getMobileBookmarksFolderId

Comment 7

[Lucas Rocha (:lucasr)]

Comment on attachment 622787 [details] [diff] [review]
part 2: get rid of getMobileBookmarksFolderId

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

Nice.

Comment 8

[:Margaret Leibovic]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a1a37a8540ca
https://hg.mozilla.org/integration/mozilla-inbound/rev/30c980f358fc

Comment 9

[:Margaret Leibovic]

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

Comment 10

[Joe Drew (not getting mail)]

https://hg.mozilla.org/mozilla-central/rev/a1a37a8540ca
https://hg.mozilla.org/mozilla-central/rev/30c980f358fc

Comment 11

[Cristian Nicolae (:xti)]

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 12

[Joe Drew (not getting mail)]

Comment on attachment 622532 [details] [diff] [review]
patch

Nom the other patch if you think it'll make things too difficult down the road.

Comment 13

[:Margaret Leibovic]

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.