553467 - move Places Transactions to PlacesUtils, make PlacesUIUtils.ptm a compatibility shim

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[Marco Bonardo [:mak]]

there is no real reason for it to be a service.
it is only used in browser UI, it is only used from js.
it does not need to cross xpcom.
it should act on resul nodes, not on item ids (or optionally be able to build a node/pseudo node from an item id).
being a jsm could be fastloaded.

Comment 1

[Marco Bonardo [:mak]]

also, this would allow to kill browserplaces.xpt

Comment 2

[Marco Bonardo [:mak]]

Attachment: patch v1.0

This won't increase speed of the service, i prefer doing things in steps.
The service is converted to a module, code does not notice any change because it is served through PlacesUIUtils.ptm.
I spent some time on cleaning up fancy naming and improve a bit readability of the code.
On the other side removes browserplaces.xpt, and an xpcom crossing.

Modules have practically the same lifecycle as services, i have confirmed that having a module in a window, closing that window and forcing GC does not collect the object.
Next bugs should improve performances, i must note test_placesTxn.js is an awesome perf test.

Before asking review i need a re-read and a tryserver pass.

Comment 3

[Marco Bonardo [:mak]]

note: test_txnGUIDs.js was full of CRLF, thus my editor replaced it all with LF.

Comment 4

[Asaf Romano (gone)]

So... how are you solving the leak issue (i.e. the fact that you must not create the transactions within the window global scope).

Comment 5

[Marco Bonardo [:mak]]

just calling clear() at Places shutdown is enough, this was done also by dbaron in an old bug regarding a similar issue.

Comment 6

[Marco Bonardo [:mak]]

also, the transactions objects are created in the module scope that is not the window scope

Comment 7

[Marco Bonardo [:mak]]

at this stage i find hard taking a decision if browser is a better destination than toolkit for the module. Transactions are clearly more UI related, but they act on bookmarks information (and in future eventually on nodes) that are absolutely part of toolkit. Plus third party Places implementers could want them (sure, they can just copy it to their repo).
Changing this later would not be a big deal, matter of changing /gre/ with //, but if we have points to bring for the decision, could be better doing it now.

Comment 8

[Asaf Romano (gone)]

I prefer toolkit.

Comment 9

[Asaf Romano (gone)]

(In reply to comment #6)
> also, the transactions objects are created in the module scope that is not the
> window scope

Ah, great.  Modules are awesome.

Comment 10

[Marco Bonardo [:mak]]

A point about browser/toolkit choice could be that we have description and load in sidebar transactions, that are concepts unknown to toolkit. But those are just shortcuts to ptm.setAnnotation and I could just move them to PUIU like getLoadInSidebarTransaction(itemId, lis) and getEditDescriptionTransaction(itemId, desc).

Comment 11

[Marco Bonardo [:mak]]

Attachment: patch v1.1

Module moved to toolkit, browser specific parts (description and loadinsidebar) to PlacesUIUtils.

Notice this uses the "brace on newline for functions" from the Global Coding Style guide, it helps increasing readability and finding methods in the module.

Comment 12

[Robert Kaiser]

(In reply to comment #11)
> Module moved to toolkit

Oh, how I love that! :)

Comment 13

[Asaf Romano (gone)]

+  // This is an alias for backwards compatibility.
+  return PlacesUtils.transactions;

Don't do that. We're changing enough in the next release to avoid that.

 
 XPCOMUtils.defineLazyServiceGetter(PlacesUIUtils, "URIFixup",
                                    "@mozilla.org/docshell/urifixup;1",
diff --git a/browser/components/places/tests/unit/test_placesTxn.js b/browser/components/places/tests/unit/test_placesTxn.js
--- a/browser/components/places/tests/unit/test_placesTxn.js
+++ b/browser/components/places/tests/unit/test_placesTxn.js

+var bmsvc = PlacesUtils.bookmarks;
+var lmsvc = PlacesUtils.livemarks;
+var mss = PlacesUtils.microsummaries;
+var ptSvc = PlacesUIUtils.ptm;
+var tagssvc = PlacesUtils.tagging;
+var annosvc = PlacesUtils.annotations;

Yay :)
 
 // create and add bookmarks observer
 var observer = {
@@ -143,9 +105,8 @@ function run_test() {
   var root = bmsvc.bookmarksMenuFolder;
 
   //Test creating a folder with a description

Please add the missing space.

 EXTRA_JS_MODULES = \
   PlacesDBUtils.jsm \
+  PlacesTransactions.jsm \
   utils.js \
…
+XPCOMUtils.defineLazyGetter(PlacesUtils, "transactions", function() {
+  Cu.import("resource://gre/modules/PlacesTransactions.jsm");
+  return PlacesTransactions;
+});
+

Thinking about this for a while, I think we should just merge it into the utils module. And, yes, as a single object (that is, put everything within PlacesUtils).

-  // nsITransactionManager
-  beginBatch: function() {
-    this.mTransactionManager.beginBatch();
+  beginBatch:
+  function PTXN_beginBatch()
+  {
+    transactionManager.beginBatch();

Now that we don't have an iface, it doesn't make sense to implement nsITransactionManager,
just add an accessor for mTransactionManager.

Comment 14

[Asaf Romano (gone)]

>--- a/browser/components/places/public/nsIPlacesTransactionsService.idl
>+++ /dev/null

The javadoc documentation should be moved into the module.


>   createFolder:
>-  function placesTxn_createFolder(aName, aContainer, aIndex,
>-                                  aAnnotations, aChildItemsTransactions) {
>-     return new placesCreateFolderTransactions(aName, aContainer, aIndex,
>-                                               aAnnotations, aChildItemsTransactions);

I think we can remove all those functions and just export symbols for the transactions. The functions were there due to xpcom limitations.

Comment 15

[Marco Bonardo [:mak]]

(In reply to comment #13)
> +  // This is an alias for backwards compatibility.
> +  return PlacesUtils.transactions;
> 
> Don't do that. We're changing enough in the next release to avoid that.

Well, the problem is that we have too many entries using the UIUtils.ptm stuff and i did not want to change all of them (especially in the test).
Should i really do the mass change? sounds like a good task for a followup, this change is large enough for a bug.

> +XPCOMUtils.defineLazyGetter(PlacesUtils, "transactions", function() {
> +  Cu.import("resource://gre/modules/PlacesTransactions.jsm");
> +  return PlacesTransactions;
> +});
> +
> 
> Thinking about this for a while, I think we should just merge it into the utils
> module. And, yes, as a single object (that is, put everything within
> PlacesUtils).

Hm, all the code?! What's the point? The code separation we have here is nice, i found already enough problems reading all the file this way, if we merge them it will become a gigantic file harder to manage. Fastload should make the thing practically the same, while preserving separation.
I'm more in favour of splitted things, like the backup thingy should go to its own component, out of utils.

> +  beginBatch:
> +  function PTXN_beginBatch()
> +  {
> +    transactionManager.beginBatch();
> 
> Now that we don't have an iface, it doesn't make sense to implement
> nsITransactionManager,
> just add an accessor for mTransactionManager.

I retained the QI to explain the choice of names and the way this works, sure i can remove it.
About the accessor, not sure what you want, mTransactionManager is already a getter.

(In reply to comment #14)
> >--- a/browser/components/places/public/nsIPlacesTransactionsService.idl
> >+++ /dev/null
> 
> The javadoc documentation should be moved into the module.

oops, yep!

> >   createFolder:
> >-  function placesTxn_createFolder(aName, aContainer, aIndex,
> >-                                  aAnnotations, aChildItemsTransactions) {
> >-     return new placesCreateFolderTransactions(aName, aContainer, aIndex,
> >-                                               aAnnotations, aChildItemsTransactions);
> 
> I think we can remove all those functions and just export symbols for the
> transactions. The functions were there due to xpcom limitations.

Sure we could, but that would require a large rewrite in all the codebase, and instead of exporting 1 object from the module i should export all the transaction types, with higher risks of conflicts, then i should readd back the Places prefix to all of them, instead of PlacesUtils.transactions.createFolder i'd have new PlacesCreateFolderTransaction. This would also make nonsense the uiutils transactions for setLoadInSidebar that should be converted to new objects and exported... sounds like again something for a followup, but i actually like the way this is self contained and hidden in a single object, allowing easier future changes.

Comment 16

[Asaf Romano (gone)]

Marco and I discussed this over IRC.  He will post a plan here later.

Comment 17

[Marco Bonardo [:mak]]

Ok here is the new plan:
- don't wrap transaction manager, just expose it in PU
- when someone gets it, register a listener to it for "undo" command updating
- have direct objects instead of functions, something like (TBD)
let txn = new PlacesUtils.transactions.CreateFolderTransaction();

Comment 18

[Marco Bonardo [:mak]]

Mano, since we talked today about compatibility problems (more than 250 entries in AMO for the old ptm interface), I think the plan to completely deCOM the interface should go in a followup, the old interface will survive, maybe even as a shim object, but I don't see anymore reasons to collapse all work in a bug to avoid breaking the usage multiple times. We can't just break it, so we can work parallel on new and old interface.
Also notice we have TON of code depending on this "interface" look (UIUtils, editBookmarkOverlay, bookmarkProperties and others), and changing all of them is regression-prone and much larger than scope of this bug.

So, I think what we should do is taking this patch as it is, implementing only the listener thing that does not break anything, than in a follow-up expose the new interface and start replacing internal usage file by file.

Comment 19

[Asaf Romano (gone)]

OK..... Fine... Sigh :(

Comment 20

[Robert Kaiser]

Getting the old interface should warn on error console though, I guess, just like we made it happen for that utils.js (if that's possible).

And how many of those extensions won't be broken anyhow by all the other changes being done to places in this release?

Comment 21

[Marco Bonardo [:mak]]

(In reply to comment #20)
> Getting the old interface should warn on error console though, I guess, just
> like we made it happen for that utils.js (if that's possible).

Not possible since we will still use it internally, we have lot of call points, updating all of them at once is just too much error prone

> And how many of those extensions won't be broken anyhow by all the other
> changes being done to places in this release?

Not a reason to do worse, unfortunatly.

Comment 22

[Asaf Romano (gone)]

Actually, it is.  It's better to change a lot at once than doing it in lots of pieces and for sure better than keeping an old interface in the tree forever, despite the fact that we never claimed to freeze it.  We should encourage this practice.

Comment 23

[Marco Bonardo [:mak]]

Attachment: patch v2.0

Implements Mano's requests:
- transactions are now objects that live in PlacesUtils module
- no more stupid forwarders hacks, use nsITransactionListener.
- moved documentation from idl to PU

plus:
- PUIU.ptm will survive for now due to high number of add-ons using it and the fact we will need some time to change internal usage. IT is a simple shim object that forwards calls.

This patch does not change any internal usage, I'd prefer leaving that to a follow-up to reduce patch size and possibility of regressions (pushing them at different times should allow for better regwindows).
test_placesTxn.js will probably stay as a test for the shim, we will need a more modern test in toolkit (that could even be a cleaned up copy of it since it's pretty complete).

I could still do some minor cleanup but I guess if we look at the patch in 2 it will take less time.

Comment 24

[Asaf Romano (gone)]

I'll finish this review later today.

Comment 25

[Marco Bonardo [:mak]]

thanks.  Actually I have a failure in test_GUIDs but should not be anything crazy, will look into it later.

Comment 26

[Marco Bonardo [:mak]]

(In reply to comment #25)
> thanks.  Actually I have a failure in test_txnGUIDs.js

the failure is just do to the fact I forgot to change the service instantiation to PlacesUIUtils.ptm. fixed locally.

Comment 27

[Asaf Romano (gone)]

Comment on attachment 454263 [details] [diff] [review]
patch v2.0

> XPCOMUtils.defineLazyServiceGetter(PlacesUIUtils, "RDF",
>@@ -1252,10 +1251,6 @@ XPCOMUtils.defineLazyGetter(PlacesUIUtil
>   return PlacesUIUtils.RDF.GetDataSource("rdf:local-store");
> });
> 
>-XPCOMUtils.defineLazyServiceGetter(PlacesUIUtils, "ptm",
>-                                   "@mozilla.org/browser/placesTransactionsService;1",
>-                                   "nsIPlacesTransactionsService");
>-
> XPCOMUtils.defineLazyServiceGetter(PlacesUIUtils, "URIFixup",
>                                    "@mozilla.org/docshell/urifixup;1",
>                                    "nsIURIFixup");
>@@ -1280,3 +1275,192 @@ XPCOMUtils.defineLazyGetter(PlacesUIUtil
> XPCOMUtils.defineLazyServiceGetter(PlacesUIUtils, "fm",
>                                    "@mozilla.org/focus-manager;1",
>                                    "nsIFocusManager");

Why are you keeping those?

>+/**
>+ * This is a compatibility shim for old PUIU.ptm users.

You should note here that it's highly recommended to use the new API.  I'd still like to remove this at some point.

>+ */
>+XPCOMUtils.defineLazyGetter(PlacesUIUtils, "ptm", function() {
>+  // Ensure PlacesUtils is imported in scope.
>+  PlacesUtils;
>+
>+  return {
>+    aggregateTransactions:
>+    function PTXN_aggregateTransactions(aName, aTransactions)
>+      new PlacesAggregatedTransaction(aName, aTransactions),

You really don't need to name these functions.

>+    /**
>+     * Transaction for setting/unsetting Load-in-sidebar annotation
>+     *
>+     * @param aBookmarkId
>+     *        id of the bookmark where to set Load-in-sidebar annotation
>+     * @param aLoadInSidebar
>+     *        boolean value

A couple of missing periods here.

>+     * @returns nsITransaction object
>+     */
>+    setLoadInSidebar:
>+    function PTXN_setLoadInSidebar(aItemId, aLoadInSidebar)
>+    {

Why did you move the brace to a new line (ditto for all other places)?

>+
>+function run_test() {
>+  // Create folder.
>+  var createFolderTxn = txnsvc.createFolder("Test folder",
>+                                            bmsvc.unfiledBookmarksFolder,
>+                                            bmsvc.DEFAULT_INDEX);
>+  test_GUID_persistance(createFolderTxn);
>+

I prefer testing the new API.

>+      if (!this.__lookupGetter__("transactionManager")) {

Use getOwnPropertyDescriptor instead.


>+function updateCommandsOnActiveWindow()
>+{
>+  let win = fm.activeWindow;

Is this always a chrome window - the top most one?

r=mano otherwise.

Comment 28

[Marco Bonardo [:mak]]

(In reply to comment #27)

> > XPCOMUtils.defineLazyServiceGetter(PlacesUIUtils, "URIFixup",
> >                                    "@mozilla.org/docshell/urifixup;1",
> >                                    "nsIURIFixup");
> >@@ -1280,3 +1275,192 @@ XPCOMUtils.defineLazyGetter(PlacesUIUtil
> > XPCOMUtils.defineLazyServiceGetter(PlacesUIUtils, "fm",
> >                                    "@mozilla.org/focus-manager;1",
> >                                    "nsIFocusManager");
> 
> Why are you keeping those?

what? these getters? they are used, or maybe you mean I should expose them just internally, and that makes sense.

> >+     * @returns nsITransaction object
> >+     */
> >+    setLoadInSidebar:
> >+    function PTXN_setLoadInSidebar(aItemId, aLoadInSidebar)
> >+    {
> 
> Why did you move the brace to a new line (ditto for all other places)?

actually it's the global mozilla code style (even if not a lot of people follow it), I find that it makes better readability of where methods are starting. Do you think it does not?

> >+function run_test() {
> >+  // Create folder.
> >+  var createFolderTxn = txnsvc.createFolder("Test folder",
> >+                                            bmsvc.unfiledBookmarksFolder,
> >+                                            bmsvc.DEFAULT_INDEX);
> >+  test_GUID_persistance(createFolderTxn);
> >+
> 
> I prefer testing the new API.

I know, but as I said I want a separate bug to use and test the new api. These tests are fine for now and are old tests (I did not change anything here, just newlines were wrong)

> >+      if (!this.__lookupGetter__("transactionManager")) {
> 
> Use getOwnPropertyDescriptor instead.

is it faster? Looks like it extracts far more information than what I need, so I'm curious about reasoning, if it's good we have other calls to change.

> >+function updateCommandsOnActiveWindow()
> >+{
> >+  let win = fm.activeWindow;
> 
> Is this always a chrome window - the top most one?

Hm, no I don't think it ensures it's always a chrome window, it's the current active window, but if a chrome window is not focused its commands are updated when it gets focus, isn't it?

Comment 29

[Asaf Romano (gone)]

* Ignore my comment about the getter - though your suggestion seems fine :)
* If it's not a chrome window, you cannot call updatecommands.
* __*Getter__ are supposed to be deprecated at some point.  The new method is the ecma standard, and we should start using it.
* Code style: well, if the guides say you're right, I can live with that.

Comment 30

[Marco Bonardo [:mak]]

Attachment: patch v2.1

Addresses comments

Comment 31

[Marco Bonardo [:mak]]

Attachment: patch v2.2

now, better making a working patch.

Comment 32

[Marco Bonardo [:mak]]

http://hg.mozilla.org/mozilla-central/rev/935b64ffa355