984898 - Places async transactions: Implement "new separator" ui command

Status: VERIFIED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[Asaf Romano (gone)]

This is the easiest one, I think.

Comment 1

[Liz Henry (:lizzard) (relman/hg->git project)]

Hi Mano, can you explain this bug a little more? I'd like to figure out if we should be doing some QA to verify this once it's fixed, and if so, what it means and how to verify it. Thanks!

Comment 2

[Asaf Romano (gone)]

Hey Liz,

This work as a whole can only be tested once it's completed (meaning that the QA should probably be done on the bug in which the feature will be turned on). The individual bugs may be used as hints for what needs to be tested (For example, here we're going to re-implement the New Separator command, accessible for places context menus and the places library).

In the short term, no QA is necessary.

Comment 3

[Asaf Romano (gone)]

Attachment: patch.diff

Comment 4

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 8402606 [details] [diff] [review]
patch.diff

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

Ok, but we need either a test or a test plan.

::: browser/components/places/content/controller.js
@@ +282,5 @@
>      case "placesCmd_new:livemark":
>        this.newItem("livemark");
>        break;
>      case "placesCmd_new:separator":
> +      Task.spawn(this.newSeparator.bind(this)).then(null, Cu.reportError);

I think all of this is not needed with Task.async, it's basically equivalent to calling a method that returns Task.spawn, with an already bound this and with the given argument. So here you should just be able to do this.newSeparator().then(null, Cu.reportError);

@@ +789,5 @@
> +    }
> +
> +    let guid = yield PlacesTransactions.transact(
> +      PlacesTransactions.NewSeparator({ parentGUID: yield ip.promiseGUID()
> +                                      , index: ip.index }));

nit: I'd prefer a temp var assignment to improve readability here.

@@ +790,5 @@
> +
> +    let guid = yield PlacesTransactions.transact(
> +      PlacesTransactions.NewSeparator({ parentGUID: yield ip.promiseGUID()
> +                                      , index: ip.index }));
> +    let itemID = yield PlacesUtils.promiseItemID(guid);

This should be itemId and promiseItemId (lowercase d)... there's no promiseItemID method, so this is broken.

and this makes me think we probably want a new mochitests folder checking these commands do actually work...

Comment 5

[Asaf Romano (gone)]

It seems we don't have tests for places UI commands (that actually perform goDoCommand and so on). I'll file a bug for tests coverage.

Comment 6

[Asaf Romano (gone)]

https://hg.mozilla.org/integration/fx-team/rev/593d57a1e4c1

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/593d57a1e4c1

Comment 8

[u279076]

Tagged [qa-] based on comment 2.