439024 - Replace HTML bookmark parser

Status: RESOLVED FIXED

(Camino Graveyard :: Bookmarks, defect)

Description

[Stuart Morgan]

The current HTML bookmark parser is a rat's nest. It's painful to maintain (including fixing the known crashers), and adding various support it's currently missing (most bookmark properties, reasonable places export handling, etc.) would be a nightmare.

I'm in the process of writing an NSXMLNode-based parser to replace it.

Comment 1

[Stuart Morgan]

Attachment: new importer

Finally got around to wiring this up. I've pulled all the HTML reading and writing out into a visitor class; we should do the same to Safari and Opera down the road (I'll file a bug).

I've tested this against every file we had attached to bugs filed against HTML import, plus exports from us, Firefox 2, Firefox 3, and Safari (maybe others; I forget) and haven't seen any regressions. Plus, we don't crash anymore, we handle corrupt files reasonably well for free, and I've added better support for importing some attributes, as well as some smarts about a Places export file (e.g., recognizing their smart folders as such, and skipping them). I also sprinkled in some string cleanup to keep bad characters out of our bookmarks at cl's request.

I'll also file a follow-up about propagating NSErrors out of the import/export, so that we can provide failure UI, but since we didn't do that before either I wanted to split it out for later.

Comment 3

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 336378 [details] [diff] [review]
new importer

+// Writes the bookmark hierarchy rooted at |rootBookmarkItem to a Netscape form

closing pipe, |rootBookmarkItem|

++ (id)htmlBoomarkConverter
+{
+  return [[[self alloc] init] autorelease];

is |self| correct usage here? Safer to use the classname specifically?

also spelling error in methodname, "boomark". Should be "bookmark".

sr=pink otherwise

Comment 4

[Stuart Morgan]

Landed on CVS trunk.

Comment 5

[Stuart Morgan]

Oh, I meant to comment: [self alloc] is correct, because it means the method works correctly in a subclass. (Spelling fixed; I'm incapable of typing bookmark for some reason, but I thought I had nailed all of those.)

Comment 6

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

(In reply to comment #1)
> I'll also file a follow-up about propagating NSErrors out of the import/export,
> so that we can provide failure UI, but since we didn't do that before either I
> wanted to split it out for later.

Did this follow-up ever get filed?

Comment 7

[Stuart Morgan]

Yeah, that's bug 453192.