374876 - need to write out bookmarksbackups/bookmarks-%Y-%m-%d.html and/or bookmarks.bak and/or bookmarks.html like Fx 2 for browser interop / handling schema changes

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, defect)

Description

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

need to write out bookmarksbackups/bookmarks-%Y-%m-%d.html and/or bookmarks.bak and/or bookmarks.html like Fx 2 for browser interop / handling schema changes

see http://lxr.mozilla.org/mozilla/source/browser/components/bookmarks/src/nsBookmarksService.cpp#4413

as our schema changes, we can use the backup to allow us to dump the bookmarks table and automatically re-import the last good backup.

this comes from an IRC conversation with dietrich.

Comment 1

[Dietrich Ayala (:dietrich)]

Attachment: fix v1

first cut at this

Comment 2

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

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

dietrich, sorry for the repeated nits.  I think I'm really just nitting the original code from the old browser bookmarks code, but there are some non nits in there as well.


x)

+  nsresult rv = NS_GetSpecialDirectory(NS_APP_BOOKMARKS_50_FILE,
+                                       getter_AddRefs(bookmarksFile));;

extra ; (note, http://lxr.mozilla.org/mozilla/source/browser/components/bookmarks/src/nsBookmarksService.cpp#4536 has it too.)

+  // create if it doesn't exist
+  if (NS_FAILED(rv) || !bookmarksFile)
+    (void)bookmarksFile->Create(nsIFile::NORMAL_FILE_TYPE, 0666);

a) if !bookmarksFile, the bookmarksFile->Create() will deref null
b) shouldn't this be 0600?
c) Where are you checking if bookmarksFile exists?  

It seems like this code should bail on NS_FAILED(rv) or
on !bookmarksFile (this would follow existing code patterns, but I'm not sure if you can pass NS_FAILED(rv) and
have a null bookmarksFile.)

Then, if do something like

PRBool exists;
rv = bookmarksFile->Exists(&exists);
if (NS_FAILED(rv) || !exists) {
  rv = bookmarksFile->Create(nsIFile::NORMAL_FILE_TYPE, 0600);
  // assert and return here, not sure....
}

x)

+  // export bookmarks.html
+  ExportBookmarksHTML(bookmarksFile);

This should either be:

rv = ExportBookmarksHTML(bookmarksFile);
// check rv

or

(void)ExportBookmarksHTML(bookmarksFile);

I think we want to check and assert / return on rv failure.

x)

+  // archive if needed
+  nsCOMPtr<nsIPrefService> prefServ(do_GetService(NS_PREFSERVICE_CONTRACTID, &rv));
+  nsCOMPtr<nsIPrefBranch> bookmarksPrefs;
+  if (prefServ)
+    prefServ->GetBranch("browser.bookmarks.", getter_AddRefs(bookmarksPrefs));

Instead, I think:

+  // archive if needed
+  nsCOMPtr<nsIPrefService> prefServ(do_GetService(NS_PREFSERVICE_CONTRACTID, &rv));
+  NS_ENSURE_SUCCESS(rv, rv);
+  nsCOMPtr<nsIPrefBranch> bookmarksPrefs;
+  rv = prefServ->GetBranch("browser.bookmarks.", getter_AddRefs(bookmarksPrefs));
+  NS_ENSURE_SUCCESS(rv, rv);

assuming that would return failure if browser.bookmarks. pref branch doesn't exist.  (but we default it in all.js)

+  PRInt32 numberOfBackups = 5;
+  if (bookmarksPrefs)
+    bookmarksPrefs->GetIntPref("max_backups", &numberOfBackups);

Since bookmarksPrefs is ensured to exist (following my comments above), we could do:

+  PRInt32 numberOfBackups = 5;
+  bookmarksPrefs->GetIntPref("max_backups", &numberOfBackups);

I didn't realize you could do that (or if that was supported), passing in a value that would not get modified 
if the pref didn't exist.  (looking at lxr, I see that the old bookmarks code does it.)  but is that really part of the API?

This seems safer:

+  PRInt32 numberOfBackups;
+  rv = bookmarksPrefs->GetIntPref("max_backups", &numberOfBackups);
+  if (NS_FAILED(rv))
+    numberOfBackups = 5;
+
+  if (numberOfBackups > 0)

(note, we default browser.bookmarks.max_backups to 5 in all.js, so is sort of moot, right?)

x)

+    ArchiveBookmarksFile(numberOfBackups, PR_FALSE);

We should either check the rv of "rv = ArchiveBookmarksFile(...);" or do "(void)ArchiveBookmarksFile(...);"

I prefer checking rv.

x)

+
+  nsCOMPtr<nsIFile> bookmarksBackupDir;
+  nsresult rv = NS_GetSpecialDirectory(NS_APP_USER_PROFILE_50_DIR,
+                                       getter_AddRefs(bookmarksBackupDir));
+  if (NS_FAILED(rv)) return rv;

I think NS_ENSURE_SUCCESS(rv, rv); is better here.  Why silently fail?

x)

+  nsDependentCString dirName("bookmarkbackups");
+  bookmarksBackupDir->AppendNative(dirName);
+  
+  bookmarksBackupDir->Create(nsIFile::DIRECTORY_TYPE, 0700);

For both AppendNative() and Create(), what about the return value?

x)

+  PRBool exists;
+  bookmarksBackupDir->Exists(&exists);
+  if (!exists) {

Would this be better:

+  rv = bookmarksBackupDir->Exists(&exists);
+  if (NS_FAILED(rv) || !exists) {

x)

+  if (forceArchive == PR_FALSE) {

why not:

+  if (forceArchive) {

and switch the blocks?

x)

+      existingBackups->GetNext(getter_AddRefs(backupFile));
+      nsAutoString backupName;
+      backupFile->GetLeafName(backupName);

should we be checking rv of GetNext() and GetLeafName()?
      

x)

+      while (numberOfBackupsToDelete--) {
+        bookmarksBackupDir->Clone(getter_AddRefs(backupFile));
+        backupFile->Append(*backupFileNames[0]);
+        backupFile->Remove(PR_FALSE);

if we don't care if this Remove() failed, can you do (void)backupFile->Remove(PR_FALSE);
same goes for Clone() and Append().

x)

+  nsCOMPtr<nsIFile> bookmarksFile;
+  rv = NS_GetSpecialDirectory(NS_APP_BOOKMARKS_50_FILE,
+                              getter_AddRefs(bookmarksFile));
+  if (NS_FAILED(rv)) return rv;
  
again, NS_ENSURE_SUCCESS(rv, rv); since this should not be a silent failure.

x)

+    
+    // notify the bookmarks service we're quitting
+    nsNavBookmarks* bookmarks = nsNavBookmarks::GetBookmarksService();
+    NS_ENSURE_TRUE(bookmarks, NS_ERROR_OUT_OF_MEMORY);
+    bookmarks->OnQuit();

if we don't care if this OnQuit() failed, can you do (void)bookmarks->OnQuit();

Comment 3

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

for the non nits, see:

a) if !bookmarksFile, the bookmarksFile->Create() will deref null
b) shouldn't this be 0600?
c) Where are you checking if bookmarksFile exists?  

Comment 4

[Dietrich Ayala (:dietrich)]

Attachment: fix v2 - fixes seth's comments

Comment 5

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

Comment on attachment 259438 [details] [diff] [review]
fix v2 - fixes seth's comments

r=sspitzer

two small nits:

1)

+  // create if it doesn't exist
+  PRBool exists;
+  rv = bookmarksFile->Exists(&exists);
+  if (NS_FAILED(rv) || !exists) {
+    rv = bookmarksFile->Create(nsIFile::NORMAL_FILE_TYPE, 0600);
+    NS_ASSERTION(rv, "Unable to create bookmarks.html!");
+  }

Shouldn't we "return rv;" if we were unable to create this file?


2)

+    bookmarksBackupDir->Clone(getter_AddRefs(currentBackup));
+    currentBackup->Append(backupFilenameString);
+    currentBackup->Exists(&exists);

Can you apply the "(void)" or check-rv pattern here?

Comment 6

[Dietrich Ayala (:dietrich)]

Attachment: fix v3 - fix broken archiving

The last patch broke archiving. Changed to only attempt to create the bookmarkbackups folder once.

Comment 7

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

Comment on attachment 259557 [details] [diff] [review]
fix v3 - fix broken archiving

r=sspitzer, sorry for not catching that in my previous review.

using https://bugzilla.mozilla.org/attachment.cgi?oldid=259438&action=interdiff&newid=259557&headers=1, I see that you have removed the extra call and adding a missing "return rv", and cleaned up more of the "(void) or check rv" nits.

thanks dietrich!

Comment 8

[Dietrich Ayala (:dietrich)]

Checking in src/nsNavBookmarks.cpp;
/cvsroot/mozilla/toolkit/components/places/src/nsNavBookmarks.cpp,v  <--  nsNavBookmarks.cpp
new revision: 1.76; previous revision: 1.75
done
Checking in src/nsNavBookmarks.h;
/cvsroot/mozilla/toolkit/components/places/src/nsNavBookmarks.h,v  <--  nsNavBookmarks.h
new revision: 1.35; previous revision: 1.34
done
Checking in src/nsNavHistory.cpp;
/cvsroot/mozilla/toolkit/components/places/src/nsNavHistory.cpp,v  <--  nsNavHistory.cpp
new revision: 1.112; previous revision: 1.111
done

Comment 9

[Dietrich Ayala (:dietrich)]

needs tests, see bug 376253.

Comment 10

[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