1025441 - [Homescreen] Removed bookmarks show up again when restarting the phone

Status: VERIFIED FIXED

(Firefox OS Graveyard :: Gaia::Homescreen, defect)

Description

[Diego Marcos [:dmarcos]]

Steps to reproduce:

1. Open browser
2. Go to a website and tap on the star icon on the menu bar
3. Tap on add to home screen
4. Confirm 

The icon shows up on the homescreen as expected

5. Long tap on the icon to go into edit mode in the home screen
6. Tap on the X to remove the recently added bookmark

The icon disappears as expected.

Now reboot the phone and you will see the icon back in the homescreen. Back from the death

Comment 1

[Diego Marcos [:dmarcos]]

Attachment: Pull request

The patch overrides _decorateIcon for bookmarks. It paints the icon circle as a background of the favicon that now seats in the middle. I feel I have not spend enough time polishing and testing the patch to mark if for review. I can jump  on this again on Monday but feel free to take it from here if you want to get it done any earlier.

Comment 2

[Diego Marcos [:dmarcos]]

Comment on attachment 8440258 [details] [review]
Pull request

Sorry PR doesn't belong to this bug

Comment 3

[Kevin Grandon :kgrandon]

I might look into this, but I think the ni? was for the other patch :) Thanks!

Comment 4

[Kevin Grandon :kgrandon]

We've also had reports of people being unable to delete bookmarks. Still trying to find a solid STR here. I filed bug 1026265 today, not sure if it's related.

Comment 5

[Cristian Rodriguez (:crdlc)]

I gonna take a look at this today

Comment 6

[Cristian Rodriguez (:crdlc)]

Well I can reproduce the bug and I've detected that the bug is the vertical homescreen. When I did the Bookmark app I implemented a Bookmark Reader app [1] and if you open this app the bookmark is not there (it means that the bookmark datastore is ok). Maybe the bookmark persists in the indexedDB of vh.

[1] https://github.com/mozilla-b2g/gaia/tree/master/dev_apps/bookmarks-reader

Comment 7

[Cristian Rodriguez (:crdlc)]

Attachment: Github pull request

Comment 8

[Cristian Rodriguez (:crdlc)]

I removed the "see also" because it was not related to this one

Comment 9

[Kevin Grandon :kgrandon]

Comment on attachment 8441207 [details]
Github pull request

I think this is probably fine to land, I don't think this should be needed though.

Synchronize should only be needed if other apps modify bookmark content. Ideally the record should be removed and saved on click.

Comment 10

[Kevin Grandon :kgrandon]

This is landed, but I don't think we've fixed the root cause, though it may mask it: https://github.com/mozilla-b2g/gaia/commit/0b77e55360ee62f4fe593bcd09606c9e7bbb3e1b

Comment 11

[Cristian Rodriguez (:crdlc)]

I see that it works like this. When you click to remove the icon this is removed from grid and datastore as well. Maybe we have to study to update indexedDB too. Right?

(In reply to Kevin Grandon :kgrandon from comment #9)
> Comment on attachment 8441207 [details]
> Github pull request
> 
> I think this is probably fine to land, I don't think this should be needed
> though.
> 
> Synchronize should only be needed if other apps modify bookmark content.
> Ideally the record should be removed and saved on click.

Comment 12

[Kevin Grandon :kgrandon]

After the datastore removal, we should fall into this codepath: https://github.com/mozilla-b2g/gaia/blob/master/apps/verticalhome/js/sources/bookmark.js#L88

Which looks like it may not actually be hooked up to indexedDB anymore. Should've written tests for this stuff :)

Comment 13

[Diego Marcos [:dmarcos]]

If the root of the issue remains unfixed. Is there a bug to keep track of it?

Comment 14

[Kevin Grandon :kgrandon]

I am looking into it, but I assume that the fix will be the same as what we do in bug 1026467, so let's investigate there. In any case, this should no longer be an issue.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

v2.0: https://github.com/mozilla-b2g/gaia/commit/c72b9a0c77e7e48a7af89e12b09c4e2f58128fb1

Comment 16

[Johan Lorenzo [:jlorenzo]]

Verified on master.

Comment 17

[SandKing]

This issue has been verified successfully on Flame2.0&2.1
Verify video:"verify_1025441.mp4".

Flame2.0 build
Gaia-Rev        824a61cccec4c69be9a86ad5cb629a1f61fa142f
Gecko-Rev       https://hg.mozilla.org/mozilla-central/rev/acde07cb4e4d
Build-ID        20141125040209
Version         36.0a1
Device-Name     flame
FW-Release      4.4.2
FW-Incremental  eng.cltbld.20141125.113029
FW-Date         Tue Nov 25 11:30:43 EST 2014
Bootloader      L1TC00011880

Flame2.1 build:
Gaia-Rev        db2e84860f5a7cc334464618c6ea9e92ff82e9dd
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/211eae88f119
Build-ID        20141126001202
Version         34.0
Device-Name     flame
FW-Release      4.4.2
FW-Incremental  eng.cltbld.20141126.033519
FW-Date         Wed Nov 26 03:35:30 EST 2014
Bootloader      L1TC00011880

Comment 18

[SandKing]

Attachment: Verified video: verify_1025441.MP4