Closed Bug 405938 Opened 13 years ago Closed 13 years ago

problems when import/exporting of saved searches

Categories

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

defect

Tracking

()

VERIFIED FIXED
Firefox 3

People

(Reporter: moco, Assigned: dietrich)

References

Details

(Keywords: dataloss, Whiteboard: [has patch][has reviews])

Attachments

(1 file, 1 obsolete file)

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).
Flags: blocking-firefox3?
this is pretty critical, and should be noticable on backup/restore, for example.
Priority: -- → P1
Flags: blocking-firefox3? → blocking-firefox3+
Priority: P1 → P2
Target Milestone: --- → Firefox 3 M11
Target Milestone: Firefox 3 beta3 → Firefox 3 beta4
folder ids need to internally consistent in both the json and html import/export.
Assignee: nobody → dietrich
Depends on: 384370
Target Milestone: Firefox 3 beta4 → Firefox 3
bug 399264 will safeguard our default searches, queries, etc. However, it doesn't safeguard user-created searches of user-created folders.
Depends on: 399264
Whiteboard: [swag: 2d]
Summary: problems when import/exporting of saved searches (including the "places" folder in the personal toolbar) → problems when import/exporting of saved searches
Whiteboard: [swag: 2d] → [swag: 2d][need status update]
Attached patch fix v1 (obsolete) — Splinter Review
- 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.
Attachment #315842 - Flags: review?(mano)
Whiteboard: [swag: 2d][need status update] → [has patch][needs review mano]
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 <dietrich@mozilla.com>

(Original Author) :)
Attachment #315842 - Flags: review?(mano) → review-
Depends on: 429505
Attached patch fix v2Splinter Review
Attachment #315842 - Attachment is obsolete: true
Attachment #316950 - Flags: review?(mano)
all comments fixed
Whiteboard: [has patch][needs review mano] → [has patch][has reviews]
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.
Attachment #316950 - Flags: approval1.9?
Comment on attachment 316950 [details] [diff] [review]
fix v2

a=mconnor on behalf of 1.9 drivers
Attachment #316950 - Flags: approval1.9? → approval1.9+
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
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Flags: in-testsuite+
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.
Status: RESOLVED → VERIFIED
Blocks: 509426
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.