Closed Bug 318057 Opened 16 years ago Closed 16 years ago

bookmarks.html exporter

Categories

(Firefox :: Bookmarks & History, defect, P1)

defect

Tracking

()

RESOLVED FIXED
Firefox 2 beta1

People

(Reporter: brettw, Assigned: brettw)

References

Details

(Keywords: fixed1.8.1, Whiteboard: swag: 2 days)

Attachments

(2 files)

The new mozStorage-based bookmark system should be able to load/save bookmarks.html from older versions of Mozilla.
This adds a bookmarks importer and also changes the initialization of the places root to use the importer to read a default bookmarks.html stored in chrome.

Also: fixed a bug in MoveFolder, added function to go from bookmark subfolder name to folder ID

No exporter yet.
Attachment #204720 - Flags: review?(bryner)
Comment on attachment 204720 [details] [diff] [review]
Importerer and default places initializer

>--- public/nsINavBookmarksService.idl	29 Nov 2005 01:22:57 -0000	1.6
>+++ public/nsINavBookmarksService.idl	1 Dec 2005 19:47:31 -0000
>+  /**
>+   * Loads the given bookmarks.html file and merges it with the current
>+   * bookmarks hierarchy.
>+   */
>+  void ImportBookmarksHTML(in nsIURI url);
>+
>+  /**
>+   * Saves the current bookmarks hierarchy to a bookmarks.html file.
>+   */
>+  void ExportBookmarksHTML(in nsIURI url);
> };

Lowercase the first letter of these methods.

>--- /dev/null	1 Jan 1970 00:00:00 -0000
>+++ src/nsBookmarksHTML.cpp	1 Dec 2005 19:47:31 -0000

For the methods that just have empty/trivial implementations, it would make things more readable if you just defined them inline in the class definition.

>+void
>+BookmarkContentSink::HandleHead1Begin(const nsIParserNode& node)
>+{
>+  PRInt32 attrCount = node.GetAttributeCount();
>+  for (PRInt32 i = 0; i < attrCount; i ++) {
>+    if (node.GetKeyAt(i).LowerCaseEqualsLiteral(KEY_PLACESROOT_LOWER)) {
>+      if (mFrames.Length() > 1) {
>+        NS_NOTREACHED("Trying to set the places root from the middle of the hierarchy. "
>+                      "This can only be set at the beginning.");

Someone could trigger this codepath with an invalid bookmarks HTML file.  So NS_NOTREACHED isn't a good error here (you're asserting that for the code to be reached, some internal constraint has been violated).  I'd say change it to NS_WARNING.

>+void
>+BookmarkContentSink::HandleLinkBegin(const nsIParserNode& node)
>+{
>+  BookmarkImportFrame& frame = CurFrame();
>+
>+  // mPreviousText will hold our link text, clear it so that can be appended to
>+  frame.mPreviousText.Truncate();
>+
>+  // get the attributes we care about
>+  nsString href;
>+  nsString icon;
>+  nsString lastCharset;

use nsAutoString for these.

>+static nsresult
>+SyncChannelStatus(nsIChannel* channel, nsresult status)
>+{
>+  nsresult channelStatus;
>+  channel->GetStatus(&channelStatus);
>+  if (NS_FAILED(channelStatus))
>+    return channelStatus;
>+
>+  if (NS_SUCCEEDED(status))
>+    return NS_OK; // caller and the channel are happy
>+
>+  // channel was OK, but caller wasn't: set the channel state
>+  channel->Cancel(status);
>+  return status;
>+}

You might want to ask Darin to look at this part.  Looks ok to me though.

Looks ok otherwise.
Attachment #204720 - Flags: review?(bryner) → review+
Comment on attachment 204720 [details] [diff] [review]
Importerer and default places initializer

>Index: public/nsINavBookmarksService.idl
...
>+  PRInt64 getChildFolder(in PRInt64 folder, in AString subFolder);

Usually a good idea to rev the UUID property of an interface when you
change it.  Of course, it surely doesn't matter in this case since this
code has never been in production.


>Index: src/nsBookmarksHTML.cpp

>+nsNavBookmarks::ImportBookmarksHTMLInternal(nsIURI* aURL,

>+  nsCOMPtr<nsIIOService> ioservice = do_GetService(
>+                                     "@mozilla.org/network/io-service;1", &rv);

note: nsNetUtil.h defines do_GetIOService(), which could be used here.


>+  rv = listener->OnStartRequest(channel, nsnull);
>+  rv = SyncChannelStatus(channel, rv);
>+  if (NS_FAILED(rv)) {
>+    listener->OnStopRequest(channel, nsnull, rv);
>+    return rv;
>+  }
>+  while(1)

perhaps you could simplify the code a bit by changing this
loop to "while (NS_SUCCEEDED(rv))".  then you could get
rid of the "if (NS_FAILED(rv))" block above.


>+  {
>+    PRUint32 available;
>+    rv = bufferedstream->Available(&available);
>+    if (NS_FAILED(rv)) {
>+      channel->Cancel(rv);
>+      listener->OnStopRequest(channel, nsnull, rv);
>+      return rv;

Some input streams will return NS_BASE_STREAM_CLOSED when
they have no more data to be read.  You should probably
treat that as a non-error.

>+    }
>+    if (! available)
>+      break; // blocking input stream has none available when done
>+
>+    rv = listener->OnDataAvailable(channel, nsnull, bufferedstream, 0, available);
>+    rv = SyncChannelStatus(channel, rv);
>+    if (NS_FAILED(rv)) {
>+      listener->OnStopRequest(channel, nsnull, rv);
>+      return rv;
>+    }
>+  }
>+  listener->OnStopRequest(channel, nsnull, NS_OK);

I think I'd break out of the loop whenever NS_FAILED(rv), and just
have the OnStopRequest at the end of the loop pass rv as the status
parameter.  The result should be less code.
Importer works. Marking as lower priority for exporter.
Priority: -- → P3
Depends on: 323472
Target Milestone: --- → Firefox 2 beta1
Assignee: brettw → annie.sullivan
Summary: Bookmark importer/exporter for bookmarks.html → bookmarks.html exporter
Blocks: 327925
Blocks: 327894
Priority: P3 → P1
Whiteboard: swag: 2 days
Assignee: annie.sullivan → brettw
Blocks: 333646
This is the bookmarks.html exporter. I added a simple hook in the places window/File/Export...

This also makes some enhancements to the importer to make round-tripping of the bookmarks.html file possible without strange problems.

I also fixed some problems with the default_places.html and the default bookmarks.html.
Attachment #219164 - Flags: review?(bugs)
Comment on attachment 219164 [details] [diff] [review]
Exporter, fixes to importer

r=ben@mozilla.org
Attachment #219164 - Flags: review?(bugs) → review+
On branch and trunk.
Status: NEW → RESOLVED
Closed: 16 years ago
Keywords: fixed1.8.1
Resolution: --- → FIXED
I did a fresh build today after this patch was committed. I was able to export my bookmarks to bookmarks.html just fine. However, when I attempt to use that exported file with 1.5.0.2, it's as if Fx doesn't recognize them properly and it refuses to use them.

Is it possible that something is syntactically incorrect with the exported bookmarks.html which is causing the problems with 1.5.0.2?
I should note that I tried both directly overwriting bookmarks.html in my profile (after removing all of the backups in the dir) and importing bookmarks.html within the bookmark manager.
(In reply to comment #9)
> I should note that I tried both directly overwriting bookmarks.html in my
> profile (after removing all of the backups in the dir) and importing
> bookmarks.html within the bookmark manager.

You're right. I tested it, but made some changes after that. I filed bug 335308 for this.
Does this export the description of an bookmark item?

When I began to use a recent trunk build the browser automatically imported bookmarks.html. (See bug 318817 comment 17, I migrated to Places after March 7) Now I've exported bookmarks.html and it contains no descriptions. Does this mean all description data are lost when imported, or the exporter doesn't support it?
Descriptions are bug 334758.
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.