Closed Bug 1305351 Opened 3 years ago Closed 3 years ago

Add ChromeCastDisplay for Presentation API.

Categories

(Firefox for Android :: Screencasting, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 52
Tracking Status
firefox52 --- fixed

People

(Reporter: kuoe0.tw, Assigned: kuoe0.tw)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

Add Chromecast as the devices for Presentation API.
Blocks: 1295087
Blocks: 1252788
Hi snorp, should I also make Sebastian as the reviewer for this patch?
Flags: needinfo?(snorp)
BTW, start() and stop() will be implemented in Bug 1285870.
Comment on attachment 8796071 [details]
Bug 1305351 - Add ChromeCastDisplay for Presentation API.

https://reviewboard.mozilla.org/r/82058/#review81750

This looks ok, but we need to make sure the new app key is created in the right account.

::: mobile/android/base/java/org/mozilla/gecko/ChromeCastDisplay.java:29
(Diff revision 3)
> +import android.support.v7.media.MediaRouter.RouteInfo;
> +import android.util.Log;
> +
> +public class ChromeCastDisplay implements GeckoPresentationDisplay {
> +
> +    static final String REMOTE_DISPLAY_APP_ID = "743AF7F7";

I assume this is a new key, right? This needs to be created in the Mozilla Google account if it's not already. Sylvestre Ledru (:sylvestre) from relman can help you with that.

::: mobile/android/base/java/org/mozilla/gecko/ChromeCastPlayer.java:37
(Diff revision 3)
> +import android.os.Bundle;
> +import android.support.v7.media.MediaRouter.RouteInfo;
> +import android.util.Log;
> +
> +/* Implementation of GeckoMediaPlayer for talking to ChromeCast devices */
> +class ChromeCastPlayer implements GeckoMediaPlayer {

OK I was very confused about this until I figured out it was just renamed. mozreview is not doing well here -- did you make any changes?

::: mobile/android/base/java/org/mozilla/gecko/MediaPlayerManager.java:106
(Diff revision 3)
>      public void handleMessage(String event, final NativeJSObject message, final EventCallback callback) {
>          debug(event);
>  
> -        final GeckoMediaPlayer display = displays.get(message.getString("id"));
> -        if (display == null) {
> -            Log.e(LOGTAG, "Couldn't find a display for this id: " + message.getString("id") + " for message: " + event);
> +        final GeckoMediaPlayer player = players.get(message.getString("id"));
> +        if (player == null) {
> +            Log.e(LOGTAG, "Couldn'tplayerisplay for this id: " + message.getString("id") + " for message: " + event);

Typo
Attachment #8796071 - Flags: review?(snorp) → review+
(In reply to Tommy Kuo [:KuoE0] from comment #3)
> Hi snorp, should I also make Sebastian as the reviewer for this patch?

It couldn't hurt, it's mostly frontend code anyway.
Comment on attachment 8796071 [details]
Bug 1305351 - Add ChromeCastDisplay for Presentation API.

https://reviewboard.mozilla.org/r/82058/#review81750

> OK I was very confused about this until I figured out it was just renamed. mozreview is not doing well here -- did you make any changes?

I didn't make any changes for this file!
So now, we just miss the new key from Mozilla account for Sylvestre Ledru [:sylvestre] ! But as I mentioned in Bug 1285870 Comment 20, I think we can use the same key in ChromeCastPlayer.java[1]. 

In addition, I think we should rename the file MediaPlayerManager.java. Now, it manages not only the player devices but also display devices. Could you have any suggestion for that?

[1]: https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/ChromeCast.java#40
Flags: needinfo?(snorp)
Flags: needinfo?(sledru)
Blocks: 1285870
If we can use the same key as before, that's fine. I just wanted to be sure we're using a Mozilla-owned key.
Flags: needinfo?(snorp)
Haha, I tried to use the same key to do that, and it failed. I think we still wait Sylvestre giving me a new api key!
Hi Sylvestre, so could you register the account for [1]? And give me the new key! Thanks!

[1]: https://cast.google.com/publish/#/signup
Flags: needinfo?(sledru)
OK, Will do that today.
Flags: needinfo?(sledru)
Hi Sylvestre,

Thanks for your help!

Could we know when the new key will be ready?
Doing it, sorry for the lag.
I paid already. Which one do you want? 1) ?


1) Custom Receiver
Receiver app you can build using the Cast Receiver SDK and host yourself.

2) Styled Media Receiver
Pre-built receiver app that you can style using CSS. No need to build or host.

3) Remote Display Receiver
Receiver app that you can use with Cast Remote Display sender applications.
Flags: needinfo?(kuoe0)
Flags: needinfo?(awu)
Hi Sylvestre, please give me the key of the 3 (remote display receiver)! Thank you!
Flags: needinfo?(kuoe0)
Flags: needinfo?(awu)
The ID is 4574A331
Application name: Firefox / Status: Unpublished
Unpublished key only can be used for dev devices (registered Chromecast). When will you publish the key?
Flags: needinfo?(sledru)
OK, looks like it is more complex than expected:
when I want to publish the app, I need to provide the android package name.
As we have 3 different builds, I generated 3 ids.
Firefox aurora: 71C2951A
Firefox beta: 0B2687A1
Firefox: 4574A331

Note that I am not an expert with this. So, one id might be enough
Flags: needinfo?(sledru)
I think one ID might be enough. I think the android package name doesn't matter. The name I registered is different to the package name I built. And I'll use "4574A331" in my patch. Thanks!
(In reply to Tommy Kuo [:KuoE0] from comment #24)
> I think one ID might be enough. I think the android package name doesn't
> matter. The name I registered is different to the package name I built. And
> I'll use "4574A331" in my patch. Thanks!
OK, thanks, I removed the others.
Pushed by schien@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/65930c6cc816
Add ChromeCastDisplay for Presentation API. r=snorp
https://hg.mozilla.org/mozilla-central/rev/65930c6cc816
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 52
Depends on: 1410218
You need to log in before you can comment on or make changes to this bug.