258821 - cancel creating bookmark/folder doesn't work

Status: VERIFIED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[Sekundes]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; rv:1.7.3) Gecko/20040910 Firefox/0.10
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; rv:1.7.3) Gecko/20040910 Firefox/0.10

.

Reproducible: Always
Steps to Reproduce:
1. New Bookmark... / New Folder... from bookmarks manager / sidebar / toolbar.
2. Click cancel.
3. Firefox sounds and the bookmark / folder still created.

Comment 1

[Mike Connor [:mconnor]]

not a PR blocker, too late for that, but we need to fix this for 1.0.

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

Comment 2

[Michaël Arnauts]

is happening here also.

Mozilla/5.0 (X11; U; Linux i686; rv:1.7.3) Gecko/20040914 Firefox/0.10

OS should be all i guess.

Comment 3

[Leon Sha]

Attachment: Patch

Comment 4

[Mike Connor [:mconnor]]

Comment on attachment 160215 [details] [diff] [review]
Patch

bump to vlad.  I don't think this is the correct fix, since this code is all
ancient and if its broken, there's a root cause out there.  but vlad knows the
bookmarks API far better so I'll let him figure out what he broke, er, what's
wrong ;)

Comment 5

[Ryan Polk (Quark)]

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

Comment 6

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 160215 [details] [diff] [review]
Patch

>   deleteBookmark: function (aSelection)
>   {
>+    BookmarksUtils.checkSelection(aSelection);
>     BookmarksUtils.removeAndCheckSelection("delete", aSelection);
>   },

Why does this fix it?  Is removeAndCheckSelection depending on something that's
filled in by checkSelection?

Comment 7

[Peter van der Woude [:Peter6]]

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

Comment 8

[Leon Sha]

When you click "New Bookmark...", firefox will create a new resource. And then
you click cancel, firefox will delete it.
As you can see here:
http://lxr.mozilla.org/seamonkey/source/browser/components/bookmarks/content/bookmarks.js#689

But when firefox create the new resource, fire first create a bookmark/folder.
Then use "getSelectionFromResource" which contain "checkSelection". But at this
time the new created bookmark/folder do not have parents. Only after
"insertAndCheckSelection" it will have parent. Since this new created
bookmark/folder do not have parents, then "checkSelection" will consider this
item as ImmutableBookmark/ImmutableFolder. That is not a valid item to be
deleted. So this solution is "checkSelection" after "insertAndCheckSelection". 

Also I will explain the reason why when new created bookmark/folder do not have
parents, "checkSelection" will fail.
http://lxr.mozilla.org/seamonkey/source/browser/components/bookmarks/src/nsBookmarksService.cpp#3065
When a bookmark do not have  parents, "IsBookmarkedResource" will not consider
this as a bookmark resource. So that the type of this item will not be
bookmark/folder. 

Hope it is clear.

Comment 9

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 160215 [details] [diff] [review]
Patch

r=[e-mail], with a comment as to why the extra checkSelection is
needed here.

Comment 10

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

Comment on attachment 160215 [details] [diff] [review]
Patch

a=[e-mail]

Comment 11

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Fixed on aviary, thanks for tracking this down!

Comment 12

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

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

Comment 13

[Mike Connor [:mconnor]]

sorry for bugspam, long-overdue mass reassign of ancient QA contact bugs, filter on "beltznerLovesGoats" to get rid of this mass change