167180 - land pch's bookmarks patch from bug 160019

Status: VERIFIED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[Asa Dotzler [:asa]]

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

Comment 1

[Asa Dotzler [:asa]]

->pch

Comment 2

[Pierre Chanial]

Attachment: 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
ds.
- 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
platform
- 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

[David Hyatt]

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

sr=hyatt

Comment 4

[Blake Ross]

r/sr=blake

Comment 5

[Brian Ryner (not reading)]

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.


>-NS_IMETHODIMP
>-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

[Pierre Chanial]

Attachment: 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.

Comment 7

[Brian Ryner (not reading)]

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.

Comment 8

[Ben Goodger (use ben at mozilla dot org for email)]

Comment on attachment 98649 [details] [diff] [review]
Patch v1.1

r=[e-mail]

Comment 9

[Pierre Chanial]

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

Comment 10

[Asa Dotzler [:asa]]

landed indeed. verifying.