[Chromecast] Enable MultizoneBackendTest on Earth.

[Chromecast] Enable MultizoneBackendTest on Earth.

BUG=internal b/62147653
TEST=cast_media_unittests on desktop, Hendrix, and Earth

Review-Url: https://codereview.chromium.org/2922003002
Cr-Commit-Position: refs/heads/master@{#477163}
Committed: https://chromium.googlesource.com/chromium/src/+/a0889dfe98719a8e00d2d55fb1f4dbeced939707

[jameswest, 2017-06-03 01:24:35]

Description was changed from

==========
[Chromecast] Enable MultizoneBackendTest on Earth.

BUG=internal b/62147653
TEST=run cast_media_unittests on Earth
==========

to

==========
[Chromecast] Enable MultizoneBackendTest on Earth.

BUG=internal b/62147653
TEST=run cast_media_unittests on Earth
==========

[jameswest, 2017-06-03 01:24:35]

jameswest@google.com changed reviewers:
+ halliwell@chromium.org, kmackay@chromium.org, slan@chromium.org

[halliwell, 2017-06-03 01:49:25]

you should probably use chromium.org account as well ;)

https://codereview.chromium.org/2922003002/diff/1/chromecast/chromecast.gni
File chromecast/chromecast.gni (right):

https://codereview.chromium.org/2922003002/diff/1/chromecast/chromecast.gni#n...
chromecast/chromecast.gni:43: supports_multizone = false
should it still default true on audio_only non-desktop builds?

[jameswest, 2017-06-03 02:10:16]

On 2017/06/03 01:49:25, halliwell wrote:
> you should probably use http://chromium.org account as well ;)

No matter what I try, I've never been able to upload a code review under my
Chromium account.

https://codereview.chromium.org/2922003002/diff/1/chromecast/chromecast.gni
File chromecast/chromecast.gni (right):

https://codereview.chromium.org/2922003002/diff/1/chromecast/chromecast.gni#n...
chromecast/chromecast.gni:43: supports_multizone = false
On 2017/06/03 01:49:24, halliwell wrote:
> should it still default true on audio_only non-desktop builds?

I'm not opposed to that, but I think I'd rather keep supports_multizone separate
from is_cast_audio_only. It seems more straightforward to me to just have a flag
that is explicitly turned on for each device rather than a condition that can be
true based on other factors.

[halliwell, 2017-06-03 02:15:38]

On 2017/06/03 02:10:16, jameswest wrote:
> On 2017/06/03 01:49:25, halliwell wrote:
> > you should probably use http://chromium.org account as well ;)
> 
> No matter what I try, I've never been able to upload a code review under my
> Chromium account.
> 
> https://codereview.chromium.org/2922003002/diff/1/chromecast/chromecast.gni
> File chromecast/chromecast.gni (right):
> 
>
https://codereview.chromium.org/2922003002/diff/1/chromecast/chromecast.gni#n...
> chromecast/chromecast.gni:43: supports_multizone = false
> On 2017/06/03 01:49:24, halliwell wrote:
> > should it still default true on audio_only non-desktop builds?
> 
> I'm not opposed to that, but I think I'd rather keep supports_multizone
separate
> from is_cast_audio_only. It seems more straightforward to me to just have a
flag
> that is explicitly turned on for each device rather than a condition that can
be
> true based on other factors.

Well, it saves setting per-device flags to have defaults that are 'mostly
correct', and doesn't stop you from setting things explicitly.  It's a pattern
used for many other gn args.

Having said that, if you want to go this way, that's fine by me, but you'll need
another CL to set it for Android Things.

[halliwell, 2017-06-03 02:24:18]

On 2017/06/03 02:15:38, halliwell wrote:
> On 2017/06/03 02:10:16, jameswest wrote:
> > On 2017/06/03 01:49:25, halliwell wrote:
> > > you should probably use http://chromium.org account as well ;)
> > 
> > No matter what I try, I've never been able to upload a code review under my
> > Chromium account.
> > 
> > https://codereview.chromium.org/2922003002/diff/1/chromecast/chromecast.gni
> > File chromecast/chromecast.gni (right):
> > 
> >
>
https://codereview.chromium.org/2922003002/diff/1/chromecast/chromecast.gni#n...
> > chromecast/chromecast.gni:43: supports_multizone = false
> > On 2017/06/03 01:49:24, halliwell wrote:
> > > should it still default true on audio_only non-desktop builds?
> > 
> > I'm not opposed to that, but I think I'd rather keep supports_multizone
> separate
> > from is_cast_audio_only. It seems more straightforward to me to just have a
> flag
> > that is explicitly turned on for each device rather than a condition that
can
> be
> > true based on other factors.
> 
> Well, it saves setting per-device flags to have defaults that are 'mostly
> correct', and doesn't stop you from setting things explicitly.  It's a pattern
> used for many other gn args.
> 
> Having said that, if you want to go this way, that's fine by me, but you'll
need
> another CL to set it for Android Things.

Also, what about all the OEM audio devices?
Plus, enable_multizone defaults to is audio build, so that's kind of weirdly
inconsistent.

[jameswest, 2017-06-03 02:30:51]

On 2017/06/03 02:24:18, halliwell wrote:
> On 2017/06/03 02:15:38, halliwell wrote:
> > On 2017/06/03 02:10:16, jameswest wrote:
> > > On 2017/06/03 01:49:25, halliwell wrote:
> > > > you should probably use http://chromium.org account as well ;)
> > > 
> > > No matter what I try, I've never been able to upload a code review under
my
> > > Chromium account.
> > > 
> > >
https://codereview.chromium.org/2922003002/diff/1/chromecast/chromecast.gni
> > > File chromecast/chromecast.gni (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2922003002/diff/1/chromecast/chromecast.gni#n...
> > > chromecast/chromecast.gni:43: supports_multizone = false
> > > On 2017/06/03 01:49:24, halliwell wrote:
> > > > should it still default true on audio_only non-desktop builds?
> > > 
> > > I'm not opposed to that, but I think I'd rather keep supports_multizone
> > separate
> > > from is_cast_audio_only. It seems more straightforward to me to just have
a
> > flag
> > > that is explicitly turned on for each device rather than a condition that
> can
> > be
> > > true based on other factors.
> > 
> > Well, it saves setting per-device flags to have defaults that are 'mostly
> > correct', and doesn't stop you from setting things explicitly.  It's a
pattern
> > used for many other gn args.
> > 
> > Having said that, if you want to go this way, that's fine by me, but you'll
> need
> > another CL to set it for Android Things.
> 
> Also, what about all the OEM audio devices?
> Plus, enable_multizone defaults to is audio build, so that's kind of weirdly
> inconsistent.

It may be necessary for the OEM devices. I'm going to replace enable_multizone
with supports_multizone. enable_multizone is confusing because it doesn't
actually enable multizone if the command line flag isn't set.

[jameswest, 2017-06-03 02:46:57]

On 2017/06/03 02:30:51, jameswest wrote:
> On 2017/06/03 02:24:18, halliwell wrote:
> > On 2017/06/03 02:15:38, halliwell wrote:
> > > On 2017/06/03 02:10:16, jameswest wrote:
> > > > On 2017/06/03 01:49:25, halliwell wrote:
> > > > > you should probably use http://chromium.org account as well ;)
> > > > 
> > > > No matter what I try, I've never been able to upload a code review under
> my
> > > > Chromium account.
> > > > 
> > > >
> https://codereview.chromium.org/2922003002/diff/1/chromecast/chromecast.gni
> > > > File chromecast/chromecast.gni (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/2922003002/diff/1/chromecast/chromecast.gni#n...
> > > > chromecast/chromecast.gni:43: supports_multizone = false
> > > > On 2017/06/03 01:49:24, halliwell wrote:
> > > > > should it still default true on audio_only non-desktop builds?
> > > > 
> > > > I'm not opposed to that, but I think I'd rather keep supports_multizone
> > > separate
> > > > from is_cast_audio_only. It seems more straightforward to me to just
have
> a
> > > flag
> > > > that is explicitly turned on for each device rather than a condition
that
> > can
> > > be
> > > > true based on other factors.
> > > 
> > > Well, it saves setting per-device flags to have defaults that are 'mostly
> > > correct', and doesn't stop you from setting things explicitly.  It's a
> pattern
> > > used for many other gn args.
> > > 
> > > Having said that, if you want to go this way, that's fine by me, but
you'll
> > need
> > > another CL to set it for Android Things.
> > 
> > Also, what about all the OEM audio devices?
> > Plus, enable_multizone defaults to is audio build, so that's kind of weirdly
> > inconsistent.
> 
> It may be necessary for the OEM devices. I'm going to replace enable_multizone
> with supports_multizone. enable_multizone is confusing because it doesn't
> actually enable multizone if the command line flag isn't set.

I think ensuring it doesn't break OEM devices or Android Things is justification
for defaulting it to true.

[halliwell, 2017-06-03 02:49:22]

On 2017/06/03 02:46:57, jameswest wrote:
> On 2017/06/03 02:30:51, jameswest wrote:
> > On 2017/06/03 02:24:18, halliwell wrote:
> > > On 2017/06/03 02:15:38, halliwell wrote:
> > > > On 2017/06/03 02:10:16, jameswest wrote:
> > > > > On 2017/06/03 01:49:25, halliwell wrote:
> > > > > > you should probably use http://chromium.org account as well ;)
> > > > > 
> > > > > No matter what I try, I've never been able to upload a code review
under
> > my
> > > > > Chromium account.
> > > > > 
> > > > >
> > https://codereview.chromium.org/2922003002/diff/1/chromecast/chromecast.gni
> > > > > File chromecast/chromecast.gni (right):
> > > > > 
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/2922003002/diff/1/chromecast/chromecast.gni#n...
> > > > > chromecast/chromecast.gni:43: supports_multizone = false
> > > > > On 2017/06/03 01:49:24, halliwell wrote:
> > > > > > should it still default true on audio_only non-desktop builds?
> > > > > 
> > > > > I'm not opposed to that, but I think I'd rather keep
supports_multizone
> > > > separate
> > > > > from is_cast_audio_only. It seems more straightforward to me to just
> have
> > a
> > > > flag
> > > > > that is explicitly turned on for each device rather than a condition
> that
> > > can
> > > > be
> > > > > true based on other factors.
> > > > 
> > > > Well, it saves setting per-device flags to have defaults that are
'mostly
> > > > correct', and doesn't stop you from setting things explicitly.  It's a
> > pattern
> > > > used for many other gn args.
> > > > 
> > > > Having said that, if you want to go this way, that's fine by me, but
> you'll
> > > need
> > > > another CL to set it for Android Things.
> > > 
> > > Also, what about all the OEM audio devices?
> > > Plus, enable_multizone defaults to is audio build, so that's kind of
weirdly
> > > inconsistent.
> > 
> > It may be necessary for the OEM devices. I'm going to replace
enable_multizone
> > with supports_multizone. enable_multizone is confusing because it doesn't
> > actually enable multizone if the command line flag isn't set.
> 
> I think ensuring it doesn't break OEM devices or Android Things is
justification
> for defaulting it to true.

Yeah, sorry it took me a few rounds of comments to come up with those reasons :)

[kmackay, 2017-06-03 02:59:48]

https://codereview.chromium.org/2922003002/diff/20001/chromecast/chromecast.gni
File chromecast/chromecast.gni (right):

https://codereview.chromium.org/2922003002/diff/20001/chromecast/chromecast.g...
chromecast/chromecast.gni:43: supports_multizone = is_cast_audio_only
Note, this will change the behaviour a bit since it turns on multizone backend
unittests for desktop audio builds (which I assume will break).

[jameswest, 2017-06-03 03:07:51]

https://codereview.chromium.org/2922003002/diff/20001/chromecast/chromecast.gni
File chromecast/chromecast.gni (right):

https://codereview.chromium.org/2922003002/diff/20001/chromecast/chromecast.g...
chromecast/chromecast.gni:43: supports_multizone = is_cast_audio_only
On 2017/06/03 02:59:48, kmackay wrote:
> Note, this will change the behaviour a bit since it turns on multizone backend
> unittests for desktop audio builds (which I assume will break).

I restored the check for not desktop in the condition for the multizone backend
tests.

[kmackay, 2017-06-03 03:14:31]

https://codereview.chromium.org/2922003002/diff/20001/chromecast/chromecast.gni
File chromecast/chromecast.gni (right):

https://codereview.chromium.org/2922003002/diff/20001/chromecast/chromecast.g...
chromecast/chromecast.gni:43: supports_multizone = is_cast_audio_only
On 2017/06/03 03:07:51, jameswest wrote:
> On 2017/06/03 02:59:48, kmackay wrote:
> > Note, this will change the behaviour a bit since it turns on multizone
backend
> > unittests for desktop audio builds (which I assume will break).
> 
> I restored the check for not desktop in the condition for the multizone
backend
> tests.

Is that really accurate though (ie, does desktop actually support multizone)?

[halliwell, 2017-06-03 05:05:43]

On 2017/06/03 03:14:31, kmackay wrote:
>
https://codereview.chromium.org/2922003002/diff/20001/chromecast/chromecast.gni
> File chromecast/chromecast.gni (right):
> 
>
https://codereview.chromium.org/2922003002/diff/20001/chromecast/chromecast.g...
> chromecast/chromecast.gni:43: supports_multizone = is_cast_audio_only
> On 2017/06/03 03:07:51, jameswest wrote:
> > On 2017/06/03 02:59:48, kmackay wrote:
> > > Note, this will change the behaviour a bit since it turns on multizone
> backend
> > > unittests for desktop audio builds (which I assume will break).
> > 
> > I restored the check for not desktop in the condition for the multizone
> backend
> > tests.
> 
> Is that really accurate though (ie, does desktop actually support multizone)?

lgtm

[jameswest, 2017-06-05 19:45:22]

https://codereview.chromium.org/2922003002/diff/20001/chromecast/chromecast.gni
File chromecast/chromecast.gni (right):

https://codereview.chromium.org/2922003002/diff/20001/chromecast/chromecast.g...
chromecast/chromecast.gni:43: supports_multizone = is_cast_audio_only
On 2017/06/03 03:14:31, kmackay wrote:
> On 2017/06/03 03:07:51, jameswest wrote:
> > On 2017/06/03 02:59:48, kmackay wrote:
> > > Note, this will change the behaviour a bit since it turns on multizone
> backend
> > > unittests for desktop audio builds (which I assume will break).
> > 
> > I restored the check for not desktop in the condition for the multizone
> backend
> > tests.
> 
> Is that really accurate though (ie, does desktop actually support multizone)?

I changed it to default to false for desktop builds because I think the
multizone backend tests should pass whenever supports_multizone is true.

[jameswest, 2017-06-05 19:55:18]

Description was changed from

==========
[Chromecast] Enable MultizoneBackendTest on Earth.

BUG=internal b/62147653
TEST=run cast_media_unittests on Earth
==========

to

==========
[Chromecast] Enable MultizoneBackendTest on Earth.

BUG=internal b/62147653
TEST=cast_media_unittests on desktop, Hendrix, and Earth
==========

[kmackay, 2017-06-05 21:29:18]

lgtm

[jameswest, 2017-06-05 21:58:04]

The CQ bit was checked by jameswest@google.com

[jameswest, 2017-06-05 21:58:05]

The patchset sent to the CQ was uploaded after l-g-t-m from
halliwell@chromium.org
Link to the patchset: https://codereview.chromium.org/2922003002/#ps60001
(title: "Default supports_multizone to false for desktop builds.")

[commit-bot: I haz the power, 2017-06-05 21:58:57]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-06 02:53:49]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1496699884687150,
"parent_rev": "1e0264fbc3ceaa70147de2f2da143eb1e9ed72b7", "commit_rev":
"a0889dfe98719a8e00d2d55fb1f4dbeced939707"}

[commit-bot: I haz the power, 2017-06-06 02:54:02]

Description was changed from

==========
[Chromecast] Enable MultizoneBackendTest on Earth.

BUG=internal b/62147653
TEST=cast_media_unittests on desktop, Hendrix, and Earth
==========

to

==========
[Chromecast] Enable MultizoneBackendTest on Earth.

BUG=internal b/62147653
TEST=cast_media_unittests on desktop, Hendrix, and Earth

Review-Url: https://codereview.chromium.org/2922003002
Cr-Commit-Position: refs/heads/master@{#477163}
Committed:
https://chromium.googlesource.com/chromium/src/+/a0889dfe98719a8e00d2d55fb1f4...
==========

[commit-bot: I haz the power, 2017-06-06 02:54:03]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/a0889dfe98719a8e00d2d55fb1f4...