nsIBookmarksService needs createSeparatorInContainer

RESOLVED FIXED

Status

()

Firefox
Bookmarks & History
RESOLVED FIXED
12 years ago
8 years ago

People

(Reporter: mkaply, Assigned: mkaply)

Tracking

2.0 Branch
x86
Windows XP
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

12 years ago
In using the bookmarks service with the CCK, there is no easy way to add separators given an RDF resource.
(Assignee)

Comment 1

12 years ago
Created attachment 231977 [details] [diff] [review]
Just the code

I'll also need to do something with the uid and create another file I think?
Assignee: nobody → mozilla
Status: NEW → ASSIGNED
(Assignee)

Comment 2

12 years ago
Created attachment 231992 [details] [diff] [review]
branch diff

This is a patch specifically for the 1.8 branch
Attachment #231992 - Flags: review?(benjamin)

Comment 3

12 years ago
Comment on attachment 231992 [details] [diff] [review]
branch diff

>Index: bookmarks/src/nsBookmarksService.cpp

> NS_IMETHODIMP
>+nsBookmarksService::CreateSeparatorInContainer(nsIRDFResource* aParentFolder,
>+                                               PRInt32 aIndex,
>+                                               nsIRDFResource** aResult)
>+{
>+    nsCOMPtr<nsIRDFNode> nodeType;
>+    GetSynthesizedType(aParentFolder, getter_AddRefs(nodeType));
>+    if (nodeType == kNC_Livemark)
>+        return NS_RDF_ASSERTION_REJECTED;
>+
>+    nsresult rv = CreateSeparator(aResult);
>+    if (NS_SUCCEEDED(rv))
>+        rv = InsertResource(*aResult, aParentFolder, aIndex);
>+    return rv;
>+}

If CreateSeparator succeeds but InsertResource fails, you'll end up leaking (or potentially leaking) aResult.
Attachment #231992 - Flags: review?(benjamin) → review-
(Assignee)

Comment 4

12 years ago
(In reply to comment #3)
> (From update of attachment 231992 [details] [diff] [review] [edit])

> If CreateSeparator succeeds but InsertResource fails, you'll end up leaking (or
> potentially leaking) aResult.
> 

Interesting. I copied the code from elsewhere in the file:

http://lxr.mozilla.org/seamonkey/source/browser/components/bookmarks/src/nsBookmarksService.cpp#2943

http://lxr.mozilla.org/seamonkey/source/browser/components/bookmarks/src/nsBookmarksService.cpp#2828

So that bug exists elsewhere as well.

Would I just NS_RELEASE on the RDFResource?
(Assignee)

Comment 5

12 years ago
Created attachment 232003 [details] [diff] [review]
New patch

This patch releases upon failure, and fixes the other places in this file that have that problem as well.
Attachment #231992 - Attachment is obsolete: true
Attachment #232003 - Flags: review?(benjamin)

Updated

12 years ago
Attachment #232003 - Flags: review?(benjamin) → review+
(Assignee)

Comment 6

12 years ago
Comment on attachment 232003 [details] [diff] [review]
New patch

This is a branch only patch that adds a new interface that allows the CCK to create separators. 

Needed for some of the custom builds that are being done by MoCo.
Attachment #232003 - Flags: approval1.8.1?
Comment on attachment 232003 [details] [diff] [review]
New patch

a=drivers, please land on the branch.
Attachment #232003 - Flags: approval1.8.1? → approval1.8.1+
Seems like you should at least get the leak fixes into the trunk, if not the whole thing.
(Assignee)

Comment 9

12 years ago
This will definitely go on the trunk as well in a little different incarnation.

Comment 10

12 years ago
Possible regression:  BeOS branch build broke sometime between 2006-07-30 and today.

Comment 11

12 years ago
Updated from CVS again - just picked up revised patch and can build now.  Apologies for bug-chatter.
Version: Trunk → 2.0 Branch
(Assignee)

Comment 12

9 years ago
This code is not in the trunk anymore, so this was 1.8 only.
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
Keywords: dev-doc-needed
Keywords: dev-doc-needed
You need to log in before you can comment on or make changes to this bug.