Places async transactions: Implement "move bookmarks" command

VERIFIED FIXED in Firefox 31

Status

()

defect
VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: mano, Assigned: mano)

Tracking

Trunk
Firefox 31
Points:
---
Dependency tree / graph
Bug Flags:
firefox-backlog +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: p=2 s=it-31c-30a-29b.3 [qa!])

Attachments

(1 attachment, 1 obsolete attachment)

Posted patch moveBookmarks.diff (obsolete) — Splinter Review
No description provided.
Attachment #8403261 - Flags: review?(mak77)
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)
Attachment #8403261 - Flags: review?(mak77) → feedback+
Flags: firefox-backlog+
...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.
Posted patch patchSplinter Review
Attachment #8403261 - Attachment is obsolete: true
Attachment #8405763 - Flags: review?(mak77)
(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.
(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.
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.
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.
Whiteboard: p=2 s=it-31c-30a-29b.3 [qa?]
Florin, please assign for verification in this iteration.
QA Contact: florin.mezei
Whiteboard: p=2 s=it-31c-30a-29b.3 [qa?] → p=2 s=it-31c-30a-29b.3 [qa+]
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 *
Attachment #8405763 - Flags: review?(mak77) → review+
QA Contact: florin.mezei → andrei.vaida
https://hg.mozilla.org/mozilla-central/rev/8774539497c7
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Hi Andrei, confirming if this resolved bug can be verified before the end of the desktop iteration on Monday April 28?
Flags: needinfo?(andrei.vaida)
(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
Status: RESOLVED → VERIFIED
Flags: needinfo?(andrei.vaida)
OS: Mac OS X → All
Hardware: x86 → All
Whiteboard: p=2 s=it-31c-30a-29b.3 [qa+] → p=2 s=it-31c-30a-29b.3 [qa!]
(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?
(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).
On OS X, undo/redo is in the standard edit menu.
And the shortcut for redo on OS X is Cmd+Shift+Z, always.
(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!
You need to log in before you can comment on or make changes to this bug.