1093030 - Proper solution for implementing "Cancel"/"Undo" in the bookmarks dialog and Star Panel

Status: NEW

(Firefox :: Bookmarks & History, defect, P3)

Description

[Asaf Romano (gone)]

Due to limited resources and due to the fact that this really isn't the time to completely rewrite Places UI files (better to wait _at least_ for Bookmarks.jsm work to be done), I'm going to use the API changes from bug 982115 for hacking cancel/undo support in the bookmarks dialog and star UI. It'll be something like this:

In browser-places.js, when opening the Star UI:
PlacesTransactions.batch(function* () {
  yield promisePanelIsClosed();
});

In editPaneOverlay.js we'll just call |transact|, and the hack above will ensure that transact is called while the batch is in progress.

This work is happening in bug 951651.

Apart being a nasty hack, this solution is suboptimal for few reasons:
1) We'll need to be very careful in editBookmarkOverlay.js not to use batch even when it makes perfect sense (because there is no support for nested batches). This is very error prone as a batch seems proper for some of the actions implemented. In particular, the library info pane has support for setting and unsetting multiple tags for multiple URL selection that should be implemented as a batch of Tag+Untag. But because editBookmarkOverlay is not wrapped with a batch when it's used in the Library info pane, we cannot just avoid the batch (it'll show up as two transaction to undo, Tag and UnTag).

2) Both the Star UI and the bookmark properties dialog are not instant apply in their essence (and this is why I considered rewriting them as a modal dialog, not using editBookmarkOverlay at all), meaning that they should ideally just batch-on-accept and call transact within the batch function. Apart that it's not a good timing for a complete rewrite, the other reason I put this idea aside was the "New Folder" button in there, that's instant-apply even in the modal mode. However, considering how Save As dialog work, I think it'll be fine to consider new folders created this way not part of the same transaction. i.e. they will stay in place even if the Cancel is clicked, and they will have separate entries in the undo stack (so, after clicking accept, the transaction history could be something like [new folder 1][new folder 2][a batch of all changes done in the dialog].

3) It promotes a very bad usage of the batch API that addons could adopt, that has all the disadvantages of the legacy beingBatch/endBatch API.

(2) leads me to think that the solution here is to, indeed, "modal" the bookmark properties dialog and Star UI, but if it's not, we should just introduce a post-facto batching API (something like groupUntil). In any event, however, it's better do get back to this once both Bookmarks.jsm work is complete, and, even better, once the specs for in-content-library are finalized and prioritized. I think that at that point we may just do away with the bookmark properties dialog and this bug scope will be a little less scary.