1397545 - recent bookmarks reappear after deleting bookmark

Status: RESOLVED FIXED

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

Description

[u601862]

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

Comment 1

[Alice0775 White]

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

Regressed by: Bug 1388687

Comment 2

[Mark Banner (:standard8)]

Thank you for the report! I think I know what I did wrong.

Comment 3

[Mark Banner (:standard8)]

Attachment: Bug 1397545 - Don't update the recently bookmarks item list when it is hidden and a bookmark is deleted.

Review commit: https://reviewboard.mozilla.org/r/177252/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/177252/

Comment 4

[Marco Bonardo [:mak]]

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?

Comment 5

[Mark Banner (:standard8)]

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 6

[Mark Banner (:standard8)]

Comment on attachment 8905437 [details]
Bug 1397545 - Don't update the recently bookmarks item list when it is hidden and a bookmark is deleted.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/177252/diff/1-2/

Comment 7

[Pulsebot]

Pushed by [e-mail]:
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

Comment 8

[Sebastian Hengst [:aryx] (needinfo on intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/3b1def512f64

Comment 9

[Anika Alam Raha]

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]

Comment 10

[Hani Yacoub]

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.

Comment 11

[Marco Bonardo [:mak]]

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.