Closed Bug 453194 Opened 16 years ago Closed 15 years ago

Refactor Safari and Opera import/export into HTMLBookmarkConverter-style classes

Categories

(Camino Graveyard :: Bookmarks, defect)

All
macOS
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Camino2.0

People

(Reporter: stuart.morgan+bugzilla, Assigned: stuart.morgan+bugzilla)

Details

(Whiteboard: [camino-2.0])

Attachments

(1 file)

All the bookmark import/export should be visitor-pattern-based, so the format details aren't wound through the various bookmark classes.
Hardware: PC → All
Attached patch refactoringSplinter Review
This moves all the import/export for Safari and Opera bookmarks into their own classes, rather than having them scattered through the bookmark system, and restructures the Opera import to be more understandable and have less duplicate code. In addition, a couple of related incidentals:
- Added support for Opera 10 (which has a new extension on the bookmarks file)
- Remove an encoding-guessing function which was no longer being used (since the HTML import rewrite, it was only ever used to guess that Opera files are UTF8, which we don't need to guess)
- Remove a loading function that would read a Safari plist as the actual bookmark backing store if someone put it in place of the Camino bookmarks file. That's completely undiscoverable, and I couldn't think of any good reason to support it.
- Tweaks the HTML import/export class slightly for consistency of return values with the new import/export classes.
Attachment #404501 - Flags: superreview?(mikepinkerton)
(In reply to comment #1)
> - Remove a loading function that would read a Safari plist as the actual
> bookmark backing store if someone put it in place of the Camino bookmarks file.
> That's completely undiscoverable, and I couldn't think of any good reason to
> support it.

As I recall, we added that because people were doing it.  Please don't ask me to explain *why* our users were doing it, but they were.
Unfortunately it was part of a big smfr checkin with no associated bug, so I can't find any discussion. My feeling is that we should take the code out, and then people will stop doing it ;)

If people feel strongly that we should still allow that, I'll have to update the patch with a modified version of that code.
Comment on attachment 404501 [details] [diff] [review]
refactoring

sr=pink
Attachment #404501 - Flags: superreview?(mikepinkerton) → superreview+
Landed on CVS trunk and CAMINO_2_0_BRANCH.
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Whiteboard: [camino-2.0]
Target Milestone: --- → Camino2.0
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: