recent bookmarks reappear after deleting bookmark

RESOLVED FIXED in Firefox 57

Status

()

Firefox
Bookmarks & History
P1
normal
RESOLVED FIXED
2 months ago
a month ago

People

(Reporter: rasq37, Assigned: standard8)

Tracking

({regression})

57 Branch
Firefox 57
regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox55 unaffected, firefox56 unaffected, firefox57+ fixed)

Details

(Whiteboard: [fxsearch])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Reporter)

Description

2 months ago
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

Updated

2 months ago
Status: UNCONFIRMED → NEW
status-firefox55: --- → unaffected
status-firefox56: --- → unaffected
status-firefox57: --- → affected
status-firefox-esr52: --- → unaffected
tracking-firefox57: --- → ?
Component: Untriaged → Bookmarks & History
Ever confirmed: true
Keywords: regression

Comment 1

2 months ago
Regression window:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=3eb1b667b3dd0566c520a99a81b6de8f53cd2f2d&tochange=83dcb598eb6568f583ff6f6b524fbd2ec99ce122

Regressed by: Bug 1388687
Blocks: 1388687
(Assignee)

Comment 2

2 months ago
Thank you for the report! I think I know what I did wrong.
Assignee: nobody → standard8
Whiteboard: [fxsearch]

Updated

2 months ago
Blocks: 979280

Updated

2 months ago
Status: NEW → ASSIGNED
Priority: -- → P1
Comment hidden (mozreview-request)

Comment 4

2 months ago
mozreview-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

::: 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+
(Assignee)

Comment 5

2 months ago
mozreview-review-reply
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.
Comment hidden (mozreview-request)

Comment 7

2 months ago
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
Last Resolved: 2 months ago
status-firefox57: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57

Comment 9

2 months ago
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]
tracking-firefox57: ? → +

Comment 10

a month ago
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.