309008 - <Menu Spacer> imported with url of " and is editable

Status: RESOLVED FIXED

(Camino Graveyard :: Bookmarks, defect)

Description

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

When you export your Camino bookmarks and then import them, all of the <Menu
Spacer>s are imported with a url(!) of " (straight-double-quote) and all of
their attributes are editable.  Normal menu spacers are not editable at all.

Camino 2005091704 (v1.0a1+)

Comment 1

[Chris Lawson (gone)]

Is this a problem if you apply my patch for bug 307743?

I ask because in my custom build with that patch applied, it isn't a problem.
(In other words, we do the right thing when the imported string is an HR.)

The bug is still valid, obviously, since we should be able to import earlier
versions of the bookmark file we've created without doing this. But it won't be
an issue for any bookmark files exported once that patch gets checked in.

cl

Comment 2

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

I don't build, so I can't verify, but I'll take your word; good to know this is
mostly fixed already :-)

Comment 3

[Chris Lawson (gone)]

This bug doesn't actually depend on bug 307743. The patch for that bug doesn't
do anything at all for import; it only prevents us from exporting something we
can't properly import later in the first place.

There will need to be a different patch written to fix this bug, and this bug
can be fixed without bug 307743 being fixed (and vice-versa).

cl

Comment 4

[Chris Lawson (gone)]

Attachment: Fixes our failure to recognise menu spacers on import

This was caused by us never actually doing the check we said we were going to
do in BookmarkManager.mm, at line 1162.

I've implemented that check and also added a couple comments to the code for
clarification.

Once the patch to bug 307743 is checked in, this will be strictly for legacy
support, since AFAIK we're the only browser that ever exported menu spacers as
"&let;Menu Spacer>" in the first place.

cl

Comment 5

[Chris Lawson (gone)]

Attachment: new revision

Fixes a bunch of minor style errors in the file.

cl

Comment 6

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

-> cl

Comment 7

[Chris Lawson (gone)]

Attachment: Updated for latest code changes, more style fixing

Updating for latest changes to fix CVS merge bustage, asking r from pink.

cl

Comment 8

[Chris Lawson (gone)]

Comment on attachment 196744 [details] [diff] [review]
new revision

clearing review flag on obsolete attachment

Comment 9

[Simon Fraser [no longer active]]

Comment on attachment 198847 [details] [diff] [review]
Updated for latest code changes, more style fixing

-  if ([[PreferenceManager sharedInstance]
getBooleanPref:"browser.chrome.favicons" withSuccess:NULL])
+  if ([[PreferenceManager sharedInstance]
getBooleanPref:"browser.chrome.favicons" withSuccess:nil])

I prefer NULL since this is a C-style pointer. We tend to use nil for Obj-C
object pointers, and NULL for other C-style pointers.

Also, please avoid all the unnecessary whitespace changes. I really don't think
that trailing whitespace matters.

Comment 10

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 198847 [details] [diff] [review]
Updated for latest code changes, more style fixing

minusing due to all the extra superfluous whitespace changes, please submit a
new patch. thanks!

Comment 11

[Chris Lawson (gone)]

Attachment: New version, patch only

There we go. Be aware there are a bunch of extra spaces throughout the file
that should probably be fixed at some point ;), but this patch doesn't attempt
to fix any of them.

cl

Comment 12

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 199232 [details] [diff] [review]
New version, patch only

r=pink

what's the +8? i see we do that elsewhere in the file but would be nice to have
a comment.

Comment 13

[Chris Lawson (gone)]

(In reply to comment #12)
> (From update of attachment 199232 [details] [diff] [review] [edit])
> r=pink
> 
> what's the +8? i see we do that elsewhere in the file but would be nice to have
> a comment.

We need to skip the first eight characters in tokenScanner; i.e., at that point,
tokenScanner consists of HREF=""><Menu Spacer> (see comment above that
line, which I could have perhaps made a little clearer to explain the +8). We
want to skip the HREF=""> part, so we start substring matching at the ninth
character.

cl

Comment 14

[Chris Lawson (gone)]

It's be nice to get this checked in for 1.0 since we've changed quite a bit in the bookmark import since the 0.8.x releases. Simon, any chance of an sr within the next few weeks?

cl

Comment 15

[Mark Mentovai]

b + t