405938 - problems when import/exporting of saved searches

Status: VERIFIED FIXED

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

Description

[(not reading, please use [e-mail] instead)]

problems when import/exporting of saved searches (including the "places" folder in the personal toolbar)

here's the problem:

you might have a bookmark to

place:folder=<x>&..

on, export, we do not write out folder item ids, but we do write out place: uris (for saved searches) that include folder items.

what we could do is write out item ids on export.

on import, we won't use them for item ids (we need to create new ids as we create new folders), but we can keep a map of exported ids to new ids, and then fix up place queries.

so, item ids are internally consistent, within a bookmarks.html file (or .json).

Comment 1

[(not reading, please use [e-mail] instead)]

this is pretty critical, and should be noticable on backup/restore, for example.

Comment 2

[Dietrich Ayala (:dietrich)]

folder ids need to internally consistent in both the json and html import/export.

Comment 3

[Dietrich Ayala (:dietrich)]

bug 399264 will safeguard our default searches, queries, etc. However, it doesn't safeguard user-created searches of user-created folders.

Comment 4

[Dietrich Ayala (:dietrich)]

Attachment: fix v1

- keep a map of old->new folder ids when restoring
- resolve the restored place URIs after restore is done

this is just for backup/restore. i'll file a separate bug HTML import/export.

Comment 5

[Asaf Romano (gone)]

Comment on attachment 315842 [details] [diff] [review]
fix v1


>Index: toolkit/components/places/src/nsNavHistoryResult.cpp
>===================================================================

>@@ -3304,25 +3304,27 @@ nsNavHistoryFolderResultNode::ReindexRan
>         node->mBookmarkIndex <= aEndIndex)
>       node->mBookmarkIndex += aDelta;
>   }
> }
> 
> 
> // nsNavHistoryFolderResultNode::FindChildById
> //
>-//    Searches this folder for a node with the given URI. Returns null if not
>+//    Searches this folder for a node with the given id. Returns null if not
> //    found. Does not addref the node!
> 
> nsNavHistoryResultNode*
> nsNavHistoryFolderResultNode::FindChildById(PRInt64 aItemId,
>     PRUint32* aNodeIndex)
> {
>   for (PRInt32 i = 0; i < mChildren.Count(); i ++) {
>-    if (mChildren[i]->mItemId == aItemId) {
>+    if (mChildren[i]->mItemId == aItemId ||
>+        (mChildren[i]->IsFolder() &&
>+         mChildren[i]->GetAsFolder()->mQueryItemId == aItemId)) {

I don't understand this change. It looks to me like it may break live update of items under folder shortcuts. Why is this part of this patch anyay?


>Index: toolkit/components/places/src/utils.js
>===================================================================
>RCS file: /cvsroot/mozilla/toolkit/components/places/src/utils.js,v
>retrieving revision 1.16
>diff -u -8 -p -r1.16 utils.js
>--- toolkit/components/places/src/utils.js	15 Apr 2008 17:17:35 -0000	1.16
>+++ toolkit/components/places/src/utils.js	15 Apr 2008 21:41:04 -0000
>@@ -1037,18 +1037,22 @@ var PlacesUtils = {
> 
>     // ensure tag folder gets processed last
>     nodes[0].children.sort(function sortRoots(aNode, bNode) {
>       return (aNode.root && aNode.root == "tagsFolder") ? 1 :
>               (bNode.root && bNode.root == "tagsFolder") ? -1 : 0;
>     });
> 
>     var batch = {
>+      _utils: PlacesUtils,

I'm pretty sure that could be |_utils: this| (or self, or whatever is right in the context _outside_ of batch).

>+
>+        // fixup imported place: uris that contain folders
>+        if (searchIds.length && folderIdMap.length) {
>+          searchIds.forEach(function(aId) {

I don't think you need the scoping if block.

>+            var oldURI = this._utils.bookmarks.getBookmarkURI(aId);
>+            var uri = this._utils.fixupQuery(this._utils.bookmarks.getBookmarkURI(aId),
>+                                             folderIdMap);
>+            this._utils.bookmarks.changeBookmarkURI(aId, uri);

we should do that only if the uri has been changed (use equal())


>   /**
>    * Takes a JSON-serialized node and inserts it into the db.
>    *
>    * @param   aData
>    *          The unwrapped data blob of dropped or pasted data.
>    * @param   aContainer
>    *          The container the data was dropped or pasted into
>    * @param   aIndex
>    *          The index within the container the item was dropped or pasted at
>+   * @returns An array containing of maps of old folder ids to new folder ids,

mega nit: s/An/an/

>+  },
>+
>+  /**
>+   * Replace imported folder ids with their local counterparts in a place: URI.

replaces

>+   *
>+   * @param   aURI
>+   *          A place: URI with folder ids.
>+   * @param   aFolderIdMap
>+   *          An array mapping old folder id to new folder ids.
>+   * @returns URI
>+   *          Returns the fixed up URI if all matched. If some matched, it returns

nit: this should be just @returns he fixed up URI...

>+   *          the URI with only the matching folders included. If none matched it
>+   *          returns the input URI unchanged.
>+   */
>+  fixupQuery: function PU_fixupQuery(aQueryURI, aFolderIdMap) {

make that _fixupQuert

>+    var queries = {};
>+    var options = {};
>+    this.history.queryStringToQueries(aQueryURI.spec, queries, {}, options);

>+    var query = queries.value[0];

what about multiple queries?

>           self.bookmarks.getItemType(aJSNode.id) == self.bookmarks.TYPE_BOOKMARK) {
>         aJSNode.type = self.TYPE_X_MOZ_PLACE;
>         aJSNode.uri = aPlacesNode.uri;
>-        aJSNode.concreteId = PlacesUtils.getConcreteItemId(aPlacesNode);

Make sure we don't assume it's there elsewhere.

>Index: toolkit/components/places/tests/bookmarks/test_405938_restore_queries.js
>===================================================================

>+ *
>+ * The Initial Developer of the Original Code is Mozilla Corp.
>+ * Portions created by the Initial Developer are Copyright (C) 2008
>+ * the Initial Developer. All Rights Reserved.
>+ *
>+ * Contributor(s):
>+ *  Dietrich Ayala <[e-mail]>

(Original Author) :)

Comment 6

[Dietrich Ayala (:dietrich)]

Attachment: fix v2

Comment 7

[Dietrich Ayala (:dietrich)]

all comments fixed

Comment 8

[Asaf Romano (gone)]

Comment on attachment 316950 [details] [diff] [review]
fix v2

r=mano

Comment 9

[Dietrich Ayala (:dietrich)]

Comment on attachment 316950 [details] [diff] [review]
fix v2

drivers: this fixes backup and restore to preserve saved-searches of user-created folders. has tests.

Comment 10

[Mike Connor [:mconnor]]

Comment on attachment 316950 [details] [diff] [review]
fix v2

a=mconnor on behalf of 1.9 drivers

Comment 11

[Dietrich Ayala (:dietrich)]

Checking in toolkit/components/places/src/utils.js;
/cvsroot/mozilla/toolkit/components/places/src/utils.js,v  <--  utils.js
new revision: 1.19; previous revision: 1.18
done
RCS file: /cvsroot/mozilla/toolkit/components/places/tests/bookmarks/test_405938_restore_queries.js,v
done
Checking in toolkit/components/places/tests/bookmarks/test_405938_restore_queries.js;
/cvsroot/mozilla/toolkit/components/places/tests/bookmarks/test_405938_restore_queries.js,v  <--  test_405938_restore_queries.js
initial revision: 1.1
done

Comment 12

[Marcia Knous [:marcia]]

verified fixed using  Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9pre) Gecko/2008042806 Minefield/3.0pre. I verified by creating several saved searches, backing up all bookmark data, removing all bookmark data, and then restoring.

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