Closed Bug 1397545 Opened 7 years ago Closed 7 years ago

recent bookmarks reappear after deleting bookmark

Categories

(Firefox :: Bookmarks & History, defect, P1)

57 Branch
defect

Tracking

()

RESOLVED FIXED
Firefox 57
Tracking Status
firefox-esr52 --- unaffected
firefox55 --- unaffected
firefox56 --- unaffected
firefox57 + fixed

People

(Reporter: u601862, Assigned: standard8)

References

Details

(Keywords: regression, Whiteboard: [fxsearch])

Attachments

(1 file)

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:57.0) Gecko/20100101 Firefox/57.0
Build ID: 20170904220027

Steps to reproduce:

In about:config, I disabled browser.bookmarks.showRecentlyBookmarked. Then I deleted a bookmark.


Actual results:

After deleting the bookmark, 'recently bookmarked' reappeared in the bookmarks menu. Closing/reopening menu makes it disappear again.


Expected results:

'recently bookmarked' stays disabled
Status: UNCONFIRMED → NEW
Component: Untriaged → Bookmarks & History
Ever confirmed: true
Keywords: regression
Thank you for the report! I think I know what I did wrong.
Assignee: nobody → standard8
Whiteboard: [fxsearch]
Status: NEW → ASSIGNED
Priority: -- → P1
Comment on attachment 8905437 [details]
Bug 1397545 - Don't update the recently bookmarks item list when it is hidden and a bookmark is deleted.

https://reviewboard.mozilla.org/r/177252/#review182262

::: browser/base/content/browser-places.js:1479
(Diff revision 1)
>     */
>    onItemRemoved(itemId, parentId, index, itemType, uri, guid) {
>      // Update the menu when a bookmark has been removed.
>      // The native menubar on Mac doesn't support live update, so this is
>      // unlikely to be called there.
> -    if (this._recentGuids.size == 0 ||
> +    if (this.visible &&

I suspect an early bailout like:
if (!this.visible)
  return;
would keep this second if a bit more readable.

::: browser/base/content/browser-places.js:1480
(Diff revision 1)
>    onItemRemoved(itemId, parentId, index, itemType, uri, guid) {
>      // Update the menu when a bookmark has been removed.
>      // The native menubar on Mac doesn't support live update, so this is
>      // unlikely to be called there.
> -    if (this._recentGuids.size == 0 ||
> -        (guid && this._recentGuids.has(guid))) {
> +    if (this.visible &&
> +        (this._recentGuids.size == 0 ||

Could you please clarify in a comment why we act if this._recentGuids.size == 0?
Doesn't that mean the thing has not been initialized and thus we should not care about removal notifications?
Attachment #8905437 - Flags: review?(mak77) → review+
Comment on attachment 8905437 [details]
Bug 1397545 - Don't update the recently bookmarks item list when it is hidden and a bookmark is deleted.

https://reviewboard.mozilla.org/r/177252/#review182262

> Could you please clarify in a comment why we act if this._recentGuids.size == 0?
> Doesn't that mean the thing has not been initialized and thus we should not care about removal notifications?

I think you're right, we don't need this - I'm not sure why I put it in there, but regardless of the size of recentGuids, there's no point in calculating if an item wasn't removed within it.

Hence, I've just removed the additional check.
Pushed by mbanner@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/3b1def512f64
Don't update the recently bookmarks item list when it is hidden and a bookmark is deleted. r=mak
https://hg.mozilla.org/mozilla-central/rev/3b1def512f64
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
I have reproduced this bug with Nightly 57.0a1 (2017-09-06) on Windows 8, 64-bit.

The bug's fix is now verified on latest nightly

Build ID 	20170907220212
User Agent 	Mozilla/5.0 (Windows NT 6.3; Win64; x64; rv:57.0) Gecko/20100101 Firefox/57.0


[bugday-20170906]
I was trying to verify this bug on the latest nightly 58.0a1 but I couldn't find "browser.bookmarks.showRecentlyBookmarked" pref in about:config.

Is there any pref that I could use?
Thanks.
Flags: needinfo?(standard8)
We removed the recent bookmarks list from the bookmarks menu a bit later this bug was fixed, because now the Library menu panel shows recent bookmarks, thus there's no point in keeping them in 2 places.
Basically this bug cannot exist anymore.
Flags: needinfo?(standard8)
You need to log in before you can comment on or make changes to this bug.