293663 - Unable to install themes or extensions manually

Status: VERIFIED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[Marcia Knous [:marcia]]

Seen using Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.7.8)
Gecko/20050509 Firefox/1.0.4

STR:

1. Try to install a theme manually using the instructions on UMO
2. Unable to install the theme.

Tested using 1.03 and it did not work either.

Comment 1

[Marcia Knous [:marcia]]

nominating for 1.05

Comment 2

[sairuh (rarely reading bugmail)]

reminder to self: manual installation means d'n'd of the jar file onto the
Themes dialog.

Comment 3

[Tracy Walker [:tracy]]

The same is true for extensions.  drag-n-drop of the downloaded extension to the
extension manager does nothing. 

Without the normal method of installing a Theme/Extension this is a smoketest
blocker on both branch and trunk. However I'll leave severity as normal.  But
once UMO and update mechanism are back in order this should be looked at. 
Drag-n-drop work on Windows and Linux.   

Comment 4

[Tracy Walker [:tracy]]

I thought sairuh had said this worked on Linux. However, I am seeing it on Linux
Fx branch build.  In fact, I am crashing after a series of about 25 errors after
drag-n-drop from desktop to Theme Manager is attempted:

(Gecko:4718): Gtk-CRITICAL **: file gtkdnd.c: line 643 (gtk_drag_get_data):
assertion `GTK_IS_WIDGET (widget)' failed

Comment 5

[sairuh (rarely reading bugmail)]

In reply to comment #4: that's covered by bug 269568, when you quickly d'n'd an
xpi into Extensions.

Comment 6

[Marcia Knous [:marcia]]

Tracy: If memory serves me correct, on the mac d-n-d of an extension to the
extension manager has not worked (I tested in 1.0, does not work even in that
build). As long as you drag into the browser window on the mac it works fine.

(In reply to comment #3)
> The same is true for extensions.  drag-n-drop of the downloaded extension to the
> extension manager does nothing. 
> 
> Without the normal method of installing a Theme/Extension this is a smoketest
> blocker on both branch and trunk. However I'll leave severity as normal.  But
> once UMO and update mechanism are back in order this should be looked at. 
> Drag-n-drop work on Windows and Linux.   

Comment 7

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

Attachment: patch

This patch adds the "application/x-moz-file" flavour so DnD works on Mac OS X.
The reason it works in the content window is that it has this flavour and the
EM does not. Turns out that getting the "text/x-moz-url" flavour for files is
platform specific. It still prefers "text/x-moz-url" over
"application/x-moz-file" so there should be no changes to other OS's.
http://lxr.mozilla.org/seamonkey/source/widget/src/windows/nsDragService.cpp#391


I changed the code so it uses ondragenter to set canDrop instead of ondragover.
The reason is that ondragover is fired repeatedly during a drag and evaluating
the drag data to the file extension of potentially multiple files repeatedly is
inefficient and I suspect may be the cause of the crash in bug 269568.

I also changed the code so if it reaches a failure condition when evaluating it
sets canDrop to false. The reason is that I was easily able to into a state
where canDrop was set to true with the previous code by tryying to drop data
urls.

This patch was tested on both Win32 and Mac OS X and I tested the following
extension and theme drops onto the EM and TM.
xpi / jar files from the file system - installed.
xpi / jar link from a web page - installed.
local xpi / jar files when viewed in the content window - installed
local xpi / jar files along with a non xpi / jar file - drop indicator was
false
data urls - drop indicator was false

Comment 8

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

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

Comment 9

[Daniel Veditz [:dveditz]]

Not blocking a security release.

Comment 10

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

Attachment: patch

In hope that mconnor has the time to review this. :)

Comment 11

[Mike Connor [:mconnor]]

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

>+      var data = { }, length = { }, flavour = { };
>+      xfer.getAnyTransferData(flavour, data, length);

nit: just for readability/obviousness, flip these around to match the order
they're used in.

>+      switch (url.fileExtension) {
>+      case "xpi":

fix the indendation in this block, and the other switch statement

btw, what's the benefit to using this._canDrop as opposed to
aDragSession.canDrop?  Doesn't the drag session value get checked elsewhere? or
is that checking the dnd observer?

r=me with nits picked and an explanation on the canDrop change.

Comment 12

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

Attachment: updated patch

The reason for using canDrop and a var to set it can best be seen here:
http://lxr.mozilla.org/seamonkey/source/toolkit/content/nsDragAndDrop.js#533
http://lxr.mozilla.org/seamonkey/source/toolkit/content/nsDragAndDrop.js#583

When there is a valid data flavour but the file does not have a jar or xpi
fileExtension we still want to set canDrop to false and normally it will just
check if it has a valid data flavour and setting aDragSession.canDrop will end
up with a canDrop indicator when we don't want to allow drops. Also, the code
optimizes this check quite a bit from the original code by setting the var
instead of evaluating it repeatedly in onDragOver.

I also fixed the order and the indentation... a lot of indentation since that
was the style for that file.

Comment 13

[Mike Connor [:mconnor]]

Comment on attachment 186415 [details] [diff] [review]
updated patch

wow, I didn't realize it was all that weird, but I suppose it ought to be
fixed.	You already have blame for enough of this code anyway!

Comment 14

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

Comment on attachment 186415 [details] [diff] [review]
updated patch

Thanks for the review and my thoughts exactly regarding fixing the indents.

Comment 15

[Tracy Walker [:tracy]]

Verified with Deer Park builds:
Windows 2005-07-01-06-trunk
Mac 2005-07-01-07-trunk