Closed Bug 983623 Opened 6 years ago Closed 6 years ago

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

Categories

(Firefox :: Bookmarks & History, defect)

x86
macOS
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 31
Tracking Status
firefox31 --- fixed

People

(Reporter: mano, Assigned: mano)

References

Details

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

Attachments

(3 files, 1 obsolete file)

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 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
Attachment #8391159 - Flags: review?(mak77) → review+
https://hg.mozilla.org/mozilla-central/rev/f36879704e4d
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Sigh, I missed the most important bit here... doCommand.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Status: REOPENED → ASSIGNED
Blocks: fxdesktopbacklog
No longer blocks: fxdesktoptriage
Whiteboard: p=0
Whiteboard: p=0 → p=3
No longer blocks: fxdesktopbacklog
Flags: firefox-backlog+
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
Attachment #8402565 - Flags: review?(mak77) → review+
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.
oops, i included PlacesUtils twice :(
https://hg.mozilla.org/mozilla-central/rev/c2a2aa6a47a3
https://hg.mozilla.org/mozilla-central/rev/98b10e3c554d
Status: ASSIGNED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
Hi Anthony, can you review to determine if this resolved bug requires further verification.  Thanks.
Flags: needinfo?(anthony.s.hughes)
Whiteboard: p=3 → p=3 s=it-31c-30a-29b.2
Mano, what manual testing does this need? Does this have automated tests? If not, does it need them? Thanks.
Flags: needinfo?(anthony.s.hughes) → needinfo?(mano)
Whiteboard: p=3 s=it-31c-30a-29b.2 → p=3 s=it-31c-30a-29b.2 [qa?]
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.
Flags: needinfo?(mano)
Okay, thanks for your help.
Whiteboard: p=3 s=it-31c-30a-29b.2 [qa?] → p=3 s=it-31c-30a-29b.2 [qa-]
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.