Closed Bug 389687 Opened 17 years ago Closed 16 years ago

Need to implement nsIContentDispatchChooser

Categories

(Camino Graveyard :: General, defect)

All
macOS
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Camino2.0

People

(Reporter: sdwilsh, Assigned: stuart.morgan+bugzilla)

References

Details

Attachments

(3 files, 1 obsolete file)

Bug 385065 added a new interface to ask a user how to handle an external protocol.  This is implemented in XUL and JS in toolkit, but that clearly won't work for Camino.
Target Milestone: --- → Camino2.0
Shawn, would this have caused mailto: links to stop working?

cl
I'll answer for him: Yes. mailto: is a protocol and non-http protocols are broken (aiui). (Although I think someone said the ftp: protocol is fine.)
Well, nsExternalHelperAppService::LoadURI calls nsIHandlerInfo::LaunchWithURI (assuming the prefs are set right), but you should have that...
From what I understand so far, I think what we probably want is to have our implementation just show the warning about first-use (which this replaces), and then SetAlwaysAskBeforeHandling(PR_FALSE) and SetPreferredAction(nsIHandlerInfo::useSystemDefault) for the handler so we don't get the dialog again (then LaunchWithURI(uri) to actually deal with it).
Assignee: nobody → stuart.morgan
Attached patch short-term version (obsolete) — Splinter Review
I finally had time to look into this more, and what I thought was just finishing up what I had turned into a mess that I wasn't able to quickly disentangle (I'll get into that in a follow-up comment). Since I don't want to leave external protocol handling completely broken for several more weeks if I get busy again, I'd like to just check this in now and leave the bug open for follow-up. This gives us a shell implementation of nsIContentDispatchChooser that does nothing but pass it off to be opened by the system. Basically everything here will be part of the correct solution as well, I just need to get a full implementation of Ask working that does what we want.

Given that the OS handles most protocol security for us, Safari shows no warning, and Firefox is considering having no warning, I'm not concerned about the security of this hack; not ever showing a dialog may be what we decide to do in the long term anyway (this is just not the right way to do it).
Attachment #282350 - Flags: superreview?(mark)
The issue I'm having with the full solution is that to do anything useful, we need an nsIHandlerService to persist any choices we or the user make about future handling, which means we need to ship nsHandlerService.js

Putting nsHandlerService.js and exthandler.xpt into MacOS/components gives me an error at startup about an include failure for XPCOMUtils.jsm, so I added that to MacOS/modules. Now I get "XPCOMUtils.generateQI is not a function"; given that that's defined in XPCOMUtils.jsm, and that's obviously being found since the error changed, I'm not sure what that means.
Comment on attachment 282350 [details] [diff] [review]
short-term version

>+static const char* handlerServiceContractID = "@mozilla.org/uriloader/handler-service;1";

Not used, remove?

>+NS_IMETHODIMP ContentDispatchChooser::Ask(nsIHandlerInfo *aHandler,

This method has no return value!
(In reply to comment #7)
> (From update of attachment 282350 [details] [diff] [review])
> >+static const char* handlerServiceContractID = "@mozilla.org/uriloader/handler-service;1";
> 
> Not used, remove?

Sorry, that was from the partially done correct version and I missed while gutting; I'll rip it out.

> >+NS_IMETHODIMP ContentDispatchChooser::Ask(nsIHandlerInfo *aHandler,
> 
> This method has no return value!

Indeed. The semantics are (confusingly to me at first; I expected a method called Ask to give an answer) to ask the user what to do, do it, and push any state about what to do in the future into the handler store directly.
Addresses comments.
Attachment #282350 - Attachment is obsolete: true
Attachment #282468 - Flags: superreview?(mark)
Attachment #282350 - Flags: superreview?(mark)
Attachment #282468 - Flags: superreview?(mark) → superreview+
Comment on attachment 282468 [details] [diff] [review]
short-term fix, v2 [landed]

Landed; leaving open for the real implementation.
Attachment #282468 - Attachment description: short-term fix, v2 → short-term fix, v2 [landed]
Depends on: 397309
Attached patch real fixSplinter Review
This takes the approach of showing a warning dialog--not in the sense of a security warning, since there's little value in doing that, but just so that the user won't be surprised that some other application is suddenly launched.

There's another dialog in this code that I'm not actually sure can be shown, but it's there just in case. It's intended for the case where there is no application that can handle the protocol, but for the moment at least we don't get called at all in that case (which is unfortunate, since our dialog for that case is way better than the one core shows).
Attachment #312492 - Flags: superreview?(mikepinkerton)
Attachment #312492 - Flags: review?(alqahira)
(In reply to comment #11)
> There's another dialog in this code that I'm not actually sure can be shown,
> but it's there just in case. It's intended for the case where there is no
> application that can handle the protocol, but for the moment at least we don't
> get called at all in that case (which is unfortunate, since our dialog for that
> case is way better than the one core shows).
I'm all for helping getting a better dialog in Core if it's needed (after 1.9 obviously).
It looks like that's covered by the very unloved bug 259741.

I'm confused though; is it correct behavior that the content chooser implementation is never called just because there's no registered handler on the OS? That seems really wrong, since it prevents a client from, say, allowing a user to pick a web handler for a protocol if they have no local handler.
Hm, and somehow Minefield is showing a different dialog there.
(In reply to comment #13)
> I'm confused though; is it correct behavior that the content chooser
> implementation is never called just because there's no registered handler on
> the OS? That seems really wrong, since it prevents a client from, say, allowing
> a user to pick a web handler for a protocol if they have no local handler.
Probably - note:  not everything was hooked up with nsIContentDispatchChooser yet.  The goal is to have everything go through it, but that work wasn't completed in time for 1.9 (sucky, I know, but protocol handling got a little better, but this particular case you cited seems to not be fixed).  If that's an easy enough fix, we might be able to get that fixed for 1.9.
Is there already a core bug (or bugs) tracking the remaining work, or should I file that as a new bug?
I am not aware of any bugs, so go ahead a file (worst case, we dupe one to the other).
Bug 425971 filed; if that's fixed at some point the "no handler" part of the attached code should Just Work.
Comment on attachment 312492 [details] [diff] [review]
real fix

sr=pink,

we may want to think more about the "NoExternalHandlerText", it seems a little unclear to me.
Attachment #312492 - Flags: superreview?(mikepinkerton) → superreview+
Comment on attachment 312492 [details] [diff] [review]
real fix

>+/* Protocol Handling */
>+"UnknownContentHandler" = "Unknown Application";

How do we ever get that?

>+"NoExternalHandlerText" = "Camino cannot open “%@” links, and no application that can open “%@” link is installed.";

Maybe "and there are no applications installed that can open “%@” links."?  That gets rid of the passive voice, which I think helps.

Some other observations:

* mailto: and the various usenet protocols don't show warnings (in either Camino or Minefield); why are we privileging them over, say, feed?  They all open external apps, and feed: is far more common than news: these days.  UE-wise, it seems a little odd that some "links that open external apps" cause warnings and others don't.

* Clicking on a feed: link in content bypasses our "make sure it's a conscious choice to use Safari" sheet and causes us to show two different UIs for launching a feed reader.  Can we, in this new nsIContentDispatchChooser world, cause feed: in content to go through our existing UI?  If so (or if we don't yet know it's impossible), can we get a follow-up filed on that?

* help: and afp: seem like they're permanently disabled (I didn't test every protocol that's registered on my Mac; there may be more that are permablocked).  help: used to be dangerous, and I don't really see any reason for exposing it to internet web content given its history, but I'm not sure why afp is blocked.  I assume this is a Core bug, but it's also not compelling enough to me to argue over; I just noticed it during testing.  Is there a blacklist somewhere of these protocols?

* When there is a registered protocol with no application (news on a default Mac OS X install), or if the app is missing (install MT-NW and set it as default, then delete it), Minefield prompts to choose an app, but we claim the protocol is not registered.  Is this also bug 425971?  I initially thought that bug was for truly unregistered protocols (foo:, skype: on my Mac), but now I'm not sure.  Minefield definitely shows two distinct UIs, its equivalent of the "foo is not a registered protocol" sheet for foo/skype, and the "choose an app" window for the usenet ones (both before I had installed a usenet client, and then after I'd deleted MT-NW).

I don't think any of these last four should hold up this bug, so r=ardissone with the string change and either bug comments or follow-up bugs on the rest.
Attachment #312492 - Flags: review?(alqahira) → review+
- I don't know that the Unknown Application case can ever happen (I would expect not); I just didn't want us to melt if it happened ;)

- I'm definitely not doing anything with mailto:, news:, help:, and afp:, so whatever is happening there is core. I'm not sure where that special-casing lives; we can investigate in a follow-up if we want to change something there.

- We'll have to shift some stuff around, but I don't see any reason we couldn't change feed:, and I think it's a very good idea. Bug 389687 filed.

- I'm not sure how Minefield is distinguishing the two cases where we get the lame protocol error, but I consider both to be part of bug 425971, since I think we should always get called.


Landed with the string change.
Status: NEW → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
(In reply to comment #22)
> - We'll have to shift some stuff around, but I don't see any reason we couldn't
> change feed:, and I think it's a very good idea. Bug 389687 filed.

By which I assume you meant bug 426423 ;)
Note:  the not warning of certain protocols has to do with prefs with the name of "network.protocol-handler.warn-external.[protocol]", where mailto's is "network.protocol-handler.warn-external.mailto".
(In reply to comment #15)
> (In reply to comment #13)
> > I'm confused though; is it correct behavior that the content chooser
> > implementation is never called just because there's no registered handler on
> > the OS? That seems really wrong, since it prevents a client from, say, allowing
> > a user to pick a web handler for a protocol if they have no local handler.
> Probably - note:  not everything was hooked up with nsIContentDispatchChooser
> yet. 

I disagree.  The way it's supposed to work, I think, is that Ask shouldn't be called in the case where there are neither OS-level handlers nor web-handlers registered.

(In reply to comment #20)
> * mailto: and the various usenet protocols don't show warnings (in either
> Camino or Minefield); why are we privileging them over, say, feed?  They all
> open external apps, and feed: is far more common than news: these days. 
> UE-wise, it seems a little odd that some "links that open external apps" cause
> warnings and others don't.

How the prefs are set for the various different apps is in each case an attempt to manually balance security against convenience.  I'd say a pretty good argument could be made to whitelist feed: in the defaults for all toolkit apps; that's probably worth spinning off another bug.
My reply to comment 25 is in bug 425971.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: