land pch's bookmarks patch from bug 160019

VERIFIED FIXED in Phoenix0.1

Status

()

Firefox
Bookmarks & History
VERIFIED FIXED
16 years ago
16 years ago

People

(Reporter: asa, Assigned: Pierre Chanial)

Tracking

unspecified
Phoenix0.1
x86
All
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment, 1 obsolete attachment)

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

Description

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

Comment 1

16 years ago
->pch
Assignee: blaker → chanial
Target Milestone: --- → Phoenix0.1
(Assignee)

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

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

sr=hyatt
Attachment #98344 - Flags: review+

Comment 4

16 years ago
r/sr=blake
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.
(Assignee)

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+
(Assignee)

Comment 9

16 years ago
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.
Status: NEW → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → FIXED
(Reporter)

Comment 10

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