Replace HTML bookmark parser

RESOLVED FIXED in Camino2.0

Status

Camino Graveyard
Bookmarks
RESOLVED FIXED
10 years ago
9 years ago

People

(Reporter: Stuart Morgan, Assigned: Stuart Morgan)

Tracking

Trunk
Camino2.0
All
Mac OS X
Dependency tree / graph

Details

Attachments

(1 attachment)

42.25 KB, patch
Mike Pinkerton (not reading bugmail)
: superreview+
Details | Diff | Splinter Review
(Assignee)

Description

10 years ago
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.
(Assignee)

Updated

10 years ago
Blocks: 386466

Updated

10 years ago
Hardware: PC → All
(Assignee)

Comment 1

10 years ago
Created attachment 336378 [details] [diff] [review]
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.
Attachment #336378 - Flags: superreview?(mikepinkerton)
(Assignee)

Updated

10 years ago
Blocks: 261474

Updated

10 years ago
Duplicate of this bug: 336269
Attachment #336378 - Flags: superreview?(mikepinkerton) → superreview+
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
(Assignee)

Comment 4

10 years ago
Landed on CVS trunk.
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Camino2.0
(Assignee)

Comment 5

10 years ago
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.)
(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?
(Assignee)

Comment 7

9 years ago
Yeah, that's bug 453192.
You need to log in before you can comment on or make changes to this bug.