947132 - [RTSP] Replace system message by activity when handing off rtsp URL loading

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[Ethan Tseng [:ethan]]

This bug is aimed to fix a mistake in bug 884702.

While handing off rtsp URL loading, we should use an activity instead of a system message. The reason is clarified by Fabrice in https://bugzilla.mozilla.org/show_bug.cgi?id=928332#c37.

Comment 1

[Ethan Tseng [:ethan]]

Attachment: bug-947132-fix.patch

Send a message "content-handler" for handing off rtsp url loading.
This message will be handled in b2g/chrome/content/shell.js and starts an activity to open the video app.

Comment 2

[Ethan Tseng [:ethan]]

Fabrice, could you help to review this patch?

Comment 3

[Ethan Tseng [:ethan]]

Attachment: bug-947132-fix.patch

Fixed a build error on B2G JB Emulator.

Comment 4

[[:fabrice] Fabrice Desré]

Comment on attachment 8343777 [details] [diff] [review]
bug-947132-fix.patch

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

r=me with comments addressed

::: uriloader/exthandler/nsExternalHelperAppService.cpp
@@ +642,5 @@
> +  // Set the "url" and "title" properties of the message.
> +  // They are the same in the case of RTSP streaming.
> +  {
> +    nsAutoCString spec;
> +    aURI->GetAsciiSpec(spec);

Why use the AsciiSpec and not Spec?

@@ +652,4 @@
>  
> +    rv = JS_SetProperty(cx, msgObj, "title", jsVal);
> +    NS_ENSURE_TRUE_VOID(rv);
> +  }

Do you really need a block there? (same for the previous one).

Comment 5

[[:fabrice] Fabrice Desré]

Comment on attachment 8343777 [details] [diff] [review]
bug-947132-fix.patch

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

Acutally, you forgot to remove the permission mapping at https://mxr.mozilla.org/mozilla-central/source/dom/messages/SystemMessagePermissionsChecker.jsm#113

Comment 6

[Ethan Tseng [:ethan]]

Attachment: bug-947132-fix.patch

1. Remove "rtsp-open-video" from SystemMessagePermissionsTable.
2. Remove an unnecessary assertion in rtsp external handler.

Comment 7

[Ethan Tseng [:ethan]]

Attachment: bug-947132-fix.patch

Comment 8

[Ethan Tseng [:ethan]]

Attachment: bug-947132-fix.patch

Remove an empty line.

Comment 9

[Ethan Tseng [:ethan]]

Attachment: bug-947132-fix.patch

Comment 10

[Ethan Tseng [:ethan]]

(In reply to Fabrice Desré [:fabrice] from comment #4)
> Review of attachment 8343777 [details] [diff] [review]:
> -----------------------------------------------------------------
> > +  {
> > +    nsAutoCString spec;
> > +    aURI->GetAsciiSpec(spec);
> 
> Why use the AsciiSpec and not Spec?
It's my carelessness. I corrected it. Thank you for pointing that.

 > > +    rv = JS_SetProperty(cx, msgObj, "title", jsVal);
> > +    NS_ENSURE_TRUE_VOID(rv);
> > +  } 
> Do you really need a block there? (same for the previous one).
I removed the assertion for "title" property but kept those for "type" and "url" since they are required properties to successfully load the media stream.

Comment 11

[Ethan Tseng [:ethan]]

The result of Try server:
https://tbpl.mozilla.org/?tree=Try&rev=062721cde2a0

Comment 12

[Ethan Tseng [:ethan]]

Attachment: bug-947132-fix.patch

Update commit message.

Comment 13

[Vincent Chang[:vchang][changyihsin]]

Push to b2g-inbound ....
https://hg.mozilla.org/integration/b2g-inbound/rev/187ec0c822f9

Comment 14

[Vincent Chang[:vchang][changyihsin]]

This is necessary to launch the Video app when rtsp source is selected.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/187ec0c822f9

Comment 16

[Wesley Huang [:wesley_huang] (EPM) (NI me)]

Requesting approval to uplift this work to 1.3.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/9f562482c0c4