Desktop Bookmarks folder is created after migration from XUL to Native

VERIFIED FIXED in Firefox 14

Status

()

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

(Assignee)

Comment 1

7 years ago
Should have responded here, but I talked about how to fix this in bug 752444 comment 13.
Assignee: nobody → margaret.leibovic
(Assignee)

Comment 2

7 years ago
Created attachment 622532 [details] [diff] [review]
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.)
Attachment #622532 - Flags: review?(lucasr.at.mozilla)

Comment 3

7 years ago
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: --- → ?
(Reporter)

Comment 4

7 years ago
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+
(Assignee)

Comment 6

7 years ago
Created attachment 622787 [details] [diff] [review]
part 2: get rid of getMobileBookmarksFolderId
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+
(Assignee)

Comment 9

7 years ago
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+
https://hg.mozilla.org/mozilla-central/rev/a1a37a8540ca
https://hg.mozilla.org/mozilla-central/rev/30c980f358fc
Status: NEW → RESOLVED
Last Resolved: 7 years ago
status-firefox15: --- → fixed
Resolution: --- → FIXED
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
status-firefox15: fixed → 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+
(Assignee)

Comment 13

7 years ago
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.
status-firefox14: --- → fixed
You need to log in before you can comment on or make changes to this bug.