Closed
Bug 335961
Opened 19 years ago
Closed 18 years ago
bookmarks export doesn't export keywords
Categories
(Firefox :: Bookmarks & History, defect, P3)
Firefox
Bookmarks & History
Tracking
()
RESOLVED
FIXED
Firefox 3 alpha4
People
(Reporter: steffen.wilberg, Assigned: dietrich)
References
Details
Attachments
(1 file, 1 obsolete file)
(deleted),
patch
|
moco
:
review+
|
Details | Diff | Splinter Review |
The Places bookmarks exporter exports all the bookmarks, but without the keywords (right-click - Properties - Keyword).
Updated•19 years ago
|
Assignee: nobody → brettw
Updated•19 years ago
|
OS: Windows XP → All
Priority: -- → P2
Hardware: PC → All
Updated•19 years ago
|
Priority: P2 → P3
Target Milestone: --- → Firefox 3 alpha2
Updated•18 years ago
|
Assignee: brettw → nobody
Assignee | ||
Comment 1•18 years ago
|
||
Assignee | ||
Updated•18 years ago
|
Target Milestone: Firefox 3 alpha2 → Firefox 3 alpha4
Comment 2•18 years ago
|
||
1)
+ // keyword (shortcuturl)
+ nsAutoString keyword;
+ rv = GetKeywordForBookmark(bookmarkId, keyword);
+ if (NS_FAILED(rv)) return rv;
+ if (keyword.Length() > 0) {
I think using IsEmpty() would be better than Length().
2)
+ char* rawKeyword = ToNewUTF8String(keyword);
Three questions:
a) Do we have to escape the keyword, like we do for the title?
b) If yes, see item #4 about unescaping.
c) Is there another way to do this, without malloc / free (using one of the helper string classes)?
3)
+ rv = aOutput->Write(kKeywordAttribute, sizeof(kKeywordAttribute)-1, &dummy);
+ rv = aOutput->Write(rawKeyword, sizeof(rawKeyword), &dummy);
+ rv = aOutput->Write(kQuoteStr, sizeof(kQuoteStr)-1, &dummy);
+ if (NS_FAILED(rv)) return rv;
+ nsMemory::Free(rawKeyword);
after the first two Write() calls, it looks like you are missing two calls to:
+ if (NS_FAILED(rv)) return rv;
Did you omit them to avoid returning and leaking the string on failure?
4)
This isn't part of your patch, but about the title, don't we have unescape the title, since we escape it when we write it out?
787 mBookmarksService->SetItemTitle(frame.mPreviousId, frame.mPreviousText);
Assignee | ||
Comment 3•18 years ago
|
||
> + if (keyword.Length() > 0) {
>
> I think using IsEmpty() would be better than Length().
fixed
> 2)
>
> + char* rawKeyword = ToNewUTF8String(keyword);
>
> Three questions:
>
> a) Do we have to escape the keyword, like we do for the title?
> b) If yes, see item #4 about unescaping.
yes, added
> c) Is there another way to do this, without malloc / free (using one of the
> helper string classes)?
now that we're escaping this, i searched around the tree, the common pattern seems to be as in this current patch.
> 3)
>
> + rv = aOutput->Write(kKeywordAttribute, sizeof(kKeywordAttribute)-1,
> &dummy);
> + rv = aOutput->Write(rawKeyword, sizeof(rawKeyword), &dummy);
> + rv = aOutput->Write(kQuoteStr, sizeof(kQuoteStr)-1, &dummy);
> + if (NS_FAILED(rv)) return rv;
> + nsMemory::Free(rawKeyword);
>
> after the first two Write() calls, it looks like you are missing two calls to:
>
> + if (NS_FAILED(rv)) return rv;
>
> Did you omit them to avoid returning and leaking the string on failure?
i changed the order around so as to handle the result values and not leak.
> 4)
>
> This isn't part of your patch, but about the title, don't we have unescape the
> title, since we escape it when we write it out?
>
> 787 mBookmarksService->SetItemTitle(frame.mPreviousId,
> frame.mPreviousText);
>
i did some round-trip testing (export, import, export) and the data is getting properly un-escaped. i haven't yet found where this is occurring. i looked through the parser implementation but didn't see anything. ugh.
Attachment #259616 -
Attachment is obsolete: true
Attachment #259616 -
Flags: review?(sspitzer)
Assignee | ||
Updated•18 years ago
|
Attachment #259716 -
Flags: review?(sspitzer)
Comment 4•18 years ago
|
||
Comment on attachment 259716 [details] [diff] [review]
fix v2
r=sspitzer, thanks dietrich.
Attachment #259716 -
Flags: review?(sspitzer) → review+
Assignee | ||
Comment 5•18 years ago
|
||
Checking in nsBookmarksHTML.cpp;
/cvsroot/mozilla/toolkit/components/places/src/nsBookmarksHTML.cpp,v <-- nsBookmarksHTML.cpp
new revision: 1.33; previous revision: 1.32
done
Status: ASSIGNED → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
Comment 6•15 years ago
|
||
Bug 451915 - move Firefox/Places bugs to Firefox/Bookmarks and History. Remove all bugspam from this move by filtering for the string "places-to-b-and-h".
In Thunderbird 3.0b, you do that as follows:
Tools | Message Filters
Make sure the correct account is selected. Click "New"
Conditions: Body contains places-to-b-and-h
Change the action to "Delete Message".
Select "Manually Run" from the dropdown at the top.
Click OK.
Select the filter in the list, make sure "Inbox" is selected at the bottom, and click "Run Now". This should delete all the bugspam. You can then delete the filter.
Gerv
Component: Places → Bookmarks & History
QA Contact: places → bookmarks
You need to log in
before you can comment on or make changes to this bug.
Description
•