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+)
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 :-)
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
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>" in the first place. cl
Created attachment 196744 [details] [diff] [review] new revision Fixes a bunch of minor style errors in the file. cl
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
Comment on attachment 196744 [details] [diff] [review] new revision clearing review flag on obsolete attachment
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!
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
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.
(In reply to comment #12) > (From update of attachment 199232 [details] [diff] [review] ) > 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
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
b + t