1033560 - Enable casting to chromecast on developer builds

Status: RESOLVED WONTFIX

(Firefox for Android Graveyard :: General, defect)

Description

[Wesley Johnston (:wesj)]

Attachment: Patch

Comment 1

[Wesley Johnston (:wesj)]

Comment on attachment 8449639 [details] [diff] [review]
Patch

Didn't mean to mark this bug secure. Need to test this patch. Not sure if you'd rather have f? or r? since its untested, so I went with f?

Comment 2

[Wesley Johnston (:wesj)]

Attachment: enablecast

Tested this locally and it works well (requires a clobber though).

Comment 3

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 8450412 [details] [diff] [review]
enablecast

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

I think you need build peer review for the configure.in change

::: mobile/android/base/ChromeCast.java
@@ +134,5 @@
>  
>      public ChromeCast(Context context, RouteInfo route) {
> +        int status =  GooglePlayServicesUtil.isGooglePlayServicesAvailable(context);
> +        if (status != ConnectionResult.SUCCESS) {
> +            throw new IllegalStateException("Play services are required for Chromecast support (go status code " + status + ")");

this will get caught here https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/MediaPlayerManager.java#202

Comment 4

[Gregory Szorc [:gps]]

Comment on attachment 8450412 [details] [diff] [review]
enablecast

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

This is essentially r+ except for the naming. I could also bikeshed about making this opt-in based on a positive configure result. i.e. should we only set this variable if we're building Fennec?

::: configure.in
@@ +7684,5 @@
> +dnl = --disable-native-devices
> +dnl ========================================================
> +
> +MOZ_ARG_DISABLE_BOOL(native-devices,
> +[  --disable-native-devices      Disable native casting devices],

"native devices" is somewhat ambiguous to me. Keep in mind that this configure script applies to desktop, Android, Firefox OS, and every other Gecko product.

Perhaps "device casting" would be better? But even then, "device casting" isn't very descriptive. I don't know the problem space well enough to suggest anything better. Naming is hard.

Comment 5

[Wesley Johnston (:wesj)]

Attachment: Patch

I renamed this to MOZ_NATIVE_CASTING since it doesn't control casting entirely, just casting using native Java interfaces. What do you think?

Comment 6

[Gregory Szorc [:gps]]

Comment on attachment 8451797 [details] [diff] [review]
Patch

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

I still think the name could be improved. But I got nothing better. We can always apply sed later.

Comment 7

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/3f846b593bcb

Comment 8

[Makoto Kato [:m_kato]]

Don't you modify android.m4 to replace MOZ_NATIVE_DEVICES with MOZ_NATIVE_CASTING?

Comment 9

[Nick Alexander :nalexander [he/him]]

(In reply to Makoto Kato (:m_kato) from comment #8)
> Don't you modify android.m4 to replace MOZ_NATIVE_DEVICES with
> MOZ_NATIVE_CASTING?

Yeah, this is busted.

Comment 10

[Nick Alexander :nalexander [he/him]]

(In reply to Nick Alexander :nalexander from comment #9)
> (In reply to Makoto Kato (:m_kato) from comment #8)
> > Don't you modify android.m4 to replace MOZ_NATIVE_DEVICES with
> > MOZ_NATIVE_CASTING?
> 
> Yeah, this is busted.

Bustage fix pushed to fx-team:

https://hg.mozilla.org/integration/fx-team/rev/13d9e025fc86

Just realized this already hit central, so I should have opened a new ticket.

Comment 11

[Wesley Johnston (:wesj)]

backed these out:
https://hg.mozilla.org/integration/fx-team/rev/04cf8536314c

The autoconf stuff is apparently running thinking this is enabled. We need to fix that.

Comment 12

[Wesley Johnston (:wesj)]

Reopening. Not a high priority on my plate. Focused more on bug 1016529, but will grab this again if it isn't going to make it.

Comment 13

[Wesley Johnston (:wesj)]

I don't want to do this anymore