1285870 - Show custom presentation on Chromecast.

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Screencasting, defect)

Description

[Chih-Hsuan Kuo [:tommykuo]]

In Bug 1252788, we are working on Chromecast remote display support. When the user selects a Chromecast as the display target, we should create a new nsScreenAndroid into nsScreenManagerAndroid as the screen context for the Chromecast device.

Comment 1

[Chih-Hsuan Kuo [:tommykuo]]

Attachment: Bug 1285870 - (Part 2) Implement RemotePresentationService to show PresentationView.

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

Comment 2

[Chih-Hsuan Kuo [:tommykuo]]

Attachment: Bug 1285870 - Show custom presentation on Chromecast.

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

Comment 3

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

Comment on attachment 8796072 [details]
Bug 1285870 - (Part 2) Implement RemotePresentationService to show PresentationView.

https://reviewboard.mozilla.org/r/82062/#review80690

Looks mostly ok to me, though you'll have to rebase it once you fix the other patches. I think Sebastian should look at this one as well since there is a lot of frontend stuff.

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

Where does that ID come from?

Comment 4

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

Comment on attachment 8796094 [details]
Bug 1285870 - Show custom presentation on Chromecast.

https://reviewboard.mozilla.org/r/82064/#review80696

::: mobile/android/app/mobile.js:30
(Diff revision 1)
>  
>  pref("toolkit.defaultChromeURI", "chrome://browser/content/browser.xul");
>  pref("browser.chromeURL", "chrome://browser/content/");
>  
> +// multi-screen
> +pref("toolkit.presentationViewURI", "chrome://browser/content/PresentationView.xul");

Do we really need a pref for this? I mean I know we have one for the default chrome URL but that's mostly for historic reasons. Would you ever want to change the value of this pref?

::: mobile/android/base/java/org/mozilla/gecko/PresentationView.java:17
(Diff revision 1)
> +
> +import android.content.Context;
> +import android.util.AttributeSet;
> +import android.util.DisplayMetrics;
> +
> +public class PresentationView extends GeckoView {

If you make the change to open() I suggested, this class could just set a screenId that the parent GeckoView will use in its own call, simplifying things a bit.

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoView.java:120
(Diff revision 1)
>          /* package */ Window() {}
>  
>          static native void open(Window instance, GeckoView view, Object compositor,
>                                  String chromeURI, int width, int height);
>  
> +        static native void openPresentationWindow(Window instance,

I think we should just add a 'screenId' argument to the existing open() call

Comment 5

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

Comment on attachment 8796094 [details]
Bug 1285870 - Show custom presentation on Chromecast.

https://reviewboard.mozilla.org/r/82064/#review80698

Comment 6

[Chih-Hsuan Kuo [:tommykuo]]

Comment on attachment 8796094 [details]
Bug 1285870 - Show custom presentation on Chromecast.

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

Comment 7

[Chih-Hsuan Kuo [:tommykuo]]

Attachment: Bug 1285870 - (Part 2) Implement RemotePresentationService to show PresentationView.

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

Comment 8

[Chih-Hsuan Kuo [:tommykuo]]

Comment on attachment 8796094 [details]
Bug 1285870 - Show custom presentation on Chromecast.

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

Comment 9

[Chih-Hsuan Kuo [:tommykuo]]

Comment on attachment 8796654 [details]
Bug 1285870 - (Part 2) Implement RemotePresentationService to show PresentationView.

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

Comment 10

[Chih-Hsuan Kuo [:tommykuo]]

(In reply to James Willcox (:snorp) ([e-mail]) from comment #3)
> Comment on attachment 8796072 [details]
> Bug 1285870 - (Part 2) Implement RemotePresentationService to show
> PresentationView.
> 
> https://reviewboard.mozilla.org/r/82062/#review80690
> 
> Looks mostly ok to me, though you'll have to rebase it once you fix the
> other patches. I think Sebastian should look at this one as well since there
> is a lot of frontend stuff.
> 
> ::: mobile/android/base/java/org/mozilla/gecko/ChromeCastDisplay.java:29
> (Diff revision 1)
> >  import android.support.v7.media.MediaRouter.RouteInfo;
> >  import android.util.Log;
> >  
> >  public class ChromeCastDisplay implements GeckoPresentationDisplay {
> >  
> >      static final String REMOTE_DISPLAY_APP_ID = "743AF7F7";
> 
> Where does that ID come from?

This ID comes from my Google Cast SDK Developer Console account. And I think we can use the same value in [1].

[1]: https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/ChromeCast.java#40

Comment 11

[Chih-Hsuan Kuo [:tommykuo]]

Comment on attachment 8796654 [details]
Bug 1285870 - (Part 2) Implement RemotePresentationService to show PresentationView.

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

Comment 12

[Chih-Hsuan Kuo [:tommykuo]]

Comment on attachment 8796094 [details]
Bug 1285870 - Show custom presentation on Chromecast.

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

Comment 13

[Chih-Hsuan Kuo [:tommykuo]]

Comment on attachment 8796654 [details]
Bug 1285870 - (Part 2) Implement RemotePresentationService to show PresentationView.

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

Comment 14

[Chih-Hsuan Kuo [:tommykuo]]

Comment on attachment 8796094 [details]
Bug 1285870 - Show custom presentation on Chromecast.

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

Comment 15

[Chih-Hsuan Kuo [:tommykuo]]

Comment on attachment 8796654 [details]
Bug 1285870 - (Part 2) Implement RemotePresentationService to show PresentationView.

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

Comment 16

[Chih-Hsuan Kuo [:tommykuo]]

Comment on attachment 8796094 [details]
Bug 1285870 - Show custom presentation on Chromecast.

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

Comment 17

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

Comment on attachment 8796094 [details]
Bug 1285870 - Show custom presentation on Chromecast.

https://reviewboard.mozilla.org/r/82064/#review81398

Looks ok now, but I would still like Sebastian to sign off as well.

Comment 18

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

(In reply to Tommy Kuo [:KuoE0] from comment #10)
> (In reply to James Willcox (:snorp) ([e-mail]) from comment #3)
> > Comment on attachment 8796072 [details]
> > Bug 1285870 - (Part 2) Implement RemotePresentationService to show
> > PresentationView.
> > 
> > https://reviewboard.mozilla.org/r/82062/#review80690
> > 
> > Looks mostly ok to me, though you'll have to rebase it once you fix the
> > other patches. I think Sebastian should look at this one as well since there
> > is a lot of frontend stuff.
> > 
> > ::: mobile/android/base/java/org/mozilla/gecko/ChromeCastDisplay.java:29
> > (Diff revision 1)
> > >  import android.support.v7.media.MediaRouter.RouteInfo;
> > >  import android.util.Log;
> > >  
> > >  public class ChromeCastDisplay implements GeckoPresentationDisplay {
> > >  
> > >      static final String REMOTE_DISPLAY_APP_ID = "743AF7F7";
> > 
> > Where does that ID come from?
> 
> This ID comes from my Google Cast SDK Developer Console account. And I think
> we can use the same value in [1].
> 
> [1]:
> https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/
> mozilla/gecko/ChromeCast.java#40

I believe that one is managed by release engineering. We probably want to use that one instead of one connected to a personal account....

Comment 19

[Sylvestre Ledru [:Sylvestre]]

s/release engineering/release management/ 
but yes, we own the developer console and we can generate a new key.
I don't think we should use the personal account  (bus factor)

Comment 20

[Chih-Hsuan Kuo [:tommykuo]]

Hi Sylvestre, I also agree that. Would you mind to give a new key for that? But, as I mentioned in Comment 10, I think we can you the same key [1]. I don't know what the different between using a new key or using the same key is.

[1]: https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/ChromeCast.java#40

Comment 21

[Chih-Hsuan Kuo [:tommykuo]]

Comment on attachment 8796094 [details]
Bug 1285870 - Show custom presentation on Chromecast.

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

Comment 22

[Chih-Hsuan Kuo [:tommykuo]]

Comment on attachment 8796094 [details]
Bug 1285870 - Show custom presentation on Chromecast.

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

Comment 23

[Chih-Hsuan Kuo [:tommykuo]]

Comment on attachment 8796094 [details]
Bug 1285870 - Show custom presentation on Chromecast.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/82064/diff/8-9/

Comment 24

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8796094 [details]
Bug 1285870 - Show custom presentation on Chromecast.

https://reviewboard.mozilla.org/r/82064/#review82184

::: mobile/android/base/java/org/mozilla/gecko/ChromeCastDisplay.java:37
(Diff revision 9)
> +    // EventCallback which is actually a GeckoEventCallback is sometimes being invoked more
> +    // than once. That causes the IllegalStateException to be thrown. To prevent a crash,
> +    // catch the exception and report it as an error to the log.

I assume there's no way to prevent invoking the callback multiple times? This /could/ be a code smell.

::: mobile/android/base/java/org/mozilla/gecko/ChromeCastDisplay.java:84
(Diff revision 9)
> -    public void start(EventCallback callback) { }
> +    public void start(EventCallback callback) {
> +        mStartCallback = callback;

The callback is only used inside this method. Do we actually need to store it as a class property? Maybe you just want to make the parameter final so that you can access it from the inline class?

::: mobile/android/base/java/org/mozilla/gecko/ChromeCastDisplay.java:124
(Diff revision 9)
> +        } catch (final IllegalArgumentException e) {
> +            Log.e(LOGTAG, "IllegalArgumentException", e);
> +            sendError(callback, "Fail to start presentation.");
> +            mStartCallback = null;
> +        }

What is actually throwing an IllegalArgumentException here? Inside the try block we are "only" creating an intent, building a settings object and starting a service. Is this actually something we need to catch or has this only happened during development with maybe actually wrong/missing arguments?

::: mobile/android/base/java/org/mozilla/gecko/ChromeCastDisplay.java:136
(Diff revision 9)
> +        } catch (final Exception e) {
> +            Log.e(LOGTAG, "Exception", e);
> +            sendError(callback, "Some exception happened when stopping presentation.");
> +        }

Can we be more specific here or do we need to catch all exceptions (including NullPointerException etc.) here?

::: mobile/android/base/java/org/mozilla/gecko/MediaPlayerManager.java:109
(Diff revision 9)
> +        final String eventPrefix = event.split(":")[0];
> +
> +        if ("MediaPlayer".equals(eventPrefix)) {

Maybe use if(event.startWith("MediaPlayer:") instead of splitting every incoming event.

Comment 25

[Chih-Hsuan Kuo [:tommykuo]]

Comment on attachment 8796094 [details]
Bug 1285870 - Show custom presentation on Chromecast.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/82064/diff/9-10/

Comment 26

[Chih-Hsuan Kuo [:tommykuo]]

Comment on attachment 8796094 [details]
Bug 1285870 - Show custom presentation on Chromecast.

https://reviewboard.mozilla.org/r/82064/#review82184

> I assume there's no way to prevent invoking the callback multiple times? This /could/ be a code smell.

Sorry, I'm not sure I got what you mean. Should I change that?

> What is actually throwing an IllegalArgumentException here? Inside the try block we are "only" creating an intent, building a settings object and starting a service. Is this actually something we need to catch or has this only happened during development with maybe actually wrong/missing arguments?

I think I can remove it. That exception happends when I give some wrong paramters. And I think the parameters for startService is fixed, so that exception wouldn't happend.

> Can we be more specific here or do we need to catch all exceptions (including NullPointerException etc.) here?

Actually, I don't know what kind of exception will be thrown from stopService(). Maybe we don't need the try-catch statement?

Comment 27

[Chih-Hsuan Kuo [:tommykuo]]

Hi sebastian, I updated the patch. Could you take a look again?

Comment 28

[Sebastian Kaspari (:sebastian; :pocmo)]

> Sorry, I'm not sure I got what you mean. Should I change that?
> 
> > What is actually throwing an IllegalArgumentException here? Inside the try block we are "only" creating an intent, building a settings object and starting a service. Is this actually something we need to catch or has this only happened during development with maybe actually wrong/missing arguments?

What I meant was that needing to catch the exception, instead of having a code flow that makes sure that the callback is only executed once, could be an indicator that there's something wrong. Is it possible to change the code in a way that success/error is called only once? If not then I'm okay with catching the exception as last resort.

> > Can we be more specific here or do we need to catch all exceptions (including NullPointerException etc.) here?
> 
> Actually, I don't know what kind of exception will be thrown from
> stopService(). Maybe we don't need the try-catch statement?

Then let's remove it. :)(In reply to Tommy Kuo [:KuoE0] from comment #27)

> Hi sebastian, I updated the patch. Could you take a look again?

Okay, I'll have  a look now.

Comment 29

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8796094 [details]
Bug 1285870 - Show custom presentation on Chromecast.

https://reviewboard.mozilla.org/r/82064/#review82814

LGTM.

::: mobile/android/base/java/org/mozilla/gecko/MediaPlayerManager.java:48
(Diff revision 10)
>              return new MediaPlayerManager();
>          }
>      }
>  
>      private static final String LOGTAG = "GeckoMediaPlayerManager";
> +    protected boolean isPresentationMode = false; // Used to prevent mirroring when Presentation API is using.

nit: "is used"?

Comment 30

[Sylvestre Ledru [:Sylvestre]]

> I also agree that. Would you mind to give a new key for that? But, as I mentioned in Comment 10, I think we can 
> you the same key [1]. I don't know what the different between using a new key or using the same key is.
Using the developer console linked (https://console.developers.google.com) with our Google play account, I don't have any access to a chromecast id. I guess this is different.
Do we know from which account this has been generated?

Comment 31

[Sylvestre Ledru [:Sylvestre]]

The Google play account is not registered on https://cast.google.com/publish/#/signup

I am happy to do it. Especially if the previous account used is not a Mozilla's account.

Comment 32

[Chih-Hsuan Kuo [:tommykuo]]

Comment on attachment 8796094 [details]
Bug 1285870 - Show custom presentation on Chromecast.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/82064/diff/10-11/

Comment 33

[Chih-Hsuan Kuo [:tommykuo]]

Comment on attachment 8796094 [details]
Bug 1285870 - Show custom presentation on Chromecast.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/82064/diff/11-12/

Comment 34

[Chih-Hsuan Kuo [:tommykuo]]

Comment on attachment 8796094 [details]
Bug 1285870 - Show custom presentation on Chromecast.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/82064/diff/12-13/

Comment 35

[Chih-Hsuan Kuo [:tommykuo]]

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

Comment 36

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b20a6010a007
Show custom presentation on Chromecast. r=sebastian,snorp

Comment 37

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b20a6010a007