993391 - Places async transactions: Implement "move bookmarks" command

Status: VERIFIED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[Asaf Romano (gone)]

Attachment: moveBookmarks.diff

Comment 1

[Marco Bonardo [:mak]]

Comment on attachment 8403261 [details] [diff] [review]
moveBookmarks.diff

Review of attachment 8403261 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/places/content/moveBookmarks.js
@@ +23,5 @@
>    },
>  
>    onOK: function MBD_onOK(aEvent) {
> +    let selectedNode = this.foldersTree.selectedNode;
> +    let selectedFolderID = PlacesUtils.getConcreteItemId(selectedNode);

please Id with lowercase d. that's what we usually did in the rest of the Places code...

@@ +46,4 @@
>      }
>  
> +    // Workaround for bug 993389.
> +    let nodes = this._nodes;

can't we workaround this in transact() method by using some other detection method instead of isGenerator()?
I don't like spreading workarounds all over the code if we can keep them in a single place

::: toolkit/components/places/PlacesTransactions.jsm
@@ +887,5 @@
>  /**
>   * Transaction for moving an item.
>   *
>   * Required Input Properties: GUID, newParentGUID, newIndex.
> + * Optional Input Properties  newIndex

how can newIndex be both required and optional?
also, missing colon.

test_async_transactions.js should be updated to verify the optional case (both that it's really optional, and the behavior when it's not provided)

Comment 2

[Asaf Romano (gone)]

http://stackoverflow.com/a/19660350

Comment 3

[Asaf Romano (gone)]

...on top of that, Task.jsm's spawn deliberately *synchronously* runs the supposedly generator-function up to the first yield. I guess I could do the same, and, if we don't get a generator, throw.

Comment 4

[Asaf Romano (gone)]

Attachment: patch

Comment 5

[Marco Bonardo [:mak]]

(In reply to Mano from comment #3)
> ...on top of that, Task.jsm's spawn deliberately *synchronously* runs the
> supposedly generator-function up to the first yield. I guess I could do the
> same, and, if we don't get a generator, throw.

Is this still the case now that Task.jsm moved to new Promise.jsm (it was still using the add-ons sdk on-same-tick version of promises before)? I mostly wonder if we can rely on that behavior to stick, I think so, but I couldn't find documentation of that. Regardless we can act as we wish, independently from it.

Comment 6

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Marco Bonardo [:mak] from comment #5)
> (In reply to Mano from comment #3)
> > ...on top of that, Task.jsm's spawn deliberately *synchronously* runs the
> > supposedly generator-function up to the first yield. I guess I could do the
> > same, and, if we don't get a generator, throw.
> 
> Is this still the case now that Task.jsm moved to new Promise.jsm (it was
> still using the add-ons sdk on-same-tick version of promises before)? I
> mostly wonder if we can rely on that behavior to stick, I think so, but I
> couldn't find documentation of that. Regardless we can act as we wish,
> independently from it.

Yeah it's still the case, the generator is executed immediately. I don't know if there is code explicitly relying on that behavior but I think it would be best not to do so if not too complicated.

Comment 7

[Asaf Romano (gone)]

I've deliberately tried to make PlacesTransactions.transact's async-related semantics as close to Task.spawn's as possible. So executing the generator imminently isn't a bad idea, imo.

Comment 8

[Asaf Romano (gone)]

i-m-m-e-d-i-a-t-e-l-y

Comment 9

[Marco Bonardo [:mak]]

Regardless, I don't think we must absolutely copy Task, if the alternative gives as advantages. in this case copying its behavior also gives us advantages, if Task changes we may not want to change and lose those advantages. The important part is that we properly document that.

Comment 10

[u279076]

Florin, please assign for verification in this iteration.

Comment 11

[Marco Bonardo [:mak]]

Comment on attachment 8405763 [details] [diff] [review]
patch

Review of attachment 8405763 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/places/content/moveBookmarks.js
@@ +46,3 @@
>      }
>  
> +    PlacesTransactions.transact(function*() {

nit: space after *

Comment 12

[Asaf Romano (gone)]

https://hg.mozilla.org/integration/fx-team/rev/8774539497c7

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8774539497c7

Comment 14

[Marco Mucci [:MarcoM]]

Hi Andrei, confirming if this resolved bug can be verified before the end of the desktop iteration on Monday April 28?

Comment 15

[Andrei Vaida [:avaida]]

(In reply to Marco Mucci [:MarcoM] from comment #14)
> Hi Andrei, confirming if this resolved bug can be verified before the end of
> the desktop iteration on Monday April 28?
Hi Marco, this bug is now verified, as per my notes below.

This issue is verified fixed on Nightly 31 (2014-04-23) using:
- Windows 7 64-bit [1],
- Ubuntu 12.04 32-bit [2],
- Mac OS X 10.9 [3].

One note though: the "Organize" menu from Nightly's Library under OS X 10.9.2, did not display an Undo or Redo option (among other options when compared to Linux or Windows) and I was only able to Undo the relocation of a bookmark using the keyboard shortcut, while cmd + Y (redo) had no effect.

1. Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:31.0) Gecko/20100101 Firefox/31.0
2. Mozilla/5.0 (X11; Linux i686; rv:31.0) Gecko/20100101 Firefox/31.0
3. Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Firefox/31.0

Comment 16

[Marco Bonardo [:mak]]

(In reply to Andrei Vaida, QA [:avaida] from comment #15)
> One note though: the "Organize" menu from Nightly's Library under OS X
> 10.9.2, did not display an Undo or Redo option (among other options when
> compared to Linux or Windows)

Is that a regression compared to the previous Nightly or even just compared to beta/aurora?

Comment 17

[Andrei Vaida [:avaida]]

(In reply to Marco Bonardo [:mak] from comment #16)
> Is that a regression compared to the previous Nightly or even just compared
> to beta/aurora?
This is not a regression, the same behavior can be encountered on the latest Beta, the latest Aurora, Nightly 29 (2014-01-02) or older builds - e.g. Nightly 13 (2012-02-01).

Comment 18

[Asaf Romano (gone)]

On OS X, undo/redo is in the standard edit menu.

Comment 19

[Asaf Romano (gone)]

And the shortcut for redo on OS X is Cmd+Shift+Z, always.

Comment 20

[Andrei Vaida [:avaida]]

(In reply to Mano from comment #19)
> And the shortcut for redo on OS X is Cmd+Shift+Z, always.
That's right, I apologize for the confusion. Cmd+Shift+Z works, as well as the Edit menu, I just wanted to make sure that the differences in terms of Organize menu options are expected for the Library. Thanks Mano!