1460849 - Bookmark folder results unnecessarily force an invalidation of a view when excluding bookmarks but bookmarks are moved to the folder

Status: VERIFIED FIXED

(Toolkit :: Places, defect, P1)

Description

[Mark Banner (:standard8)]

STR

1. Set up a profile, create a couple of new folders at the same level in the bookmarks tree.
2. Add a lot of bookmarks to one of the folders (e.g. 300).
3. Now go into that folder, select all and drag & drop the bookmarks (as a move) to the other folder, timing how long it takes.
4. Now select the other folder, select all again, and drag back to the first folder, again timing.

=> At this stage, the timings will be about the same.

5. Double click on one of the folders in the left-pane list.
6. Repeat steps 3 & 4.

=> Notice that the time taken to drag to the folder that has been double clicked on has increased.

On my machine I was seeing up to a 5 times increase in length of time (~700ms -> ~3500 ms, though I did have another patch applied that speeds up the baseline times).

This type of effect can also be seen when dragging to any open folder, e.g. the other bookmarks if it is expanded.


The cause of this is relatively simple. When a folder is in the left-hand pane, the folder query has "excludeItems=1" set to exclude all bookmarks from it (as we don't want them displaying in the left pane).

Currently the folder query handler receives the "OnItemMoved" notification, and looks for the node in the folder. If it finds the node, and excludeItems is set and the node is not a uri (aka bookmark) nor separator, then it will ignore the result.

Unfortunately this isn't handling the case where a bookmark or separator is moved into the folder. As a result, it starts processing it, and does a full refresh of the folder.

Comment 1

[Mark Banner (:standard8)]

Attachment: Bug 1460849 - Allow new items (as well as existing) to be skipped in nsNavHistoryFolderResultNode::OnItemMoved if excludeItems is set.

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

Comment 2

[Marco Bonardo [:mak]]

Comment on attachment 8974971 [details]
Bug 1460849 - Allow new items (as well as existing) to be skipped in nsNavHistoryFolderResultNode::OnItemMoved if excludeItems is set.

https://reviewboard.mozilla.org/r/243356/#review249656

::: toolkit/components/places/nsNavHistoryResult.cpp:4002
(Diff revision 1)
>                 "Got a bookmark message that doesn't belong to us");
>  
>    RESTART_AND_RETURN_IF_ASYNC_PENDING();
>  
> +  bool excludeItems = mOptions->ExcludeItems();
> +  if (excludeItems && aItemType != nsINavBookmarksService::TYPE_FOLDER) {

What about a query? that's not TYPE_FOLDER, but it should be handled. The previous check was probably more correct, but I see you need to fin the node for that first.
Maybe we should add url to onItemMoved and check if it's a query, and if it is bypass this?

Comment 3

[Mark Banner (:standard8)]

Comment on attachment 8974971 [details]
Bug 1460849 - Allow new items (as well as existing) to be skipped in nsNavHistoryFolderResultNode::OnItemMoved if excludeItems is set.

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

Comment 4

[Marco Bonardo [:mak]]

Comment on attachment 8974971 [details]
Bug 1460849 - Allow new items (as well as existing) to be skipped in nsNavHistoryFolderResultNode::OnItemMoved if excludeItems is set.

https://reviewboard.mozilla.org/r/243356/#review250220

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e3c0dd79cbfc
Allow new items (as well as existing) to be skipped in nsNavHistoryFolderResultNode::OnItemMoved if excludeItems is set. r=mak

Comment 6

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/e3c0dd79cbfc

Comment 7

[Cristian Baica [:cbaica], Release Desktop QA]

I have managed to reproduce this issue using Firefox 62.0a1 buildID: 20180511100427 but with different STR.

1. Launch Firefox.2
2. Add 300 bookmarks using the following .js inside the stratchpad: https://gist.github.com/kitcambridge/ae61d4e8ce54db74d18379c5d8393258 (300 bookmars are added to the bookmark menu plus another folder inside the bookmark menu).
3. Create a new empty folder inside the bookmark menu.
4. Select the 300 created bookmarks from the bookmarkmenu and move them to the newly created folder (drag&drop).
5. Go inside the folder where the bookmarks were moved.
6. Select all bookmarks and move them back to the bookmark menu (drag&drop).

The issue is verified as fixed using Firefox 62.0b7 buildID: 20180709172241 on Windows 10 x64, macOS 10.14 and Ubuntu 16.04 LTS. The bookmarks move operation has greatly improved, reducing the wait time to 1-2 seconds for the 300 bookmarks move.