Closed Bug 947132 Opened 6 years ago Closed 6 years ago

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

Categories

(Firefox OS Graveyard :: General, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

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

RESOLVED FIXED
blocking-b2g 1.3+
Tracking Status
firefox27 --- wontfix
firefox28 --- fixed
firefox29 --- fixed
b2g-v1.3 --- fixed

People

(Reporter: ethan, Assigned: ethan)

References

Details

Attachments

(1 file, 6 obsolete files)

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
Attached patch bug-947132-fix.patch (obsolete) — Splinter Review
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?
Attached patch bug-947132-fix.patch (obsolete) — Splinter Review
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-
Attached patch bug-947132-fix.patch (obsolete) — Splinter Review
1. Remove "rtsp-open-video" from SystemMessagePermissionsTable.
2. Remove an unnecessary assertion in rtsp external handler.
Attachment #8343777 - Attachment is obsolete: true
Attached patch bug-947132-fix.patch (obsolete) — Splinter Review
Attachment #8344140 - Attachment is obsolete: true
Attached patch bug-947132-fix.patch (obsolete) — Splinter Review
Remove an empty line.
Attachment #8344141 - Attachment is obsolete: true
Attached patch bug-947132-fix.patch (obsolete) — Splinter Review
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+
Update commit message.
Attachment #8344337 - Attachment is obsolete: true
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
Closed: 6 years ago
Resolution: --- → FIXED
Requesting approval to uplift this work to 1.3.
Flags: needinfo?(fabrice)
blocking-b2g: 1.3? → 1.3+
You need to log in before you can comment on or make changes to this bug.