376302 - unable to drop favicon or link onto personal toolbar folder, favicon drop leaves the cursor behind

Status: VERIFIED FIXED

(Firefox :: Bookmarks & History, defect)

Description

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

unable to drop favicon or link onto personal toolbar folder, favicon drop leaves the cursor behind

this is with my own debug, windows, trunk-with-places-bookmarks build:

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a4pre) Gecko/20070402 Minefield/3.0a4pre

I'll attach a screen shot.

see also bug #375481, but with this regression i don't see anything in the console.

Comment 1

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

>  favicon drop leaves the cursor behind

but note, url drops (from links in a page) do not leave the cursor behind.

Comment 2

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

Attachment: screen shot

Comment 3

[Asaf Romano (gone)]

Does this work on windows? I think this is the same issue as reported on bug 358446 (custom d&d flavors don't work).

Comment 4

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

> Does this work on windows? I think this is the same issue as reported 
> on bug 358446 (custom d&d flavors don't work).

That's a cocoa bug, and I'm seeing the problem on Windows XP.  (I haven't tried Mac OS X.)

Note, I know this used to work very recently, as it regressed (but we fixed it) on 03-26-2007, see bug #375481.

Comment 5

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

fwiw, I'm still seeing this, even with a fresh profile.

dietrich / mano:  do you think this is bad enough to be added to the list of places-bm-blockers for the next alpha?

Comment 6

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

a silent failure in PCDH_onDrop()

ex = TypeError: createTxn.childTransactions has no properties

working on it...

Comment 7

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

this appears to be a case of a missing underscore:

-      createTxn.childTransactions.push(
+      createTxn._childTransactions.push(

it moved from childTransactions to _childTransactions as a result of bug #373504

After switching to use _childTransactions, I get further, but now I'm not getting the title, and I'm getting an error:

************************************************************
* Call to xpconnect wrapped JSObject produced this error:  *
[Exception... "Component returned failure code: 0x80070057 (NS_ERROR_ILLEGAL_VAL
UE) [nsINavBookmarksService.getItemTitle]"  nsresult: "0x80070057 (NS_ERROR_ILLE
GAL_VALUE)"  location: "JS frame :: chrome://browser/content/places/controller.j
s :: PEITT_doTransaction :: line 1935"  data: no]
************************************************************

that resembles bug #371675 (which was fixed.)

still investigating...

Comment 8

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

we are attempting to set the title as the child transaction, after the create:

      // Creating and Setting the title is a two step process
      var createTxn =
        new PlacesCreateItemTransaction(data.uri, container, index);
      var title = type == this.TYPE_X_MOZ_URL ? data.title : data.uri;
      createTxn._childTransactions.push(
          new PlacesEditItemTitleTransaction(-1, title));
      return createTxn;

The problem is that in  PlacesEditItemTitleTransaction() the call to this.bookmarks.getItemTitle(this._id); will fail when id is -1.

but it should not be -1, as the parent transaction should be setting the id of the child txns.

more investigating...

Comment 9

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

Attachment: patch

note, there are other instances in controller.js where we use "id" instead of "_id" that might need to be fixed, too.

but for now, this fixes this bug.

Comment 10

[Asaf Romano (gone)]

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

Rather use the fourth argument of the create-item transaction and avoid the child-transaction mess. Transactions which support the child-transactions pseudo-API expose a real |id| setter.

Comment 11

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

Attachment: revised patch, per asaf

Comment 12

[Asaf Romano (gone)]

Comment on attachment 263423 [details] [diff] [review]
revised patch, per asaf

r=mano

Comment 13

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

fixed

Checking in utils.js;
/cvsroot/mozilla/browser/components/places/content/utils.js,v  <--  utils.js
new revision: 1.32; previous revision: 1.31
done

This should have also impacted mac

Comment 14

[Tracy Walker [:tracy]]

verified with: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a8pre) Gecko/2007080805 Minefield/3.0a8pre

this is in litmus

Comment 15

[Gervase Markham [:gerv]]

Bug 451915 - move Firefox/Places bugs to Firefox/Bookmarks and History. Remove all bugspam from this move by filtering for the string "places-to-b-and-h".

In Thunderbird 3.0b, you do that as follows:
Tools | Message Filters
Make sure the correct account is selected. Click "New"
Conditions: Body   contains   places-to-b-and-h
Change the action to "Delete Message".
Select "Manually Run" from the dropdown at the top.
Click OK.

Select the filter in the list, make sure "Inbox" is selected at the bottom, and click "Run Now". This should delete all the bugspam. You can then delete the filter.

Gerv