200048 - "Set as New Bookmark Folder / New Personal Toolbar" not working

Status: VERIFIED FIXED

(SeaMonkey :: Bookmarks & History, defect, P3)

Description

[Nahor]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.4a) Gecko/20030331
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.4a) Gecko/20030331

The "View | Set as New Bookmark Folder" item in the Bookmark Manager doesn't
have any effet.

Reproducible: Always

Steps to Reproduce:
1.Select a folder.
2.Select the menu "View | Set as New Bookmark Folder"
3.Go to any web page
4.Press "Ctrl+D" or select the menu item "Bookmarks | Bookmark this page"
5.Open the bookmark manager and look for the new bookmarked page

Actual Results:  
The bookmark is appended at the end of the bookmark list (child of the root
folder, aka "Bookmarks for <profile name>").

Expected Results:  
The bookmark appended to the folder selected when I click on "Set as New
Bookmark Folder"

Bookmarks aren't appended to the correct folder since the checking of the new
bookmark infrastructure.

Comment 1

[Chris Lyon]

This should have been fixed by the patch to bug 53835

Comment 2

[Chris Lyon]

Nahor, are you seeing an error in the Javascript console?

Comment 3

[Jason Bassford]

Confirming the bug.

Here's the JavaScript Console error on setting a different "New" folder:

Error: [Exception... "Component returned failure code: 0x80070057
(NS_ERROR_ILLEGAL_VALUE) [nsIXULTreeBuilder.getResourceAtIndex]"
nsresult: "0x80070057 (NS_ERROR_ILLEGAL_VALUE)"  location: "JS frame ::
chrome://communicator/content/bookmarks/bookmarksTree.xml#bookmarks-tree.getRowResource()
:: getRowResource :: line 2"  data: no]
Source File:
chrome://communicator/content/bookmarks/bookmarksTree.xml#bookmarks-tree.getRowResource()
Line: 2

Actually doing a Bookmark This Page function doesn't produce an error.

Also, it's doing more than just not working.  I already had a "New Bookmark
Folder" in place.  (Which didn't work any more.)  But as soon as I set a
different bookmark folder as the "New" folder, the Bookmark Manager (while
marking the other one as new, even though it still doesn't work) decided to put
"rdf:#$V++Kq1" under the Location field of my previous "new" folder.  It's also
greyed out the option to delete it so I can't do anything to get rid of it. 
Additionally, it's marked the Location field of my updated "New" folder with
"NC: PersonalToolbarFolder".  I suppose that makes sense, but I'm not used to
ever seeing anything listed in the Location field for any of my folders in the
past.  (I will restore a backup copy of my bookmarks.html file to clean up this
mess.)

Comment 4

[Jason Bassford]

I should have mentioned that I'm also using XP with the 3/31-08 build.

Comment 5

[Nahor]

I get the same error from comment #3 but only I do a "bookmark this page" or
when I delete the bookmark. I don't get any error when I select "Set as New
Bookmark Folder".

Actually, it seems worse than I thought, it looks like it isn't just the "New
Bookmark Folder" that doesn't work but "Set as Personal Folder" too. I don't
know what "New Internet Search Folder" is supposed to do so I can't really test
this it but a 'NEW_SEARCH_FOLDER="true"' is correctly added in the
bookmarks.html file.

Comment 6

[Paul Wyskoczka]

Nav triage team: nsbeta1+/adt3

Comment 7

[Paul Wyskoczka]

Nav triage team: over to Jan for a look.

Comment 8

[Jo Hermans]

*** Bug 201940 has been marked as a duplicate of this bug. ***

Comment 9

[Jo Hermans]

Note that bug 201940 mentions that it doesn't work for "New Personal Toolbar
Folder" either, so I'm changing the summary.
 
I can confirm both bugs on the 20030412 build on Mac OS X 10.2.5 (it was driving
me nuts this weekend), so All/All.

Comment 10

[Stephen Fitch]

Confirming this report. I can reproduce the problem on

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.4a) Gecko/20030401

This is a clear regression from the way 1.3 was working.

Comment 11

[Alexander Dietrich]

I have the same problem with Linux build 2003050714, but I get this JavaScript
error:

Error: uncaught exception: [Exception... "Cannot find interface information for
parameter arg 0 [nsIBookmarksService.transactionManager]"  nsresult: "0x80570006
(NS_ERROR_XPC_CANT_GET_PARAM_IFACE_INFO)"  location: "JS frame :: <unknown
filename> :: onxblconstructor :: line 2"  data: no]

Comment 12

[Stephan Slabihoud (:slabbi)]

Confirming with 1.4b on Windows XP.

Comment 13

[Erik]

*** Bug 205739 has been marked as a duplicate of this bug. ***

Comment 14

[Erik]

Changing summary from "toobar" to "toolbar" in order to make it more easy to
find this bug ;)

Comment 15

[slice1900]

I see the same error as comment #11 in the Linux 1.4b gtk2 build, 1.3 didn't
have this.  Build ID is 2003050809.  I can't delete any bookmarks, but I can
still add new ones via "bookmark this page".

Comment 16

[Jan Varga [:janv]]

I have a fix for "Set as New Bookmark Folder", the latter is more difficult to
fix and the patch in bug 205378 must land first.

Comment 17

[Torben]

*** Bug 205941 has been marked as a duplicate of this bug. ***

Comment 18

[Chris Petersen]

Verifying that Set as New Bookmark Folder is functioning properly in the
2003-05-27-03 Macho and 2003-05-27-08 Win32 trunk builds. Still need a fix for
"Set as Personal Toolbar Folder" in this bug though.

Comment 19

[Randell Jesup [:jesup] (needinfo me)]

205378 is fixed on trunk and is approved for checkin to 1.4, so getting a fix to
this one would be great.

Comment 20

[Jan Varga [:janv]]

I'm working on a fix, but it's not trivial.

Comment 21

[Jan Varga [:janv]]

Attachment: patch

ok, here it is

Comment 22

[Jan Varga [:janv]]

This patch also fixes bug 207394. We need to get this in ASAP to bake it on the
trunk first.

Comment 23

[jag (Peter Annema)]

Comment on attachment 125030 [details] [diff] [review]
patch

+	 else {
+	     // Do nothing.
+	     rv = NS_OK;
+	 }

No need for this, if you've reached this code you know that NS_SUCCEEDED(rv)
before the first |if| and that rv hasn't changed since.

+    rv = CopyResource(tempResource, aFolder);
+
+    return rv;

Just make this |return CopyResource(...);|

Could you add a comment above the first CopyResource along the lines of //
swap(aFolder, kNC_PersonalToolbarFolder);?

r=jag with that, or sr= if you can find someone to r=.

Comment 24

[Phil Pishioneri]

Re: comment 23 about coalescing a function call and a return.

In a review at bug 177280 comment 56, Darin Fisher asked someone to split up a
|return function()|.  The same reasoning would seem to apply here.

Comment 25

[Jan Varga [:janv]]

I don't see any code size difference (linux, gcc 3.3)

Comment 26

[(not reading, please use [e-mail] instead)]

Comment on attachment 125030 [details] [diff] [review]
patch

sr=sspitzer

Comment 27

[Jan Varga [:janv]]

fixed on the trunk

Comment 28

[(not reading, please use [e-mail] instead)]

Comment on attachment 125030 [details] [diff] [review]
patch

a=sspitzer for 1.4, since asa has already listed this as a 1.4 blocker.

Comment 29

[Rafael Ebron (:rebron)]

a=adt.  Please land on branch and add fixed1.4 keyword.

Comment 30

[Jan Varga [:janv]]

fixed on the branch

Comment 31

[Chris Petersen]

Verified on both the Macho and Win32 branch (2003-06-09-05) and (2003-09-08)
trunk builds.