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)
Firefox
Bookmarks & History
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.
Reporter | ||
Updated•17 years ago
|
Summary: Places Transactions must not live in the global scope → Places Transactions must not live in the window global scope
Updated•17 years ago
|
Flags: blocking-firefox3+
Updated•17 years ago
|
Target Milestone: --- → Firefox 3 alpha6
Comment 1•17 years ago
|
||
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
Assignee | ||
Updated•17 years ago
|
Whiteboard: 3d
Assignee | ||
Updated•17 years ago
|
Whiteboard: 3d → [swag: 3d]
Reporter | ||
Comment 2•17 years ago
|
||
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.
Assignee | ||
Updated•17 years ago
|
Whiteboard: [swag: 3d] → [swag: 7d]
Assignee | ||
Comment 3•17 years ago
|
||
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)
Assignee | ||
Updated•17 years ago
|
Whiteboard: [swag: 7d]
Comment 4•17 years ago
|
||
(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
Reporter | ||
Comment 5•17 years ago
|
||
you can Cu.import PlacesUtils
Reporter | ||
Comment 6•17 years ago
|
||
Or, I guess, loadSubScript it until Cu.import supports chrome:// uris.
Reporter | ||
Comment 7•17 years ago
|
||
> 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.
Reporter | ||
Updated•17 years ago
|
Attachment #268900 -
Flags: review?(mano)
Assignee | ||
Comment 8•17 years ago
|
||
Assignee | ||
Updated•17 years ago
|
Attachment #269804 -
Attachment is patch: true
Attachment #269804 -
Attachment mime type: application/octet-stream → text/plain
Assignee | ||
Comment 9•17 years ago
|
||
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
Assignee | ||
Comment 10•17 years ago
|
||
Seems like it might be related to the bookmarks, livemarks, annotations, microsummaries, and history services being called in nsIPlacesTransactionManagerService.js
Status: NEW → ASSIGNED
Comment 11•17 years ago
|
||
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 :/
Comment 12•17 years ago
|
||
this is not causing hangs, freezes or extreme-brokeness of critical features, so pushing to B1.
Target Milestone: Firefox 3 alpha6 → Firefox 3 beta1
Assignee | ||
Comment 13•17 years ago
|
||
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
Assignee | ||
Comment 14•17 years ago
|
||
Patch. Also includes a unit test that tests most of the functions.
Attachment #270049 -
Attachment is obsolete: true
Attachment #270080 -
Flags: review?(mano)
Assignee | ||
Comment 15•17 years ago
|
||
Unit test missing tests for: * setLoadInSidebar * editURIPostData * editBookmarkMicrosummary * sortFolderByName
Assignee | ||
Comment 16•17 years ago
|
||
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)
Assignee | ||
Comment 17•17 years ago
|
||
Er, "Only editURIPostData isn't being tested** now, and I am not sure what would be the best way to test that."
Reporter | ||
Comment 18•17 years ago
|
||
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-
Comment 19•17 years ago
|
||
(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.
Assignee | ||
Comment 20•17 years ago
|
||
> >+//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.
Assignee | ||
Comment 21•17 years ago
|
||
Patch with some fixes except the ones I mentioned in the previous comment.
Attachment #270236 -
Attachment is obsolete: true
Attachment #270608 -
Flags: review?(mano)
Reporter | ||
Comment 22•17 years ago
|
||
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.
Assignee | ||
Updated•17 years ago
|
Attachment #270608 -
Flags: review?(mano)
Assignee | ||
Comment 23•17 years ago
|
||
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)
Assignee | ||
Comment 24•17 years ago
|
||
A spelling fix.
Attachment #271609 -
Attachment is obsolete: true
Attachment #271722 -
Flags: review?(mano)
Attachment #271609 -
Flags: review?(mano)
Reporter | ||
Comment 25•17 years ago
|
||
Hrm, any reason you didn't mark optional arguments as optional (see comment 11).
Assignee | ||
Comment 26•17 years ago
|
||
Oops. Fixing that now. Sorry.
Reporter | ||
Comment 27•17 years ago
|
||
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)
Assignee | ||
Comment 28•17 years ago
|
||
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)
Assignee | ||
Comment 29•17 years ago
|
||
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)
Reporter | ||
Comment 30•17 years ago
|
||
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.
Reporter | ||
Comment 31•17 years ago
|
||
* 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
Reporter | ||
Comment 32•17 years ago
|
||
* installer manifests
Reporter | ||
Comment 33•17 years ago
|
||
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)
Assignee | ||
Comment 34•17 years ago
|
||
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)
Reporter | ||
Comment 35•17 years ago
|
||
* 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
Reporter | ||
Comment 36•17 years ago
|
||
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
Reporter | ||
Updated•17 years ago
|
Flags: in-testsuite+
Reporter | ||
Comment 37•17 years ago
|
||
*Oops*, that was not the very last version... (didn't include wrppaedJSObject usage and such), I will fix that later today
Reporter | ||
Comment 38•17 years ago
|
||
Assignee | ||
Comment 39•17 years ago
|
||
Small fix. I must have missed this before. Oi! Sorry.
Attachment #272516 -
Flags: review?(mano)
Reporter | ||
Comment 40•17 years ago
|
||
Comment on attachment 272516 [details] [diff] [review] Small fix. Check in please? r=mano
Attachment #272516 -
Flags: review?(mano) → review+
Reporter | ||
Comment 41•17 years ago
|
||
mozilla/browser/components/places/content/bookmarkProperties.js 1.55
Reporter | ||
Comment 42•17 years ago
|
||
gah mozilla/browser/components/places/src/nsPlacesTransactionsService.js 1.3
Comment 43•17 years ago
|
||
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
Comment 44•15 years ago
|
||
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.
Description
•