891303 - Async-friendly transaction manager for Places (back-end)

Status: VERIFIED FIXED

(Toolkit :: General, defect)

Description

[Asaf Romano (gone)]

Following the old bookmarks system, Places has been using the editor transaction manager since ever. Some xpconnect issues aside, this worked for us pretty well until now.

However this setup now blocks the transition to asynchronous places APIs. There are few issues:
1) nsITransaction.doTransaction is expected to commit the transaction synchronously, so that calling undoTransaction. Note that this issue by itself *could* be workaround in the implementation for each transaction.
2) Allowing transactions to actually run in parallel may result in unexpected results from the user POV. Consider adding two bookmarks, the first with some annotations, the second without: The second may be done before the first one, leading to incorrect bookmarks order
3) Some transactions depend on the result of previous transaction. Consider "Bookmark All Tabs": first we need to create the folder, then we need to create items under that folder. However, because the call to createFolder is "hidden" by doTransaction, we cannot tell the folder id needed for the second step.

So, given all that, and given that Promises is such a nice api, and given that an async-freindly transaction manager may be useful elsewhere (devtools for example), I think it's time to introduce a non-XPCOM transaction manager implementation in toolkit.

My plan is to initially port over the editor TM almost as-is (in API terms) and switch places to use the new implementation. Then we will discuss the exact async API changes necessary. It should be quite straightforward.

Comment 1

[Asaf Romano (gone)]

Attachment: port the old tm to a JS module and use it for places

This actually works!

Comment 2

[Asaf Romano (gone)]

Not surprisingly this is more complicated than more than I thought, but I think it's worth the time. Let me explain.

Over the last few days I evaluated the following issues.

(1) Are we really in trouble?

At this point I can confidently say that it won't be possible to remove the synchronous bookmarks and/or annotations API without resolving the asynchronous transactions issue. It's also an issue for transactions that wish to use some asynchronous APIs (bug 885246).

Luckily most callers in browser/ won't need much of a refactoring (we're probably going to break addons though), so the refactoring is scoped to PlacesUtils,

All in all, this is now a P1 for me, preceding any new asynchronous places API work, because new APIs that cannot be used are not all that useful.

(2) Could we just wrap the current transaction manager (the one provided by editor) in PlacesUtils in some way that would add support for asynchronous transactions?

We cannot. The major (but not the only) reason being that it is the manger itself that calls nsITransaction.doTransaction, and it clearly expects the transaction to be fully processed by the time doTransaction returns. Furthermore, the transaction manager "changes its mode" when doTransaction is running: any call to doTransaction in that scope result in child transactions recursively, similar to batches. This doesn't fit an asynchronous model where the child transactions are to be created well after doTransaction returns.

(3) Are minor changes to nsITransactionManager API enough (e.g. doTransaction returning a promise/having a callback function)?

Initially I was convinced that was the way to go. That's why I ported the TM to js (so doTransaction could just returns a promise). This has the advantage of backwards compatibility, and it would also make it easy for gecko's editor to adopt the new TM when/if it's ported to JS (I'm hearing that's in the cards due to WebIDL conversion).

But those advantages are overwhelmed by the very blurry semantics this would create. Consider the following questions:
  a) What happens if doTransaction synchronously calls doTransaction? Do we nest the transaction as the current API does? If so, how would asynchronous achieve the same goal? Resolve the promise to transactions?
  b) Wouldn't this hide the fundamental change for counsumers? Until now, callers of nsITransactionManager.doTransaction could expect the transaction to be fully processed by the time it returns. So, for instance, one could do something like this (pseudo code):
  
  let createFolderTxn = new CreatePlacesFolderTransaction(...);
  PlacesUtils.tm.doTransaction(createFolderTxn);
  if (someUrls) {
    let folderId = createFolderTxn.folderId;
    for (url of someUrls)  {
      let createBookmarkTxn = new CreateBokmarkTransaction(ufolderId, url...);
      PlacesUtils.tm.doTransaction(createBookmarkTxn);
    }
  }
  

    Not forcing "PlacesUtils.tm.doTransaction" to not work ("PlacesUtils.tm is not an object"), but changing in semantics may hide the need for a change in such cases.

  c) what should un/redoTransaction do if there're still transactions that did not complete? Current TM api disallows this, by design, and it was fine because it really doesn't make sense for a nsITransaction.doTransaction call to result in nsITransactionManager.undoTransaction call. However, in an asynchronous environment the user could ask to Undo some action before it's completed. I'm still not sure if we should allow that or disable the undo and redo commands temporarily, but either way requires some API changes.
  
(4) Is the current current transactions API bloated?

Somewhat, but not much. There are some features that were implemented back in 1998 but are still unsued. One example is the "interrupt" return value of transaction listener methods (for example, "willDo" can return true in order to block a transaction).

(5) Is the just-implemented HTML5 UndoManager implementation (bug 617532) of any use?

Recently an UndoManager DOM API was implemented on top of the editor's TM. Unfortunately we cannot use it for the following reasons:
  (a) It's not ready for asynchronous transactions as-is.... I do expect this to change at some point, because I'm hearing that Promises for web is already in discusstion. We cannot wait for that to happen though.
  (b) It's tied to DOM documents and elements.
  (c) It's upper-most undo scope is a document. An UndoManager cannot be shared across documents, which is what places does.

However, I do find the HTML 5 UndoManger API spec quite useful for the purpose of designing an API for places, for three reasons
  (a) While it's not compatible as-is with asynchronous transactions. It doesn't conflict nearly with them nearly as much as nsITransactionManager.
  (b) It's a much more compact and elegant API, limited almost exactly to the needs of Places.
  (c) If the issues raised above are fixed at some point, we can remove our own implementation and reuse the DOM one.

(6) Should the "batching" (beginBatch/) API be ported as is?

I never liked the way the transaction manager handles batching. It makes it way to easy to render the undo/redo commands unusable. More specifically, if one calls beginBatch in one place, but fails to call endBatch (due to some js exceptions for example), the undo and redo commands will be broken for the rest of the browsing session. This is a problem because most of the time beginBatch and endBatch are not called in the same scope, so it's not be addressed using the try-finally approach.

I was looking how various Undo APIs (outside Mozilla) address batching. Some do the same with different naming (Cocoa for example), but provide a less fatal solution for undo (think of endBatch called implicitly on undo) and some use an "undoUntil" solution. HTML5 Undo Manger solves this by offerng a boolean "merge" parameter when executing transactions (so there's no "endBatch" kind of thing). Each approach has its advantages and disadvantages. I'm going with the HTML5 solution simply because I'm modeling the rest of the API after it.

(7) An API for Places or something generic?

In theory, transactions should be used much more then they're in practice. Some just do away without it (session store implements an undo stack without a command pattern), some use other APIs (devtools users whatever the orion editor provides) but mostly undo-redo is left unimplemented.

So it's just the various types of places editors, the HTML5 undo manager and Places that use "generic" transactions APi. The first two are unlikely to adopt an alternative non-XPCOM API before they're ported to JS (if that ever happens). On top of that, places doesn't need a lot of the functionality that's provided by the current transactions API.

Thus it seems we should not attempt to design a generic API at this point, at least not initially.

Comment 3

[Asaf Romano (gone)]

Initial implementation is almost done. Here's what the API is going to look like:

interface Transaction {
  /**
   * Execute the command
   * If a promise is returned, then the command is considered executed once
   * it's resolved, and the resolution value will be used for resolving
   * the promise returned by UndoManager.transact.
   * If anything else is returned (or not), the command is considered executed
   * right away, and the return value is used as if a promise was resolved to
   * that value.
   * This is done so current places transactions won't need a full rewrtie
   */
  Promise execute();

  /**
   * Same same, except that the return value/resolution value is ignored.
   */
  Promise undo();
  Promise rede();
};

interface UndoManager {
  /**
   * False if any transaction, or an undo or redo action, is in progress.
   */
   boolean isIdle;

  /**
   * Transact a transaction. It's illegal to call this if isIdle is false.
   * 
   * @param aTransaction
   *        the transaction
   * @param aMerge
   *        whether or not to merge this transaction with the last committed transaction.
   * @param aChildTransactionToFollow
   *        If true, the idle queue won't be traversed before at least one more call
   *        to transact (without this parameter set) is done. This is useful for chaining
   *        transactions together while ensuring that no transactions are processed
   *        in the middle.
   * @promise a promise to be resolved once the transaction is committed with the
   * resolution/return value of aTransaction.execute. It'll be rejected if aTransaction.execute
   * throws or rejects.
   * @note if aTransaction.execute throws, aChildTransactionToFollow is ignored.
   */
  Promise transact(Transaction aTransaction, aMerge = false, aChildTransactionToFollow = false);

  /**
   * Returns a promise to be resolved when there are no transactions, or undo or redo actions, in progress, and all previous promises made by this method are resolved or rejected.
   */
  Promise onIdle();
  
  /* See the html 5 api. The returned promises is always resolved. */
  Promise undoTransaction();
  Promise redoTransaction();

  /* more trivial stuff from the HTML5 UndoManager spec */
};

Comment 4

[Asaf Romano (gone)]

And many thanks to Yorik for this aChildTransactionToFollow idea.

Comment 5

[David Teller [:Yoric] - still alive but not very active]

Albeit I never insisted on giving such a long name to the argument :)

Comment 6

[Asaf Romano (gone)]

Hehe...

I'm actually going to rename it to something like |aCanProcessIdleQueue = true|. Less concepts, less confusion. I also think we can avoid the timeout-workaround you suggested, because the next call to transact (with the default arguments) will process the idle queue anyway.

All that said, the fact that there's no clear way to handle this situation is not great. It's not _that_ exotic. I wonder if we could make deferred.resolve return a promise like I thought it does. That's completely out of the scope of this bug, of course.

Comment 7

[Asaf Romano (gone)]

Attachment: Async undo manager

Working implementation.

Comment 8

[Asaf Romano (gone)]

As part of this complete rewrite, it might be a good time to rethink our module/component solution for transactions scoping. First, for those that were not around when it was introduced, here's what it is about:
1) Transaction manager keep strong references to transactions.
2) If a transaction is implemented in JS, as all places transactions are, referencing it means referencing it scope, which includes its global object.
3) The places transaction manager is a singleton across the application. It's not "attached" to any window
4) Thus, if a transaction is implemented within a window's scope, and is passed to the transaction manager, that window would be leaked when its close, until the transactions manager stops referencing it.
5) To work around that, all *in-tree* transactions are implemented in PlacesUtils.jsm, so that they're not attached to any window.

This is the current setup, but variants of it have been around since ever, even in the pre-places bookmarks system.

Now there are two problem with this solution:
1) Addons are not protected. I just found out an addon named "Places Context Menu" implements transactions in the window scope, meaning it leaks windows.
2) It forces some sort of encapsulation that doesn't always make sense. Just look at the giant list of exported symbols in PlacesUtils.

Those are just the facts. I double checked with Blake Kaplan that they are still true.

However, I think we can solve this some other way: JSON and unJSON transaction objects that come in. Keep in mind those objects tend to be very simple. The downside of this solution:
(1) Performance - we can work around that by making by doing that only in the "public" api. Transactions implemented by toolkit won't be serialized.
(2)  Unexpected scoping behavior  - |let foo = bar; tm.do({ execute: function() { ..access foo... }}); won't work.

Comment 9

[Asaf Romano (gone)]

It turns out bug 890203 blocks any asynchronous solution for transactions. It doesn't block me from working on it meanwhile, because batching is just a matter of performance, but it'd block checking in my work, because batching a matter of performance.

Comment 10

[Asaf Romano (gone)]

Attachment: checkpoint

Comment 11

[Asaf Romano (gone)]

Attachment: Another checkpoint

Comment 12

[Asaf Romano (gone)]

Attachment: Feedback-able checkpoint

Comment 13

[Asaf Romano (gone)]

Attachment: Feedback-able checkpoint with more meaningful tests

Comment 14

[Asaf Romano (gone)]

Comment on attachment 779134 [details] [diff] [review]
Feedback-able checkpoint with more meaningful tests

The documentation within the module is still incomplete, to say the least, and most of the transactions are still untested, and there's still a whole lot more to do, but this is good for a first pass. I suggest reviewing the documentation first, then the *test*, and only then the implementation.

Comment 15

[Asaf Romano (gone)]

By the way, the final API is quite from what I described in comment 3.

Comment 16

[Asaf Romano (gone)]

Comment on attachment 779134 [details] [diff] [review]
Feedback-able checkpoint with more meaningful tests

If you've haven't started yet, please wait for the GUID-ed version.

Comment 17

[Asaf Romano (gone)]

Attachment: Switch to GUIDs

There are two problems with this change, and both also affect the wrtieGuid of the livemarks service:

(1) minor: OnItemAdded is called with the initial GUID. This is mostly a problem for test, esp. because there's no notification for the GUID change.
(2) major: GUIDs are cached in the backend, and therefore some of the notifications are called with the old GUID even after the GUID is changed.

Again, this probably affects the livemarks service too.

So, for now I've this messy fixGUIDsMap function in the tests file, but because I don't believe that's going to be left as is I only fixed one of the tests for now.

Comment 18

[Asaf Romano (gone)]

Attachment: Switch to GUIDs

Workaround the cache issue. The other one can be left as is for now, IMO.

All the tests that I wrote so far are now fixed to work with GUIDs too.

Comment 19

[Asaf Romano (gone)]

Attachment: and now to RemoveItem

Now that I switched to GUIDs it's easy (well, easier) to work on removeItem. The additional test only covers only one of the many cases this transaction is going to support. Additional tests will be added in the next iteration.

Livemarks are not yet supported by this transaction. That's on hold until the patch for bug 896193 lands.

Comment 20

[Marco Bonardo [:mak]]

Comment on attachment 780838 [details] [diff] [review]
and now to RemoveItem

I just don't like this "merge" parameter, sounds like something that could be easily misunderstood and my expectation is that when someone PT.spawn() the transactions in that specific tasks are related.

Comment 21

[Asaf Romano (gone)]

So, I really don't know. On one hand, it's indeed hard to think of a use case for multiple "spawned" transactions which aren't merged. On the other hand, implicit merging doesn't play well with the fact that PlacesTransaction.spawn is also supposed to be used for any undo-manager action, not just transact. So what if one calls undo and then transact in the same task? Should undo within Task.spawn behave as if the spawned transactions aren't merged yet?

The plus of the |merge| argument is that it works the same way the HTML5 API does.

Keep in mind that multiple transactions are actually going to be pretty rare. All those createBookmark-then-do-something-with-it cases are not valid anymore. The only use case for merging transactions is going to be something like Bookmark All Tabs.

By the way, I still need to figure out what to do about the batch-hack we use in the edit panel, but I'm pretty sure it's not going to be merging. I'll probably either implement something like undoUntil, or hack something in the edit panel itself.

Comment 22

[Marco Bonardo [:mak]]

(In reply to Mano from comment #21)
> So what if one calls undo and then transact in
> the same task? Should undo within Task.spawn behave as if the spawned
> transactions aren't merged yet?

I think a call to undo/redo should reset merge to zero. Still, that use-case looks fancy, the only case we undo is when a dialog is canceled or undo is chosen from a menu, I can't think of a case where that should also try to merge a transaction.

> The plus of the |merge| argument is that it works the same way the HTML5 API
> does.

Possibly, though, compared to that, we have the spawn() advantage. That's already a grouping system, somehow, so it is a more natural way to think about grouping changes together.

> The only use case for merging transactions is going to be something
> like Bookmark All Tabs.

Another reason that makes me think merge is pointless...

Comment 23

[Asaf Romano (gone)]

(In reply to Marco Bonardo [:mak] from comment #22)
> (In reply to Mano from comment #21)
> > So what if one calls undo and then transact in
> > the same task? Should undo within Task.spawn behave as if the spawned
> > transactions aren't merged yet?
> 
> I think a call to undo/redo should reset merge to zero. Still, that use-case
> looks fancy, the only case we undo is when a dialog is canceled or undo is
> chosen from a menu, I can't think of a case where that should also try to
> merge a transaction.

Well, there's the test use case ;)

I think I'll just make it illegal to call both transact and undo/redo in the same "spawn".

The test will issue few "spawned" functions then.

Comment 24

[Asaf Romano (gone)]

Attachment: checkpoint (not ready for review)

Reviewable patch will be posted tomorrow.

Comment 25

[Asaf Romano (gone)]

Attachment: backend part checkpoint

Working on consumers now. I'll post a detailed update on Thursday.

Comment 26

[Asaf Romano (gone)]

GUIDs API is landed. livemarks changes will be landed once the tree reopens.

The backend patch is missing the livemarks transaction. Other than that, it needs some documentation and then it's ready for review.

I'm now working on consumers (i.e. browser/); almost done there too.

Comment 27

[Asaf Romano (gone)]

Attachment: backend part, all the stable stuff

I'm asking for review, but the documentation is still unfinished, so please don't pay too much attention to the comments at this point. I'm asking for review because I consider almost everything in this patch "stable" in the sense that I don't expect major changes.

The GUID support has landed in the dependent bug, so it's not included in this patch. The livermarks stuff was backed out, so it's still included in this patch.

There are few missing tests (about 4 transactions). I'm on that now.

Comment 28

[Asaf Romano (gone)]

Attachment: backend part, all the stable stuff, more tests

More tests.

Comment 29

[Asaf Romano (gone)]

Attachment: backend part, all the stable stuff, more tests

Couple more.

Comment 30

[Marco Bonardo [:mak]]

thanks for the patches, I will look at this in the next couple of days

Comment 31

[Marco Bonardo [:mak]]

Comment on attachment 8349106 [details] [diff] [review]
backend part, all the stable stuff, more tests

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

I skimmed fast through the test since I prefer to first give some feedback on the code and let you proceed with it.
But so far it looks satisfying and I'm happy we can also replace removeFolderTransaction that is currently done in cpp!

::: toolkit/components/places/PlacesTransactions.jsm
@@ +16,5 @@
> +/**
> + * The internal object that hold the transactions history.
> + * It's exposed in a read-only manner by the undo manager.
> + */
> +let UndoHistory = [];

what's the direction of this array? are newer transaction at the beginning or at the end?
I wonder if we should abstract this a little bit, to provide better named methods (than things like unshift([]) or UndoHistoryy[0].unshift), that will make the following code much more readable... Thinking of an object with a readable interface wrapping the array, with things like canUndo, canRedo, lastEntry, firstEntry (feel free to figure better naming), things like UndoHistory[UndoHistory.position - 1] can surely have better naming.

@@ +20,5 @@
> +let UndoHistory = [];
> +
> +// The undo position in the transactions history - entries at and past this
> +// point are redo entries.
> +UndoHistory.position = 0;

nit: it's unclear if "past this point" means smaller or larger, something like "equal or larger" would be clearer

@@ +23,5 @@
> +// point are redo entries.
> +UndoHistory.position = 0;
> +
> +/**
> + * Internal helper that takes care of queueing the undo manager operations.

some more details about how it's going to queue things is appreciated

@@ +31,5 @@
> +  spawn: function (aTask, aFunctionName) {
> +    let prev = this.currentTask;
> +    return this.currentTask = Task.spawn(function* () {
> +      if (prev)
> +        yield this.currentTask;

this needs some comments since it's unclear how it's achieving the enqueueing and why it's yielding on currenttask, rather than on prev

@@ +38,5 @@
> +        let rv = yield aTask();
> +        return rv;
> +      }
> +      catch(ex) {
> +        this.running = false;

.running property is set to false here, but it's never used anywhere, so looks like you may not need to catch

@@ +51,5 @@
> +   * Asynchronously transact one transaction, or a sequence of transactions that
> +   * would be treated as a single entry in the transaction history.
> +   *
> +   * The undo history is affected if and only if the transaction was executed
> +   * successfully.

there is already a @note about this, so it looks redundant to state it here and then repeat it in the @note

@@ +91,5 @@
> +        if (!createdEntry) {
> +          UndoHistory.splice(0, UndoHistory.position);
> +          UndoHistory.position = 0;
> +          UndoHistory.unshift([]);
> +          createdEntry = true;

some comment about what happens in this generator would be appreciated (it's clear but you have to follow the code line by line to figure it out)

@@ +124,5 @@
> +        return rv;
> +      }
> +      else {
> +        let rv = yield transactOneTransaction(aTransactionOrGeneratorFunction);
> +        return rv;

I guess in these cases you may just return without assign

@@ +132,5 @@
> +
> +  /**
> +   * Asynchronously undo the transaction immediately after the current undo
> +   * position in the transactions history in the reverse order, if any, and
> +   * adjusts the undo position.

the "in the reverse order" part of the phrase made my mind go into the cloud... since I suspect anyone has an idea how undo works, please just remove "in the reverse order"

@@ +200,5 @@
> +   * history.
> +   *
> +   * @return {Promises). The promise always resolves.
> +   * @note All undo manager operations are queued. This means that undo manager
> +   * state may change by the time aTransactionOrGeneratorFunction is processed.

the params should be documented

@@ +253,5 @@
> +  }
> +};
> +
> +function DefineTransaction(aRequiredFields = [], aOptionalFields = []) {
> +  for (let field of [...aRequiredFields, ... aOptionalFields]) {

inconsistent spacing after ...

@@ +307,5 @@
> +  }
> +  return false;
> +};
> +
> +DefineTransaction.inputFields = new Map();

I'm a little bit confused by us speaking of inputFields in a toolkit module, I guess one may use this module without having input fields. these look more like rules to be applied... or did you mean input fields as properties of an input value?

@@ +335,5 @@
> +    if (aRequired)
> +      throw new Error("Required property is missing: " + aProp);
> +    return this.inputFields.get(aProp).defaultValue;
> +  }
> +  else {

else after if () return

@@ +348,5 @@
> +  return aValue;
> +};
> +
> +DefineTransaction.verifyInput =
> +function (aInput, aRequired = [], aOptional = []) {

documentation please, what is aInput representing, what are required and optional?

@@ +458,5 @@
> +  getAdditionalTags: function (aURI, aTagsToSet) {
> +    let currentTags = PlacesUtils.tagging.getTagsForURI(aURI)
> +    return [t for (t of aTagsToSet) if (currentTags.indexOf(t) == -1)];
> +  }
> +};

why an object and not just a plain function helper?

@@ +467,5 @@
> + * Transaction for creating a bookmark.
> + *
> + * Required input properties:
> + * - uri (nsIURI)
> + *   the new bookmark's uri .

nit: space before period

@@ +508,5 @@
> +        }
> +
> +        return itemId;
> +      },
> +      function _additionalOnUndo() {

doesn't seem to be used anywhere?

@@ +625,5 @@
> +    };
> +
> +    let livemark = yield createItem(), guid = livemark.guid;
> +    this.undo = function* () {
> +      // XXX: How is RemoveLivemark different from RemoveItem?

it's just that it ensures proper serialization of livemarks operations (so that the service has finished loading its internal cache before trying to do anything) and you can get a callback out of it

@@ +705,5 @@
> +
> +    PlacesUtils.bookmarks.changeBookmarkURI(itemId, aURI);
> +
> +    // Move tags from old URI to new URI.
> +    let tags = PlacesUtils.tagging.getTagsForURI(oldURI);

I suspect changing the uri before saving the tags is not going to work as we expect since the old uri may end up being removed (may be the cause of bug 767939? Having a test would be nice!)

@@ +720,5 @@
> +      // Move tags from new URI to old URI.
> +      if (tags.length > 0) {
> +        // Only untag the new URI if this is the only bookmark.
> +        if (PlacesUtils.getBookmarksForURI(aURI, {}).length == 0)
> +          PlacesUtils.tagging.untagURI(aURI, tags);

I suspect we should only remove the tags that were not present before... if the uri had A and we added A and B, we should remove B. A test is welcome

::: toolkit/components/places/PlacesUtils.jsm
@@ +1919,5 @@
>  
> +// Sometime soon, likely as part of the transition to mozIAsyncBookmarks,
> +// itemIds will be deprecated in favour of GUIDs, which play much better
> +// with multiple undo/redo operations.  Because these GUIDs are already stored,
> +// and because we don't wantto revise the transactions API once more when this

typo: wantto

::: toolkit/components/places/tests/unit/test_async_transactions.js
@@ +5,5 @@
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +Components.utils.import("resource://gre/modules/XPCOMUtils.jsm");
> +Components.utils.import("resource://gre/modules/PlacesTransactions.jsm");
> +Components.utils.import("resource://gre/modules/NetUtil.jsm");

I think xpcomutils and netutil are already imported by head_common.js (you may also add a lazy getter for PlacesTransactions if you wish)

Comment 33

[Tracy Walker [:tracy]]

qa+: a full functional test run of places will need to be performed around this.

Comment 34

[Asaf Romano (gone)]

Attachment: Documented backend, comments addressed

Comment 35

[Marco Mucci [:MarcoM]]

Carry over to Iteration it-30c-29a-28b.3

Comment 36

[Asaf Romano (gone)]

I'm soon going to morph this bug into covering just the back-end side of this feature, and then (1) file a meta bug for the feature as whole; (2) file a bug for turning it on (it's going to be initially disabled, until the last front-end piece lands; (3) file separate bugs for each front end consumer, so Marco and I can work on this in parallel.

Either the meta bug or the flip-it-on bug is where QA should happen. QA for the backend alone isn't possible (but it's covered by automated tests).

Comment 37

[Asaf Romano (gone)]

Bug 979280 is the meta bug for this feature.

This bug now covers cover just the back-end.

Comment 38

[Marco Bonardo [:mak]]

Comment on attachment 8384477 [details] [diff] [review]
Documented backend, comments addressed

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

I think we need a bug filed to handle expiration of transactions, after a good amount of time elapsed from its execution. When a profile is kept open for a week, we shouldn't hold onto days old transactions.

Btw, there is nothing blocking here, so we should land this part and start converting consumers to ensure the functionality and the API are exactly what we need.

Thanks for this giant work.

::: toolkit/components/places/PlacesTransactions.jsm
@@ +22,5 @@
> + * GUIDs and item-ids
> + * -------------------
> + * The Bookmarks API still relies heavily on item-ids, but since those do not
> + * play nicely with the concept of undo and redo (especially not in an
> + * asynchronous environment), this API only accepts bookmark GUIDs, both for 

trailing space

@@ +45,5 @@
> + * property for NewBookmark).  Once a transaction is created, you may pass it
> + * to |transact| or use it in the for batching (see next section).
> + *
> + * The constructors throw right away when any required input is missing or when
> + * some input is invalid ״on the surface" (e.g. GUID values are validated to be

wrong apices

@@ +50,5 @@
> + * 12-characters strings, but are not validated to point to existing item.  Such
> + * an error will reveal when the transaction is executed).
> + *
> + * To make things simple, a given input property has the same basic meaning and
> + * valid values across all transactions which accept it in the input object. 

trailing space

@@ +53,5 @@
> + * To make things simple, a given input property has the same basic meaning and
> + * valid values across all transactions which accept it in the input object. 
> + * Here is a list of all supported input properties along with their expected
> + * values:
> + *  - uri: a nsIURI object.

nit: s/a/an/ (also in the next lines)

@@ +59,5 @@
> + *  - siteURI: a nsIURI object, holding the url for the site with which
> + *             a live bookmark is associated.
> + *  - GUID, parentGUID, newParentGUID: a valid places GUID string.
> + *  - title: a string
> + *  - index, newIndex: a places index.  Valid values are any non-negative

I don't think we have the concept a places index... what about "position of the bookmark inside its ancestor, starting from 0."

@@ +61,5 @@
> + *  - GUID, parentGUID, newParentGUID: a valid places GUID string.
> + *  - title: a string
> + *  - index, newIndex: a places index.  Valid values are any non-negative
> + *    integer and PlacesUtils.bookmarks.DEFAULT_INDEX
> + *  - annoatationObject: see PlacesUtils.setAnnotationsForItem

typo: annoatation

@@ +132,5 @@
> +
> +// The internal object for managing the transactions history.
> +// The public API is included in PlacesTransactions.
> +// XXXmano: extending the array "properly" makes it painful to implement
> +// getters.  If/when ES6 gets proper array subclassing we can revise this/

file a bug and make this a "TODO (bug N): desc." please
That said, is that really planned for ES6? I don't think it will go much further than Proxies :/
On the other side, this array is for internal use, so it doesn't matter much.

@@ +162,5 @@
> +        yield transaction.undo();
> +      }
> +      catch(ex) {
> +        // For now, just report the error.
> +        // This is what the HTML 5 UndoManager and in practice trying to

I think it's missing a "does"

@@ +165,5 @@
> +        // For now, just report the error.
> +        // This is what the HTML 5 UndoManager and in practice trying to
> +        // "recover" by redoing everything won't buy us much.
> +        Components.utils.reportError(ex);
> +        throw ex;

So, while redoing won't buy us much, shoudn't we restart the transactions history at this point? we can't move back more than this point, so there's no point to keep broken stuff around.

@@ +191,5 @@
> +      catch(ex) {
> +        // For now, just report the error.
> +        // This is what the HTML 5 UndoManager and in practice trying to
> +        // "recover" by undoing everything won't buy us much.
> +        Components.utils.reportError(ex);

in undo you are re-throwing, while here you continue after dispatching the error. what's the wanted behavior?
I think again here we are in a situation where the redo history is broken, so we should throw it away.

I don't want to be in a situation where the manager ends up being blocked by a single broken transaction, it should always be able to recover.

@@ +204,5 @@
> +   *
> +   * @param aTransaction
> +   *        the transaction object to be added to the transaction history.
> +   * @param [optional] aForceNewEntry
> +   *        Force a new entry for the transaction. Default: false.

would be nice if the javadoc would clarify what's the situation where one should "force" the transaction, I see it's documented later, but this sounds a little bit strange

Or maybe this method should just be split into 2 methods, one that acts normally and one that enforces the addition (I leave fancy naming bikeshed to you)

@@ +296,5 @@
> +      // transaction is committed. This means that if |transact| is called
> +      // in its "generator mode" and no transactions are committed by the
> +      // generator, the transactions history is left unchanged.
> +      // Depending on how this API is actually used we may revise this decision
> +      // and make it so |transact| always forces a new entry.

please file a bug explaining the idea and stating the 2 cases we should evaluate. it's a little obscure to me so far, I guess we'll see during the old TM consumers conversion? if so the bug should be blocked by the conversions

@@ +327,5 @@
> +      }
> +
> +      if (typeof(aTransactionOrGeneratorFunction) == "function") {
> +        let rv = yield transactBatch(aTransactionOrGeneratorFunction);
> +        return rv;

just return without assignment

@@ +344,5 @@
> +   * @return {Promises).  The promise always resolves.
> +   * @note All undo manager operations are queued. This means that transactions
> +   * history may change by the time your request is fulfilled.
> +   */
> +  undo: function () Serialize(() => TransactionsHistory.undo()),

missing newline before undo

@@ +385,5 @@
> +    });
> +  },
> +
> +  /**
> +   * The numbers of entires in the transactions history.

typo: entires

@@ +395,5 @@
> +   * of one or more transaction objects.
> +   *
> +   * THIS METHOD SHOULD BE USED ONLY AS A WAY TO MONITOR THE TRANSACTIONS
> +   * HISTORY STATE.  NEVER CALL THE TRANSACTION METHODS (execute, undo, redo)
> +   * DIRECTLY.

May we return Proxies where execute, undo, redo just don't work? I suppose this is just for testing purpose, do we need those methods there? Eventually may be a follow-up.

@@ +434,5 @@
> + *
> + * This magic serves two purposes:
> + * (1) It completely hides the transactions' internals from the module
> + *     consumers.
> + * (2) It keeps each transaction implementation to what is is about, bypassing

typo: "is is"

@@ +465,5 @@
> +  return ctor;
> +}
> +
> +DefineTransaction.isStr = v => typeof(v) == "string";
> +DefineTransaction.isURI = v => v instanceof Components.interfaces.nsIURI;

why not defining Ci?

@@ +527,5 @@
> +
> +  // Arrays and other JS Objects cannot be referenced by this module
> +  // because they reference their global object through their prototype.
> +  if (Array.isArray(aValue))
> +    return JSON.parse(JSON.stringify(aValue))

please use Components.utils.cloneInto instead of parse/stringify

also, you state "other JS Objects", but the check is only handling Array here...


As a side note, in the old code we were also cloning nsIURIs cause they were causing leaks, that caused bug 974406, so I used .clone() to fix that. Here doesn't look like we are cloning nsIURIs, is that no more needed? it's possible some of the recent "memory leaks" magic fixes solved the original issue by breaking cycles. Though, here I mostly care that 1. we don't add back a leak, 2. we don't do the dumb old newURI(spec) cloning

@@ +597,5 @@
> +// remove properties.
> +DefineTransaction.defineInputProps(["uri", "feedURI", "siteURI"],
> +                                   DefineTransaction.isURI, null);
> +DefineTransaction.defineInputProps(["GUID", "parentGUID", "newParentGUID"],
> +                                   DefineTransaction.isValidGUID);

I think I'd remove the "valid" part from isValidGUID and isValidIndex... all of the other methods are validators and they are not isValidURI or isValidStr

@@ +622,5 @@
> + *        The guid of the parent folder
> + * @param aCreateItemFunction(aParentId, aGUIDToRestore)
> + *        The function to be called for creating the item on execute and redo.
> + *        It should return the itemId for the new item
> + *        aGUIDToRestore - the GUID to set for the item (used for redo).

this aGUIDToRestore javadoc should probably be indented more or inlined

@@ +667,5 @@
> + * See the documentation at the top of this file. The valid values for input
> + * are also documented there.
> + *****************************************************************************/
> + 
> +let PT = PlacesTransactions;

trailing space above

@@ +854,5 @@
> +
> +    // Move tags from old URI to new URI.
> +    if (oldURITags.length > 0) {
> +      // Only untag the old URI if this is the only bookmark.
> +      if (PlacesUtils.getBookmarksForURI(oldURI, {}).length == 0)

I think it may be interesting to use PlacesUtils.asyncGetBookmarkIds here... though it won't make things much better since anything else is sync... maybe we should just have a bug apart to deprecate getBookmarksForURI, and maybe it's not even worth to do that before having an async bookmarking api...

@@ +857,5 @@
> +      // Only untag the old URI if this is the only bookmark.
> +      if (PlacesUtils.getBookmarksForURI(oldURI, {}).length == 0)
> +        PlacesUtils.tagging.untagURI(oldURI, oldURITags);
> +
> +      let currentNewURITags = PlacesUtils.tagging.getTagsForURI(aURI); 

trailing space

@@ +1033,5 @@
> +      }();
> +
> +      let node = aNode;
> +      if (!node && aItem.itemType == bms.TYPE_FOLDER)
> +        node = PlacesUtils.getFolderContents(itemId).root;

where is this node being close?

::: toolkit/components/places/tests/unit/test_async_transactions.js
@@ +4,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +XPCOMUtils.defineLazyModuleGetter(this, "PlacesTransactions",
> +                                  "resource://gre/modules/PlacesTransactions.jsm");

just add this to head_common.js

@@ +997,5 @@
> +  yield PT.undo();
> +  ensureOrder(originalOrder);
> +  yield PT.undo();
> +  ensureItemsRemoved(...originalOrder, folder_info);
> +});

I don't think I'm going to look at each single test case here, so just asking:
are the test (somehow) a copy of the existing ones? did you add the additional tests for bug 767939 (it seems so, but asking just in case)

Comment 39

[Asaf Romano (gone)]

Attachment: comments addressed

Comment 40

[Asaf Romano (gone)]

Attachment: fix & test the livemarks stuff

Comment 41

[Asaf Romano (gone)]

Marco reviewed the nsLivemarkService changes over IRC.

Comment 42

[Asaf Romano (gone)]

Attachment: for checkin

Comment 43

[Asaf Romano (gone)]

checkin-needed for attachment 8389602 [details] [diff] [review] either on fx-team or on mozilla-inbound. Something is wrong with my hg account.

Comment 44

[Asaf Romano (gone)]

Landed on fx-team:

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

Comment 45

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 46

[Marco Mucci [:MarcoM]]

Hi Andrei, will you be able to verify this bug before the end of our iteration on Monday March 17.

Comment 47

[Marco Bonardo [:mak]]

there isn't much to verify here, we just added a new code module, that is, so far, unused.

Comment 48

[Marco Mucci [:MarcoM]]

Hi Tracy, please see Comment #47 and let me know how you would like to proceed.

Comment 49

[Tracy Walker [:tracy]]

If this new code isn't even used, than I agree there is nothing to test here.  A basic Places smoketest wouldn't hurt.  But don't spend too much time on it, Andrei. Mark this verified if the basic smoketests pass.

I assume other fixes will be landed that will use the new code. Specific verifications can be done in those bugs.

Comment 50

[Marco Bonardo [:mak]]

yes, frontend changes will happen on top of this, but will be enabled through a pref (see bug 983623) until we are satisfied with the functionality.

Comment 51

[Tracy Walker [:tracy]]

ok, so let's even go as far as [qa-] this one.  Thanks Marco.

Comment 52

[Andrei Vaida [:avaida]]

(In reply to Marco Mucci [:MarcoM] from comment #46)
> Hi Andrei, will you be able to verify this bug before the end of our
> iteration on Monday March 17.
Hi Marco, taking Comment #47 and Comment #49 into consideration, this bug will be verified fixed some time Monday, March 17, if manual QA is still necessary.

(In reply to [:tracy] Tracy Walker - QA Mentor from comment #49)
> If this new code isn't even used, than I agree there is nothing to test
> here.  A basic Places smoketest wouldn't hurt.  But don't spend too much
> time on it, Andrei. Mark this verified if the basic smoketests pass.
> 
> I assume other fixes will be landed that will use the new code. Specific
> verifications can be done in those bugs.
Hi Tracy, I was actually knee-deep in writing and running extensive test cases for all the Places-related areas (according to Comment #33), but since this is no longer required (or a priority) at the moment, I will be able to run a smoketest and finish verifying this bug first thing Monday morning (17 March). 

Please let me know if I should still carry on with this.

Comment 53

[Tracy Walker [:tracy]]

Andrei, please continue with fleshing out test cases. But it does not have to be completed ASAP.

Comment 54

[Marco Mucci [:MarcoM]]

Marking as verified based on comment #51.