318057 - bookmarks.html exporter

Status: RESOLVED FIXED

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

Description

[Brett Wilson]

The new mozStorage-based bookmark system should be able to load/save bookmarks.html from older versions of Mozilla.

Comment 1

[Brett Wilson]

Attachment: Importerer and default places initializer

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.

Comment 2

[Brian Ryner (not reading)]

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.

Comment 3

[Darin Fisher]

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.

Comment 4

[Brett Wilson]

Importer works. Marking as lower priority for exporter.

Comment 5

[Brett Wilson]

Attachment: Exporter, fixes to importer

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.

Comment 6

[Ben Goodger (use ben at mozilla dot org for email)]

Comment on attachment 219164 [details] [diff] [review]
Exporter, fixes to importer

r=[e-mail]

Comment 7

[Brett Wilson]

On branch and trunk.

Comment 8

[Ryan VanderMeulen [:RyanVM]]

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?

Comment 9

[Ryan VanderMeulen [:RyanVM]]

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.

Comment 10

[Brett Wilson]

(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.

Comment 11

[Ryuichi KUBUKI]

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?

Comment 12

[Brett Wilson]

Descriptions are bug 334758.

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