1305351 - Add ChromeCastDisplay for Presentation API.

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Screencasting, defect)

Description

[Chih-Hsuan Kuo [:tommykuo]]

Add Chromecast as the devices for Presentation API.

Comment 1

[Chih-Hsuan Kuo [:tommykuo]]

Attachment: Bug 1305351 - Add ChromeCastDisplay for Presentation API.

Review commit: https://reviewboard.mozilla.org/r/82058/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/82058/

Comment 2

[Chih-Hsuan Kuo [:tommykuo]]

Comment on attachment 8796071 [details]
Bug 1305351 - Add ChromeCastDisplay for Presentation API.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/82058/diff/1-2/

Comment 3

[Chih-Hsuan Kuo [:tommykuo]]

Hi snorp, should I also make Sebastian as the reviewer for this patch?

Comment 4

[Chih-Hsuan Kuo [:tommykuo]]

BTW, start() and stop() will be implemented in Bug 1285870.

Comment 5

[Chih-Hsuan Kuo [:tommykuo]]

Comment on attachment 8796071 [details]
Bug 1305351 - Add ChromeCastDisplay for Presentation API.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/82058/diff/2-3/

Comment 6

[James Willcox (:snorp) ([e-mail]) (he/him)]

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

Comment 7

[James Willcox (:snorp) ([e-mail]) (he/him)]

(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 8

[Chih-Hsuan Kuo [:tommykuo]]

Comment on attachment 8796071 [details]
Bug 1305351 - Add ChromeCastDisplay for Presentation API.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/82058/diff/3-4/

Comment 9

[Chih-Hsuan Kuo [:tommykuo]]

Comment on attachment 8796071 [details]
Bug 1305351 - Add ChromeCastDisplay for Presentation API.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/82058/diff/4-5/

Comment 10

[Chih-Hsuan Kuo [:tommykuo]]

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!

Comment 11

[Chih-Hsuan Kuo [:tommykuo]]

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

Comment 12

[Chih-Hsuan Kuo [:tommykuo]]

Comment on attachment 8796071 [details]
Bug 1305351 - Add ChromeCastDisplay for Presentation API.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/82058/diff/5-6/

Comment 13

[James Willcox (:snorp) ([e-mail]) (he/him)]

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.

Comment 14

[Chih-Hsuan Kuo [:tommykuo]]

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!

Comment 15

[Sylvestre Ledru [:Sylvestre]]

I answered here:
https://bugzilla.mozilla.org/show_bug.cgi?id=1285870#c30

Comment 16

[Chih-Hsuan Kuo [:tommykuo]]

Hi Sylvestre, so could you register the account for [1]? And give me the new key! Thanks!

[1]: https://cast.google.com/publish/#/signup

Comment 17

[Sylvestre Ledru [:Sylvestre]]

OK, Will do that today.

Comment 18

[Aaron Wu]

Hi Sylvestre,

Thanks for your help!

Could we know when the new key will be ready?

Comment 19

[Sylvestre Ledru [:Sylvestre]]

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.

Comment 20

[Chih-Hsuan Kuo [:tommykuo]]

Hi Sylvestre, please give me the key of the 3 (remote display receiver)! Thank you!

Comment 21

[Sylvestre Ledru [:Sylvestre]]

The ID is 4574A331
Application name: Firefox / Status: Unpublished

Comment 22

[Chih-Hsuan Kuo [:tommykuo]]

Unpublished key only can be used for dev devices (registered Chromecast). When will you publish the key?

Comment 23

[Sylvestre Ledru [:Sylvestre]]

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

Comment 24

[Chih-Hsuan Kuo [:tommykuo]]

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!

Comment 25

[Chih-Hsuan Kuo [:tommykuo]]

Comment on attachment 8796071 [details]
Bug 1305351 - Add ChromeCastDisplay for Presentation API.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/82058/diff/6-7/

Comment 26

[Chih-Hsuan Kuo [:tommykuo]]

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ae9a2624ae81

Comment 27

[Sylvestre Ledru [:Sylvestre]]

(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.

Comment 28

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/65930c6cc816
Add ChromeCastDisplay for Presentation API. r=snorp

Comment 29

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/65930c6cc816