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

VERIFIED FIXED in Firefox OS v2.0

Status

Firefox OS
Gaia::Homescreen
VERIFIED FIXED
4 years ago
2 years ago

People

(Reporter: dmarcos, Assigned: crdlc)

Tracking

unspecified
2.0 S4 (20june)
ARM
Gonk (Firefox OS)

Firefox Tracking Flags

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

Details

(Whiteboard: [systemsfe])

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

4 years ago
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
(Reporter)

Comment 1

4 years ago
Created attachment 8440258 [details] [review]
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.
Attachment #8440258 - Flags: feedback?(kgrandon)
Flags: needinfo?(kgrandon)
(Reporter)

Updated

4 years ago
Assignee: nobody → dmarcos
(Reporter)

Comment 2

4 years ago
Comment on attachment 8440258 [details] [review]
Pull request

Sorry PR doesn't belong to this bug
Attachment #8440258 - Flags: feedback?(kgrandon)
(Reporter)

Updated

4 years ago
Assignee: dmarcos → nobody
(Reporter)

Updated

4 years ago
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)

Updated

4 years ago
Blocks: 989848
blocking-b2g: --- → 2.0?
QA Whiteboard: [VH-FL-blocking+][VH-FC-blocking+]
Whiteboard: [systemsfe]

Updated

4 years ago
Blocks: 1015336
No longer blocks: 989848
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: → bug 1026265
(Assignee)

Comment 5

4 years ago
I gonna take a look at this today
(Assignee)

Updated

4 years ago
Assignee: nobody → crdlc
Status: NEW → ASSIGNED
(Assignee)

Comment 6

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

Comment 7

4 years ago
Created attachment 8441207 [details]
Github pull request
Attachment #8441207 - Flags: review?(kgrandon)
(Assignee)

Updated

4 years ago
See Also: bug 1026265
(Assignee)

Comment 8

4 years ago
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
Last Resolved: 4 years ago
Resolution: --- → FIXED
(Assignee)

Comment 11

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

Comment 13

4 years ago
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
v2.0: https://github.com/mozilla-b2g/gaia/commit/c72b9a0c77e7e48a7af89e12b09c4e2f58128fb1
status-b2g-v2.0: --- → fixed
status-b2g-v2.1: --- → fixed
Verified on master.
Status: RESOLVED → VERIFIED
Flags: in-moztrap?(jlorenzo)

Comment 17

3 years ago
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
status-b2g-v2.0: fixed → verified
status-b2g-v2.1: fixed → verified

Comment 18

3 years ago
Created attachment 8529473 [details]
Verified video: verify_1025441.MP4
Flags: in-moztrap?(jlorenzo)
You need to log in before you can comment on or make changes to this bug.