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 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 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 on attachment 8796094 [details] Bug 1285870 - Show custom presentation on Chromecast. https://reviewboard.mozilla.org/r/82064/#review80698

(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) 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 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.

(In reply to Tommy Kuo [:KuoE0] from comment #10) > (In reply to James Willcox (:snorp) (jwillcox@mozilla.com) 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....

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)

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 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 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?

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

> 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 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"?

> 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?

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.

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

Pushed by schien@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/b20a6010a007 Show custom presentation on Chromecast. r=sebastian,snorp