Closed Bug 371812 Opened 16 years ago Closed 16 years ago

add bookmark id support to bookmarks html import/export

Categories

(Firefox :: Bookmarks & History, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: dietrich, Assigned: dietrich)

References

Details

(Whiteboard: [Fx2-parity])

Attachments

(1 file, 4 obsolete files)

Need to add bookmark ids to exported bookmarks.html, and handle imports.
Blocks: 375108
Assignee: nobody → dietrich
Attached patch fix v1 (obsolete) — Splinter Review
adds import and export of bookmark and folder ids.
Attachment #259840 - Flags: review?(sspitzer)
Comment on attachment 259840 [details] [diff] [review]
fix v1

never mind, not handling livemarks right yet.
Attachment #259840 - Attachment is obsolete: true
Attachment #259840 - Flags: review?(sspitzer)
Attached patch fix v2 (obsolete) — Splinter Review
import/export of ids for bookmarks, folders, livemarks
Attachment #259858 - Flags: review?(sspitzer)
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)
Attachment #259858 - Flags: review?(sspitzer) → review+
Attached patch fix v3 (obsolete) — Splinter Review
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.
Attachment #259858 - Attachment is obsolete: true
Attachment #260097 - Flags: review?(sspitzer)
Attached patch fix v4 (obsolete) — Splinter Review
minor update for consistency of value comparisons per seth's last comment, and removing a whitespace change.
Attachment #260097 - Attachment is obsolete: true
Attachment #260100 - Flags: review?(sspitzer)
Attachment #260097 - Flags: review?(sspitzer)
Comment on attachment 260100 [details] [diff] [review]
fix v4

clearing review request, dietrich and I are figuring out simpler string foo.
Attachment #260100 - Flags: review?(sspitzer)
Attached patch fix v5Splinter Review
Attachment #260100 - Attachment is obsolete: true
Attachment #260103 - Flags: review?(sspitzer)
Comment on attachment 260103 [details] [diff] [review]
fix v5

r=sspitzer
Attachment #260103 - Flags: review?(sspitzer) → review+
Checking in nsBookmarksHTML.cpp;
/cvsroot/mozilla/toolkit/components/places/src/nsBookmarksHTML.cpp,v  <--  nsBookmarksHTML.cpp
new revision: 1.34; previous revision: 1.33
done
Status: NEW → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
setting testsuite-, we need to add tests for all of bookmarks import and export.
Flags: in-testsuite-
sorry, misunderstood the in-testsuite flag (thought - meant "needs tests"). correcting flag, see 376253 for import/export tests.
Flags: in-testsuite- → in-testsuite?
Whiteboard: [Fx2-parity]
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.