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

RESOLVED FIXED in Camino1.0

Status

Camino Graveyard
Bookmarks
--
trivial
RESOLVED FIXED
12 years ago
12 years ago

People

(Reporter: Smokey Ardisson (offline for a while; not following bugs - do not email), Assigned: Chris Lawson (gone))

Tracking

({fixed1.8})

unspecified
Camino1.0
PowerPC
Mac OS X
fixed1.8

Details

Attachments

(1 attachment, 3 obsolete attachments)

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+)
(Assignee)

Comment 1

12 years ago
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
I don't build, so I can't verify, but I'll take your word; good to know this is
mostly fixed already :-)
Depends on: 307743
(Assignee)

Comment 3

12 years ago
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
No longer depends on: 307743
(Assignee)

Comment 4

12 years ago
Created attachment 196620 [details] [diff] [review]
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&gt;" in the first place.

cl
(Assignee)

Updated

12 years ago
Attachment #196620 - Flags: review?
(Assignee)

Comment 5

12 years ago
Created attachment 196744 [details] [diff] [review]
new revision

Fixes a bunch of minor style errors in the file.

cl
Attachment #196620 - Attachment is obsolete: true
Attachment #196744 - Flags: review?
(Assignee)

Updated

12 years ago
Attachment #196620 - Flags: review?
-> cl
Assignee: pinkerton → bugzilla
(Assignee)

Comment 7

12 years ago
Created attachment 198847 [details] [diff] [review]
Updated for latest code changes, more style fixing

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

cl
(Assignee)

Updated

12 years ago
Attachment #196744 - Attachment is obsolete: true
Attachment #198847 - Flags: review?(mikepinkerton)
(Assignee)

Updated

12 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 8

12 years ago
Comment on attachment 196744 [details] [diff] [review]
new revision

clearing review flag on obsolete attachment
Attachment #196744 - Flags: review?

Comment 9

12 years ago
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 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!
Attachment #198847 - Flags: review?(mikepinkerton) → review-
(Assignee)

Comment 11

12 years ago
Created attachment 199232 [details] [diff] [review]
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
Attachment #198847 - Attachment is obsolete: true
Attachment #199232 - Flags: review?(mikepinkerton)
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.
Attachment #199232 - Flags: superreview?(sfraser_bugs)
Attachment #199232 - Flags: review?(mikepinkerton)
Attachment #199232 - Flags: review+
(Assignee)

Comment 13

12 years ago
(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="">&lt;Menu Spacer&gt; (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

Target Milestone: --- → Camino1.0
(Assignee)

Comment 14

12 years ago
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

Updated

12 years ago
Attachment #199232 - Flags: superreview?(sfraser_bugs) → superreview+

Comment 15

12 years ago
b + t
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Keywords: fixed1.8
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.