260068 - Dropping extension/theme files to manager didn't install it

Status: RESOLVED FIXED

(Toolkit :: Add-ons Manager, enhancement)

Description

[Che-Ching Wu]

User-Agent:       Mozilla/5.0 (OS/2; U; Warp 4.5; rv:1.7.3) Gecko/20040914 Firefox/0.10
Build Identifier: Mozilla/5.0 (OS/2; U; Warp 4.5; rv:1.7.3) Gecko/20040914 Firefox/0.10

Dropping extension/theme files to manager didn't install it

Reproducible: Always
Steps to Reproduce:
1.
2.
3.

Comment 1

[Peter Weilbacher]

Mozilla/5.0 (OS/2; U; Warp 4.5; en-US; rv:1.7.3) Gecko/20040927 Firefox/0.10
Right, I downloaded the qute_2.1.3.jar file, dropped it on the theme manager and
nothing happened. But why did you think that it should, the help pages do not
suggest anything like that, does it work on Windows?
Severity -> enhancement.

Comment 2

[Nickolay_Ponomarev]

Yes, it works on Windows. afaik, it doesn't work very well on linux.
I haven't seen OS/2, if Firefox ever supported drag-and-drop to EM, it's a bug
and a regression, otherwise it is an enhancement.

I think it would help if you told what application are you dragging from.

confirming per comment 1

Comment 3

[Che-Ching Wu]

I think I got this information from firefox help web site or forum at
mozillazine.

I forgot which xpi/jar I tried, but I think that I had tried these:
red cats (green flavor) theme and toolbar enhancements.

And, if I can't drag and drop xpi/jar to managers, how do I install
a new one which is not at update.mozilla.org and just an URL?

Comment 4

[Peter Weilbacher]

Rich, any chance you can have a look why this works on Windows and Linux but not
on OS/2?

ccwu: You just drop it on the main window.

Comment 5

[Robin Monks]

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

Comment 6

[Peter Weilbacher]

Hmm? I thought that this bug is OS/2 specific. I just tried today under Linux
myself and there it works...

Comment 7

[Rich Walsh]

This is actually a cross-platform bug.  The d&d code in extensions.js fails to
remove the title from a kURLMime string before passing it to nsURI.  nsURI then
concatenates the URL & title, producing an unusable filename.  While Firefox &
OS/2's native-drag code provide a title for file URLs, the native-drag code on
other platforms apparently doesn't, so the problem wasn't widely recognized.

The problem can be reproduced on any platform using these scenarios:

(1) Use Firefox to display a local directory that contains .xpi and/or .jar
files.  Drag a file from the browser to the Extensions or Themes window.  The
target window will indicate a drop is permitted;  however, that drop will fail.
 In this scenario, if the filepath were "c:\downloads\extension.xpi", the code
would see "c:\downloads\extension.xpiextension.xpi".

(2) Go to a the download page for some extension, e.g. Adblock
<https://addons.update.mozilla.org/extensions/moreinfo.php?application=firefox&version=1.0&id=10">.
 Drag from the "Install Now" link
<http://ftp.mozilla.org/pub/mozilla.org/extensions/adblock/adblock-0.5.2.039-fx.xpi>
to the Extensions Manager window.  The window will display a "no-drop" pointer
because what the codes sees is "[...].xpiInstall Now".

The patch fixes this bug by truncating the URL string at the linefeed which
separates URL from title, if present.  It has been tested on Windows XP and OS/2.

Comment 8

[Rich Walsh]

Attachment: d&d fix for extensions.js

Comment 9

[Rich Walsh]

This bug should be reclassified:  OS= All,  Severity= Normal

Comment 10

[Hiro]

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

Comment 11

[Hiro]

The Mac version reproduces. 
Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; ja-JP; rv:1.8b) Gecko/20050205
Firefox/1.0+

Hardware and OS -> ALL

Comment 12

[Mike Connor [:mconnor]]

Comment on attachment 172846 [details] [diff] [review]
d&d fix for extensions.js

r=[e-mail]

Comment 13

[Boris Zbarsky [:bzbarsky]]

Patch checked in to trunk.  Not sure whether that fully resolves the bug, so
leaving it to the patch author to resolve.

Comment 14

[Peter Weilbacher]

This checkin certainly fixed the problem on OS/2. If one of the Linux and Mac
users who saw this before can confirm, please resolve fixed (Tony, crot0?).

Comment 15

[Hiro]

It still reproduces. 
Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8b2) Gecko/20050311
Firefox/1.0+

Comment 16

[Robert Strong (they/them - no direct email)]

Attachment: patch

This patch adds the flavour application/x-moz-file as well as the existing
text/x-moz-url. With Max OS X dragging an xpi to the content window starts and
install but dragging it the the Extensions Manager did not. After finding that
it was the application/x-moz-file flavour that was succeeding in the content
window I took the same route for the DnD code in extensions.js. I also noticed
that it was very easy to end up with canDrop in the incorrect state in
onDragOver in the previous code and decided to set canDrop to false by default
instead of true so if it fails canDrop will be false. Having said that, I was
no longer able to get into the incorrect state when using this code but I
believe this is safer. This was tested successfully on Win32 and Mac OS X by
dropping files as well as links onto both the Extensions Manager and the Themes
Manager.

Comment 17

[Robert Strong (they/them - no direct email)]

Attachment: cleaner patch

Had some time to do a little more testing and now this properly sets canDrop
when the uri can't be QI'd to a nsIURL.

Comment 18

[Robert Strong (they/them - no direct email)]

Attachment: patch

Comment 19

[Rich Walsh]

(In reply to comment #16)
> With Max OS X dragging an xpi to the content window starts and install but
> dragging it the the Extensions Manager did not.


None of the comments wrt Mac OS X have identified a specific scenario where a
drop fails to install a theme or extension.  Does it fail when dragging from:


1- Firefox's listing of a local folder?
2- a link on a page?
3- a Mac folder (i.e. a native drag)?
4- all of the above


If it's number 3, then extensions.js may not be the most appropriate place to
fix the problem.

Comment 20

[Robert Strong (they/them - no direct email)]

(In reply to comment #19)
> 1- Firefox's listing of a local folder?
Not tested
> 2- a link on a page?
That is successful and worked previously
> 3- a Mac folder (i.e. a native drag)?
Yes
> If it's number 3, then extensions.js may not be the most appropriate place to
> fix the problem.
Agree wholeheartedly... like I said, I took the scenario where install works and
duplicated it in extensions.js while also taking care of a failure scenario in
the onDragOver code where it set canDrop incorrectly. I seldom have a Mac handy
and wanted to get the code to a state where it would at least work on Mac OS X
before 1.1a.

I think this fix is appropriate for 1.1a if it can get reviewed in time. Then if
a core problem is identified and fixed then extensions.js can be fixed
afterward. If application/x-moz-file is not needed then I am quite sure the
workaround I used is fairly prolific from looking through lxr (e.g. browser.js
just for starters). Also, it seems it would be appropriate for extensions.js to
use retrieveURLFromData instead of
data.value.QueryInterface(Components.interfaces.nsISupportsString).data.split("\n")[0];
but I experienced some issues getting that to work properly and don't have the
time currently to review all of the consumers to check how changing that could
adversely affect other code... is that why you didn't use it?

Comment 21

[Rich Walsh]

(In reply to comment #20)
> > If it's number 3 [a native drag], then extensions.js may not be the most
> > appropriate place to fix the problem.
>
> [I] wanted to get the code to a state where it would at least work on Mac
> OS X before 1.1a.

No objections there.

> Then if a core problem is identified and fixed then extensions.js can be
> fixed afterward. If application/x-moz-file is not needed then I am quite
> sure the workaround I used is fairly prolific from looking through lxr

On other platforms (AFAICT), native file drags are described as
"text/x-moz-url".  On the Mac they're "application/x-moz-file", whence the need
for this patch.  In other code, I see comments about problems with uniquely
identifying files on a Mac, so perhaps file:// URLs aren't workable.  But if
they are, then the native dnd code should use them to avoid bugs like this one.

> Also, it seems it would be appropriate for extensions.js to use
> retrieveURLFromData instead of data.value.[...]data.split("\n")[0];
> but I experienced some issues getting that to work properly ...
> is that why you didn't use it?

No, my goal was to make as minimal a change as possible in the hope that would
expedite review and check-in (it seems to have worked :-)

Comment 22

[Robert Strong (they/them - no direct email)]

(In reply to comment #21)
> On other platforms (AFAICT), native file drags are described as
> "text/x-moz-url".  On the Mac they're "application/x-moz-file", whence the need
> for this patch.  In other code, I see comments about problems with uniquely
> identifying files on a Mac, so perhaps file:// URLs aren't workable.  But if
> they are, then the native dnd code should use them to avoid bugs like this one.
Actually, this is due to the following OS specific code that appears was not
added as a one-off for Mac OS X. It would be better to either avoid OS specific
code and add this for all OS's or just not add this type of coercion in the code
and thereby force specifying "application/x-moz-file" for DnD code to avoid bugs
like this one. I personally prefer forcing the use of "application/x-moz-file"
since that simplifies knowing whether or not it is truly an url vs. a file
though it could add code for the consumers. Then again, it appears most of those
consumers already have added the code since this doesn't work with Mac OS X.
http://lxr.mozilla.org/seamonkey/source/widget/src/windows/nsDragService.cpp#391

Comment 23

[Robert Strong (they/them - no direct email)]

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

Obsolescing and canceling review since there is a more complete patch in bug
293663

Comment 24

[Robert Strong (they/them - no direct email)]

I believe the remaining issues have been fixed by checkin of the patch in bug
293663. Reolving fixed.