371812 - add bookmark id support to bookmarks html import/export

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[Dietrich Ayala (:dietrich)]

Need to add bookmark ids to exported bookmarks.html, and handle imports.

Comment 1

[Dietrich Ayala (:dietrich)]

Attachment: fix v1

adds import and export of bookmark and folder ids.

Comment 2

[Dietrich Ayala (:dietrich)]

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

never mind, not handling livemarks right yet.

Comment 3

[Dietrich Ayala (:dietrich)]

Attachment: fix v2

import/export of ids for bookmarks, folders, livemarks

Comment 4

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

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

r=sspitzer, but some comments / questions and nits:

1)

+  // Container Id, see above
+  PRInt64 mLastContainerId;

Should we initialize this value to 0 here?

2)

+  frame.mLastContainerId = nsnull;

mLastContainerId is a PRInt64, not a nsCOMPtr, so this should be 0, right?

3)

+      } else if (node.GetKeyAt(i).LowerCaseEqualsLiteral(KEY_ID_LOWER)) {
+        nsAutoString id = nsAutoString(node.GetKeyAt(i));
+        if (!id.IsEmpty() && Substring(id, 0, 3) != NS_LITERAL_STRING("rdf")) {
+          PRInt32 err;
+          PRInt64 intId = id.ToInteger(&err);
+          if (err == 0)
+            frame.mLastContainerId = intId;
+        }

A few small requests:

a)  can you move this into helper function and make a few changes:

frame.mLastContainerId = ConvertIdToContainerId(id);

pseudo code I had in mind:

Note "rdf" -> "rdf:", CaseInsensitiveCompare, using nsresult instead of PRInt32 for rv, and setting return value to 0 by default or on NS_FAILED(rv)

PRInt64 ConvertIdToContainerId(string id)
{
  PRInt64 intId = 0;
  if (!id.IsEmpty() && !Substring(id, 0, 4).Equals(NS_LITERAL_STRING("rdf:"), CaseInsensitiveCompare)) {
    nsresult rv;
    intId = id.ToInteger(&rv);
    if (NS_FAILED(rv))
      intId = 0;
  }
  return intId;
}
 
Please feel free to find a better name than ConvertIdToContainerId(), and clean up my sloppy pseudo code

x)

+  nsresult rv;
+
   BookmarkImportFrame& frame = CurFrame();

Why the added nsresult rv here?

  
x)

+  // if there's a pre-existing Places bookmark id, use it
+  if (!id.IsEmpty() && Substring(id, 0, 3) != NS_LITERAL_STRING("rdf")) {
+    PRInt32 err;
+    PRInt64 intId = id.ToInteger(&err);
+    if (err == 0)
+      frame.mPreviousId = intId;
+  } 

please call ConvertIdToContainerId() instead.

x)

-  // create the bookmark
-  nsresult rv = mBookmarksService->InsertItem(frame.mContainerID, frame.mPreviousLink,
-                                     mBookmarksService->DEFAULT_INDEX, &frame.mPreviousId);

oh, is that why rv was moved?

x)

+  // if no previous id (or a legacy id), create a new bookmark
+  if (frame.mPreviousId == 0) {
+    // create the bookmark
+    rv = mBookmarksService->InsertItem(frame.mContainerID, frame.mPreviousLink,
+                                       mBookmarksService->DEFAULT_INDEX, &frame.mPreviousId);
+    NS_ASSERTION(NS_SUCCEEDED(rv), "InsertItem failed");
+  }

should we be sanity checking that frame.mContainerID is not 0, or is 0 OK here?

x)

+    if (frame.mPreviousId > 0) {
+      // It's a pre-existing livemark, so update it's properties

s/it's properties/its properties

x)


+      if (mIsImportDefaults) {
+        mLivemarkService->CreateLivemarkFolderOnly(mBookmarksService,
+                                                   frame.mContainerID,
+                                                   frame.mPreviousText,
+                                                   frame.mPreviousLink,
+                                                   frame.mPreviousFeed,
+                                                   -1,
+                                                   &folderId);
+      } else {
+        mLivemarkService->CreateLivemark(frame.mContainerID,
+                                         frame.mPreviousText,
+                                         frame.mPreviousLink,
+                                         frame.mPreviousFeed,
+                                         -1,
+                                         &folderId);
+      }

Should we be checking rv of CreateLivemarkFolderOnly() and CreateLivemark()
     }

x)

+        if (! ourID) {

nit:  can you make that if (!ourID) {


x)

+          updateFolder = PR_TRUE;
+          SetFaviconForFolder(ourID, NS_LITERAL_CSTRING(BOOKMARKS_MENU_ICON_URI));

should we check the rv of SetFaviconForFolder()?

x)

+      case BookmarkImportFrame::Container_Toolbar:
+        // get toolbar folder
+        PRInt64 btf;

since we are changing this code, how about bookmarkToolbarFolder instead of btf?

x)

+          // set favicon
+          SetFaviconForFolder(ourID, NS_LITERAL_CSTRING(BOOKMARKS_TOOLBAR_ICON_URI));

should we check the rv of SetFaviconForFolder()?

x)

just a quick comment, can you make sure that we always use the same check for id validity?  (either x > 0 or !x)

Comment 5

[Dietrich Ayala (:dietrich)]

Attachment: fix v3

fixed the fixable issues, other comments below. i'll file a follow-up on moving to the frozen string apis.

(In reply to comment #4)
> (From update of attachment 259858 [details] [diff] [review])
> r=sspitzer, but some comments / questions and nits:
> 
> 1)
> 
> +  // Container Id, see above
> +  PRInt64 mLastContainerId;
> 
> Should we initialize this value to 0 here?

it's initialized in the constructor. plus ISO C++ forbids in-class initialization of non-const static members. (er, well that's the error i got when i tried).

> x)
> 
> -  // create the bookmark
> -  nsresult rv = mBookmarksService->InsertItem(frame.mContainerID,
> frame.mPreviousLink,
> -                                     mBookmarksService->DEFAULT_INDEX,
> &frame.mPreviousId);
> 
> oh, is that why rv was moved?
>

moved it up top b/c used in multiple scopes
 
> x)
> 
> +  // if no previous id (or a legacy id), create a new bookmark
> +  if (frame.mPreviousId == 0) {
> +    // create the bookmark
> +    rv = mBookmarksService->InsertItem(frame.mContainerID,
> frame.mPreviousLink,
> +                                       mBookmarksService->DEFAULT_INDEX,
> &frame.mPreviousId);
> +    NS_ASSERTION(NS_SUCCEEDED(rv), "InsertItem failed");
> +  }
> 
> should we be sanity checking that frame.mContainerID is not 0, or is 0 OK here?

getting here is predicated upon the creation of a container. we'd fail in NewFrame if we can't create a container.

Comment 6

[Dietrich Ayala (:dietrich)]

Attachment: fix v4

minor update for consistency of value comparisons per seth's last comment, and removing a whitespace change.

Comment 7

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

Comment on attachment 260100 [details] [diff] [review]
fix v4

clearing review request, dietrich and I are figuring out simpler string foo.

Comment 8

[Dietrich Ayala (:dietrich)]

Attachment: fix v5

Comment 9

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

Comment on attachment 260103 [details] [diff] [review]
fix v5

r=sspitzer

Comment 10

[Dietrich Ayala (:dietrich)]

Checking in nsBookmarksHTML.cpp;
/cvsroot/mozilla/toolkit/components/places/src/nsBookmarksHTML.cpp,v  <--  nsBookmarksHTML.cpp
new revision: 1.34; previous revision: 1.33
done

Comment 11

[Dietrich Ayala (:dietrich)]

setting testsuite-, we need to add tests for all of bookmarks import and export.

Comment 12

[Dietrich Ayala (:dietrich)]

sorry, misunderstood the in-testsuite flag (thought - meant "needs tests"). correcting flag, see 376253 for import/export tests.

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