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

RESOLVED FIXED

Status

Firefox OS
General
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: ethan, Assigned: ethan)

Tracking

unspecified
ARM
Gonk (Firefox OS)

Firefox Tracking Flags

(blocking-b2g:1.3+, firefox27 wontfix, firefox28 fixed, firefox29 fixed, b2g-v1.3 fixed)

Details

Attachments

(1 attachment, 6 obsolete attachments)

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.
Blocks: 928332
Created attachment 8343683 [details] [diff] [review]
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.
Attachment #8343683 - Flags: review?(fabrice)
Assignee: nobody → ettseng
Fabrice, could you help to review this patch?
Created attachment 8343777 [details] [diff] [review]
bug-947132-fix.patch

Fixed a build error on B2G JB Emulator.
Attachment #8343683 - Attachment is obsolete: true
Attachment #8343683 - Flags: review?(fabrice)
Attachment #8343777 - Flags: review?(fabrice)
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).
Attachment #8343777 - Flags: review?(fabrice) → review+
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
Attachment #8343777 - Flags: review+ → review-
Created attachment 8344140 [details] [diff] [review]
bug-947132-fix.patch

1. Remove "rtsp-open-video" from SystemMessagePermissionsTable.
2. Remove an unnecessary assertion in rtsp external handler.
Attachment #8343777 - Attachment is obsolete: true
Created attachment 8344141 [details] [diff] [review]
bug-947132-fix.patch
Attachment #8344140 - Attachment is obsolete: true
Created attachment 8344142 [details] [diff] [review]
bug-947132-fix.patch

Remove an empty line.
Attachment #8344141 - Attachment is obsolete: true
Created attachment 8344337 [details] [diff] [review]
bug-947132-fix.patch
Attachment #8344142 - Attachment is obsolete: true
Attachment #8344337 - Flags: review?(fabrice)
(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.
Attachment #8344337 - Flags: review?(fabrice) → review+
The result of Try server:
https://tbpl.mozilla.org/?tree=Try&rev=062721cde2a0
Created attachment 8345078 [details] [diff] [review]
bug-947132-fix.patch

Update commit message.
Attachment #8344337 - Attachment is obsolete: true
Push to b2g-inbound ....
https://hg.mozilla.org/integration/b2g-inbound/rev/187ec0c822f9
This is necessary to launch the Video app when rtsp source is selected.
blocking-b2g: --- → 1.3?
https://hg.mozilla.org/mozilla-central/rev/187ec0c822f9
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Requesting approval to uplift this work to 1.3.
Flags: needinfo?(fabrice)
blocking-b2g: 1.3? → 1.3+
https://hg.mozilla.org/releases/mozilla-aurora/rev/9f562482c0c4
status-b2g-v1.3: --- → fixed
status-firefox27: --- → wontfix
status-firefox28: --- → fixed
status-firefox29: --- → fixed
Flags: needinfo?(fabrice)
You need to log in before you can comment on or make changes to this bug.