Vincent Chang[:vchang][changyihsin] Reporter
	Description • 11 years ago

Bug 831645 supports Rtsp protocol in video tag,  

<video id=home_video controls preload=none width=300 height=300>
  <source src="rtsp://x.x.x.x/x.mp4" />
</video>

We should also allow user to enter rtsp://xxxx in url bar. We may support this by 
1. pass the url by system message to trigger the video app. 
2. embedded video frame in browser app.

	(inactive after 6/18) Alive Kuo [:alive]
	Comment 1 • 11 years ago

(In reply to Vincent Chang[:vchang] from comment #0)
> Bug 831645 supports Rtsp protocol in video tag,  
> 
> <video id=home_video controls preload=none width=300 height=300>
>   <source src="rtsp://x.x.x.x/x.mp4" />
> </video>
> 
> We should also allow user to enter rtsp://xxxx in url bar. We may support
> this by 
> 1. pass the url by system message to trigger the video app. 

Vote this. Much reasonable than 2.

> 2. embedded video frame in browser app.

	Ken Chang[:kenkai]
	Comment 2 • 11 years ago

Ethan is fixing this bug.

	Ethan Tseng [:ethan] Assignee
	Comment 3 • 11 years ago

What I did in this patch:
1. Modified nsDocShell::DoURILoad(): after the RTSP channel is created, create RtspDelegateChild to send the delegation request and break the URI loading.
2. Modified NeckoParent/Child: to support the new RtspDelegateParent/Child.
3. Added IPC RtspDelegateParent/Child: to pass the delegation request containing "url" spec.
4. RtspDelegateParent: broadcast system messages "rtsp-open-video".

In my unit test, if we modified the manifest of video app to register the "rtsp-open-video" message, the video app could be launched once we input an RTSP link in the browser URL bar. However, for now the video app was only launched but could not play the video successfully. More follow-ups are required.

	Ethan Tseng [:ethan] Assignee
	Comment 4 • 11 years ago

Remove a careless debugging macro in the previous patch.

	Ethan Tseng [:ethan] Assignee
	Comment 5 • 11 years ago

Resolved build errors on Windows XP platforms.

	Ethan Tseng [:ethan] Assignee
	Comment 6 • 11 years ago

Comment on attachment 827325 [details] [diff] [review]
Bug 884702: Delegate RTSP request using broadcast system message.

Hi, Stephen:

Could you help to review this patch?

Ethan

	Steve Workman [:sworkman] (INACTIVE)
	Comment 7 • 11 years ago

Comment on attachment 827325 [details] [diff] [review]
Bug 884702: Delegate RTSP request using broadcast system message.

Review of attachment 827325 [details] [diff] [review]:
-----------------------------------------------------------------

Some cleanup things. r- for now, but almost there.

Please ask someone familiar with nsDocShell to review those changes.

::: docshell/base/nsDocShell.cpp
@@ +9663,5 @@
> +    /* Hack for RTSP protocol.
> +     * If the URL scheme is "rtsp", stop URI loading and delegate the RTSP
> +     * request to video apps.
> +     * See bug 884702 - Support rtsp protocol in URL bar.
> +     */

Please add to this comment: "Note: This only works for multi-process builds."

If we decide to add this to Firefox desktop/mobile at some point, then the code will have to be changed.

@@ +9669,5 @@
> +      nsAutoCString contentType;
> +      channel->GetContentType(contentType);
> +      if (contentType.LowerCaseEqualsLiteral("rtsp")) {
> +        URIParams uriParams;
> +        RtspDelegateChild *rdc = new RtspDelegateChild();

Do you need to use an nsRefPtr here?

@@ +9671,5 @@
> +      if (contentType.LowerCaseEqualsLiteral("rtsp")) {
> +        URIParams uriParams;
> +        RtspDelegateChild *rdc = new RtspDelegateChild();
> +        if (!rdc) {
> +           return NS_ERROR_OUT_OF_MEMORY;

You shouldn't need this. "new" is infallible.

@@ +9679,5 @@
> +        return NS_OK;
> +      }
> +    }
> +#endif
> +

You should get someone more familiar with nsDocShell to review this change.

::: netwerk/ipc/RtspDelegateChild.cpp
@@ +25,5 @@
> +NS_IMETHODIMP_(nsrefcnt) RtspDelegateChild::Release()
> +{
> +  NS_PRECONDITION(0 != mRefCnt, "dup release");
> +  // Enable this to find non-threadsafe destructors:
> +  // NS_ASSERT_OWNINGTHREAD(RtspDelegateChild);

If this class is only used on the main thread (see comment re class definition in header file), then you should uncomment this assertion.

@@ +44,5 @@
> +}
> +
> +NS_INTERFACE_MAP_BEGIN(RtspDelegateChild)
> +  NS_INTERFACE_MAP_ENTRY(nsISupports)
> +NS_INTERFACE_MAP_END

NS_IMPL_ISUPPORTS0(RtspDelegateChild) ?

::: netwerk/ipc/RtspDelegateChild.h
@@ +15,5 @@
> +
> +namespace mozilla {
> +namespace net {
> +
> +class RtspDelegateChild : public PRtspDelegateChild

Please add a brief comment to explain what this class does.

@@ +19,5 @@
> +class RtspDelegateChild : public PRtspDelegateChild
> +                        , public nsISupports
> +{
> +public:
> +  NS_DECL_THREADSAFE_ISUPPORTS

Does this class need to be threadsafe? Seems like it's only used in DocShell::LoadURI and that's main thread only, right?

@@ +26,5 @@
> +  ~RtspDelegateChild();
> +
> +  void AddIPDLReference();
> +  void ReleaseIPDLReference();
> +

I don't see SendDelegate being overridden in this class - is it definitely ok to use the generated version from PRtspDelegate.ipdl?

@@ +32,5 @@
> +  bool mIPCOpen;
> +  // RTSP URL refer to a stream or an aggregate of streams.
> +  nsCOMPtr<nsIURI> mURI;
> +  // ASCII encoded URL spec
> +  nsCString mSpec;

Do you need mSpec as a separate variable here? I don't see it being used anywhere.

::: netwerk/ipc/RtspDelegateParent.cpp
@@ +43,5 @@
> + */
> +bool
> +RtspDelegateParent::RecvDelegate(const URIParams& aURI)
> +{
> +  nsAutoString msgType(NS_LITERAL_STRING("rtsp-open-video"));

You could make this a static in the class definition:

static nsString msgType;

and define it in the cpp:

/*static*/ nsString RtspDelegateParent::msgType(NS_LITERAL_STRING("rtsp-open-video"));

@@ +46,5 @@
> +{
> +  nsAutoString msgType(NS_LITERAL_STRING("rtsp-open-video"));
> +  nsCOMPtr<nsIURI> uri = DeserializeURI(aURI);
> +  nsAutoCString spec;
> +  uri->GetAsciiSpec(spec);

Please get the scheme here and add an assertion for scheme == rtsp.

@@ +61,5 @@
> +  // Set the "url" property of the data.
> +  JS::Rooted<JS::Value> val(cx);
> +  JSString *urlStr = JS_NewStringCopyN(cx, spec.get(), spec.Length());
> +  NS_ENSURE_TRUE(urlStr, false);
> +  val = STRING_TO_JSVAL(urlStr);

Please put the three lines for urlStr in their own block:

JS::Rooted<JS::Value> val(cx);
{
  JSString *urlStr = JS_NewStringCopyN(cx, spec.get(), spec.Length());
  NS_ENSURE_TRUE(urlStr, false);
  val = STRING_TO_JSVAL(urlStr);
}

It isolates the urlStr symbol from the other code in the function, and the indentation can help with reading the code. Please apply this to other parts of the function as well.

@@ +63,5 @@
> +  JSString *urlStr = JS_NewStringCopyN(cx, spec.get(), spec.Length());
> +  NS_ENSURE_TRUE(urlStr, false);
> +  val = STRING_TO_JSVAL(urlStr);
> +  if (!JS_SetProperty(cx, dataObj, "url", val)) {
> +    return false;

Please add a boolean for the return value for JS_SetProperty, and then use NS_ENSURE_TRUE(rv, rv). This may help debugging in the future because the failure will show up on the console log. Do the same for the other calls to JS_SetProperty.

@@ +90,5 @@
> +  nsCOMPtr<nsISystemMessagesInternal> systemMessenger =
> +    do_GetService("@mozilla.org/system-message-internal;1");
> +  if (!systemMessenger) {
> +    return false;
> +  }

NS_ENSURE_TRUE(systemMessenger, false);

::: netwerk/ipc/moz.build
@@ +47,5 @@
>      'NeckoChannelParams.ipdlh',
>      'PNecko.ipdl',
>      'PRemoteOpenFile.ipdl',
>      'PRtspController.ipdl',
> +    'PRtspDelegate.ipdl',

Can you try putting both of these RTSP ipdl files in an "if CONFIG['MOZ_RTSP']" block please. I think it should work, and I think they should be isolated like this.

	Vincent Chang[:vchang][changyihsin] Reporter
	Comment 8 • 11 years ago

Comment on attachment 827325 [details] [diff] [review]
Bug 884702: Delegate RTSP request using broadcast system message.

Review of attachment 827325 [details] [diff] [review]:
-----------------------------------------------------------------

Cancel the review request since sworkman has done the review.

	Ethan Tseng [:ethan] Assignee
	Comment 9 • 11 years ago

Hi, Steve, thanks for your review! I appreciated. :)
Here are my replies and questions about this review.

(In reply to Steve Workman [:sworkman] from comment #7)
> Comment on attachment 827325 [details] [diff] [review]
> Bug 884702: Delegate RTSP request using broadcast system message.
> 
> Review of attachment 827325 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Some cleanup things. r- for now, but almost there.
> 
> Please ask someone familiar with nsDocShell to review those changes.
> 
Okay. I will talk with Vincent to consult a reviewer for nsDocShell.

> ::: docshell/base/nsDocShell.cpp
> @@ +9663,5 @@
> > +    /* Hack for RTSP protocol.
> > +     * If the URL scheme is "rtsp", stop URI loading and delegate the RTSP
> > +     * request to video apps.
> > +     * See bug 884702 - Support rtsp protocol in URL bar.
> > +     */
> 
> Please add to this comment: "Note: This only works for multi-process builds."
> 
> If we decide to add this to Firefox desktop/mobile at some point, then the
> code will have to be changed.
> 
> @@ +9669,5 @@
> > +      nsAutoCString contentType;
> > +      channel->GetContentType(contentType);
> > +      if (contentType.LowerCaseEqualsLiteral("rtsp")) {
> > +        URIParams uriParams;
> > +        RtspDelegateChild *rdc = new RtspDelegateChild();
> 
> Do you need to use an nsRefPtr here?
>
Good advice. I will add the comment and use nsRefPtr.

> @@ +9671,5 @@
> > +      if (contentType.LowerCaseEqualsLiteral("rtsp")) {
> > +        URIParams uriParams;
> > +        RtspDelegateChild *rdc = new RtspDelegateChild();
> > +        if (!rdc) {
> > +           return NS_ERROR_OUT_OF_MEMORY;
> 
> You shouldn't need this. "new" is infallible.
>
How about using "NS_ENSURE_TRUE(rdc, NS_ERROR_OUT_OF_MEMORY)" instead.
I think we still protect the code from the out-of-memory situation.

> @@ +9679,5 @@
> > +        return NS_OK;
> > +      }
> > +    }
> > +#endif
> > +
> 
> You should get someone more familiar with nsDocShell to review this change.
> 
> ::: netwerk/ipc/RtspDelegateChild.cpp
> @@ +25,5 @@
> > +NS_IMETHODIMP_(nsrefcnt) RtspDelegateChild::Release()
> > +{
> > +  NS_PRECONDITION(0 != mRefCnt, "dup release");
> > +  // Enable this to find non-threadsafe destructors:
> > +  // NS_ASSERT_OWNINGTHREAD(RtspDelegateChild);
> 
> If this class is only used on the main thread (see comment re class
> definition in header file), then you should uncomment this assertion.
> 
> @@ +44,5 @@
> > +}
> > +
> > +NS_INTERFACE_MAP_BEGIN(RtspDelegateChild)
> > +  NS_INTERFACE_MAP_ENTRY(nsISupports)
> > +NS_INTERFACE_MAP_END
> 
> NS_IMPL_ISUPPORTS0(RtspDelegateChild) ?
> 
It seems unnecessary to define our own Release() method here. I will remove my Release() and use NS_IMPL_ISUPPORTS0() instead.

> ::: netwerk/ipc/RtspDelegateChild.h
> @@ +15,5 @@
> > +
> > +namespace mozilla {
> > +namespace net {
> > +
> > +class RtspDelegateChild : public PRtspDelegateChild
> 
> Please add a brief comment to explain what this class does.
> 
Thanks for your reminder. I will add it.

> @@ +19,5 @@
> > +class RtspDelegateChild : public PRtspDelegateChild
> > +                        , public nsISupports
> > +{
> > +public:
> > +  NS_DECL_THREADSAFE_ISUPPORTS
> 
> Does this class need to be threadsafe? Seems like it's only used in
> DocShell::LoadURI and that's main thread only, right?
> 
Yes. It only used in DocShell::LoadURI(). I will further confirm it's main thread only.
If it is, I will remove the macro.

> @@ +26,5 @@
> > +  ~RtspDelegateChild();
> > +
> > +  void AddIPDLReference();
> > +  void ReleaseIPDLReference();
> > +
> 
> I don't see SendDelegate being overridden in this class - is it definitely
> ok to use the generated version from PRtspDelegate.ipdl?
> 
The only thing SendDelegate() does is to pass the URI, which is defined as "URIParams aURI", to the parent side.
I think it is enough to use the generated version. Or is there anything I should concern?

> @@ +32,5 @@
> > +  bool mIPCOpen;
> > +  // RTSP URL refer to a stream or an aggregate of streams.
> > +  nsCOMPtr<nsIURI> mURI;
> > +  // ASCII encoded URL spec
> > +  nsCString mSpec;
> 
> Do you need mSpec as a separate variable here? I don't see it being used
> anywhere.
> 
Oops. My mistake. I will remove it.

> ::: netwerk/ipc/RtspDelegateParent.cpp
> @@ +43,5 @@
> > + */
> > +bool
> > +RtspDelegateParent::RecvDelegate(const URIParams& aURI)
> > +{
> > +  nsAutoString msgType(NS_LITERAL_STRING("rtsp-open-video"));
> 
> You could make this a static in the class definition:
> 
> static nsString msgType;
> 
> and define it in the cpp:
> 
> /*static*/ nsString
> RtspDelegateParent::msgType(NS_LITERAL_STRING("rtsp-open-video"));
> 
> @@ +46,5 @@
> > +{
> > +  nsAutoString msgType(NS_LITERAL_STRING("rtsp-open-video"));
> > +  nsCOMPtr<nsIURI> uri = DeserializeURI(aURI);
> > +  nsAutoCString spec;
> > +  uri->GetAsciiSpec(spec);
> 
> Please get the scheme here and add an assertion for scheme == rtsp.
> 
> @@ +61,5 @@
> > +  // Set the "url" property of the data.
> > +  JS::Rooted<JS::Value> val(cx);
> > +  JSString *urlStr = JS_NewStringCopyN(cx, spec.get(), spec.Length());
> > +  NS_ENSURE_TRUE(urlStr, false);
> > +  val = STRING_TO_JSVAL(urlStr);
> 
> Please put the three lines for urlStr in their own block:
> 
> JS::Rooted<JS::Value> val(cx);
> {
>   JSString *urlStr = JS_NewStringCopyN(cx, spec.get(), spec.Length());
>   NS_ENSURE_TRUE(urlStr, false);
>   val = STRING_TO_JSVAL(urlStr);
> }
> 
> It isolates the urlStr symbol from the other code in the function, and the
> indentation can help with reading the code. Please apply this to other parts
> of the function as well.
> 
> @@ +63,5 @@
> > +  JSString *urlStr = JS_NewStringCopyN(cx, spec.get(), spec.Length());
> > +  NS_ENSURE_TRUE(urlStr, false);
> > +  val = STRING_TO_JSVAL(urlStr);
> > +  if (!JS_SetProperty(cx, dataObj, "url", val)) {
> > +    return false;
> 
> Please add a boolean for the return value for JS_SetProperty, and then use
> NS_ENSURE_TRUE(rv, rv). This may help debugging in the future because the
> failure will show up on the console log. Do the same for the other calls to
> JS_SetProperty.
>
> @@ +90,5 @@
> > +  nsCOMPtr<nsISystemMessagesInternal> systemMessenger =
> > +    do_GetService("@mozilla.org/system-message-internal;1");
> > +  if (!systemMessenger) {
> > +    return false;
> > +  }
> 
> NS_ENSURE_TRUE(systemMessenger, false);
>
Thanks for your great advice for RtspDelegateParent.cpp. I will follow your suggestions. :)
 
> ::: netwerk/ipc/moz.build
> @@ +47,5 @@
> >      'NeckoChannelParams.ipdlh',
> >      'PNecko.ipdl',
> >      'PRemoteOpenFile.ipdl',
> >      'PRtspController.ipdl',
> > +    'PRtspDelegate.ipdl',
> 
> Can you try putting both of these RTSP ipdl files in an "if
> CONFIG['MOZ_RTSP']" block please. I think it should work, and I think they
> should be isolated like this.
Sure. I will try it, thanks!

	Steve Workman [:sworkman] (INACTIVE)
	Comment 10 • 11 years ago

(In reply to Ethan Tseng [:ethan] from comment #9)
> > @@ +26,5 @@
> > > +  ~RtspDelegateChild();
> > > +
> > > +  void AddIPDLReference();
> > > +  void ReleaseIPDLReference();
> > > +
> > 
> > I don't see SendDelegate being overridden in this class - is it definitely
> > ok to use the generated version from PRtspDelegate.ipdl?
> > 
> The only thing SendDelegate() does is to pass the URI, which is defined as
> "URIParams aURI", to the parent side.
> I think it is enough to use the generated version. Or is there anything I
> should concern?

I don't think there's anything you should be concerned about - I wanted to make sure that not overriding was your intention.

Can you add a comment in the class definition to clarify your choice please? This will help future readers.

	Ethan Tseng [:ethan] Assignee
	Comment 11 • 11 years ago

Hi Steve,
I fixed the codes according to your comments.
BTW, I am still new to Mozilla. Your advice is quite invaluable to me, thanks a lot! :)

	Ethan Tseng [:ethan] Assignee
	Comment 13 • 11 years ago

Resolved build error in the last patch.

	Ethan Tseng [:ethan] Assignee
	Comment 14 • 11 years ago

Minor change: set the "title" property the same as the url.

	Ethan Tseng [:ethan] Assignee
	Comment 15 • 11 years ago

Comment on attachment 830052 [details] [diff] [review]
Bug 884702: Delegate RTSP request using broadcast system message.

Hi Ms2ger,
We added something within nsDocShell::DoURILoad() to handle RTSP url scheme. Steve suggested we get someone familiar with nsDocShell to review these changes. Could you help to review this patch?

	:Ms2ger (he/him; ⌚ UTC+1/+2)
	Comment 16 • 11 years ago

Comment on attachment 830052 [details] [diff] [review]
Bug 884702: Delegate RTSP request using broadcast system message.

Review of attachment 830052 [details] [diff] [review]:
-----------------------------------------------------------------

I don't think I'm the person you're looking for :)

	Ethan Tseng [:ethan] Assignee
	Comment 17 • 11 years ago

(In reply to :Ms2ger from comment #16)
> Comment on attachment 830052 [details] [diff] [review]
> Bug 884702: Delegate RTSP request using broadcast system message.
> 
> Review of attachment 830052 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I don't think I'm the person you're looking for :)

Hi Ms2ger, thank you for helping transfer to the appropriate reviewer. :)

	Boris Zbarsky [:bzbarsky]
	Comment 18 • 11 years ago

Why do we want to do this in the docshell as opposed to the unknown protocol handler?

	Boris Zbarsky [:bzbarsky]
	Comment 19 • 11 years ago

In particular, normally the unknown protocol handler is what handles handing off URIs to helper apps and whatnot.

	Ethan Tseng [:ethan] Assignee
	Comment 20 • 11 years ago

Boris, sorry I am not so familiar with all of these. What is the unknown protocol handler? Is it related the external helper app service?

	Boris Zbarsky [:bzbarsky]
	Comment 21 • 11 years ago

Ethan, it's the code in uriloader/exthandler/nsExternalProtocolHandler.cpp plus the code in nsExternalHelperAppService::LoadURI.

Note that the latter also does some security checks and whatnot; presumably your code would go after those but before the attempt to look for an OS-level handler for the URI, right?  Though note the check for a content process up front in the method, which might complicate things.

	Ethan Tseng [:ethan] Assignee
	Comment 22 • 11 years ago

Boris, thanks for your information. Let me take some time to study them and see if we can hand off RTSP loading there.

BTW, the reason why we chose to do this in nsDocShell::DoURILoad() in the first place is because RTSP channel is not the real protocol handler. It is a dummy channel used to aid MediaResource creation in HTMLMediaElement. The actual network control and data flows are managed by RtspController object. So we thought we could break URI loading once the RTSP channel is created, just like the case of "mailto".

	Ethan Tseng [:ethan] Assignee
	Comment 23 • 11 years ago

I think the point is, we should not handle particular protocol in nsDocShell, right? I will try to do this in uriloader/exthandler.

	Boris Zbarsky [:bzbarsky]
	Comment 24 • 11 years ago

Right, exactly.  I'm 99.9% sure that mailto is handled in the code I mentioned in comment 21, for example.

	Steve Workman [:sworkman] (INACTIVE)
	Comment 25 • 11 years ago

Comment on attachment 830052 [details] [diff] [review]
Bug 884702: Delegate RTSP request using broadcast system message.

Ethan, I'll review the patch once you have had time to figure out the unknown protocol handler.

	Boris Zbarsky [:bzbarsky]
	Comment 26 • 11 years ago

Comment on attachment 830052 [details] [diff] [review]
Bug 884702: Delegate RTSP request using broadcast system message.

Unsetting review request for now.

	Ethan Tseng [:ethan] Assignee
	Comment 29 • 11 years ago

Hi Boris, I added the handling for RTSP scheme in the parent side of nsExternalHelperAppService::DoContent(). Could you please review it for me?

	Boris Zbarsky [:bzbarsky]
	Comment 30 • 11 years ago

Hmm.  So is rtsp not an unknown protocol, in the sense that we load the data ourselves?  Is it guaranteed to come with an unhandleable MIME type that we then hand off to DoContent?

	Boris Zbarsky [:bzbarsky]
	Comment 31 • 11 years ago

And in particular, with this patch what happens to the data we're receiving from necko?

	Ethan Tseng [:ethan] Assignee
	Comment 32 • 11 years ago

(In reply to Boris Zbarsky [:bz] from comment #30)
> Hmm.  So is rtsp not an unknown protocol, in the sense that we load the data
> ourselves?  Is it guaranteed to come with an unhandleable MIME type that we
> then hand off to DoContent?
I think the answer is yes, in Firefox OS. Rtsp protocol is specially handled and the MIME type is always a fake type "RTSP" specified by RTSP channel.

	Ethan Tseng [:ethan] Assignee
	Comment 33 • 11 years ago

(In reply to Boris Zbarsky [:bz] from comment #31)
> And in particular, with this patch what happens to the data we're receiving
> from necko?
Sorry I didn't aware of this. So far I just know that, for rtsp protocol, the actual control and data flows are managed by RtspController object and owned by RtspMediaResource. I will try to figure out this issue.

	Boris Zbarsky [:bzbarsky]
	Comment 34 • 11 years ago

OK.  I mean, this patch both kicks off the load to the new codepath _and_ does the normal exthandler stuff on whatever data comes in, which seems a bit odd.  I would like to understand why it's doing that.

	Ethan Tseng [:ethan] Assignee
	Comment 36 • 11 years ago

Set rtsp as non-exposed protocol.
pref("network.protocol-handler.expose.rtsp", false);

Then handle rtsp protocol in nsExternalHelperAppService::LoadURI().
With this dispatch, we did not handle the case for <iframe>, such as:
<iframe src=rtsp://whatever>

	Ethan Tseng [:ethan] Assignee
	Comment 37 • 11 years ago

Comment on attachment 8334414 [details] [diff] [review]
bug-884702-fix.patch

Boris, could you please review my updated patch?

	Boris Zbarsky [:bzbarsky]
	Comment 38 • 11 years ago

Comment on attachment 8334414 [details] [diff] [review]
bug-884702-fix.patch

Thanks.  I like this code location a lot more.  ;)

>+NS_IMETHODIMP nsExternalHelperAppService::launchVideoAppForRtsp(nsIURI* aURI)

s/NS_IMETHODIMP/nsresult/, since this wasn't declared NS_IMETHOD.

But also, just make this return void, since the caller ignores the return value anyway.

>+  nsAutoString msgType(NS_LITERAL_STRING("rtsp-open-video"));

Please use NS_NAMED_LITERAL_STRING instead.

>+  NS_ENSURE_TRUE(isRTSP, false);

That return value is not an nsresult; I'n a little surprised this compiled.  But, again, the caller already ensures this.  So just skip this check, or turn it into an assert.

>+  NS_ENSURE_TRUE(msgObj, false);

Again, not an nsresult.

Worse, this can leave a pending exception on cx.  You need to JS_ClearPendingException in all your failure cases here.  Might be worth creating a stack helper for that or something.  Alternately, create a WebIDL dictionary to do the conversion for you so you don't have to hand-code it all...

>+    NS_ENSURE_TRUE(urlStr, false);

As above.

>+    jsVal = STRING_TO_JSVAL(urlStr);

  jsVal.setString(urlStr);

>+    NS_ENSURE_TRUE(rv, false);

As above.  This occurs in several places.

>+    JSString *typeStr = JS_NewStringCopyN(cx, "video/rtsp", strlen("video/rtsp"));

NS_NAMED_LITERAL_CSTRING would keep you from typing out the string twice, I'd think.

>+    NS_ENSURE_TRUE(typeStr, false);

The usual.

>+    jsVal = STRING_TO_JSVAL(typeStr);

setString().

>+  systemMessenger->BroadcastMessage(msgType, OBJECT_TO_JSVAL(msgObj),

This is an unsafe reference hazard.  You want to do something more like:

  jsVal.setObject(*msgObj);

and pass jsVal to BroadcastMessage.

> +  // Handle for rtsp protocol.

Drop the "for", please.

r=me with those issues fixed, but I want to see the updated patch before it gets checked in, please.

	Ethan Tseng [:ethan] Assignee
	Comment 39 • 11 years ago

(In reply to Boris Zbarsky [:bz] from comment #38)
> Comment on attachment 8334414 [details] [diff] [review]
> bug-884702-fix.patch
> r=me with those issues fixed, but I want to see the updated patch before it
> gets checked in, please.
Boris, thanks for your review!
Sure. I will update a patch by your suggestions. Thanks. :)

	Ethan Tseng [:ethan] Assignee
	Comment 40 • 11 years ago

Fixed the issues from the reviewer's comments.

	Ethan Tseng [:ethan] Assignee
	Comment 41 • 11 years ago

Fix naming.

	Ethan Tseng [:ethan] Assignee
	Comment 42 • 11 years ago

(In reply to Boris Zbarsky [:bz] from comment #38)
> Comment on attachment 8334414 [details] [diff] [review]
> bug-884702-fix.patch
> Worse, this can leave a pending exception on cx.  You need to
> JS_ClearPendingException in all your failure cases here.  Might be worth
> creating a stack helper for that or something.  Alternately, create a WebIDL
> dictionary to do the conversion for you so you don't have to hand-code it
> all...

Hi Boris, could you please take a look at my updated patch?
I am not quite sure whether the stack helper I implemented is what you mentioned.

	Boris Zbarsky [:bzbarsky]
	Comment 43 • 11 years ago

That looks good, but maybe call the helper AutoClearPendingException?

	Ethan Tseng [:ethan] Assignee
	Comment 44 • 11 years ago

Refine a function name.

	Ethan Tseng [:ethan] Assignee
	Comment 45 • 11 years ago

(In reply to Boris Zbarsky [:bz] from comment #43)
> That looks good, but maybe call the helper AutoClearPendingException?
Yeah, I was thinking of a similar name like this.
Thanks for your advice! I've renamed it.

	Ethan Tseng [:ethan] Assignee
	Comment 46 • 11 years ago

Refresh comment.

	Ethan Tseng [:ethan] Assignee
	Comment 47 • 11 years ago

The result of try server:
https://tbpl.mozilla.org/?tree=Try&rev=39556a02beb3

	Ryan VanderMeulen [:RyanVM]
	Comment 48 • 11 years ago

https://hg.mozilla.org/integration/b2g-inbound/rev/4c959d937a76

	Ryan VanderMeulen [:RyanVM]
	Comment 49 • 11 years ago

https://hg.mozilla.org/mozilla-central/rev/4c959d937a76

	Ethan Tseng [:ethan] Assignee
	Comment 50 • 11 years ago

(In reply to Steve Workman [:sworkman] from comment #25)
> Comment on attachment 830052 [details] [diff] [review]
> Ethan, I'll review the patch once you have had time to figure out the
> unknown protocol handler.

Hi Steve, in the final solution with external helper, we don't need to establish IPC ourselves.
However, I really appreciated you helped to review my middle work and thanks for your advices. I learned a lot, thanks. :)

	Steve Workman [:sworkman] (INACTIVE)
	Comment 51 • 11 years ago

(In reply to Ethan Tseng [:ethan] from comment #50)
> I really appreciated you helped to review my middle work and thanks
> for your advices. I learned a lot, thanks. :)

You're very welcome. Glad it helped.