306182 - Theme ->Show Item Contents throws exception (in [nsILocalFile.reveal])

Status: RESOLVED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[Magnus Melin [:mkmelin]]

Right clicking a theme and chosing 'Show Item Contents' does nothing but produce
an exception in the JavaScript console. 

Error: uncaught exception: [Exception... "Component returned failure code:
0x80004005 (NS_ERROR_FAILURE) [nsILocalFile.reveal]"  nsresult: "0x80004005
(NS_ERROR_FAILURE)"  location: "JS frame ::
chrome://mozapps/content/extensions/extensions.js :: anonymous :: line 1068" 
data: no]

Seen at least in 
Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8b4) Gecko/20050827 Firefox/1.0+

Comment 1

[Adam Guthrie]

I'm guessing this regressed from the check-in from bug 300116, but I haven't
verified this.

Comment 2

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

It is extremely doubtful that bug 300116 caused this... it didn't change any of
the code for this. Can you check if you are able to open the containing folder
for an item in the download manager? It uses essentially the same calls.

Comment 3

[Adam Guthrie]

Sorry about the bad call about the regression. That check-in isn't to blame. I
don't see this when I open the containing folder of a download, though, only
when I try to do this for extensions and themes.

Comment 4

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

Adam, this is on linux for you as well? Both trunk and branch? Do you know if it
ever worked for you and if so is there a regression range? I'll take a look at
this tomorrow but any additional info would be appreciated.

Comment 5

[Magnus Melin [:mkmelin]]

Seems this never worked. Don't know exactly when that menu item got added, seems
sometime in beginning of june. Anyways, tried 20050613 and the problem does
exist already then. 

Comment 6

[Adam Guthrie]

Okay. Kinda sorta getting somewhere with this. This was first added in the
check-in for bug 296868. I checked the 2005-06-07 builds and this didn't work
then, so I don't think this has ever worked on Linux. Works fine on Windows XP
for me, though.

Comment 7

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

Just got my linux env back up and see this as well and I'll take a look this
evening at it... should hopefully be easy to fix.

Comment 8

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

Attachment: simple patch

Comment 9

[Asaf Romano (gone)]

Comment on attachment 194302 [details] [diff] [review]
simple patch


>+          var protocolSvc = Components.classes["@mozilla.org/uriloader/external-protocol-service;1"]
>+                                      .getService(Components.interfaces.nsIExternalProtocolService);
>+          protocolSvc.loadUrl(uri);

Check for isExpectedProtocol on the uri scheme before passing it to loadUri.

Comment 10

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

I'm not sure how that applies here. I believe you are referring to
isExposedProtocol which tells us whether the protocol is valid for viewing in
the browser and the desired behavior is to open a file manager... preferably
using reveal but since that isn't implemented on UNIX we hand it off to
nsIExternalProtocolService to do the right thing. This is the same as with
opening the containing folder for a download.

Comment 11

[Asaf Romano (gone)]

Question, what do we do for "Open Containing Folder" in the DM, in the past, i
think we did open a browser window (if possible).

Comment 12

[Scott MacGregor]

Just to summarize:

1) We believe this never worked in Firefox (but this particular menu item is
relatively recent, hence the JS exception)

2) nsIFile::Reveal has never been implemented on Unix.

What happens when you click on "Desktop" from the download manager? Isn't that
using .reveal on the path to the desktop? Maybe we don't show that feature in
Linux builds.

I'm worried that this fix bandaids things up just for this one particular
instance instead of implementing reveal on linux. If we do approve this patch,
maybe it should be for the branch only, leaving a new bug open for the trunk to
implement reveal properly.

Comment 13

[:Gavin Sharp [email: [e-mail]]]

Clicking "Desktop" from the download manager falls back to using
nsIExternalProtocolService's loadURI on unix:

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/toolkit/mozapps/downloads/content/downloads.js&mark=859-865#852

Comment 14

[:Gavin Sharp [email: [e-mail]]]

Which, had I bothered to look at the patch before commenting, is the same thing
as this patch does, like Rob mentions above. Please disregard! :)

Comment 15

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

(In reply to comment #11)
As Gavin stated, toolkit's download code falls back to using
nsIExternalProtocolService and loadURI and does not attempt to load the
directory except with reveal and if that fails nsIExternalProtocolService along
with loadURI. So I'd have to say that it does not attempt to open it in a
browser window if posssible.

(In reply to comment #12)
I considered removing the menuitem for this but open but using
nsIExternalProtocolService and loadURI has been working well when reveal has
failed on UNIX as can be seen with the download manager and I suspect elsewhere.
Even if it was possible to add reveal support for UNIX it is way too late in the
cycle and since reveal is platform specific I suspect it would need to be
implemented for other platforms as well if this was not added as a fallback.

Adding reveal support is bug 67001.

Comment 16

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

Comment on attachment 194302 [details] [diff] [review]
simple patch

changing review request since mconnor is away.

Benjamin, this makes opening a directory in the file system consistent with the
rest of toolkit (e.g. download manager).

Comment 17

[Scott MacGregor]

We talked about this in the bug triage meeting today for Firefox 1.5. We
actually were confused about why this menu item is there. It didn't seem to add
much value for end users. If someone has the time to investigate the bug that
actually added the menu item to confirm if this UI really is something we should
have, that would be helpful. Otherwise, removing the menu item is an alternative
that would be worth exploring for this particular case. 

Comment 18

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

This was added by Ben in bug 296868... the only reason for it that is not
related to extension developers is to provide an easy method to find an
extension's location in the file system. Since now it is possible to uninstall
an extension by deleting it from the file system this has some value especially
if the normal method to uninstall fails.

Comment 19

[Benjamin Smedberg]

Comment on attachment 194302 [details] [diff] [review]
simple patch

I prefer that we just remove this altogether, per the meeting this is not
end-user UI.

Comment 20

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

Attachment: remove "Show Item Contents" from EM ui

Comment 21

[Benjamin Smedberg]

Comment on attachment 194476 [details] [diff] [review]
remove "Show Item Contents" from EM ui

On the branch, don't commit the string changes.

Comment 22

[Asa Dotzler [:asa]]

Comment on attachment 194476 [details] [diff] [review]
remove "Show Item Contents" from EM ui

thanks! please get this landed asap.

Comment 23

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

Fixed on MOZILLA_1_8_BRANCH and Trunk

Branch check in
Checking in mozilla/toolkit/mozapps/extensions/content/extensions.js;
/cvsroot/mozilla/toolkit/mozapps/extensions/content/extensions.js,v  <-- 
extensions.js
new revision: 1.72.2.3; previous revision: 1.72.2.2
done
Checking in mozilla/toolkit/mozapps/extensions/content/extensions.xul;
/cvsroot/mozilla/toolkit/mozapps/extensions/content/extensions.xul,v  <-- 
extensions.xul
new revision: 1.37.2.1; previous revision: 1.37
done

Trunk check in
Checking in mozilla/toolkit/mozapps/extensions/content/extensions.js;
/cvsroot/mozilla/toolkit/mozapps/extensions/content/extensions.js,v  <-- 
extensions.js
new revision: 1.75; previous revision: 1.74
done
Checking in mozilla/toolkit/mozapps/extensions/content/extensions.xul;
/cvsroot/mozilla/toolkit/mozapps/extensions/content/extensions.xul,v  <-- 
extensions.xul
new revision: 1.38; previous revision: 1.37
done
Checking in mozilla/toolkit/locales/en-US/chrome/mozapps/extensions/extensions.dtd;
/cvsroot/mozilla/toolkit/locales/en-US/chrome/mozapps/extensions/extensions.dtd,v
 <--  extensions.dtd
new revision: 1.9; previous revision: 1.8
done