[Media Router] Add features to control browser side discovery

[Media Router] Add features to control browser side discovery

Split from https://codereview.chromium.org/2837363002/

- Add kEnableDialLocalDiscovery and kEnableCastDiscovery feature to turn on/off browser side DIAL and Cast discovery
- Pass those features to MRP
- Update unit tests

cl/154204127 (landed) handles enable_dial_discovery and enable_dial_discovery flag in extension side.

BUG=687375
Review-Url: https://codereview.chromium.org/2873893003
Cr-Commit-Position: refs/heads/master@{#471367}
Committed: https://chromium.googlesource.com/chromium/src/+/7f161a688a7af9b17a63fea84cb0312c5c297903

[zhaobin, 2017-05-10 05:03:44]

Description was changed from

==========
[Media Router] Add features to control browser side discovery

Split from https://codereview.chromium.org/2837363002/

- Add kEnableDialLocalDiscovery and kEnableCastDiscovery feature to turn on/off
browser side DIAL and Cast discovery
- Pass those features to MRP
- Update unit tests

BUG=687375
==========

to

==========
[Media Router] Add features to control browser side discovery

Split from https://codereview.chromium.org/2837363002/

- Add kEnableDialLocalDiscovery and kEnableCastDiscovery feature to turn on/off
browser side DIAL and Cast discovery
- Pass those features to MRP
- Update unit tests

cl/154204127 (landed) handles enable_dial_discovery and enable_dial_discovery
flag in extension side.

BUG=687375
==========

[zhaobin, 2017-05-10 05:04:07]

The CQ bit was checked by zhaobin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-10 05:04:51]

Dry run: 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-05-10 06:17:34]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-10 06:17:38]

Dry run: This issue passed the CQ dry run.

[zhaobin, 2017-05-10 18:18:16]

zhaobin@chromium.org changed reviewers:
+ kmarshall@chromium.org, mfoltz@chromium.org

[Kevin M, 2017-05-10 23:17:50]

lgtm (but please wait for Mark; I'm not a project owner)

[zhaobin, 2017-05-10 23:19:44]

zhaobin@chromium.org changed reviewers:
+ dcheng@chromium.org

[zhaobin, 2017-05-10 23:19:44]

+dcheng@ to review:

chrome/common/media_router/mojo/media_router.mojom

[dcheng, 2017-05-11 20:43:24]

https://codereview.chromium.org/2873893003/diff/20001/chrome/browser/media/ro...
File chrome/browser/media/router/mojo/media_router_mojo_impl.cc (right):

https://codereview.chromium.org/2873893003/diff/20001/chrome/browser/media/ro...
chrome/browser/media/router/mojo/media_router_mojo_impl.cc:158:
config->enable_dial_discovery = !media_router::DialLocalDiscoveryEnabled();
Are these booleans backwards?

[zhaobin, 2017-05-11 20:48:35]

https://codereview.chromium.org/2873893003/diff/20001/chrome/browser/media/ro...
File chrome/browser/media/router/mojo/media_router_mojo_impl.cc (right):

https://codereview.chromium.org/2873893003/diff/20001/chrome/browser/media/ro...
chrome/browser/media/router/mojo/media_router_mojo_impl.cc:158:
config->enable_dial_discovery = !media_router::DialLocalDiscoveryEnabled();
On 2017/05/11 20:43:23, dcheng wrote:
> Are these booleans backwards?

On purpose. media_router::DialLocalDiscoveryEnabled() means browser (Media
Router) enables DIAL discovery; extension (Media Router Provider) disables DIAL
discovery.

[dcheng, 2017-05-11 20:57:28]

On 2017/05/11 20:48:35, zhaobin wrote:
>
https://codereview.chromium.org/2873893003/diff/20001/chrome/browser/media/ro...
> File chrome/browser/media/router/mojo/media_router_mojo_impl.cc (right):
> 
>
https://codereview.chromium.org/2873893003/diff/20001/chrome/browser/media/ro...
> chrome/browser/media/router/mojo/media_router_mojo_impl.cc:158:
> config->enable_dial_discovery = !media_router::DialLocalDiscoveryEnabled();
> On 2017/05/11 20:43:23, dcheng wrote:
> > Are these booleans backwards?
> 
> On purpose. media_router::DialLocalDiscoveryEnabled() means browser (Media
> Router) enables DIAL discovery; extension (Media Router Provider) disables
DIAL
> discovery.

Maybe the mojo fields should have a different name then?

[zhaobin, 2017-05-11 21:12:30]

On 2017/05/11 20:57:28, dcheng wrote:
> On 2017/05/11 20:48:35, zhaobin wrote:
> >
>
https://codereview.chromium.org/2873893003/diff/20001/chrome/browser/media/ro...
> > File chrome/browser/media/router/mojo/media_router_mojo_impl.cc (right):
> > 
> >
>
https://codereview.chromium.org/2873893003/diff/20001/chrome/browser/media/ro...
> > chrome/browser/media/router/mojo/media_router_mojo_impl.cc:158:
> > config->enable_dial_discovery = !media_router::DialLocalDiscoveryEnabled();
> > On 2017/05/11 20:43:23, dcheng wrote:
> > > Are these booleans backwards?
> > 
> > On purpose. media_router::DialLocalDiscoveryEnabled() means browser (Media
> > Router) enables DIAL discovery; extension (Media Router Provider) disables
> DIAL
> > discovery.
> 
> Maybe the mojo fields should have a different name then?

Is enable_dial_provider_discovery OK?

[dcheng, 2017-05-11 21:26:09]

On 2017/05/11 21:12:30, zhaobin wrote:
> On 2017/05/11 20:57:28, dcheng wrote:
> > On 2017/05/11 20:48:35, zhaobin wrote:
> > >
> >
>
https://codereview.chromium.org/2873893003/diff/20001/chrome/browser/media/ro...
> > > File chrome/browser/media/router/mojo/media_router_mojo_impl.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2873893003/diff/20001/chrome/browser/media/ro...
> > > chrome/browser/media/router/mojo/media_router_mojo_impl.cc:158:
> > > config->enable_dial_discovery =
!media_router::DialLocalDiscoveryEnabled();
> > > On 2017/05/11 20:43:23, dcheng wrote:
> > > > Are these booleans backwards?
> > > 
> > > On purpose. media_router::DialLocalDiscoveryEnabled() means browser (Media
> > > Router) enables DIAL discovery; extension (Media Router Provider) disables
> > DIAL
> > > discovery.
> > 
> > Maybe the mojo fields should have a different name then?
> 
> Is enable_dial_provider_discovery OK?

LGTM with an added comment: the original name is fine, but it wasn't obvious (to
me) that having browser-side enabled discovery meant that we were disabling it
in the extension. It just looks confusing to see enabled = !enabled =)

[zhaobin, 2017-05-11 22:35:59]

@mfoltz, it seems that this confuses people:

config->enable_dial_discovery = !media_router::DialLocalDiscoveryEnabled();

Is it ok if we add some comments here explaining enable browser side discovery
is disabling extension side discovery, or any suggestions of better naming?

[mark a. foltz, 2017-05-11 22:51:05]

On 2017/05/11 at 22:35:59, zhaobin wrote:
> @mfoltz, it seems that this confuses people:
> 
> config->enable_dial_discovery = !media_router::DialLocalDiscoveryEnabled();
> 
> Is it ok if we add some comments here explaining enable browser side discovery
is disabling extension side discovery, or any suggestions of better naming?

Yeah that would help.  For example explaining we are migrating discovery from
the external Media Route Provider to the Media Router, so we need to disable it
in the provider.

[zhaobin, 2017-05-11 23:08:12]

On 2017/05/11 22:51:05, mark a. foltz wrote:
> On 2017/05/11 at 22:35:59, zhaobin wrote:
> > @mfoltz, it seems that this confuses people:
> > 
> > config->enable_dial_discovery = !media_router::DialLocalDiscoveryEnabled();
> > 
> > Is it ok if we add some comments here explaining enable browser side
discovery
> is disabling extension side discovery, or any suggestions of better naming?
> 
> Yeah that would help.  For example explaining we are migrating discovery from
> the external Media Route Provider to the Media Router, so we need to disable
it
> in the provider.

Done :)

[zhaobin, 2017-05-11 23:08:13]

On 2017/05/11 22:51:05, mark a. foltz wrote:
> On 2017/05/11 at 22:35:59, zhaobin wrote:
> > @mfoltz, it seems that this confuses people:
> > 
> > config->enable_dial_discovery = !media_router::DialLocalDiscoveryEnabled();
> > 
> > Is it ok if we add some comments here explaining enable browser side
discovery
> is disabling extension side discovery, or any suggestions of better naming?
> 
> Yeah that would help.  For example explaining we are migrating discovery from
> the external Media Route Provider to the Media Router, so we need to disable
it
> in the provider.

Done :)

[mark a. foltz, 2017-05-11 23:17:59]

LGTM with one nit

https://codereview.chromium.org/2873893003/diff/60001/chrome/browser/media/ro...
File chrome/browser/media/router/mojo/media_router_mojo_test.h (right):

https://codereview.chromium.org/2873893003/diff/60001/chrome/browser/media/ro...
chrome/browser/media/router/mojo/media_router_mojo_test.h:189:
MOCK_METHOD2(InvokeRaw,
In general we have been using InvokeInternal for these adapters to mock methods
with move-only types.

[zhaobin, 2017-05-12 00:16:38]

The CQ bit was checked by zhaobin@chromium.org to run a CQ dry run

[zhaobin, 2017-05-12 00:16:51]

https://codereview.chromium.org/2873893003/diff/60001/chrome/browser/media/ro...
File chrome/browser/media/router/mojo/media_router_mojo_test.h (right):

https://codereview.chromium.org/2873893003/diff/60001/chrome/browser/media/ro...
chrome/browser/media/router/mojo/media_router_mojo_test.h:189:
MOCK_METHOD2(InvokeRaw,
On 2017/05/11 23:17:59, mark a. foltz wrote:
> In general we have been using InvokeInternal for these adapters to mock
methods
> with move-only types.

Done.

[commit-bot: I haz the power, 2017-05-12 00:17:55]

Dry run: 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-05-12 01:52:54]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-12 01:52:55]

Dry run: This issue passed the CQ dry run.

[zhaobin, 2017-05-12 17:45:32]

The CQ bit was checked by zhaobin@chromium.org

[zhaobin, 2017-05-12 17:45:32]

The patchset sent to the CQ was uploaded after l-g-t-m from
kmarshall@chromium.org, dcheng@chromium.org, mfoltz@chromium.org
Link to the patchset: https://codereview.chromium.org/2873893003/#ps80001
(title: "resolve code review comments from Mark")

[commit-bot: I haz the power, 2017-05-12 17:47:16]

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-05-12 18:07:54]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1494611132235960,
"parent_rev": "04c17f56d6af6f1b52ef9418ed4c6bd67305954b", "commit_rev":
"7f161a688a7af9b17a63fea84cb0312c5c297903"}

[commit-bot: I haz the power, 2017-05-12 18:08:04]

Description was changed from

==========
[Media Router] Add features to control browser side discovery

Split from https://codereview.chromium.org/2837363002/

- Add kEnableDialLocalDiscovery and kEnableCastDiscovery feature to turn on/off
browser side DIAL and Cast discovery
- Pass those features to MRP
- Update unit tests

cl/154204127 (landed) handles enable_dial_discovery and enable_dial_discovery
flag in extension side.

BUG=687375
==========

to

==========
[Media Router] Add features to control browser side discovery

Split from https://codereview.chromium.org/2837363002/

- Add kEnableDialLocalDiscovery and kEnableCastDiscovery feature to turn on/off
browser side DIAL and Cast discovery
- Pass those features to MRP
- Update unit tests

cl/154204127 (landed) handles enable_dial_discovery and enable_dial_discovery
flag in extension side.

BUG=687375

Review-Url: https://codereview.chromium.org/2873893003
Cr-Commit-Position: refs/heads/master@{#471367}
Committed:
https://chromium.googlesource.com/chromium/src/+/7f161a688a7af9b17a63fea84cb0...
==========

[commit-bot: I haz the power, 2017-05-12 18:08:06]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/7f161a688a7af9b17a63fea84cb0...