335961 - bookmarks export doesn't export keywords

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, defect, P3)

Description

[Steffen Wilberg]

The Places bookmarks exporter exports all the bookmarks, but without the keywords (right-click - Properties - Keyword).

Comment 1

[Dietrich Ayala (:dietrich)]

Attachment: fix v1

Comment 2

[(not reading, please use [e-mail] instead)]

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);

Comment 3

[Dietrich Ayala (:dietrich)]

Attachment: fix v2

> +  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.

Comment 4

[(not reading, please use [e-mail] instead)]

Comment on attachment 259716 [details] [diff] [review]
fix v2

r=sspitzer, thanks dietrich.

Comment 5

[Dietrich Ayala (:dietrich)]

Checking in nsBookmarksHTML.cpp;
/cvsroot/mozilla/toolkit/components/places/src/nsBookmarksHTML.cpp,v  <--  nsBookmarksHTML.cpp
new revision: 1.33; previous revision: 1.32
done

Comment 6

[Gervase Markham [:gerv]]

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