Closed Bug 754276 Opened 8 years ago Closed 8 years ago

Desktop Bookmark dividers are displayed in Awesomebar > Bookmarks Tab as blank bookmark entries after migration from XUL > Native

Categories

(Firefox for Android :: General, defect)

ARM
Android
defect
Not set

Tracking

()

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

People

(Reporter: xti, Assigned: Margaret)

References

Details

Attachments

(2 files)

Attached image screenshot
Steps to reproduce:
1. Open Fennec
2. Add some dividers in Desktop Bookmarks
3. Sync Fennec
4. Go to AwesomeBar > Bookmarks > Desktop Bookmarks and check if there are blank entries or not

Expected result:
No blank entries are listed.

Actual result:
Totally blank entries are listed in Desktop Bookmarks folder (see screenshot)
If I tap on such a empty bookmark the following errors occur in console:

E/StrictMode(23679): class org.mozilla.gecko.AwesomeBar; instances=2; limit=1
E/StrictMode(23679): android.os.StrictMode$InstanceCountViolation: class org.mozilla.gecko.AwesomeBar; instances=2; limit=1
E/StrictMode(23679): 	at android.os.StrictMode.setClassInstanceLimit(StrictMode.java:1)
Actually this bug occurs only after Sync is done in XUL. So the actual str are:
1.Add some dividers in Desktop Bookmarks
2. Install and open XUL Fennec
3. Perform sync
4. Upgrade it to Native Fennec
5. Open Fennec
6. Go to AwesomeBar > Bookmarks > Desktop Bookmarks and check if there are blank entries or not


--
Firefox 15.0a1 (2012-05-11)
Device: Galaxy Nexus
OS: Android 4.0.2
Summary: Desktop Bookmark dividers are displayed in Awesomebar > Bookmarks Tab as blank bookmark entries → Desktop Bookmark dividers are displayed in Awesomebar > Bookmarks Tab as blank bookmark entries after migration from XUL > Native
Blocks: 754286
If one blank bookmark is opened in a new tab these errors occur in console:

E/GeckoApp(24100): Exception handling message "DOMContentLoaded":
E/GeckoApp(24100): java.lang.NullPointerException
E/GeckoApp(24100): 	at org.mozilla.gecko.GeckoApp.handleMessage(GeckoApp.java:854)
E/GeckoApp(24100): 	at org.mozilla.gecko.GeckoAppShell.handleGeckoMessage(GeckoAppShell.java:1857)
E/GeckoApp(24100): 	at org.mozilla.gecko.GeckoAppShell.nativeRun(Native Method)
E/GeckoApp(24100): 	at org.mozilla.gecko.GeckoAppShell.nativeRun(Native Method)
E/GeckoApp(24100): 	at org.mozilla.gecko.GeckoAppShell.runGecko(GeckoAppShell.java:501)
E/GeckoApp(24100): 	at org.mozilla.gecko.GeckoThread.run(GeckoThread.java:112)
Hm, even if we're migrating separators, we shouldn't be showing them in the UI. That's what bug 737024 is about. Maybe we're somehow messing up migrating the type?
Aha, yes, we're just checking if it's a folder or not:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/ProfileMigrator.java#1100

We should migrate the actual type.

Requesting blocking since this messes up bookmark data.
Assignee: nobody → margaret.leibovic
blocking-fennec1.0: --- → ?
Attached patch patchSplinter Review
This patch migrates separators, and it drops any other bookmark types that we aren't explicitly handling (looking at the places code, there really should only be bookmark/folder/separator types).

When I tested this it successfully migrated the separators (they showed up in my db), but they were hidden from the UI as expected.
Attachment #623203 - Flags: review?(gpascutto)
Comment on attachment 623203 [details] [diff] [review]
patch

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

If you rework this with a Map please request rereview. We're very unlikely to have to keep updating this code so I can live with the current patch as well. It's up to you.

::: mobile/android/base/ProfileMigrator.java
@@ +195,1 @@
>  

nit: add whitespace before = and realign

@@ +1106,1 @@
>  

nit: not a fan of this ternary construction

@@ +1220,5 @@
> +                              type == PLACES_TYPE_SEPARATOR)) {
> +                            cursor.moveToNext();
> +                            continue;
> +                        }
> +

How about putting the mapping in a map, checking for key existence in the map here and doing a map lookup in the earlier snippet? This would prevent the two from getting out of sync, it reflects better what you're doing, and I think it might be a bit cleaner.
Attachment #623203 - Flags: review?(gpascutto) → review+
While I agree a map would be a bite cleaner, I decided to just stick with the current approach, since as you mentioned, we're unlikely to need to update this code, and I don't think it's *too* messy for just 3 types.

https://hg.mozilla.org/integration/mozilla-inbound/rev/a3cea421cd6e
Target Milestone: --- → Firefox 15
Comment on attachment 623203 [details] [diff] [review]
patch

[Approval Request Comment]
Regression caused by (bug #): n/a
User impact if declined: bookmark separators will be migrated incorrectly, resulting in blank bookmarks
Testing completed (on m-c, etc.): just landed on inbound 
Risk to taking this patch (and alternatives if risky): low-risk, just fixes up some checks for bookmark types
String changes made by this patch: n/a
Attachment #623203 - Flags: approval-mozilla-aurora?
blocking-fennec1.0: ? → +
Attachment #623203 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
https://hg.mozilla.org/mozilla-central/rev/a3cea421cd6e
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Verified fixed on:

Firefox 15.0a1 (2012-05-15)
Device: Galaxy Nexus
OS: Android 4.0.2
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.