Closed Bug 921963 Opened 7 years ago Closed 7 years ago

[RTSP] Support RTSP in Android JB build

Categories

(Firefox OS Graveyard :: General, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(blocking-b2g:1.3+, firefox28 fixed)

RESOLVED FIXED
1.3 Sprint 6 - 12/6
blocking-b2g 1.3+
Tracking Status
firefox28 --- fixed

People

(Reporter: vchang, Assigned: vchang)

References

Details

(Whiteboard: [ft:ril])

Attachments

(1 file, 2 obsolete files)

The name of logging functions are different in ICS and Android JB. We should force the code to load our own logging header to prevent this problem. 

[Ref]: Bug 908797
We should support RTSP on JB.
blocking-b2g: --- → 1.3+
Whiteboard: [ft:ril]
The log function name is different in Android ICS and Jelly Beam platform. 
Re-define them to prlog.
Attachment #831424 - Flags: review?(sworkman)
Comment on attachment 831424 [details] [diff] [review]
Patch v1.0 for android Jelly beam platform

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

Almost :) Logging changes look fine. But I'd rather that you changed the IPDL function params to use nsresult than doing casting. Maybe there is some reason I'm not aware of.

r- for now.

::: netwerk/protocol/rtsp/controller/RtspControllerParent.cpp
@@ +65,5 @@
>    nsresult rv = mController->AsyncOpen(this);
>    if (NS_SUCCEEDED(rv)) return true;
>  
>    mController = nullptr;
> +  return SendAsyncOpenFailed((uint8_t)rv);

I'd rather that you changed the IPDL to use an nsresult in the param list than do all this casting. See [Send|Recv]CancelHTMLDNSPrefetch for an example.

@@ +72,5 @@
>  bool
>  RtspControllerParent::RecvPlay()
>  {
>    LOG(("RtspControllerParent::RecvPlay()"));
> +  NS_ENSURE_TRUE(mController, true);

Do you want the error return value to be true here? Is the point just to flag the error on the console and return safely?

There are other statements like this in this file.

::: netwerk/protocol/rtsp/rtsp/APacketSource.cpp
@@ +13,5 @@
>   * See the License for the specific language governing permissions and
>   * limitations under the License.
>   */
>  
>  //#define LOG_NDEBUG 0

Is this used for anything? If it enables a particular kind of logging, can you add a comment please? Otherwise, please remove this line.

Please repeat for the other instances of this #define.
Attachment #831424 - Flags: review?(sworkman) → review-
Target Milestone: --- → 1.3 Sprint 6 - 12/6
Attached patch Patch v1.1 (obsolete) — Splinter Review
Address the review comments.
Attachment #831424 - Attachment is obsolete: true
Attachment #832877 - Flags: review?(sworkman)
Comment on attachment 832877 [details] [diff] [review]
Patch v1.1

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

Thanks for attending to the comments. You don't need to remove 'reason' from the log in RecvOnDisconnected (and other places), unless you really want to remove it.

r=me.

::: netwerk/protocol/rtsp/controller/RtspControllerChild.cpp
@@ +142,2 @@
>  {
> +  LOG(("RtspControllerChild::RecvOnDisconnected %d", index));

You can still keep reason in the log, right? Just use 0x%x instead of %d.

Check other places.

@@ +248,5 @@
>  NS_IMETHODIMP
>  RtspControllerChild::Play(void)
>  {
>    LOG(("RtspControllerChild::Play()"));
> +  NS_ENSURE_TRUE(mIPCOpen, NS_ERROR_FAILURE);

Good catch. I've done this myself several times :)
Attachment #832877 - Flags: review?(sworkman) → review+
Attached patch Patch v1.2Splinter Review
Update the patch to address the review comments.
Attachment #832877 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/0def2d6e3665
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.