land pch's bookmarks patch from bug 160019

VERIFIED FIXED in Phoenix0.1



Bookmarks & History
16 years ago
16 years ago


(Reporter: asa, Assigned: Pierre Chanial)



Firefox Tracking Flags

(Not tracked)




(1 attachment, 1 obsolete attachment)

16.57 KB, patch
Brian Ryner (not reading)
: review+
Details | Diff | Splinter Review


16 years ago
We want bookmarks to not suck for Phoenix. Pierre is gonna help us with that :)

Comment 1

16 years ago
Assignee: blaker → chanial
Target Milestone: --- → Phoenix0.1

Comment 2

16 years ago
Created attachment 98344 [details] [diff] [review]
Patch v1.0 for the mozilla trunk

Since nsIBookmarkService is not forked, I need few new methods.
What this patch does is:
- rename Create(Bookmark|Group|Folder)WithDetails into Create(...)InContainer
to match what the methods do and their names ie inserting a new resource in the
- remove Create(Bookmark|Group|Folder) methods: only used in two places. They
were creating a resource and appending it to the specified folder and were
redundent with Create(...)WithDetails called with an index -1
- add Create(Bookmark|Group|Folder|Separator) methods: they create and return a
resource but they do not insert it in the ds. These are the methods I need for
the bookmark transaction manager.
- fix a cpp warning: systemBookmarksURL is only used in the WIN or BeOS
- fix a js warning (my bad) in bookmarksOverlay.js
- fix a regression in navigatorDD.js: when dropping a item in the empty area of
the personal toolbar, the feedback line was not removed.

Comment 3

16 years ago
Comment on attachment 98344 [details] [diff] [review]
Patch v1.0 for the mozilla trunk

Attachment #98344 - Flags: review+

Comment 4

16 years ago
Comment on attachment 98344 [details] [diff] [review]
Patch v1.0 for the mozilla trunk

>--- mozilla/xpfe/components/bookmarks/public/nsIBookmarksService.idl	1 Apr 2002 08:47:11 -0000	1.20
>+++ mozilla/xpfe/components/bookmarks/public/nsIBookmarksService.idl	8 Sep 2002 19:10:22 -0000
>@@ -56,17 +56,19 @@
>     void addBookmarkImmediately(in string aURI, in wstring aTitle, in long bmType, in wstring docCharset);
>-    nsIRDFResource createFolder(in wstring aName, in nsIRDFResource aParentFolder);
>-    nsIRDFResource createFolderWithDetails(in wstring aName, in nsIRDFResource aParentFolder, 
>+    nsIRDFResource createFolder           (in wstring aName);
>+    nsIRDFResource createFolderInContainer(in wstring aName, in nsIRDFResource aParentFolder, 
>                                            in long aIndex);

I think the general format we're using in this idl file is not to put extra
space between the function name and the parameters.

>-nsBookmarksService::CreateFolderWithDetails(const PRUnichar* aName, 
>+nsBookmarksService::CreateFolderInContainer(const PRUnichar* aName, 
>                                             nsIRDFResource* aParentFolder, PRInt32 aIndex,
>                                             nsIRDFResource** aResult)
> {
>-  return CreateFolderWithDetails(aName, aParentFolder, aIndex, aResult, PR_FALSE);
>+  nsresult rv;
>+  rv = CreateFolder(aName, aResult);

Just collapse this onto 1 line.  Same thing several other places in this file.

>+    if (aIndex && aIndex >= 0) 

Are you sure this condition is correct?  At the very least, change >= to >,
since it will always be false if aIndex == 0.

Comment 6

16 years ago
Created attachment 98649 [details] [diff] [review]
Patch v1.1

I am a fool!
I was asserting the folder group arc after that the resource were inserted in
the ds. As a consequence, new groupmarks were appearing as folder.
This patch corrects that and also fixes an another regression with the drag
into folder feature (the personal toolbar submenu of the bookmark menu in the
ptf may not close because its id is the same as the recently changed toolbar
hbox one)

It includes bryner comments: when the argument is |null| or |undefined| in the
js caller, aIndex is 0 in the cpp method. I then only have to check if
aIndex<=0 to append it to the folder.
Attachment #98344 - Attachment is obsolete: true
Comment on attachment 98649 [details] [diff] [review]
Patch v1.1

I know the style in nsBookmarksService is kind of a mess, but in general I
think we'd like C++ function names to start with a capital letter.  Change
insertResource to InsertResource and r/sr=bryner.
Attachment #98649 - Flags: review+

Comment 9

16 years ago
There is still work to do, I filed some bugs in the phoenix bookmark component,
esp. bug 168410.
I have modified the context menus by dropping items and changing some properties.
This can be very easily modified.
Last Resolved: 16 years ago
Resolution: --- → FIXED

Comment 10

16 years ago
landed indeed. verifying.
You need to log in before you can comment on or make changes to this bug.