302683 - Get More Themes (Extensions) doesn't launch a browser

Status: VERIFIED FIXED

(Thunderbird :: General, defect)

Description

[Tracy Walker [:tracy]]

Seen on Windows Tbird 2005-07-29-08-trunk and Mac Tbird 2005-07-29-06-trunk

-Go to either the Themes or extension Manager
-Click on the Get More Themes (Extensions) link

tested results:
Windows: Nothing happens
Mac: The addons.mozilla.org page is loaded as the content of a new compose window

expected results:
The system default browser opens with addons.mozilla.org page Thunderbird Themes
(Extensions)

Note: Theme and Extension "Install"  work as expected from previously downloaded
 files.

Comment 1

[Scott MacGregor]

I wouldn't call this a blocker bug, you can always go to the addons website your
self. Downgrading so we don't keep the tree closed for this.

putting in the 1.1. bucket. Nice find Tracy.

Comment 2

[Tracy Walker [:tracy]]

okay, marked as critical, per mscotts approval of the workaround. requesting
blocking  1.5 (is Tbird to follow the same versioning scheme?)

Comment 3

[Scott MacGregor]

I put it in the 1.1 bucket which means its a blocker for us. Restoring things

Comment 4

[Scott MacGregor]

tracy sent me e-mail saying this was on the Mac too so I changed the os setting.
I saw some JS errors that were blocking the http url load....

Comment 5

[Scott MacGregor]

This regression was introduced by Bug #301398 which got rid of the openURL
method on the text node for the Get New Themes / Get New Extensions link in the
extension manager.

As a result we no longer call openURL which is the routine which kicked http
urls out to the desktop for Thunderbird. Instead, we assume we are a browser and
try to open up the url on our own. 

Aaron, here's the routine you stopped us from calling:

http://lxr.mozilla.org/mozilla/source/toolkit/mozapps/extensions/content/extensions.js#86

You can see how it knows how to handle URLs for Thunderbird. Is that something
we can add to your text link widget that you converted this dialog into using? 

Comment 6

[Scott MacGregor]

I think we'd be golden if we could make the text widget's call to open(aUrl)
smarter:

http://lxr.mozilla.org/mozilla/source/toolkit/content/widgets/text.xml#302

but I can't find where open is defined yet. Must be some base JS file

Comment 7

[Scott MacGregor]

Attachment: the fix

Here's one way to fix this problem, and it doesn't involve a Thunderbird ifdef
in toolkit. Make the text-link method smart enough to send non exposed urls out
to the desktop instead of trying to open them internally. 

I believe by making the text-link widget smarter, we may be able to get rid of
some of some other ifdefs in toolkit which try to work around the same problem
for Thunderbird. (see openURL in extensions.js)

http://lxr.mozilla.org/mozilla/source/toolkit/mozapps/extensions/content/extensions.js#86

Comment 8

[Benjamin Smedberg]

Comment on attachment 191295 [details] [diff] [review]
the fix

Yeah, something like this was on my 1.9 radar... but I don't understand why we
can't just use the uriloader directly, instead of skipping it to use the
nsIExternalProtocolService.

Comment 9

[Aaron Leventhal]

Comment on attachment 191295 [details] [diff] [review]
the fix

I'm not actually a toolkit peer.

Comment 10

[Scott MacGregor]

(In reply to comment #8)
> (From update of attachment 191295 [details] [diff] [review] [edit])
> Yeah, something like this was on my 1.9 radar... but I don't understand why we
> can't just use the uriloader directly, instead of skipping it to use the
> nsIExternalProtocolService.
> 

If we write our own uri content listener class and call the uri loader directly,
then the uri loader will end up actually running the url, getting the content
type back from the server, discovering that no one is interested in that content
type and will then bring up the helper application dialog. I don't think that's
the behavior we want when non exposed URLs are clicked inside of chrome links.
The url should get tossed out to the OS without being opened and without a
helper app dialog. 

Comment 11

[Benjamin Smedberg]

Comment on attachment 191295 [details] [diff] [review]
the fix

ok, we're at least not doing any worse than we were before. We really need a
better long-term solution (one that obeys tabbing prefs, for example), but
that's a 1.9 task.

Comment 12

[[e-mail]]

Comment on attachment 191295 [details] [diff] [review]
the fix

>+          var uri = Components.classes["@mozilla.org/network/io-service;1"]
>+                              .getService(Components.interfaces.nsIIOService)
>+                              .newURI(href, null, null);
>+
>+          var protocolSvc = Components.classes["@mozilla.org/uriloader/external-protocol-service;1"]
>+                                      .getService(Components.interfaces.nsIExternalProtocolService);
>+          // if the scheme is not an exposed protocol, then opening this link should 
>+          // be deferred to the system's external protocol handler
>+          if (!protocolSvc.isExposedProtocol(uri.scheme))
>+            protocolSvc.loadUrl(uri);
sr=me if you put this in a try/catch block, just in case someone sets the href
to something open understands but newURI doesn't.

Comment 13

[Scott MacGregor]

fixed with a try/catch

Comment 14

[Tracy Walker [:tracy]]

Verified with Builds:
Windows 2005-08-09-10-trunk
Mac 2005-08-09-05-trunk
Linux 2005-08-09-06-trunk

Comment 15

[Scott MacGregor]

Attachment: [patch checked in] convert tbird's dictionary link to text-link

For historical purposes. I also fixed the dictionary link Thunderbird uses in
the compose window to use the new text link widget.