1413843 - Cut/Copy/Move operations involving the left pane of the library are typically very slow due to non-batching

Status: VERIFIED FIXED

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

Description

[Mark Banner (:standard8)]

Some of the cut/copy/move operations involving the left pane are typically very slow due to currently being non-batching.

The basic issue is that the view the code has, means applying batching on the left-pane, rather than (or as well as) for the right-pane.

STR a:

1) Set up a folder with 300 bookmarks.
2) Drag all of those bookmarks to a different folder on the left-hand pane, as a move.

STR b:

1) Set up a folder with 300 bookmarks.
2) Select all bookmarks, right-click and select Cut.
3) Right-click on a folder in the left-hand pane and select Paste.

In both cases you can see the UI being updated for each bookmark in turn, rather than one single update after the moves are actually complete.

Comment 1

[Mark Banner (:standard8)]

I have a patch for this that switches us to use the right hand pane to notify for batching purposes if we detect we've been passed the left-hand pane.

It needs bug 1410940 first, as it is affecting some of the code there.

Comment 2

[Mark Banner (:standard8)]

Attachment: Bug 1413843 - Improve speed of operations when pasting to the left-hand library pane by getting the right-hand pane to use for batching.

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

Comment 3

[Mark Banner (:standard8)]

(note: this depends on the patches in bug 1410940 which are now on autoland).

Comment 4

[Marco Bonardo [:mak]]

Comment on attachment 8924920 [details]
Bug 1413843 - Improve speed of operations when pasting to the left-hand library pane by getting the right-hand pane to use for batching.

https://reviewboard.mozilla.org/r/196188/#review201672

::: browser/components/places/content/controller.js:1331
(Diff revision 1)
>            items, insertionIndex, ip.guid, doCopy);
>          if (newTransactions.length) {
>            transactions = [...transactions, ...newTransactions];
>          }
>  
> -        await PlacesUIUtils.batchUpdatesForNode(this._view.result, transactions.length, async () => {
> +        // Note: this._view is a tree element.

couldn't the view be a menupopup or the toolbar? the controller is view agnostic.

Comment 5

[Mark Banner (:standard8)]

Comment on attachment 8924920 [details]
Bug 1413843 - Improve speed of operations when pasting to the left-hand library pane by getting the right-hand pane to use for batching.

https://reviewboard.mozilla.org/r/196188/#review201672

> couldn't the view be a menupopup or the toolbar? the controller is view agnostic.

Yeah, you're right. I think we can still use the code almost as it is, I've updated the comment though, and improved the function.

Comment 6

[Mark Banner (:standard8)]

Comment on attachment 8924920 [details]
Bug 1413843 - Improve speed of operations when pasting to the left-hand library pane by getting the right-hand pane to use for batching.

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

Comment 7

[Marco Bonardo [:mak]]

Comment on attachment 8924920 [details]
Bug 1413843 - Improve speed of operations when pasting to the left-hand library pane by getting the right-hand pane to use for batching.

https://reviewboard.mozilla.org/r/196188/#review201914

sorry if it took a little bit, rusty memory and the controller code is not the clearer.
If you want a second look after the changes, I'm happy to, but there is nothing critical, mostly naming and comments.

::: browser/components/places/content/controller.js:1331
(Diff revision 2)
>            items, insertionIndex, ip.guid, doCopy);
>          if (newTransactions.length) {
>            transactions = [...transactions, ...newTransactions];
>          }
>  
> -        await PlacesUIUtils.batchUpdatesForNode(this._view.result, transactions.length, async () => {
> +        // Note: this._view may be a view or an element.

"or the tree element."

::: browser/components/places/content/controller.js:1332
(Diff revision 2)
>          if (newTransactions.length) {
>            transactions = [...transactions, ...newTransactions];
>          }
>  
> -        await PlacesUIUtils.batchUpdatesForNode(this._view.result, transactions.length, async () => {
> +        // Note: this._view may be a view or an element.
> +        let treeElement = ensureRightPaneTreeIfPossible(this._view);

let's just change to:
let resultForBatching = getResultForBatching(this._view);

and document the choice in the javadoc of that method. It should clarify that in case multiple views are open, the method picks the most expensive view to update.
This is actually a limitation of this batching approach, we'll overcome it with our new notifications system (hopefully).

::: browser/components/places/content/controller.js:1620
(Diff revision 2)
>     * @param {Object} dt             The dataTransfer information for the drop.
> -   * @param {Object} view           The tree view where this object is being
> +   * @param {Object} treeElement    The tree element where this object is being
>     *                                dropped to. This allows batching to take
>     *                                place.
>     */
> -  async onDrop(insertionPoint, dt, view) {
> +  async onDrop(insertionPoint, dt, treeElement) {

let's leave this named view.
We already have the mess where this._view may be a view of the tree element, this will just be the same. You can fix the javadoc similarly to the above comment about this._view.

::: browser/components/places/content/controller.js:1731
(Diff revision 2)
>      if (!transactions.length) {
>        return;
>      }
>      if (PlacesUIUtils.useAsyncTransactions) {
> -      await PlacesUIUtils.batchUpdatesForNode(view && view.result, transactions.length, async () => {
> +      let element = ensureRightPaneTreeIfPossible(treeElement);
> +      await PlacesUIUtils.batchUpdatesForNode(element && element.result,

ditto about the rename

::: browser/components/places/content/controller.js:1823
(Diff revision 2)
>  
>  /**
> + * Attempts to ensure that we have a right-hand pane selected. If it detects
> + * the left-hand pane, then it will look for and return the reference to the
> + * right-hand pane. We use positive identification here, as the passed
> + * in item could be a view or something else.

This could be a little bit more generic like "in case multiple views are related, the method returns the most expensive result to batch. For example ...(the Library left/right pane example)...

::: browser/components/places/content/controller.js:1827
(Diff revision 2)
> + * right-hand pane. We use positive identification here, as the passed
> + * in item could be a view or something else.
> + *
> + * @param {Object} viewOrElement The item to check.
> + * @return {Object} The passed in item if unchanged, or the tree
> + *                  associated with the right-hand pane ("placeContent").

could also be a bit more generic, will just return the best result to batch.

::: browser/components/places/content/controller.js:1830
(Diff revision 2)
> + * @param {Object} viewOrElement The item to check.
> + * @return {Object} The passed in item if unchanged, or the tree
> + *                  associated with the right-hand pane ("placeContent").
> + */
> +function ensureRightPaneTreeIfPossible(viewOrElement) {
> +  if (viewOrElement && ("id" in viewOrElement) && viewOrElement.id === "placesList") {

maybe we could first of all check if it's an element... can we use instanceof Ci.nsIDOMElement?

Comment 8

[Mark Banner (:standard8)]

Comment on attachment 8924920 [details]
Bug 1413843 - Improve speed of operations when pasting to the left-hand library pane by getting the right-hand pane to use for batching.

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

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a695ef964398
Improve speed of operations when pasting to the left-hand library pane by getting the right-hand pane to use for batching. r=mak

Comment 10

[Eliza Balazs [:ebalazs_]]

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

Comment 11

[Oana Botisan, Desktop QA]

I managed to reproduce the bug using an older version of Nightly (2017-10-02) on Windows 10 x64.
Then I retested on latest Nightly and beta 58.b4 using Windows 10 x64, Ubuntu 16.04 x64 and macOS 10.13, but I can still see the bug reproducing when you click between folders using the drag and drop method. 
Can you please make a screen cast of the issue exactly, just to be sure that I am looking for the right thing.
Thank you.

Comment 12

[Mark Banner (:standard8)]

(In reply to Oana Botisan from comment #11)
> I managed to reproduce the bug using an older version of Nightly
> (2017-10-02) on Windows 10 x64.
> Then I retested on latest Nightly and beta 58.b4 using Windows 10 x64,
> Ubuntu 16.04 x64 and macOS 10.13, but I can still see the bug reproducing
> when you click between folders using the drag and drop method. 

Can you explain what you mean by click between folders? If you drag and drop, and then click on different folders whilst the drag is completing, I think it will revert to filling the folders gradually rather in one block.

This bug was specifically about dragging and dropping, but then waiting for the move to complete.

Clicking between folders whilst the move is ongoing is something we'll have to address later. Bug 1404909 is going to reduce the amount of time it takes (so less likely for the user to see it), but we're also going to be redoing the notification system early next year, which should improve the changing views whilst drop is in progress issue (if it is still taking a long time).

Comment 13

[Oana Botisan, Desktop QA]

Alright, thank you for the information. That was exactly what I meant with clicking between folders. While the move is in progress I clicked between the folder that I was dragging from and the folder I pasted the bookmarks to. 
In this case I think I can close the bug as verified, because if you wait for the move to complete, the bug is not reproducing anymore on any of the platforms I mentioned before both on latest Nightly and beta 58.0b4.