Closed Bug 347210 Opened 18 years ago Closed 16 years ago

nsIBookmarksService needs createSeparatorInContainer

Categories

(Firefox :: Bookmarks & History, defect)

2.0 Branch
x86
Windows XP
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: mkaply, Assigned: mkaply)

Details

Attachments

(2 files, 1 obsolete file)

In using the bookmarks service with the CCK, there is no easy way to add separators given an RDF resource.
Attached patch Just the codeSplinter Review
I'll also need to do something with the uid and create another file I think?
Assignee: nobody → mozilla
Status: NEW → ASSIGNED
Attached patch branch diff (obsolete) — Splinter Review
This is a patch specifically for the 1.8 branch
Attachment #231992 - Flags: review?(benjamin)
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-
(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?
Attached patch New patchSplinter Review
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)
Attachment #232003 - Flags: review?(benjamin) → review+
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.
This will definitely go on the trunk as well in a little different incarnation.
Possible regression:  BeOS branch build broke sometime between 2006-07-30 and today.
Updated from CVS again - just picked up revised patch and can build now.  Apologies for bug-chatter.
Version: Trunk → 2.0 Branch
This code is not in the trunk anymore, so this was 1.8 only.
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: