380415 - Implement web-based protocol handlers

Status: RESOLVED FIXED

(Firefox :: File Handling, enhancement)

Description

[Dan Mosedale (:dmosedale, :dmose)]

A spin-off from bug 372441, as this is targeted for Alpha5, but registerContentHandler is not.

Comment 1

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: web protocol handler strawman impl, v2

Comment 2

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: web protocol handler backend patch, v3

Here's the basic back end scaffolding for web-based protocol handlers.  What I propose to do is this: file bugs on the XXXs in the code (at least several of them should be sorted out before the front end code gets turned on).  XXXreview comments are questions that I'd like to address now, at biesi's discretion.  Then: 

* work with the design folks to create a similar interstitial page for selecting protocol handlers to the feed handler selection page possibly (probably?) including replacing the warning dialog for external protocols.

* move most of the feed-handling code from browser/ to toolkit/

* refactor the feed-handling code slightly in order to make it handle both mime-types and protocols

* finish off the protocol front-end code and wire up registerProtocolHandler

Comment 3

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 264914 [details] [diff] [review]
web protocol handler backend patch, v3

+  nsCOMPtr<nsIPrefBranch> prefBranch = do_GetService(NS_PREFSERVICE_CONTRACTID, &rv);

can you linewrap this so that it doesn't exceed 80 characters?

-    NS_ASSERTION(haveHandler, "Why do we have a channel for this url if we don't support the protocol?");

hmm, perhaps you should change ExternalProtocolHandlerExists to also check for the pref. I wonder what callers expect...

+    NS_IMETHOD Run() 
+      {
+        nsresult rv;

not sure that this placement of { matches the style of the file

+        // XXX need to strip passwd & username from URI to handle, as per the
+        // spec.

so, what spec is this referring to? that's not really obvious...

+        nsCAutoString quotedUriSpecToHandle;

I'd s/quoted/escaped/; "quoted" makes me think of quotes (", ')

+        // XXXreview: should loadGroup, callbacks, or flags be carried over 
+        // from the old channel?

Yes, I think they should. the loadgroup and some flags are needed to make the progressbar/throbber work correctly. callbacks may also be needed for some things.

+          // XXXreview should we abort here if there's an error?          

yes, see nsIChannelEventSink.idl

+        // XXXreview should we abort here if there's an error?
+        (void)newChannel->AsyncOpen(mListener, mContext);

Hm... if you get an error there, what you should do is set mStatus to that failure code, and call the listener's onStartRequest and onStopRequest methods. it is an important guarantee of necko that whenever asyncOpen succeeds (the original one), that onStartRequest/onStopRequest will be called.

(Also when onChannelRedirect returns an error)

+  // XXX after we refactor 

is there something missing from this sentence?

Comment 4

[cmtalbert]

Comment on attachment 264914 [details] [diff] [review]
web protocol handler backend patch, v3


Dmose: I have a question about the following block of code...
> 
>+class nsWebProtocolRedirect : public nsRunnable {
..snip..
>+        // get the URI spec so we can quote it for the template 
>+        nsCAutoString uriSpecToHandle;
>+        rv = mURI->GetSpec(uriSpecToHandle);
>+        NS_ENSURE_SUCCESS(rv, rv);
>+
>+        // XXX need to strip passwd & username from URI to handle, as per the
>+        // spec.  nsSimpleURL, which is what we're going to get, can't do this
>+        // directly.  Ideally, we'd fix nsStandardURL to make it possible to
>+        // turn off all of its quirks handling, and use that...
>+
>+        // XXX this doesn't exactly match how the spec is requesting us to
>+        // escape; at the very least, it should be escaping @ signs, and there
>+        // may well be more issues.  However, this code will probably be thrown
>+        // out when we do the front-end work, as we'll be using a refactored 
>+        // nsIWebContentConverterInfo to do this work for us
>+        nsCAutoString quotedUriSpecToHandle;
>+        NS_EscapeURL(uriSpecToHandle, esc_Minimal | esc_Forced | esc_Colon,
>+                     quotedUriSpecToHandle);

You keep mentioning a spec, and it's not clear whether you mean a some specification somewhere or the URI spec, or both.  If there's a specification you're using, can you let me know what it is?

Comment 5

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: web protocol handler backend patch, v4

Comment 6

[Dan Mosedale (:dmosedale, :dmose)]

Ok, I believe v4 addresses all the questions, though a bit of refactoring was necessary. 

(In reply to comment #3)
> 
> -    NS_ASSERTION(haveHandler, "Why do we have a channel for this url if we
> don't support the protocol?");
> 
> hmm, perhaps you should change ExternalProtocolHandlerExists to also check for
> the pref. I wonder what callers expect...

You're right.  Callers are almost certainly asking "is there any external protocol handler" not "is there an external protocol handler that happens to be an OS application and not a web application".

So I renamed the existing implementation of ExternalProtocolHandlerExists (in the Mac-specific subclass) was to OSProtocolHandlerExists.  Then, the ExternalProtocolHandlerExists interface method was implemented once in the superclass to first check for a web-based handler before falling back to calling the subclass implemented OSProtocolHandlerExists.  To do this, I ended up moving the pref checking code into the helper app service.  I made a number of classes |friend|ly with each other along the way in order to avoid unnecessary COM interface exposure.  If you're OK with this strategy, I'll go do the same thing for the other platforms.  Are you?

> +        // XXX need to strip passwd & username from URI to handle, as per the
> +        // spec.
> 
> so, what spec is this referring to? that's not really obvious...

The WhatWG HTML5 draft.  I've updated the comments to be clear on that.

> +        // XXXreview: should loadGroup, callbacks, or flags be carried over 
> +        // from the old channel?
> 
> Yes, I think they should. the loadgroup and some flags are needed to make the
> progressbar/throbber work correctly. callbacks may also be needed for some
> things.

Done.  This required me to clean up the loadFlags getter and setter in nsExtProtocolChannel, as they were (reasonably, until now) simply always setting 0 (== LOAD_NORMAL).

Comment 7

[Christian :Biesinger (don't email me, ping me on IRC)]

> I made a number
> of classes |friend|ly with each other along the way in order to avoid
> unnecessary COM interface exposure.  If you're OK with this strategy, I'll go
> do the same thing for the other platforms.  Are you?

would just making some functions public work too? I'd prefer that to making the classes friends.

Comment 8

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: web protocol handler backend patch, v5

This version has only one friend class remaining (and it's necessary), and a bunch of now-public methods.  Additionally, I've made the appropriate tweaks for the code to all platforms.

Comment 9

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 265571 [details] [diff] [review]
web protocol handler backend patch, v5

+  nsresult rv = this->GetWebProtocolHandlerURITemplate(

is there a reason for the this-> prefix here? I've never seen it in mozilla code before.

(other places in this patch too)


+  if (NS_SUCCEEDED(rv)) {
+    *aHandlerExists = PR_TRUE;
+  }

shouldn't you have a return statement here too?


+      nsresult rv;
+      // get the URI spec so we can escape it for insertion into the template 
+      nsCAutoString uriSpecToHandle;
+      rv = mURI->GetSpec(uriSpecToHandle);

could you do me a favour and declare the rv where you first use it? :) as in:

     nsresult rv = mURI->GetSpec(uriSpecToHandle);

+  nsCOMPtr<nsIExternalProtocolService> extProtService =
+    (do_GetService(NS_EXTERNALPROTOCOLSERVICE_CONTRACTID, &rv));

don't think the parens on the second line are necessary :)

+  rv = NS_STATIC_CAST(nsExternalHelperAppService *, extProtService.get())->
+    GetWebProtocolHandlerURITemplate(urlScheme, uriTemplate);

hmm. an embeddor/extension better not have replaced that contract id... (could that function be static perhaps?)

+    rv =  NS_DispatchToCurrentThread(event);

remove one of the spaces after the =

-  return NS_ERROR_NO_CONTENT; // force caller to abort.
+  return NS_ERROR_NO_CONTENT; // force caller to abort.     

please don't add trailing whitespace :)

+      NS_STATIC_CAST(nsExternalHelperAppService *, m_extProtService.get())->
+        OSProtocolHandlerExists(scheme.get(), &haveHandler);

hm, making this static won't work...

Comment 10

[Dan Mosedale (:dmosedale, :dmose)]

(In reply to comment #9)
> (From update of attachment 265571 [details] [diff] [review])
> +  nsresult rv = this->GetWebProtocolHandlerURITemplate(
> 
> is there a reason for the this-> prefix here? I've never seen it in mozilla
> code before.

Just habit because I've been working in JS a bunch lately.  Removed here and other places.

> +  if (NS_SUCCEEDED(rv)) {
> +    *aHandlerExists = PR_TRUE;
> +  }
> 
> shouldn't you have a return statement here too?

Yes; good catch. 

All other comments addressed, except:

> +      NS_STATIC_CAST(nsExternalHelperAppService *, m_extProtService.get())->
> +        OSProtocolHandlerExists(scheme.get(), &haveHandler);
> 
> hm, making this static won't work...

So after our IRC discussion, I realized I don't see how we can get to the service from C++ linkage instead of GetService and still ensure that the object will be a singleton.  Ideas?

Comment 11

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: web protocol handler backend patch, v6

Comment 12

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: web protocol handler backend patch, v7 (landed)

Updated patch that uses the pattern suggested by biesi to avoid the static cast.

Comment 13

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 266019 [details] [diff] [review]
web protocol handler backend patch, v7 (landed)

Looks good, sr=me

Comment 14

[Dan Mosedale (:dmosedale, :dmose)]

Spinoff bugs filed for XXX comments:

* web-based proto-handlers should be disableable via single pref for enterprises (bug 382016)

* strip username and passwd as per spec (bug 382017)

* clarify URI escaping in spec and implement (bug 382019)

* decide on and audit for correct history behavior (bug 382020)

Comment 15

[Dan Mosedale (:dmosedale, :dmose)]

Removing "registerProtocolHandler" from the summary, as that work is now covered by bug 385106.

Comment 16

[cmtalbert]

Understandably, there are not a bunch of web sites that can handle this type of association in the wild right now.  Scouring across the web, I've found a few web applications that I was able to reverse engineer to fit into some preliminary, extremely simple test cases.  Those cases are here: http://people.mozilla.org/~ctalbert/index.html

I think these might be useful to the developers as a simple end-user, unit-test mechanism as the code is being developed.

If the tests are incorrect, please let me know.

Comment 17

[Dan Mosedale (:dmosedale, :dmose)]

This is great; thanks Clint!

Comment 18

[Dan Mosedale (:dmosedale, :dmose)]

These won't actually be exposed through the UI at M7; pushing out.

Comment 19

[Dan Mosedale (:dmosedale, :dmose)]

At this point, this bug is a tracking bug.  All the concrete deliverables have blocking and target milestone flags set in other bugs.  Removing those from this bug. 

Comment 20

[Dan Mosedale (:dmosedale, :dmose)]

This feature itself has landed.  Bugs have been filed for the remaining work.