Closed Bug 376253 Opened 13 years ago Closed 12 years ago

places bookmarks.html import/export needs comprehensive tests

Categories

(Firefox :: Bookmarks & History, defect)

defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: dietrich, Assigned: hello)

References

Details

Attachments

(1 file, 6 obsolete files)

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.
the test framework is started in the patch to bug 376008.
Assignee: nobody → dietrich
Depends on: 376008
Attached patch fix v1 (obsolete) — Splinter Review
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
Attachment #263702 - Flags: review?(sspitzer)
Attachment #263702 - Flags: review?(mano)
Status: NEW → ASSIGNED
(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 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?
Attachment #263702 - Flags: review?(sspitzer) → review+
Attachment #263702 - Flags: review?(mano)
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
Attached patch fix v2 (obsolete) — Splinter Review
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.
Attachment #263702 - Attachment is obsolete: true
Attached patch fix v3 (obsolete) — Splinter Review
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.
Attachment #263809 - Attachment is obsolete: true
Attachment #264159 - Flags: review?(mano)
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)
Assignee: dietrich → thunder
Status: ASSIGNED → NEW
Attached patch fiv v4 (obsolete) — Splinter Review
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.
Attachment #264159 - Attachment is obsolete: true
Attached patch fix v5 (obsolete) — Splinter Review
* 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
Attachment #264854 - Attachment is obsolete: true
Attached patch fix v6 (obsolete) — Splinter Review
* 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)
Attachment #264983 - Attachment is obsolete: true
Attachment #265064 - Flags: review?(mano)
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.
Attachment #265064 - Flags: review?(mano) → review+
(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.
Attached patch fix v7Splinter Review
Same as v6, plus fixes Mano pointed out, except for the ones I commented on (see last comment).
Attachment #265064 - Attachment is obsolete: true
Attachment #265083 - Flags: review?(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 on attachment 265083 [details] [diff] [review]
fix v7

r=me, thanks.
Attachment #265083 - Flags: review?(dietrich) → review+
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
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
(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 ?
> 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!
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.