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

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Cristian Nicolae (:xti)]

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

Comment 1

[Cristian Nicolae (:xti)]

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)

Comment 2

[Cristian Nicolae (:xti)]

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

Comment 3

[Cristian Nicolae (:xti)]

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)

Comment 4

[:Margaret Leibovic]

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?

Comment 5

[:Margaret Leibovic]

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.

Comment 6

[:Margaret Leibovic]

Attachment: patch

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.

Comment 7

[Gian-Carlo Pascutto [:gcp]]

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.

Comment 8

[:Margaret Leibovic]

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

Comment 9

[:Margaret Leibovic]

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

Comment 10

[:Margaret Leibovic]

https://hg.mozilla.org/releases/mozilla-aurora/rev/2370b385ad3f

Comment 11

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/mozilla-central/rev/a3cea421cd6e

Comment 12

[Cristian Nicolae (:xti)]

Verified fixed on:

Firefox 15.0a1 (2012-05-15)
Device: Galaxy Nexus
OS: Android 4.0.2