Implement web-based protocol handlers

RESOLVED FIXED

Status

()

Firefox
File Handling
--
enhancement
RESOLVED FIXED
11 years ago
11 years ago

People

(Reporter: dmose, Assigned: dmose)

Tracking

(Blocks: 2 bugs)

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(6 obsolete attachments)

(Assignee)

Description

11 years ago
A spin-off from bug 372441, as this is targeted for Alpha5, but registerContentHandler is not.
Flags: blocking-firefox3?
(Assignee)

Comment 1

11 years ago
Created attachment 264484 [details] [diff] [review]
web protocol handler strawman impl, v2
(Assignee)

Comment 2

11 years ago
Created attachment 264914 [details] [diff] [review]
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
Attachment #264484 - Attachment is obsolete: true
Attachment #264914 - Flags: review?
(Assignee)

Updated

11 years ago
Attachment #264914 - Flags: review? → review?(cbiesinger)
(Assignee)

Updated

11 years ago
Depends on: 368021
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?
Attachment #264914 - Flags: review?(cbiesinger) → review+

Comment 4

11 years ago
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?
(Assignee)

Comment 5

11 years ago
Created attachment 265311 [details] [diff] [review]
web protocol handler backend patch, v4
Attachment #264914 - Attachment is obsolete: true
(Assignee)

Comment 6

11 years ago
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).
> 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.
(Assignee)

Comment 8

11 years ago
Created attachment 265571 [details] [diff] [review]
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.
Attachment #265311 - Attachment is obsolete: true
Attachment #265571 - Flags: review?
(Assignee)

Updated

11 years ago
Attachment #265571 - Flags: review? → review?(cbiesinger)
(Assignee)

Updated

11 years ago
Attachment #265571 - Flags: superreview?(jonas)
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...
Attachment #265571 - Flags: review?(cbiesinger) → review+
(Assignee)

Comment 10

11 years ago
(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?



(Assignee)

Comment 11

11 years ago
Created attachment 265872 [details] [diff] [review]
web protocol handler backend patch, v6
Attachment #265571 - Attachment is obsolete: true
Attachment #265872 - Flags: superreview?(jonas)
Attachment #265872 - Flags: review+
Attachment #265571 - Flags: superreview?(jonas)
(Assignee)

Comment 12

11 years ago
Created attachment 266019 [details] [diff] [review]
web protocol handler backend patch, v7 (landed)

Updated patch that uses the pattern suggested by biesi to avoid the static cast.
Attachment #265872 - Attachment is obsolete: true
Attachment #266019 - Flags: superreview?
Attachment #266019 - Flags: review+
Attachment #265872 - Flags: superreview?(jonas)
(Assignee)

Updated

11 years ago
Attachment #266019 - Flags: superreview? → superreview?(jonas)
Comment on attachment 266019 [details] [diff] [review]
web protocol handler backend patch, v7 (landed)

Looks good, sr=me
Attachment #266019 - Flags: superreview?(jonas) → superreview+
(Assignee)

Updated

11 years ago
Status: NEW → ASSIGNED
(Assignee)

Updated

11 years ago
Attachment #266019 - Attachment description: web protocol handler backend patch, v7 → web protocol handler backend patch, v7 (landed)
Attachment #266019 - Attachment is obsolete: true
(Assignee)

Updated

11 years ago
Blocks: 382016
(Assignee)

Updated

11 years ago
Depends on: 382019
(Assignee)

Updated

11 years ago
Depends on: 382020
(Assignee)

Comment 14

11 years ago
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)
Flags: blocking-firefox3? → blocking-firefox3+
(Assignee)

Updated

11 years ago
Target Milestone: Firefox 3 alpha5 → Firefox 3 alpha6
(Assignee)

Updated

11 years ago
No longer depends on: 368021
(Assignee)

Updated

11 years ago
Depends on: 384374
(Assignee)

Updated

11 years ago
Depends on: 383396
(Assignee)

Updated

11 years ago
Depends on: 385065
(Assignee)

Comment 15

11 years ago
Removing "registerProtocolHandler" from the summary, as that work is now covered by bug 385106.
Summary: Implement web-based protocol handlers (registerProtocolHandler) → Implement web-based protocol handlers
(Assignee)

Updated

11 years ago
Depends on: 385740

Updated

11 years ago
Target Milestone: Firefox 3 alpha6 → Firefox 3 beta1

Comment 16

11 years ago
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.
(Assignee)

Comment 17

11 years ago
This is great; thanks Clint!
(Assignee)

Comment 18

11 years ago
These won't actually be exposed through the UI at M7; pushing out.
Target Milestone: Firefox 3 M7 → Firefox 3 M8
(Assignee)

Updated

11 years ago
Depends on: 391144
(Assignee)

Updated

11 years ago
Depends on: 390890

Updated

11 years ago
No longer blocks: 372441
Depends on: 372441
(Assignee)

Updated

11 years ago
Blocks: 382019
No longer depends on: 382019
(Assignee)

Updated

11 years ago
No longer blocks: 382019
Depends on: 382019
(Assignee)

Updated

11 years ago
Blocks: 382019
No longer depends on: 382019
(Assignee)

Updated

11 years ago
Blocks: 382020
No longer depends on: 382020
(Assignee)

Updated

11 years ago
No longer blocks: 382020
Depends on: 382020
(Assignee)

Updated

11 years ago
Blocks: 372949
(Assignee)

Comment 19

11 years ago
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. 
Flags: blocking-firefox3+
Target Milestone: Firefox 3 M8 → ---
(Assignee)

Updated

11 years ago
Depends on: 389969

Updated

11 years ago
Depends on: 392957

Updated

11 years ago
Depends on: 392964
(Assignee)

Updated

11 years ago
Depends on: 392978
(Assignee)

Updated

11 years ago
Blocks: 382020
No longer depends on: 382020
Depends on: 377784
(Assignee)

Comment 20

11 years ago
This feature itself has landed.  Bugs have been filed for the remaining work.
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
(Assignee)

Updated

11 years ago
No longer blocks: 372949
(Assignee)

Updated

11 years ago
No longer blocks: 382019
You need to log in before you can comment on or make changes to this bug.