983623 - Async transactions: Add a preference for turning it, implement undo & redo commands

Status: VERIFIED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[Asaf Romano (gone)]

While reading the browser/ part, keep in mind that my goal is to have the final "remove old PTM" patch a code-removal-only patch (well, as much as possible).

Comment 1

[Asaf Romano (gone)]

Attachment: implementUndoRedo.diff

Comment 2

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

Comment on attachment 8391159 [details] [diff] [review]
implementUndoRedo.diff

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

r=me with these comments addressed

::: browser/components/places/content/controller.js
@@ +123,5 @@
>      return (aCommand.substr(0, CMD_PREFIX.length) == CMD_PREFIX);
>    },
>  
>    isCommandEnabled: function PC_isCommandEnabled(aCommand) {
> +    if (PlacesUIUtils.asyncTransactions) {

i initially though this was a shortcut to the transactions module... what about .useAsyncTransactions instead?

::: browser/components/places/src/PlacesUIUtils.jsm
@@ +1023,5 @@
>  });
>  
> +XPCOMUtils.defineLazyGetter(PlacesUIUtils, "asyncTransactions", function() {
> +  try {
> +    return Services.prefs.getBoolPref("browser.places.asyncTransactions");

ditto, also useAsyncTransactions in the pref.

::: toolkit/components/places/PlacesTransactions.jsm
@@ +185,5 @@
>          return;
>        }
>      }
>      this._undoPosition++;
> +    //updateCommandsOnActiveWindow();

why is this commented out?

@@ +214,5 @@
>          return;
>        }
>      }
>      this._undoPosition--;
> +    //updateCommandsOnActiveWindow();

ditto

::: toolkit/components/places/tests/unit/test_async_transactions.js
@@ +121,2 @@
>  
> +  let check_entry_throws = (f) => {

no need to wrap f with parentheses

Comment 3

[Asaf Romano (gone)]

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

Comment 4

[Phil Ringnalda (:philor)]

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

Comment 5

[Asaf Romano (gone)]

Sigh, I missed the most important bit here... doCommand.

Comment 7

[Asaf Romano (gone)]

Attachment: the missing bits

Comment 8

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

Comment on attachment 8402565 [details] [diff] [review]
the missing bits

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

r=me with the following comments addressed.

What's the testing plan for this, shouldn't we have a mochitest checking it's working? do you have a plan to convert existing tests later?

::: browser/components/places/content/controller.js
@@ +225,5 @@
>      case "cmd_undo":
> +      if (!PlacesUIUtils.useAsyncTransactions)
> +        PlacesUtils.transactionManager.undoTransaction();
> +
> +      PlacesTransactions.undo().then(null, Cu.reportError);

shouldn't you if/else, or early bailout? as it is, it's going to execute both...

@@ +231,5 @@
>      case "cmd_redo":
> +      if (!PlacesUIUtils.useAsyncTransactions)
> +        PlacesUtils.transactionManager.redoTransaction();
> +
> +      PlacesTransactions.redo().then(null, Cu.reportError);

ditto

Comment 9

[Asaf Romano (gone)]

I'm going to file a bug for fixing the current set of tests to work in this environment (the problem here is that doCommand will work asynchronously, but it cannot return a promise or call a callback) and add tests for anything that's missing. I'm going to do all of that in a single bug though, and only after we're done with the very initial implementation.

Comment 10

[Asaf Romano (gone)]

https://hg.mozilla.org/integration/fx-team/rev/3fbf2620c5de

Comment 11

[Carsten Book [:Tomcat]]

(In reply to Mano from comment #10)
> https://hg.mozilla.org/integration/fx-team/rev/3fbf2620c5de

sorry had to back this out for problems like https://tbpl.mozilla.org/php/getParsedLog.php?id=37422304&tree=Fx-Team

Comment 12

[Asaf Romano (gone)]

Attachment: fixed patch

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

Comment 13

[Asaf Romano (gone)]

oops, i included PlacesUtils twice :(

Comment 14

[Asaf Romano (gone)]

Attachment: fix that error

https://hg.mozilla.org/integration/fx-team/rev/98b10e3c554d

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c2a2aa6a47a3
https://hg.mozilla.org/mozilla-central/rev/98b10e3c554d

Comment 16

[Marco Mucci [:MarcoM]]

Hi Anthony, can you review to determine if this resolved bug requires further verification.  Thanks.

Comment 17

[u279076]

Mano, what manual testing does this need? Does this have automated tests? If not, does it need them? Thanks.

Comment 18

[Asaf Romano (gone)]

As I said in few other places, there's nothing worth manual testing until the initial implementation is complete (that is, the entire UI feature set is reimplemented with the new async TM). We're not there yet (the work is tracked in bug 979280).

Before enabling the feature on trunk we're going to introduce some automated tests for the UI actions which are affected by the new TM. I'm going to file a bug tracking that work soon.

Comment 19

[u279076]

Okay, thanks for your help.