Closed Bug 376004 Opened 17 years ago Closed 17 years ago

Places Transactions must not live in the window global scope

Categories

(Firefox :: Bookmarks & History, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED
Firefox 3 alpha7

People

(Reporter: asaf, Assigned: stevewon)

References

Details

Attachments

(4 files, 12 obsolete files)

115.07 KB, patch
Details | Diff | Splinter Review
4.88 KB, patch
Details | Diff | Splinter Review
1.37 KB, patch
asaf
: review+
Details | Diff | Splinter Review
811 bytes, patch
Details | Diff | Splinter Review
I like how we manage to have bugs-parity, say hello to bug 168411.
Summary: Places Transactions must not live in the global scope → Places Transactions must not live in the window global scope
Flags: blocking-firefox3+
Target Milestone: --- → Firefox 3 alpha6
finding owners for all the A6 blockers. can you please put a swag in the whiteboard? then we can review and load-balance at the next places meeting.

mano, can you elaborate on what the problem is here?
Assignee: nobody → swon
Whiteboard: 3d
Whiteboard: 3d → [swag: 3d]
Basically we leak windows because the transactions prototypes live in the window global context; To fix this, the transactions should be moved to a JS/C++ service; see bug 168411 for the parallel bookmarks/ fix.
Whiteboard: [swag: 3d] → [swag: 7d]
Attached patch Patch (sort of wip) (obsolete) — Splinter Review
A couple of problems and thing's I couldn't figure out.
1. I am not able to add in bookmarks with live titles. Problem occurs at line 438 in nsPlacesTransactionManager.js where the function is modifying the id of a transaction. 

Error printed from Console:
Call to xpconnect wrapped JSObject produced this error:  *
[Exception... "Cannot modify properties of a WrappedNative"  nsresult: "0x80570034 (NS_ERROR_XPC_CANT_MODIFY_PROP_ON_WN)"  location: "JS frame :: file:///Users/steve/Desktop/moz_sources/061207/mozilla/obj-i386-apple-darwin8.9.1/dist/MinefieldDebug.app/Contents/MacOS/components/nsPlacesTransactionManager.js :: PCIT_doTransaction :: line 437"  data: no]

Does this have anything to do with functions in idl returning nsITransaction?

2. I used nsIVariant (as a temporary solution) for the datatype for several input parameters in idl... wasn't exactly sure what I should be using.

3. I had to copy over some functions from utils.js and toolkit/content/debug.js.
Not many though. Those snippets have either "Copy from utils.js" or "Copy from toolkit/content/debug.js" above them as comments.
Attachment #268900 - Flags: review?(mano)
Whiteboard: [swag: 7d]
(In reply to comment #3)
> Created an attachment (id=268900) [details]
> Patch (sort of wip)
> 
> A couple of problems and thing's I couldn't figure out.
> 1. I am not able to add in bookmarks with live titles. Problem occurs at line
> 438 in nsPlacesTransactionManager.js where the function is modifying the id of
> a transaction. 
> 
> Error printed from Console:
> Call to xpconnect wrapped JSObject produced this error:  *
> [Exception... "Cannot modify properties of a WrappedNative"  nsresult:
> "0x80570034 (NS_ERROR_XPC_CANT_MODIFY_PROP_ON_WN)"  location: "JS frame ::
> file:///Users/steve/Desktop/moz_sources/061207/mozilla/obj-i386-apple-darwin8.9.1/dist/MinefieldDebug.app/Contents/MacOS/components/nsPlacesTransactionManager.js
> :: PCIT_doTransaction :: line 437"  data: no]
> 
> Does this have anything to do with functions in idl returning nsITransaction?
> 

yep, you can only access methods/properties of nsITransaction, unless you queryInterface to another type. maybe create a nsIPlacesTransaction, and expose the id that way?

> 2. I used nsIVariant (as a temporary solution) for the datatype for several
> input parameters in idl... wasn't exactly sure what I should be using.
> 

for arrays, that's fine i think. that seems to be the preferred approach lately (see FUEL and the new tagging service).

> 3. I had to copy over some functions from utils.js and
> toolkit/content/debug.js.
> Not many though. Those snippets have either "Copy from utils.js" or "Copy from
> toolkit/content/debug.js" above them as comments.
> 

http://lxr.mozilla.org/mozilla/search?string=content%2Fdebug
you can Cu.import PlacesUtils
Or, I guess, loadSubScript it until Cu.import supports chrome:// uris.
> yep, you can only access methods/properties of nsITransaction, unless you
> queryInterface to another type. maybe create a nsIPlacesTransaction, and expose
> the id that way?

wrappedJSObject is more appropriate here, I think.
Attachment #268900 - Flags: review?(mano)
Blocks: 337190
Blocks: 385251
Attached patch checkpt 1 wip (obsolete) — Splinter Review
Attachment #269804 - Attachment is patch: true
Attachment #269804 - Attachment mime type: application/octet-stream → text/plain
The patch does run the transactions correctly.
But if you run places transactions and quit Minefield,
you will get infinite repeats of 

"WARNING: XPCOM objects created/destroyed from static ctor/dtor: 'gActivityTLS != BAD_TLS_INDEX && NS_PTR_TO_INT32(PR_GetThreadPrivate(gActivityTLS)) == 0', file /Users/steve/Desktop/moz_sources/062507/mozilla/xpcom/base/nsTraceRefcntImpl.cpp, line 1134
WARNING: XPCOM objects created/destroyed from static ctor/dtor: 'gActivityTLS != BAD_TLS_INDEX && NS_PTR_TO_INT32(PR_GetThreadPrivate(gActivityTLS)) == 0', file /Users/steve/Desktop/moz_sources/062507/mozilla/xpcom/base/nsTraceRefcntImpl.cpp, line 983
WARNING: XPCOM objects created/destroyed from static ctor/dtor: 'gActivityTLS != BAD_TLS_INDEX && NS_PTR_TO_INT32(PR_GetThreadPrivate(gActivityTLS)) == 0', file /Users/steve/Desktop/moz_sources/062507/mozilla/xpcom/base/nsTraceRefcntImpl.cpp, line 1037"

and

"WARNING: NS_ENSURE_TRUE(compMgr) failed: file nsComponentManagerUtils.cpp, line 90
WARNING: XPCOM objects created/destroyed from static ctor/dtor: 'gActivityTLS != BAD_TLS_INDEX && NS_PTR_TO_INT32(PR_GetThreadPrivate(gActivityTLS)) == 0', file /Users/steve/Desktop/moz_sources/062507/mozilla/xpcom/base/nsTraceRefcntImpl.cpp, line 1037
###!!! ASSERTION: can't get xpt search path!: 'Error', file /Users/steve/Desktop/moz_sources/062507/mozilla/xpcom/reflect/xptinfo/src/xptiInterfaceInfoManager.cpp, line 67
###!!! ASSERTION: null monitor: 'mMonitor', file ../../dist/include/xpcom/nsAutoLock.h, line 284
###!!! ASSERTION: null monitor: 'mMonitor', file ../../dist/include/xpcom/nsAutoLock.h, line 292"

here and there.
until you get "Minefield quit unexpectedly" alert from the mac.

This does not happen with "Attachment 268900 [details] [diff]: Patch (sort of wip)" btw...
Attachment #268900 - Attachment is obsolete: true
Attachment #269804 - Attachment is obsolete: true
Seems like it might be related to the bookmarks, livemarks, annotations, microsummaries, and history services being called in nsIPlacesTransactionManagerService.js
Status: NEW → ASSIGNED
Comment on attachment 269884 [details] [diff] [review]
Working patch.. with a bad problem

Drive by review since you asked me to look at this ;)

>+      if (newKeyword != this._bookmarkKeyword) 
>+        transactions.push(PlacesUtils.ptm.
>+                          editBookmarkKeyword(itemId, newKeyword));
Generally, if you go multiple lines (even if it is before a ;), you should use braces so it's easier to read.

>   /**
>+   * The Places Transaction Manager.
>+   */
>+  _ptm: null,
>+  get ptm() {
>+    if (!this._ptm) {
>+      this._ptm = Cc["@mozilla.org/browser/placesTransactionManager;1"].
>+				 getService(Ci.nsIPlacesTransactionManagerService);
nit: tabs

>-    var createTxn =
>-      new PlacesCreateFolderTransaction(title, aContainer, aIndex, annos,
>-                                        getChildItemsTransactions(aData.id));
>+    var createTxn = this.ptm.createFolder(title, aContainer, aIndex, annos,
>+										  getChildItemsTransactions(aData.id));
nit: tabs

>     case this.TYPE_UNICODE:
>       var title = type == this.TYPE_X_MOZ_URL ? data.title : data.uri.spec;
>       var createTxn =
>-        new PlacesCreateItemTransaction(data.uri, container, index, title);
>+        this.ptm.createItemTransaction(data.uri, container,
>+									   index, title, "", [], []);
nit: tabs

>+ * The Initial Developer of the Original Code is
>+ *  Mozilla
Mozilla Corporation

>+ *
>+ * Portions created by the Initial Developer are Copyright (C) 2007
>+ * the Initial Developer. All Rights Reserved.
>+ *
>+ * Contributor(s):
>+ *   Sungjoon Steve Won <stevewon@gmail.com>
nit: add "(Original Author)" to the end

>+interface nsITransactionManager;
You never actually use this.

>+  /**
>+   * Transaction for creating a new folder item.
>+   *
>+   * @param aName
>+   *        the name of the new folder
>+   * @param aContainer
>+   *        the identifier of the folder in which the new folder should be
>+   *        added.
>+   * @param [optional] aIndex
>+   *        the index of the item in aContainer, pass -1 or nothing to create
>+   *        the item at the end of aContainer.
>+   * @param [optional] aAnnotations
>+   *        the annotations to set for the new folder.
>+   * @param [optional] aChildItemsTransactions
>+   *        array of transactions for items to be created under the new folder.
>+   * @returns nsITransaction object
>+   */
>+  nsITransaction createFolder(in AString aName, in long long aContainer, in long long aIndex,
>+                              in nsIVariant aAnnotations, in nsIVariant aChildItemsTransactions);
if it truly is optional, you should use the optional flag.  See Bug 382034.  You can also have all your JS callers not pass in empty arrays/strings.

>+  nsITransaction setBookmarksToolbar(in long long aFolderId);
>+};
>\ No newline at end of file
don't forget your newlines!

General idl file nit: you don't seem to specify a lot of your arguments in the function headers with @param ...


>+
>+var microsumBkmkId = -1; //Used to store bookmark id with microsummary title
>+                         //Work around for all transactions being type nsITrasaction and 
>+                         //not being able to set transaction.id in JS.
Have you thought about extending nsITransaction and create an nsIPlacesTransaction that has the data you need stored in it?

>+function placesTransactionManagerService() {
>+  const nsISupports = Ci.nsISupports;
You don't use this constant.

>+  const CLASS_ID = Components.ID("bec866cc-9dd0-42a0-a196-6fdaa16021c4");
>+  const CLASS_NAME = "";
>+  const CONTRACT_ID = "@mozilla.org/browser/placesTransactionManager;1";
>+  const MIN_TRANSACTIONS_FOR_BATCH = 5;
this constant should be pulled out since you use it (or should use it) outside of this constructor.

>+  bookmarks = Cc["@mozilla.org/browser/nav-bookmarks-service;1"].
>+				getService(Ci.nsINavBookmarksService);
>+  history =  Cc["@mozilla.org/browser/nav-history-service;1"].
>+			   getService(Ci.nsINavHistoryService);
>+  livemarks = Cc["@mozilla.org/browser/livemark-service;2"].
>+				getService(Ci.nsILivemarkService);
>+  annotations = Cc["@mozilla.org/browser/annotation-service;1"].
>+				  getService(Ci.nsIAnnotationService);
>+  microsummaries = Cc["@mozilla.org/microsummary/service;1"].
>+					 getService(Ci.nsIMicrosummaryService);
nit: tabs in all of these

>+  this.wrappedJSObject = this;
you don't need this line anymore since you have an idl defining this

>+  this.classInfo = {
>+    getInterfaces: function (count) {
>+      var ifaces = [
>+          Ci.nsISupports,
>+          Ci.nsIClassInfo
>+      ];
do you need to add Ci.nsIPlacesTransactionManagerService here?

>+    contractID: "@mozilla.org/browser/placesTransactionManager;1",
use your const
>+    classDescription: "Places Transaction Manager",
>+    classID: Components.ID("bec866cc-9dd0-42a0-a196-6fdaa16021c4"),
use your const

>+  // The minimum amount of transactions we should tell our observers to begin
>+  // batching (rather than letting them do incremental drawing).
>+  MIN_TRANSACTIONS_FOR_BATCH: 5,
use your constant that you will pull out (see previous comment)

>+  redoTransaction: function PIT_redoTransaction() {
>+    throw Cr.NS_ERROR_NOT_IMPLEMENTED;
you never declared Cr

>+  QueryInterface:function placesTransactionsMgrQI(aIID) {
>+    if (aIID.equals(Ci.nsIPlacesTransactionManagerService) ||
>+      aIID.equals(Ci.nsISupports))
nit: spacing doesn't match up

>+      throw Components.results.NS_ERROR_NO_INTERFACE;
nit: use Cr to match the style of the rest of the file

>+  commitTransaction: function placesCommitTransaction(txn) {
>+    this.mTransactionManager.doTransaction(txn);
>+  }
>+}
nit: closing ; at the end of the object prototype

>+  undoTransaction: function PAT_undoTransaction() {
>+    LOG("== UN" + this._name + " (UNAggregate) ============");
>+    if (this._transactions.length >= this.MIN_TRANSACTIONS_FOR_BATCH) {
>+      var callback = {
>+        _self: this,
>+		runBatched: function() {
>+          this._self.commit(true);
>+        }
>+      };
>+      bookmarks.runInBatchMode(callback, null);
>+	}
>+	else
nit: tabs

>+placesCreateFolderTransactions.prototype = {
>+  __proto__: placesTransactionManagerService.prototype,
I'm not so sure this is correct (it seems wrong to me, but I could be wrong here).  Care to explain?

>+function placesCreateSeparatorTransactions(aContainer, aIndex) {
>+this._container = aContainer;
>+this._index = typeof(aIndex) == "number" ? aIndex : -1;
>+this._id = null;
>+}
nit: indent please

>+placesEditItemDescriptionTransactions.prototype = {
>+  __proto__: placesTransactionManagerService.prototype,
>+  _oldDescription: "",
>+  //DESCRIPTION_ANNO: DESCRIPTION_ANNO,
>+  //nsIAnnotationService: Ci.nsIAnnotationService,
???

>+
>+  doTransaction: function PSLIST_doTransaction() {
>+    const annos = annotations;
where is annotations coming from?  I'm not seeing it

>+/*Private functions and variables*/
>+/**
>+ * Annotate a URI with a batch of annotations.
>+ * @param aItemId
>+ *        The identifier of the item for which annotations are to be set
>+ * @param aAnnotations
>+ *        Array of objects, each containing the following properties:
>+ *        name, flags, expires, type, mimeType (only used for binary
>+ *        annotations) value.
>+ */
>+function PTM_setAnnotationsForItem(aItemId, aAnnos) {
nit: function header doesn't match with documentation

>+  var annosvc = annotations;
where is annotations coming from again?

>+// Module
>+var placesTransactionManagerServiceModule = {
>+  CLASS_ID: Components.ID("bec866cc-9dd0-42a0-a196-6fdaa16021c4"),
>+  CLASS_NAME: "",
>+  CONTRACT_ID: "@mozilla.org/browser/placesTransactionManager;1",
use your constants

That __proto__ bit is the only thing that didn't make sense to me.  I didn't see anything seriously wrong otherwise :/
this is not causing hangs, freezes or extreme-brokeness of critical features, so pushing to B1.
Target Milestone: Firefox 3 alpha6 → Firefox 3 beta1
Problem from Attachment 269884 [details] [diff] above fixed now. Problem was caused by something while loading microsummaries service. Instead of loading it in function placesTransactionManagerService, now loading in editBookmarkMicrosummary.

Also currently using a wrappedJSObject for txn.id, but will file a bug on that.
Attachment #269884 - Attachment is obsolete: true
Attached patch Patch (obsolete) — Splinter Review
Patch. Also includes a unit test that tests most of the functions.
Attachment #270049 - Attachment is obsolete: true
Attachment #270080 - Flags: review?(mano)
Unit test missing tests for:
* setLoadInSidebar
* editURIPostData
* editBookmarkMicrosummary
* sortFolderByName
Blocks: 382639
Attached patch Patch (obsolete) — Splinter Review
Patch with updated unit testing.
Only editURIPostData isn't test now, and I am not sure what would be the best way to test that.

Also filled in missing stubs in the function descriptions in the interface.
Attachment #270080 - Attachment is obsolete: true
Attachment #270236 - Flags: review?(mano)
Attachment #270080 - Flags: review?(mano)
Er, "Only editURIPostData isn't being tested** now, and I am not sure what would be the best
way to test that."
Comment on attachment 270236 [details] [diff] [review]
Patch


>Index: browser/components/places/public/nsIPlacesTransactionManagerService.idl
>===================================================================
>RCS file: browser/components/places/public/nsIPlacesTransactionManagerService.idl
>diff -N browser/components/places/public/nsIPlacesTransactionManagerService.idl
>--- /dev/null	1 Jan 1970 00:00:00 -0000
>+++ browser/components/places/public/nsIPlacesTransactionManagerService.idl	28 Jun 2007 23:01:29 -0000
>@@ -0,0 +1,323 @@
>+/* -*- Mode: IDL; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
>+/* ***** BEGIN LICENSE BLOCK *****
>+ * Version: MPL 1.1/GPL 2.0/LGPL 2.1
>+ *
>+ * The contents of this file are subject to the Mozilla Public License Version
>+ * 1.1 (the "License"); you may not use this file except in compliance with
>+ * the License. You may obtain a copy of the License at
>+ * http://www.mozilla.org/MPL/
>+ *
>+ * Software distributed under the License is distributed on an "AS IS" basis,
>+ * WITHOUT WARRANTY OF ANY KIND, either express or implied. See the License
>+ * for the specific language governing rights and limitations under the
>+ * License.
>+ *
>+ * The Original Code is Places.
>+ *
>+ * The Initial Developer of the Original Code is
>+ *  Mozilla
>+ *

Corporation


>+#include "nsITransaction.idl"
>+

you can simply forward decalre it too.

>+interface nsIVariant;
>+interface nsIURI;
>+interface nsIInputStream;
>+interface nsIMicrosummary;
>+
>+/**
>+ * nsIPlacesTransactionService is a service designed to handle
>+ * nsITransactions taht correspond to changes in Places. It is here as a

s/taht/that

>+ * service sot hat we can keep the transactions around without holding onto

so that

>+ * the whole global js scope+window.

"the global scope of a js window"

>+ */
>+
>+[scriptable, uuid(b1d44f9e-ad7f-4baf-af18-36d766599405)]
>+interface nsIPlacesTransactionManagerService : nsISupports
>+{
>+  /**
>+   * Performs a transaction.
>+   */
>+  void commitTransaction(in nsITransaction transactions);
>+
>+  /**
>+   * Transaction for performing several Places Transactions in a single batch. 
>+   * @returns nsITransaction object
>+   */
>+  nsITransaction aggregateTransactions(in AString name, in nsIVariant transactions);
>+

@params?

>+  /**
>+   * Transaction for creating a new folder item.
>+   *
>+   * @param aName
>+   *        the name of the new folder
>+   * @param aContainer
>+   *        the identifier of the folder in which the new folder should be
>+   *        added.
>+   * @param [optional] aIndex
>+   *        the index of the item in aContainer, pass -1 or nothing to create
>+   *        the item at the end of aContainer.
>+   * @param [optional] aAnnotations
>+   *        the annotations to set for the new folder.
>+   * @param [optional] aChildItemsTransactions
>+   *        array of transactions for items to be created under the new folder.

In the world of XPConnect, these are no longer implicitly optional (same for all other methods). sdwish pointed out bug 382034.


>Index: browser/components/places/src/nsPlacesTransactionManagerService.js
>===================================================================

>+ *
>+ * The contents of this file are subject to the Mozilla Public License Version
>+ * 1.1 (the "License"); you may not use this file except in compliance with
>+ * the License. You may obtain a copy of the License at
>+ * http://www.mozilla.org/MPL/
>+ *
>+ * Software distributed under the License is distributed on an "AS IS" basis,
>+ * WITHOUT WARRANTY OF ANY KIND, either express or implied. See the License
>+ * for the specific language governing rights and limitations under the
>+ * License.
>+ *
>+ * The Original Code is Places.

is the Places Command Controller.

>+ *
>+ * The Initial Developer of the Original Code is
>+ *   Mozilla Corporation

Google Inc

>+ *
>+ * Portions created by the Initial Developer are Copyright (C) 2007

2005 ;)

>+//Copy from utils.js
>+function QI_node(aNode, aIID) {
>+  var result = null;
>+  try {
>+    result = aNode.QueryInterface(aIID);
>+  }
>+  catch (e) {
>+  }
>+  NS_ASSERT(result, "Node QI Failed");
>+  return result;
>+}
>+

hrm, you could use the subscript loader (or Cu.import if that works for chrome:// by now) instead.

>+const CLASS_NAME = "";

?
>+var toolbarFolder = null;
>+var bookmarks;
>+var history;
>+var livemarks;
>+var annotations;
>+
>+function placesTransactionManagerService() {
>+
>+  bookmarks = Cc["@mozilla.org/browser/nav-bookmarks-service;1"].
>+                getService(Ci.nsINavBookmarksService);
>+  history =  Cc["@mozilla.org/browser/nav-history-service;1"].
>+               getService(Ci.nsINavHistoryService);
>+  livemarks = Cc["@mozilla.org/browser/livemark-service;2"].
>+                getService(Ci.nsILivemarkService);
>+  annotations = Cc["@mozilla.org/browser/annotation-service;1"].
>+                  getService(Ci.nsIAnnotationService);
>+

use getters instead.

>+  this.mTransactionManager = Cc["@mozilla.org/transactionmanager;1"]
>+                               .createInstance(Ci.nsITransactionManager);

nit: browser/ code style convension is
 = Cc["@mozilla.org/transactionmanager;1"].
   createInstance(Ci.nsITransactionManager);


>+  loader.loadSubScript("chrome://global/content/debug.js");

>+
>+  this.classInfo = {
...
>+  };
>+}

what's this for?

>+
>+placesTransactionManagerService.prototype = {
>+
>+  QueryInterface:function placesTransactionsMgrQI(aIID) {
>+    if (aIID.equals(Ci.nsIPlacesTransactionManagerService) ||
>+        aIID.equals(Ci.nsISupports))
>+      return this;
>+    else if (aIID.equals(Ci.nsIClassInfo))
>+      return this.classInfo;
>+    else
>+      throw Cr.NS_ERROR_NO_INTERFACE;
>+  },


use XPCOMUtils.

>+  merge: function placesMergeFunc(txn) {
>+    return false;
>+

?

>+function placesAggregateTransactions(name, transactions) {
>+  this._transactions = transactions;
>+  this._name = name;
>+  this.container = -1;
>+  this.redoTransaction = this.doTransaction;
>+}
>+
>+placesAggregateTransactions.prototype = {
>+  __proto__: placesTransactionManagerService.prototype,

....
>+      var callback = {
>+        _self: this,

ugh, it would probably be better to keep a global variable pointing to the service.

>+function placesCreateFolderTransactions(aName, aContainer, aIndex,
>+                                        aAnnotations,
>+                                        aChildItemsTransactions) {
>+  this._name = aName;
>+  this._container = aContainer;
>+  this._index = typeof(aIndex) == "number" ? aIndex : -1;

xpidl forces typeof(aIndex) == "number" here, all typeof usage in the transaction should
be double-checked (callers too).

>+placesCreateItemTransactions.prototype = {
...
>+    //microsumBkmkId = this._id;

?


>+function placesMoveItemTransactions(aItemId, aNewContainer, aNewIndex) {
>+  NS_ASSERT(!isNaN(aItemId + aNewContainer + aNewIndex), "Parameter is NaN!");

cannot happen either imo.

>+placesRemoveSeparatorTransactions.prototype = {
>+  __proto__: placesTransactionManagerService.prototype,
>+  doTransaction: function PRST_doTransaction() {
>+    LOG("Remove Separator from: " + this._oldContainer + "," + this._oldIndex);
>+    bookmarks.removeChildAt(this._oldContainer, this._oldIndex);
>+  },
>+
>+  undoTransaction: function PRST_undoTransaction() {
>+    LOG("UNRemove Separator from: " + this._oldContainer + "," + this._oldIndex);
>+    bookmarks.insertSeparator(this._oldContainer, this._oldIndex);

whose bookmarks?


>+/* Private functions and variables.
>+ * Copied from Utils.js
>+ */

loadSubScript ;)

>+// Factory
...
>+function NSGetModule(aCompMgr, aFileSpec) { return placesTransactionManagerServiceModule; }

XPCOMUtils.

>Index: browser/components/places/tests/unit/test_placesTxn.js
>===================================================================

>+//const Cc = Components.classes;
>+//const Ci = Components.classes;
>+

these are defined in *head.js IIRC.
Attachment #270236 - Flags: review?(mano) → review-
(In reply to comment #18)
> (From update of attachment 270236 [details] [diff] [review])
> 
> hrm, you could use the subscript loader (or Cu.import if that works for
> chrome:// by now) instead.

Cu.import doesn't yet. If making Cu.import understand chrome:// will keep people from writing COM in JS, I will Dive into Fastload to make it work asap.
> >+//Copy from utils.js
> >+function QI_node(aNode, aIID) {
> >+  var result = null;
> >+  try {
> >+    result = aNode.QueryInterface(aIID);
> >+  }
> >+  catch (e) {
> >+  }
> >+  NS_ASSERT(result, "Node QI Failed");
> >+  return result;
> >+}
> >+
> 
> hrm, you could use the subscript loader (or Cu.import if that works for
> chrome:// by now) instead.
> 
Can't in the current state, since PlacesTransactionManager is declared as ptm in PlacesUtils var right now.

> >+  merge: function placesMergeFunc(txn) {
> >+    return false;
> >+
> 
> ?
> 
> >+function placesAggregateTransactions(name, transactions) {
> >+  this._transactions = transactions;
> >+  this._name = name;
> >+  this.container = -1;
> >+  this.redoTransaction = this.doTransaction;
> >+}
> >+
> >+placesAggregateTransactions.prototype = {
> >+  __proto__: placesTransactionManagerService.prototype,
> 
> ....

merge is getting called somewhere when calling transaction functions in some cases. Not exactly sure which part, but it is being called. Things still work without merge, but exception is thrown then.


> >+      var callback = {
> >+        _self: this,
> 
> ugh, it would probably be better to keep a global variable pointing to the
> service.
> 
Not sure if I understand what you mean here.

> >+function placesCreateFolderTransactions(aName, aContainer, aIndex,
> >+                                        aAnnotations,
> >+                                        aChildItemsTransactions) {
> >+  this._name = aName;
> >+  this._container = aContainer;
> >+  this._index = typeof(aIndex) == "number" ? aIndex : -1;
> 
> xpidl forces typeof(aIndex) == "number" here, all typeof usage in the
> transaction should
> be double-checked (callers too).
> 
Where would I double check?

> >+function placesMoveItemTransactions(aItemId, aNewContainer, aNewIndex) {
> >+  NS_ASSERT(!isNaN(aItemId + aNewContainer + aNewIndex), "Parameter is NaN!");
> 
> cannot happen either imo.
?

 
> >+/* Private functions and variables.
> >+ * Copied from Utils.js
> >+ */
> 
> loadSubScript ;)
Can't load utils.js since ptm is declared in there right now.
 
Attached patch Patch (obsolete) — Splinter Review
Patch with some fixes except the ones I mentioned in the previous comment.
Attachment #270236 - Attachment is obsolete: true
Attachment #270608 - Flags: review?(mano)
I still don't get this at all:
 _proto_: placesTransactionManagerService.prototype

as for loadSubScript, you don't have to pass |this| as the closure... loadSubScript(...utils.js) will load PlacesUtils onto the global scope afaict (in which, pointing to the service is fine).

> > >+      var callback = {
> > >+        _self: this,
> > 
> > ugh, it would probably be better to keep a global variable pointing to the
> > service.
> > 
> Not sure if I understand what you mean here.

var gIntstance = null; in the global scope, then set it to the service whenever it's initialized.


> > >+function placesMoveItemTransactions(aItemId, aNewContainer, aNewIndex) {
> > >+  NS_ASSERT(!isNaN(aItemId + aNewContainer + aNewIndex), "Parameter is > NaN!");
> > 
> > cannot happen either imo.
> ?

integer arguments passed into a xpconnect method cannot be NaN afaict.
Attachment #270608 - Flags: review?(mano)
Attached patch Patch. (obsolete) — Splinter Review
Hey Mano. I made the changes you suggested except the one with var gInstance.
I am not exactly sure why it's going wrong, but using that way seems to result in an error that's identical to Comment #9. So I didn't make that change in the patch. 

With _proto_: placesTransactionManagerService.prototype, I was misunderstanding the purpose slightly. But I replaced it with placesBaseTransaction.prototype,like PlacesBaseTransaction.prototype in controller.js. I removed utils, bookmarks, and livemarks within placesBaseTransaction b/c they didn't seem like they were really being used. But please let me know otherwise.
Attachment #270608 - Attachment is obsolete: true
Attachment #271609 - Flags: review?(mano)
Attached patch A spelling fix (obsolete) — Splinter Review
A spelling fix.
Attachment #271609 - Attachment is obsolete: true
Attachment #271722 - Flags: review?(mano)
Attachment #271609 - Flags: review?(mano)
Hrm, any reason you didn't mark optional arguments as optional (see comment 11).
Oops. Fixing that now. Sorry.
Comment on attachment 271722 [details] [diff] [review]
A spelling fix

and please check the callers as well... I didn't test this myself
Attachment #271722 - Flags: review?(mano)
Attached patch Patch (obsolete) — Splinter Review
Optional arguments are flagged as optional now.

Also, instead of using wrappedJSObject, I created nsIPlacesTransaction instead.
Attachment #271722 - Attachment is obsolete: true
Attachment #271945 - Flags: review?(mano)
Comment on attachment 271945 [details] [diff] [review]
Patch

I think I broke undo. Canceling review until that's fixed... and anything more I find. sorry.
Attachment #271945 - Flags: review?(mano)
Yeah (and I was going to comment on this yesterday)... I figured this while updating-to-tip/cleaning-up/refactoring the service a little. Please hold on working on this.
Attached patch almost there (obsolete) — Splinter Review
* Make undo/redo work again
* drop nsIPlacesTransaction (see classinfo usage instead)
* merge some apis
* remove unnecessary arguments
* update to [some sort of] cvs tip
* random cleanup
Attachment #271945 - Attachment is obsolete: true
* installer manifests
Comment on attachment 272337 [details] [diff] [review]
almost there

steve, can you review the changes based on the comments above? r=me otherwise ;)
Attachment #272337 - Flags: review?(swon)
Attached patch One spacing fixed (obsolete) — Splinter Review
Just one line spacing fix. Changed tabs to spaces.
Thanks Mano!
Attachment #272337 - Attachment is obsolete: true
Attachment #272339 - Flags: review+
Attachment #272337 - Flags: review?(swon)
Attached patch for checkinSplinter Review
* update to cvs tip
 * go back to wrappedJSObject usage, the classinfo hack (or nsisecuritycheckedcomponent) didn't work so well
 * more cleanup
Attachment #272339 - Attachment is obsolete: true
mozilla/browser/components/places/content/bookmarkProperties.js 1.54
mozilla/browser/components/places/content/controller.js 1.169
mozilla/browser/components/places/content/moveBookmarks.js 1.9
mozilla/browser/components/places/content/utils.js 1.51
mozilla/browser/components/places/public/Makefile.in 1.11
mozilla/browser/components/places/public/nsIPlacesTransactionsService.idl
initial revision: 1.1
mozilla/browser/components/places/src/Makefile.in 1.24
mozilla/browser/components/places/src/nsPlacesTransactionsService.js 
initial revision: 1.1
mozilla/browser/components/places/tests/unit/test_placesTxn.js
initial revision: 1.1
mozilla/browser/installer/unix/packages-static 1.115
mozilla/browser/installer/windows/packages-static 1.127
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Flags: in-testsuite+
*Oops*, that was not the very last version... (didn't include wrppaedJSObject usage and such), I will fix that later today
Small fix.
I must have missed this before. Oi! Sorry.
Attachment #272516 - Flags: review?(mano)
Comment on attachment 272516 [details] [diff] [review]
Small fix. Check in please?

r=mano
Attachment #272516 - Flags: review?(mano) → review+
mozilla/browser/components/places/content/bookmarkProperties.js 1.55
gah
mozilla/browser/components/places/src/nsPlacesTransactionsService.js 1.3
verified with: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a7pre) Gecko/2007071705 Minefield/3.0a7pre.. per Gavin's  suggestion, tested against 168411

Status: RESOLVED → VERIFIED
Depends on: 388672
Depends on: 448804
Bug 451915 - move Firefox/Places bugs to Firefox/Bookmarks and History. Remove all bugspam from this move by filtering for the string "places-to-b-and-h".

In Thunderbird 3.0b, you do that as follows:
Tools | Message Filters
Make sure the correct account is selected. Click "New"
Conditions: Body   contains   places-to-b-and-h
Change the action to "Delete Message".
Select "Manually Run" from the dropdown at the top.
Click OK.

Select the filter in the list, make sure "Inbox" is selected at the bottom, and click "Run Now". This should delete all the bugspam. You can then delete the filter.

Gerv
Component: Places → Bookmarks & History
QA Contact: places → bookmarks
You need to log in before you can comment on or make changes to this bug.