376253 - places bookmarks.html import/export needs comprehensive tests

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[Dietrich Ayala (:dietrich)]

we need to ensure no data loss when moving from old bookmarks to places bookmarks. this would best be accomplished with a comprehensive test suite that compares an old bookmarks.html file w/ a new bookmarks.html file (exported from places), and which exercises all possible fields.

Comment 1

[Dietrich Ayala (:dietrich)]

the test framework is started in the patch to bug 376008.

Comment 2

[Dietrich Ayala (:dietrich)]

Attachment: fix v1

adds round-tripping of the canonical bookmarks dataset (eg: import legacy file, run tests, export, import the exported file, run tests again), and testing of import-to-folder.

also fixes a few bugs exposed by the tests:

- bug 376008 regressed the "don't kick off http traffic at startup", so added arguments to the import apis to indicate whether or not it's an initial import

- import of bookmarks, livemarks, folders with pre-existing ids was not checking validity of the ids

- typo in bug 376008 regressed manual import via the organizer

- deletion of the bookmarks toolbar folder was not resetting mToolbarFolder

and some clean-up:

- removed unnecessary specification of OnlyBookmarked in bookmark export code

- removed export of places root, is unnecessary now that we don't import it anymore

- remove file created by the tests

Comment 3

[Dietrich Ayala (:dietrich)]

(In reply to comment #2)
> 
> - deletion of the bookmarks toolbar folder was not resetting mToolbarFolder
> 

hrm, removing the toolbar folder is a bug, per bug 220932.

Comment 4

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

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

r=sspitzer, two questions / comments:

+  // unset toolbar folder
+  if (mToolbarFolder == aFolder) {
+    mToolbarFolder = 0;
+  }

are you going to keep that code (per your comment #3?)

also, for nsIPlacesImportExportService, shouldn't you be bumping the uuid since the interface is changing?

Comment 5

[Dietrich Ayala (:dietrich)]

cancelled review. import is creating duplicates w/ this patch. (eg: if i import the bookmarks.html file from a profile into itself, there should be no duplicates after the import).

(In reply to comment #4)
> (From update of attachment 263702 [details] [diff] [review])
> r=sspitzer, two questions / comments:
> 
> +  // unset toolbar folder
> +  if (mToolbarFolder == aFolder) {
> +    mToolbarFolder = 0;
> +  }
> 
> are you going to keep that code (per your comment #3?)

no, going to make the toolbar folder un-delete-able.

> 
> also, for nsIPlacesImportExportService, shouldn't you be bumping the uuid since
> the interface is changing?
> 

yep

Comment 6

[Dietrich Ayala (:dietrich)]

Attachment: fix v2

wip patch. adds a test for importing over an import. not passing, so commented out atm. is importing livemark duplicates, and moving a separator (?!).

> > 
> > +  // unset toolbar folder
> > +  if (mToolbarFolder == aFolder) {
> > +    mToolbarFolder = 0;
> > +  }
> > 
> > are you going to keep that code (per your comment #3?)
> 
> no, going to make the toolbar folder un-delete-able.
> 

i left this in for now, as the front-end prevents deleting the special read-only folders.

adding protection in the api causes blow-ups elsewhere (eg removeFolderChildren(bmsvc.bookmarksRoot), unless we silently fail, which is also not a good option.

Comment 7

[Dietrich Ayala (:dietrich)]

Attachment: fix v3

fixed the duplicate livemark problem, but not the duplicate separator issue. however, i don't think we should block on fixing that issue to get these tests and fixes landed. i'll file a follow-up for the separator problem.

Comment 8

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

a reminder from bug #334758:  we should make sure to add test cases that test the escaping / unescaping of values (like descriptions, titles, uris, etc)

Comment 9

[Asaf Romano (gone)]

Comment on attachment 264159 [details] [diff] [review]
fix v3

needs update

Comment 10

[Dan Mills [:thunder]]

Attachment: fiv v4

Updated patch for latest changes in the trunk.

Applies on top of 'wip patch mk IV' from bug 373501 and disables importing the microsummary to prevent bug 380468.

Comment 11

[Dan Mills [:thunder]]

Attachment: fix v5

* update for trunk changes
* add livemark, load-in-sidebar, and post data checks
* last charset test (and import/export code) to be added to bug 334408 shortly
* I believe this version tests everything we currently import (which is missing a few things like date added, etc) except icons

Comment 12

[Dan Mills [:thunder]]

Attachment: fix v6

* Enables import-to-folder test, from pre-places and from exported bookmarks.html files.
* Enables importing on top of existing bookmarks
* Adds id exporting to separators (needed to fix 'on top' import above)
* Fixes validity checks for separators and livemarks
* Adds some debug statements (disabled)

Comment 13

[Asaf Romano (gone)]

Comment on attachment 265064 [details] [diff] [review]
fix v6


>diff -r 6a5ef010195b browser/components/places/src/nsPlacesImportExportService.cpp
>--- a/browser/components/places/src/nsPlacesImportExportService.cpp	Tue May 15 18:03:33 2007 -0700
>+++ b/browser/components/places/src/nsPlacesImportExportService.cpp	Wed May 16 16:38:44 2007 -0700

> // BookmarkContentSink::HandleSeparator
> //
> //    Inserts a separator into the current container
> void
>-BookmarkContentSink::HandleSeparator(const nsIParserNode& node)
>+BookmarkContentSink::HandleSeparator(const nsIParserNode& aNode)
> {
>   BookmarkImportFrame& frame = CurFrame();
> 
>-  // bookmarks.html contains a separator between the toolbar menu and the
>-  // rest of the items.  Since we pull the toolbar menu out into the top level,
>-  // we want to skip over this separator since it looks out of place.
>-  if (frame.mLastContainerType != BookmarkImportFrame::Container_Toolbar) {
>+  // create the separator
>+
>+  // check for pre-existing id
>+  PRInt64 id = 0;
>+  nsAutoString idAttr;
>+  PRInt32 attrCount = aNode.GetAttributeCount();
>+  for (PRInt32 i = 0; i < attrCount; i ++) {
>+    const nsAString& key = aNode.GetKeyAt(i);
>+    if (key.LowerCaseEqualsLiteral(KEY_ID_LOWER)) {
>+      idAttr = aNode.GetValueAt(i);
>+    }
>+  }
>+
>+  idAttr.Trim(kWhitespace);
>+  id = ConvertImportedIdToInternalId(NS_ConvertUTF16toUTF8(idAttr));
>+
>+  // check id validity
>+  if (id > 0) {
>+    PRInt64 parent;
>+    nsresult rv = mBookmarksService->GetFolderIdForItem(id, &parent);
>+    if (NS_FAILED(rv) || parent != frame.mContainerID) {
>+      id = 0;
>+    }
>+  }
>+
>+  if (id > 0) {
>+    PRUint16 type;
>+    nsresult rv = mBookmarksService->GetItemType(id, &type);
>+    if (NS_FAILED(rv) || type != mBookmarksService->TYPE_SEPARATOR) {

NS_ENSURE_SUCCESS(rv, rv) here, GetItemType should never throw if say,
GetFolderIdForItem succeeded

> nsresult
> nsPlacesImportExportService::ImportHTMLFromFileInternal(nsILocalFile* aFile,
>                                        PRBool aAllowRootChanges,
>                                        PRInt64 aFolder,
>                                        PRBool aIsImportDefaults)
> {
> #endif
>+
>+  // confirm file exists
>+  PRBool exists;
>+  rv = file->Exists(&exists);
>+  if (!NS_SUCCEEDED(rv) || !exists) {

if NS_FAILED(rv), or better: NS_ENSURE_SUCCESS.


> NS_IMETHODIMP
> nsPlacesImportExportService::ExportHTMLToFile(nsILocalFile* aBookmarksFile)
> {
>   if (!aBookmarksFile)
>     return NS_ERROR_NULL_POINTER;
> 
>+#ifdef DEBUG_EXPORT
>+  nsCOMPtr<nsIFile> file(do_QueryInterface(aBookmarksFile));
>+  nsAutoString path;
>+  file->GetPath(path);
>+  printf("\nExporting %s\n", NS_ConvertUTF16toUTF8(path).get());

nit of the day: nsILocalFile inherits from nsIFile, thus you can just use aBookmarksFile.

> 
>   // prologue
>   rv = WriteContainerPrologue(EmptyCString(), strm);
>   NS_ENSURE_SUCCESS(rv, rv);
> 
>   // indents
>   nsCAutoString indent;
>   indent.Assign(kIndent);
>-
>-  // places root
>-  rv = WriteContainer(mPlacesRoot, indent, strm);
>-  NS_ENSURE_SUCCESS(rv, rv);
>

Why is this change? (I'm not yet sure whether we should keep the placeRoot folder in place; though it's broken on trunnk, we might need if later for tags UI).


>diff -r 6a5ef010195b browser/components/places/tests/unit/head_bookmarks.js
>--- a/browser/components/places/tests/unit/head_bookmarks.js	Tue May 15 18:03:33 2007 -0700
>+++ b/browser/components/places/tests/unit/head_bookmarks.js	Wed May 16 16:38:44 2007 -0700
>@@ -78,18 +78,24 @@ if (!profileDir) {
> }
> 
> var iosvc = Cc["@mozilla.org/network/io-service;1"].getService(Ci.nsIIOService);
> 
> function uri(spec) {
>   return iosvc.newURI(spec, null, null);
> }
> 
>-// Delete a previously created sqlite file
>-function clearDB() {
>+function cleanUp() {
>   try {
>+    // Delete a previously created sqlite file
>     var file = dirSvc.get('ProfD', Ci.nsIFile);
>     file.append("places.sqlite");
>     if (file.exists())
>       file.remove(false);
>+
>+    // Delete exported bookmarks html file
>+    file = firSvc.get('ProfD', Ci.nsIFile);
>+    file.append("bookmarks.exported.html");
>+    if (file.exists())
>+      file.remove(false);

I would rather do that change in the test_bookmarks.js only.


>diff -r 6a5ef010195b browser/components/places/tests/unit/test_bookmarks_html.js
>--- a/browser/components/places/tests/unit/test_bookmarks_html.js	Tue May 15 18:03:33 2007 -0700
>+++ b/browser/components/places/tests/unit/test_bookmarks_html.js	Wed May 16 16:38:44 2007 -0700

>+  var query = histsvc.getNewQuery();
>+  query.setFolders([aFolder], 1);
>+  var result = histsvc.executeQuery(query, histsvc.getNewQueryOptions());
>+  var rootNode = result.root;
>+  rootNode.containerOpen = true;
>+  do_check_eq(rootNode.childCount, 6);
>+  for (var i = 0; i < rootNode.childCount; i++) {
>+    var child = rootNode.getChild(i);
>+    var title = child.title;

Unused? 

>+    if (i == 2) { // test bookmark toolbar contents
>+      child.QueryInterface(Ci.nsINavHistoryQueryResultNode);
>+      child.containerOpen = true;
>+      do_check_eq(child.childCount, 2);
>+
>+      // livemark
>+      var livemark = child.getChild(1);
>+      // title
>+      do_check_eq("Latest Headlines", livemark.title);
>+      // livemark check
>+      do_check_true(livemarksvc.isLivemark(livemark.itemId));
>+      // site url
>+      do_check_eq("http://en-us.fxfeeds.mozilla.com/en-US/firefox/livebookmarks/",
>+                  livemarksvc.getSiteURI(livemark.itemId).spec);
>+      // feed url
>+      do_check_eq("http://en-us.fxfeeds.mozilla.com/en-US/firefox/headlines.xml",
>+                  livemarksvc.getFeedURI(livemark.itemId).spec);
>+
>+      child.containerOpen = false;
>+    }
>   }

what is this loop for if you only need detials for getChild(2)?


>diff -r 6a5ef010195b toolkit/components/places/src/nsNavBookmarks.cpp
>--- a/toolkit/components/places/src/nsNavBookmarks.cpp	Tue May 15 18:03:33 2007 -0700
>+++ b/toolkit/components/places/src/nsNavBookmarks.cpp	Wed May 16 16:38:45 2007 -0700
>@@ -1247,16 +1247,20 @@ nsNavBookmarks::RemoveFolder(PRInt64 aFo
>   NS_ENSURE_SUCCESS(rv, rv);
> 
>   rv = AdjustIndices(parent, index + 1, PR_INT32_MAX, -1);
>   NS_ENSURE_SUCCESS(rv, rv);
> 
>   rv = transaction.Commit();
>   NS_ENSURE_SUCCESS(rv, rv);
> 
>+  if (aFolder == mToolbarFolder) {
>+    mToolbarFolder = 0;
>+  }
>+

I would rather throw here.


r=mano otherwise, I didn't review each change here though so please get another review for Seth or Dietrich.

Comment 14

[Dan Mills [:thunder]]

(In reply to comment #13)

> >+  if (id > 0) {
> >+    PRUint16 type;
> >+    nsresult rv = mBookmarksService->GetItemType(id, &type);
> >+    if (NS_FAILED(rv) || type != mBookmarksService->TYPE_SEPARATOR) {
> 
> NS_ENSURE_SUCCESS(rv, rv) here, GetItemType should never throw if say,
> GetFolderIdForItem succeeded

It's a void function, so I could do

if (NS_FAILED(rv)
  return;

Do you think that's better than what's there now?

> >-  // places root
> >-  rv = WriteContainer(mPlacesRoot, indent, strm);
> >-  NS_ENSURE_SUCCESS(rv, rv);
> >
> 
> Why is this change? (I'm not yet sure whether we should keep the placeRoot
> folder in place; though it's broken on trunnk, we might need if later for tags
> UI).

Just discussed it with Dietrich over irc.  Our general feeling is that atm it doesn't get used and it adds confusion, as well as an additional empty folder when imported back into a non-places build.  We should be able to add it back if we ever need it.

> >-// Delete a previously created sqlite file
> >-function clearDB() {
> >+function cleanUp() {
> >   try {
> >+    // Delete a previously created sqlite file
> >     var file = dirSvc.get('ProfD', Ci.nsIFile);
> >     file.append("places.sqlite");
> >     if (file.exists())
> >       file.remove(false);
> >+
> >+    // Delete exported bookmarks html file
> >+    file = firSvc.get('ProfD', Ci.nsIFile);
> >+    file.append("bookmarks.exported.html");
> >+    if (file.exists())
> >+      file.remove(false);
> 
> I would rather do that change in the test_bookmarks.js only.

That would mean that if the test_bookmarks.js file fails horribly, that file won't get cleaned up, no?

> what is this loop for if you only need detials for getChild(2)?

I've removed the loop now.

> >+  if (aFolder == mToolbarFolder) {
> >+    mToolbarFolder = 0;
> >+  }
> >+
> 
> I would rather throw here.

That causes the tests to fail.  I'm leaving it in for now, and will investigate more after dinner.

Comment 15

[Dan Mills [:thunder]]

Attachment: fix v7

Same as v6, plus fixes Mano pointed out, except for the ones I commented on (see last comment).

Comment 16

[Dietrich Ayala (:dietrich)]

> > >+
> > >+    // Delete exported bookmarks html file
> > >+    file = firSvc.get('ProfD', Ci.nsIFile);
> > >+    file.append("bookmarks.exported.html");
> > >+    if (file.exists())
> > >+      file.remove(false);
> > 
> > I would rather do that change in the test_bookmarks.js only.
> 
> That would mean that if the test_bookmarks.js file fails horribly, that file
> won't get cleaned up, no?
> 

yep, that's why put this stuff in the header instead of in the test.

> 
> > >+  if (aFolder == mToolbarFolder) {
> > >+    mToolbarFolder = 0;
> > >+  }
> > >+
> > 
> > I would rather throw here.
> 
> That causes the tests to fail.  I'm leaving it in for now, and will investigate
> more after dinner.
> 

see comment #6 for why i did it this way.

Comment 17

[Dietrich Ayala (:dietrich)]

Comment on attachment 265083 [details] [diff] [review]
fix v7

r=me, thanks.

Comment 18

[Dan Mills [:thunder]]

Checking in browser/components/nsBrowserGlue.js;
/cvsroot/mozilla/browser/components/nsBrowserGlue.js,v  <--  nsBrowserGlue.js
new revision: 1.24; previous revision: 1.23
done
Checking in browser/components/migration/src/nsBrowserProfileMigratorUtils.cpp;
/cvsroot/mozilla/browser/components/migration/src/nsBrowserProfileMigratorUtils.cpp,v  <--  nsBrowserProfileMigratorUtils.cpp
new revision: 1.23; previous revision: 1.22
done
Checking in browser/components/places/content/places.js;
/cvsroot/mozilla/browser/components/places/content/places.js,v  <--  places.js
new revision: 1.86; previous revision: 1.85
done
Checking in browser/components/places/public/nsIPlacesImportExportService.idl;
/cvsroot/mozilla/browser/components/places/public/nsIPlacesImportExportService.idl,v  <--  nsIPlacesImportExportService.idl
new revision: 1.2; previous revision: 1.1
done
Checking in browser/components/places/src/nsPlacesImportExportService.cpp;
/cvsroot/mozilla/browser/components/places/src/nsPlacesImportExportService.cpp,v  <--  nsPlacesImportExportService.cpp
new revision: 1.10; previous revision: 1.9
done
Checking in browser/components/places/tests/unit/head_bookmarks.js;
/cvsroot/mozilla/browser/components/places/tests/unit/head_bookmarks.js,v  <--  head_bookmarks.js
new revision: 1.8; previous revision: 1.7
done
Checking in browser/components/places/tests/unit/tail_bookmarks.js;
/cvsroot/mozilla/browser/components/places/tests/unit/tail_bookmarks.js,v  <--  tail_bookmarks.js
new revision: 1.6; previous revision: 1.5
done
Checking in browser/components/places/tests/unit/test_bookmarks_html.js;
/cvsroot/mozilla/browser/components/places/tests/unit/test_bookmarks_html.js,v  <--  test_bookmarks_html.js
new revision: 1.5; previous revision: 1.4
done
Checking in toolkit/components/places/src/nsNavBookmarks.cpp;
/cvsroot/mozilla/toolkit/components/places/src/nsNavBookmarks.cpp,v  <--  nsNavBookmarks.cpp
new revision: 1.91; previous revision: 1.90
done

Comment 19

[pal-moz]

regression by this checkin ?
see http://forums.mozillazine.org/viewtopic.php?p=2886790#2886790

Comment 20

[pal-moz]

(In reply to comment #19)
> regression by this checkin ?
> see http://forums.mozillazine.org/viewtopic.php?p=2886790#2886790
> 

no problem with backup file created by normal Trunk.
problem with it created by Places Build.

related to item's ID ?

Comment 21

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

> regression by this checkin ?
> see http://forums.mozillazine.org/viewtopic.php?p=2886790#2886790

pal-moz, is there a bug on this regression?

> related to item's ID ?

Note since this checkin, we have switched from ID to ITEM_ID.  See bug #381209.

Can you try to reproduce the problem again?  Thanks!

Comment 22

[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