379880 - clean up Places Transactions

Status: RESOLVED INVALID

(Firefox :: Bookmarks & History, defect)

Description

[tabmix.onemen]

changes to Places Transactions
===================================================================
PlacesCreateFolderTransaction undoTransaction:
  fixes typo in _childItemsTransactions loop.
  remove the folder after undoing its _childItemsTransactions.

PlacesCreateItemTransaction undoTransaction:
  remove the item after undoing its _childTransactions.

PlacesCreateSeparatorTransaction doTransaction:
  save the new separator index if we pass new index as -1 we can't undo the transaction without a valid index.

PlacesMoveItemTransaction doTransaction/undoTransaction:
  move keyword.

PlacesRemoveFolderTransaction doTransaction:
  save annotations.
  don't remove children for livmark here we do it in nsILivemarksService::deleteLivemarkChildren.

PlacesRemoveFolderTransaction undoTransaction:
   restore annotations and if the folder is livemark reloadLivemarkFolder.
   also changed nsILivemarksService::reloadLivemarkFolder to update livemark internal table.

PlacesRemoveItemTransaction doTransaction:
  save keyword.

PlacesRemoveItemTransaction undoTransaction:
  restore keyword.
  restore annotations.

PlacesSetLoadInSidebarTransaction doTransaction:
  clean up.

PlacesEditItemDescriptionTransaction undoTransaction:
  fixes - hasAnnotation is not a function of this.utils.

PlacesEditBookmarkMicrosummaryTransaction doTransaction/undoTransaction:
  clean up.

PlacesSetBookmarksToolbarTransaction
  fixes typo, also fixes the PlacesUtils.toolbarFolderId getter

Comment 1

[tabmix.onemen]

Attachment: proposing patch

Comment 2

[Asaf Romano (gone)]

Comment on attachment 263931 [details] [diff] [review]
proposing patch

Many thanks for this work! You may want to wait for bug 379211 to land before processing this further though.

>Index: toolkit/components/places/src/nsLivemarkService.js
>===================================================================
>@@ -378,17 +378,27 @@ LivemarkService.prototype = {
> 
>   reloadAllLivemarks: function LS_reloadAllLivemarks() {
>     for (var i = 0; i < this._livemarks.length; ++i) {
>       this._updateLivemarkChildren(i, true);
>     }
>   },
> 
>   reloadLivemarkFolder: function LS_reloadLivemarkFolder(folderID) {
>-    var livemarkIndex = this._getLivemarkIndex(folderID);  
>+    try {
>+      var livemarkIndex = this._getLivemarkIndex(folderID);
>+    } catch (e) {
>+      // if this folder is livemark add it to our internal table
>+      // we probably after undo removeFolder.

I don't understand this comment, please rephrase.

>Index: browser/components/places/content/controller.js
>===================================================================
> /**
>  * Transaction for creating a new separator item
>  *
>  * @param aContainer
>  *        the identifier of the folder in which the separator should be
>  *        added.
>  * @param [optional] aIndex
>  *        the index of the item in aContainer, pass -1 or nothing to create
>  *        the separator at the end of aContainer.
>  */
> function PlacesCreateSeparatorTransaction(aContainer, aIndex) {
>   this._container = aContainer;
>   this._index = typeof(aIndex) == "number" ? aIndex : -1;
>-  this._id = null;
> }
> PlacesCreateSeparatorTransaction.prototype = {
>   __proto__: PlacesBaseTransaction.prototype,
> 
>   // childItemsTransaction support
>   get container() { return this._container; },
>   set container(val) { return this._container = val; },
> 
>+  observer: {
>+    onBeginUpdateBatch: function() {    },
>+    onEndUpdateBatch: function() {    },
>+    onSeparatorAdded: function(folder, index) {
>+      this.txn._index = index;
>+      this.txn.bookmarks.removeObserver(this);
>+    }
>+  },
>+
>   doTransaction: function PCST_doTransaction() {
>     this.LOG("Create separator in: " + this.container + "," + this._index);
>-    this._id = this.bookmarks.insertSeparator(this.container, this._index);
>+    //XXX insertSeparator don't return id for new separator
>+    // so we can't use getItemIndex to get the index
>+    // we can't undo this transaction if this._index is -1
>+    if (this._index < 0) {
>+      this.observer.txn = this;
>+      this.bookmarks.addObserver(this.observer, false);
>+    }
>+

Rather file a bug on insertSeparator to return the new id, cc or assign it me.

> PlacesCreateLivemarkTransaction.prototype = {
>   __proto__: PlacesBaseTransaction.prototype,
> 
>   // childItemsTransaction support
>   get container() { return this._container; },
>   set container(val) { return this._container = val; },
> 
>   doTransaction: function PCLT_doTransaction() {
>-    this._id = this.utils.livemarks
>+    this._id = this.livemarks

prefer this.utils.livemarks for now.

>                    .createLivemark(this._container, this._name, this._siteURI,
>                                    this._feedURI, this._index);
>-    if (this._annotations)  {
>-      var placeURI = this.utils.bookmarks.getFolderURI(this._id);
>+    if (this._annotations.length > 0) {
>+      var placeURI = this.bookmarks.getFolderURI(this._id);

ditto (we were planning to get rid of the per transaction varibles).

> /**
>  * Remove an Item
>  */
> function PlacesRemoveItemTransaction(id, uri, oldContainer, oldIndex) {
>   this._id = id;
>   this._uri = uri;
>   this._oldContainer = oldContainer;
>   this._oldIndex = oldIndex;
>+  this._keyword = "";
>   this._annotations = [];
>   this.redoTransaction = this.doTransaction;
> }
> PlacesRemoveItemTransaction.prototype = {
>-  __proto__: PlacesBaseTransaction.prototype, 
>-  
>+  __proto__: PlacesBaseTransaction.prototype,
>+
>   doTransaction: function PRIT_doTransaction() {
>     this.LOG("Remove Item: " + this._uri.spec + " from: " + this._oldContainer + "," + this._oldIndex);
>     this._title = this.bookmarks.getItemTitle(this._id);
>-    this._placeURI = this.bookmarks.getItemURI(this._id);
>-    this._annotations = this.utils.getAnnotationsForURI(this._placeURI);
>-    PlacesUtils.annotations.removePageAnnotations(this._placeURI);
>+    this._keyword = this.bookmarks.getKeywordForBookmark(this._id);
>+    var placeURI = this.bookmarks.getItemURI(this._id);
>+    this._annotations = this.utils.getAnnotationsForURI(placeURI);
>+    //XXX leave this here until bug 375629 land
>+    this.utils.annotations.removePageAnnotations(placeURI);

Or you could just remove this, that bug is blocking turning on places-bookmarks on trunk so.

> /**
>  * Remove a separator
>  */
> function PlacesRemoveSeparatorTransaction(oldContainer, oldIndex) {
>   this._oldContainer = oldContainer;
>@@ -2071,29 +2115,26 @@ PlacesSetLoadInSidebarTransaction.protot
>     type: Ci.nsIAnnotationService.TYPE_INT32,
>     value: 1,
>     flags: 0,
>     expires: Ci.nsIAnnotationService.EXPIRE_NEVER
>   },
> 
>   doTransaction: function PSLIST_doTransaction() {
>     if (!("_placeURI" in this))
>-      this._placeURI = this.utils.bookmarks.getItemURI(this.id);
>+      this._placeURI = this.bookmarks.getItemURI(this.id);

>@@ -2117,19 +2158,19 @@ PlacesEditItemDescriptionTransaction.pro
>   DESCRIPTION_ANNO: DESCRIPTION_ANNO,
>   nsIAnnotationService: Components.interfaces.nsIAnnotationService,
> 
>   doTransaction: function PSLIST_doTransaction() {
>     const annos = this.utils.annotations;
> 
>     if (!("_placeURI" in this)) {
>       if (this._isFolder)
>-        this._placeURI = this.utils.bookmarks.getFolderURI(this.id);
>+        this._placeURI = this.bookmarks.getFolderURI(this.id);
>       else
>-        this._placeURI = this.utils.bookmarks.getItemURI(this.id);
>+        this._placeURI = this.bookmarks.getItemURI(this.id);
>     }


Again, prefer this.utils.bookmarks.

Comment 3

[tabmix.onemen]

(In reply to comment #2)
> >   reloadLivemarkFolder: function LS_reloadLivemarkFolder(folderID) {
> >-    var livemarkIndex = this._getLivemarkIndex(folderID);  
> >+    try {
> >+      var livemarkIndex = this._getLivemarkIndex(folderID);
> >+    } catch (e) {
> >+      // if this folder is livemark add it to our internal table
> >+      // we probably after undo removeFolder.
> 
> I don't understand this comment, please rephrase.

from bug 378997 comment 5:
> when we remove livemark nsILivemarksService::onContainerRemoving splice the
> livemark from _livemarks list.
> 
> -----> this._livemarks.splice(livemarkIndex, 1);
> 
> 
> if we undo the transaction there is no code to put the live mark back to to the
> list.
> so after the undo every call to nsILivemarksService::_getLivemarkIndex throw
> Cr.NS_ERROR_INVALID_ARG
> 
> we need to add new function to nsILivemarksService to add livemark to the list
> 

i hope this explained what i was trying to do.
even if we add new public function to nsILivemarksService. we still have to verify that the livemark is not in the list before we add it, so we end up with function that look the same as my modified reloadLivemarkFolder.

if you think that we need a comment in the function, can you suggest the comment. 


> >+    //XXX insertSeparator don't return id for new separator
> >+    // so we can't use getItemIndex to get the index
> >+    // we can't undo this transaction if this._index is -1
> >+    if (this._index < 0) {
> >+      this.observer.txn = this;
> >+      this.bookmarks.addObserver(this.observer, false);
> >+    }
> Rather file a bug on insertSeparator to return the new id, cc or assign it me.

filed new bug 379952


ansaction() {
> >-    this._id = this.utils.livemarks
> >+    this._id = this.livemarks
> 
> prefer this.utils.livemarks for now.

> >-        this._placeURI = this.utils.bookmarks.getItemURI(this.id);
> >+        this._placeURI = this.bookmarks.getItemURI(this.id);
> 
> Again, prefer this.utils.bookmarks.

can you explain why?  
do you this this code in PlacesBaseTransaction is unnecessary?
  bookmarks: Cc["@mozilla.org/browser/nav-bookmarks-service;1"].
             getService(Ci.nsINavBookmarksService),
  _livemarks: null,
  get livemarks() {
    if (!this._livemarks) {
      this._livemarks =
        Cc["@mozilla.org/browser/livemark-service;2"].
        getService(Ci.nsILivemarkService);
    }
    return this._livemarks;
  },

Comment 4

[tabmix.onemen]

Attachment: patch ver 2

Comment 5

[Asaf Romano (gone)]

> +    //XXX insertSeparator don't return id for new separator

It now does.

Comment 6

[Asaf Romano (gone)]

(In reply to comment #3)
> > >-    this._id = this.utils.livemarks
> > >+    this._id = this.livemarks
> > 
> > prefer this.utils.livemarks for now.
> 
> > >-        this._placeURI = this.utils.bookmarks.getItemURI(this.id);
> > >+        this._placeURI = this.bookmarks.getItemURI(this.id);
> > 
> > Again, prefer this.utils.bookmarks.
> 
> can you explain why?  
> do you this this code in PlacesBaseTransaction is unnecessary?

Well, kinda; There's no point in keeping a reference to the each service in every transaction. All of this may change when we fix bug 376004 though.

Comment 7

[Asaf Romano (gone)]

And also, if we're undoing a livemark-removal, it has to be added using the livemark service (not using createFolder, that is), otherwise the moz_bookmarks entry for the livemark is not set properly.

Comment 8

[tabmix.onemen]

(In reply to comment #7)
> And also, if we're undoing a livemark-removal, it has to be added using the
> livemark service (not using createFolder, that is), otherwise the moz_bookmarks
> entry for the livemark is not set properly.
> 
i see.

if the only problem is sending delay notifications, can we do it from the undoTransaction with setTimeout ?

if not is it safe to call createLivemark.?
the new livemark id need to be the same as the old one

Comment 9

[Asaf Romano (gone)]

delay notifications?

I'm fine with _not_ keeping the old livemark-children cached or any such mess. Just call createLivemark and it will get all the work done for you.

createFolder alone does not set the folderType field of the folder item (in the moz_bookmarks table) to the livemark-service contanct-id, this could lead to all sort of wrong behaviors.

Comment 10

[tabmix.onemen]

look like we need PlacesRemoveLivemarkTransaction 

i'll be busy all week. hope to find some time next week end :)

Comment 11

[Dietrich Ayala (:dietrich)]

not modified since May 07, likely all rotted. i haven't looked through the patch, anything here worth un-rotting and pushing into the release?

Comment 12

[Dietrich Ayala (:dietrich)]

over a year old, no response from owner, and seeing as this is pre-placesTransactionService, much probably is no longer valid -> INVALID.

Comment 13

[Gervase Markham [:gerv]]

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