Closed Bug 1025441 Opened 5 years ago Closed 5 years ago

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

Categories

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

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(blocking-b2g:2.0+, b2g-v2.0 verified, b2g-v2.1 verified)

VERIFIED FIXED
2.0 S4 (20june)
blocking-b2g 2.0+
Tracking Status
b2g-v2.0 --- verified
b2g-v2.1 --- verified

People

(Reporter: dmarcos, Assigned: crdlc)

References

Details

(Whiteboard: [systemsfe])

Attachments

(2 files, 1 obsolete file)

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
Attached file Pull request (obsolete) —
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.
Attachment #8440258 - Flags: feedback?(kgrandon)
Flags: needinfo?(kgrandon)
Assignee: nobody → dmarcos
Comment on attachment 8440258 [details] [review]
Pull request

Sorry PR doesn't belong to this bug
Attachment #8440258 - Flags: feedback?(kgrandon)
Assignee: dmarcos → nobody
Attachment #8440258 - Attachment is obsolete: true
I might look into this, but I think the ni? was for the other patch :) Thanks!
Flags: needinfo?(kgrandon)
blocking-b2g: --- → 2.0?
QA Whiteboard: [VH-FL-blocking+][VH-FC-blocking+]
Whiteboard: [systemsfe]
Blocks: 1015336
No longer blocks: vertical-homescreen
blocking-b2g: 2.0? → 2.0+
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.
See Also: → 1026265
I gonna take a look at this today
Assignee: nobody → crdlc
Status: NEW → ASSIGNED
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
Attached file Github pull request
Attachment #8441207 - Flags: review?(kgrandon)
See Also: 1026265
I removed the "see also" because it was not related to this one
Target Milestone: --- → 2.0 S4 (20june)
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.
Attachment #8441207 - Flags: review?(kgrandon) → review+
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
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
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.
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 :)
If the root of the issue remains unfixed. Is there a bug to keep track of it?
Flags: needinfo?(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.
Flags: needinfo?(kgrandon)
Depends on: 1026467
No longer depends on: 1026467
Verified on master.
Status: RESOLVED → VERIFIED
Flags: in-moztrap?(jlorenzo)
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
Flags: in-moztrap?(jlorenzo)
You need to log in before you can comment on or make changes to this bug.