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

adds import and export of bookmark and folder ids.

Comment on attachment 259840 [details] [diff] [review] fix v1 never mind, not handling livemarks right yet.

import/export of ids for bookmarks, folders, livemarks

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)

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.

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

Comment on attachment 260100 [details] [diff] [review] fix v4 clearing review request, dietrich and I are figuring out simpler string foo.

Comment on attachment 260103 [details] [diff] [review] fix v5 r=sspitzer

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

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

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

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