1887736 - no longer possible to delete more than one bookmark separator

Status: VERIFIED FIXED

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

Description

[Sören Hentzschel]

STR:

1. Make sure that you have more than one bookmark separator.
2. Open the library, make a right click on a bookmark separator and delete it.
3. Repeat step 2.

Actual:

Removing the separator works for the first one, but not for the second.

Expected:

Removing the separator always work.

Comment 1

[Marco Bonardo [:mak]]

thanks, there may be a bug in how the fingerprint is calculated (I suspect guid is not the right property to use here and url is wallpapering the bug).

Comment 2

[Marco Bonardo [:mak]]

Attachment: Bug 1887736 - Fix bookmark removal signature to use the proper guid properties. r=daisuke

nsINavHistoryResultNode.guid doesn't exist, so this always falls back to uris;
unfortunately separators don't have a uri.

Comment 3

[Dianna Smith [:diannaS]]

:mak can you set a severity on this? is this something we are going to want to uplift to beta and possibly the planned dot release?

Comment 4

[Marco Bonardo [:mak]]

Yeah sorry, I don't think it's super common to delete multiple consecutive separators as separate operations, but I think it would be nice to fix it as it's a pretty safe code change.
The workaround is also a bit awkward, user must delete something else between one separator and the next one.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e9f94a7954e1
Fix bookmark removal signature to use the proper guid properties. r=daisuke,places-reviewers

Comment 7

[Serban Stanca [:SerbanS]]

Backed out for causing mochitests failures in test_bug549192.xhtml.

• Backout link
• Push with failures
• Failure Log
• Failure line: TEST-UNEXPECTED-FAIL | browser/components/places/tests/chrome/test_bug549192.xhtml | Test timed out. -

Comment 8

[Marco Bonardo [:mak]]

The failure is because pageGuid and bookmarkGuid, according to the idl, may return an empty string instead of null. It doesn't make a lot of sense, but I also don't want to take risks here changing that detail, as I'd like to uplift the fix.
I'll just use ||, instead of the nullish coalesce operator, that will handle empty strings correctly.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9c458764557d
Fix bookmark removal signature to use the proper guid properties. r=daisuke,places-reviewers

Comment 10

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/9c458764557d

Comment 11

[Marco Bonardo [:mak]]

Attachment: Bug 1887736 - Fix bookmark removal signature to use the proper guid properties. r=daisuke

nsINavHistoryResultNode.guid doesn't exist, so this always falls back to uris;
unfortunately separators don't have a uri.

Original Revision: https://phabricator.services.mozilla.com/D205716

Comment 12

[Phabricator Automation]

Uplift Approval Request

• Fix verified in Nightly: no
• Steps to reproduce for manual QE testing: Steps are well explained in the bug opening comment, see also Bug 1887989.
• Needs manual QE test: yes
• Risk associated with taking this patch: low
• User impact if declined: Removing multiple separators consecutively, or removing multiple bookmarks with the same uri silently fails
• String changes made/needed: no
• Is Android affected?: no
• Explanation of risk level: Simple change with good test coverage
• Code covered by automated testing: yes

Comment 13

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-beta/rev/0ded3257fc15

Comment 14

[Bianca Hidecuti, Desktop Test Engineering [:bhidecuti]]

I was able to reproduce the issue on an affected Firefox Nightly build 126.0a1 20240326164915 on macOS 12.6.6.
Verified the issue as fixed on Firefox Nightly 126.0a1 build ID 20240328213634 / Firefox Beta 125.0b6 build ID 20240328143539, from Treeherder, on macOS 12.6.6, Windows 11 and Ubuntu 22.04.
Now it is possible to remove multiples bookmark separators.
I have also verified as fixed the scenarios specified in the duplicate https://bugzilla.mozilla.org/show_bug.cgi?id=1887989#c5 in all the above mentioned OSes.