1398753 - If a page is bookmarked multiple times it has to be removed just as many times in order to fully remove it

Status: RESOLVED WONTFIX

(Firefox :: New Tab Page, defect, P4)

Description

[Ciprian Muresan [:cmuresan], Ecosystem QA]

Attachment: not all bookmarks of the same page removed.gif

[Affected versions]:
- Firefox 57.0a1 Build ID 20170910220126

[Affected Platforms]:
- All Windows
- All Mac
- All Linux

[Prerequisites]:
- browser.newtabpage.activity-stream.enabled is set to true in about:config on a clean new profile.
- Have enough history so that all 12 Top Sites are populated.

[Steps to reproduce]:
1. Start the browser with the profile from prerequisites.
2. Navigate to a website and bookmark it in three locations.
3. Open a New Tab and hover the first Highlights card.
4. Remove it as a bookmark.
5. Refresh the page and observe the card's state.

[Expected results]:
- Card's status is "Visited".

[Actual results]:
- Card's status is "Bookmarked".

[Notes]:
- Attached a screen recording of the issue.

Comment 1

[Quentin Headen [:qheaden]]

Hello. I actually can reproduce this issue myself. I am interested in taking this bug. What exactly is the desired behavior here? When "Remove Bookmark" is clicked, should all bookmark instances with that URL be removed?

Thanks!

Comment 2

[Dan Mosedale (:dmosedale, :dmose)]

Great, thanks, Quentin!  I've assigned it to you; I hope that's ok.

According to https://github.com/mozilla/activity-stream/issues/3408#issuecomment-328617228 the desired behavior is to simply remove all of the references to the bookmark itself (i.e. it would disappear from the menus shown in the gif).

Comment 3

[Dan Mosedale (:dmosedale, :dmose)]

Quentin, https://github.com/mozilla/activity-stream/blob/master/docs/v2-system-addon/1.GETTING_STARTED.md is a good place to start if you haven't done activity stream work before.  If you need anything feel free to ask here or in irc (irc.mozilla.org / #activity-stream).

Comment 4

[Quentin Headen [:qheaden]]

I'm still working my way around learning the code. It is coming together for me. When I do actually make a patch for this bug, should the patch be made here, or should a pull request be made in GitHub since the activity stream project lives there?

Thanks!

Comment 5

[Dan Mosedale (:dmosedale, :dmose)]

Great!  As a github pull request.  We export the github changes to mozilla-central regularly.

Comment 6

[Cory Goodwin]

This issue is quite old and I was able to reproduce it still. I'm interested in taking responsibility of this if possible as a good first bug to tackle. I'll go ahead and look into a possible solution whenever I can to see what I can contribute, but let me know if this is being handled.

Comment 7

[Ciprian Muresan [:cmuresan], Ecosystem QA]

@Scott, do you know if this issue would be considered as a good first bug? And could you help Cory out with some instructions if it's safe to be taken over?

Comment 8

[Scott [:thecount] Downe]

Hm, that's a good question. Not something I'm super familiar with. From my testing, this looks like it might actually be an issue in the places/bookmarking state management, which I don't think is in the newtab component? A bit of looking around, seems to suggest this is the bulk of the logic for this?

https://searchfox.org/mozilla-central/source/toolkit/components/places/Bookmarks.jsm#1233

This looks to be where we do the removal, and where we have the issue:
https://searchfox.org/mozilla-central/source/toolkit/components/places/Bookmarks.jsm#1277

Seems like it might be a good starter. It looks like to me if we want to fix this, the remove bookmark needs to remove all instances of the id, and not stopping once it finds one, based on the above links.

Ed, thoughts?

Comment 9

[Cory Goodwin]

Just wanted to add that I'm still interested in fixing this bug and I'm following the responses made here. I haven't gotten around to looking into it yet but I will be soon to see if I can produce a fix, even if it isn't accepted (personally I think this may be working as intended).

Comment 10

[Cory Goodwin]

Following up on this issue. I looked into the code further and this definitely feels intentional. In the Bookmarks.jsm file mentioned by Scott, the Remove Bookmark function(s) start at line 1213. There, it states that each item (bookmark) has a globally unique ID. It seems to me that this is the case even for the same URL (the URL portion of the bookmark entry is optional, in order to be able to remove bookmarks with invalid URLs as described in that section of the code, anyways), which makes me believe that it's this way by design. If I were to have several folders which I right clicked to open all at once for productivity purposes for example, one website may exist in several different folders to fit different categories. Hence, removing one may actually be more of a hindrance than it's worth, and the solution to this for someone who doesn't need to bookmark the site more than once is to simply not do it.

I may dig deeper and propose changes to the code for individual users to make if they wish (or develop an add-on or such that makes the change for them), but I don't know if this kind of feature would be desired enough to even warrant that. As such, I think the issue should be closed, but I'm just an amateur so I can't make the judgement on that. Thoughts?

Comment 11

[Scott [:thecount] Downe]

I think closing it is a reasonable action given the above.

Reopening it, in case we change our minds or have reason to believe we should make changes after all, is super easy, so there isn't much harm in closing this atm.