[Media Router] Conditionally enable mDNS on Windows.

[Media Router] Conditionally enable mDNS on Windows.

This change enables mDNS on Windows only when the user is in a context related
to the Media Router. Previously, a firewall prompt could be triggered on browser
startup which is confusing.

BUG=593167
R=apacible@chromium.org,amp@chromium.org

Committed: https://crrev.com/74d51922f5dc2516ce51cb49a36c0b4180ada30e
Cr-Commit-Position: refs/heads/master@{#383689}

[btolsch, 2016-03-22 23:46:00]

btolsch@chromium.org changed reviewers:
+ imcheng@chromium.org

[btolsch, 2016-03-22 23:46:01]

The extension seems to have settled and the issue around resolving the tests has
a solution for now, so PTAL, thanks!

[btolsch, 2016-03-23 21:37:23]

It looks like the issues with Windows and Android tests have been resolved so it
should really be ready for review this time. Thanks!

[amp, 2016-03-23 21:55:02]

lgtm

[apacible, 2016-03-23 22:04:45]

Thanks for doing this! lgtm + nitpicky comments

https://codereview.chromium.org/1821823002/diff/100001/chrome/browser/media/a...
File chrome/browser/media/android/router/media_router_android.h (right):

https://codereview.chromium.org/1821823002/diff/100001/chrome/browser/media/a...
chrome/browser/media/android/router/media_router_android.h:68: 
nit: remove extra line here so it's obvious these two functions are from the
MediaRouter implementation.

https://codereview.chromium.org/1821823002/diff/100001/chrome/browser/media/r...
File chrome/browser/media/router/media_router_mojo_impl.cc (right):

https://codereview.chromium.org/1821823002/diff/100001/chrome/browser/media/r...
chrome/browser/media/router/media_router_mojo_impl.cc:32: 
nit: remove newline.

https://codereview.chromium.org/1821823002/diff/100001/chrome/browser/media/r...
File chrome/browser/media/router/media_router_mojo_impl.h (right):

https://codereview.chromium.org/1821823002/diff/100001/chrome/browser/media/r...
chrome/browser/media/router/media_router_mojo_impl.h:101: 
nit: same comment on removing newlines.

https://codereview.chromium.org/1821823002/diff/100001/chrome/browser/media/r...
File chrome/browser/media/router/test_helper.h (right):

https://codereview.chromium.org/1821823002/diff/100001/chrome/browser/media/r...
chrome/browser/media/router/test_helper.h:153: 
nit: remove newline.

[mark a. foltz, 2016-03-24 00:04:40]

mfoltz@chromium.org changed reviewers:
+ mfoltz@chromium.org

[mark a. foltz, 2016-03-24 00:04:41]

I would be okay naming things enableMdns(), is_mdns_enabled_, etc. since that's
exactly what we are doing, but I don't feel strongly.

Please update media_router_mojo_impl_unittest.cc (which is slightly complicated
I know) with test cases for EnableMdns() after addressing comments

https://codereview.chromium.org/1821823002/diff/100001/chrome/browser/media/a...
File chrome/browser/media/android/router/media_router_android.cc (right):

https://codereview.chromium.org/1821823002/diff/100001/chrome/browser/media/a...
chrome/browser/media/android/router/media_router_android.cc:248:
NOTIMPLEMENTED();
I don't think this is necessary, you have an implementation below?
Unless it's a bug to call this at all in Android.

https://codereview.chromium.org/1821823002/diff/100001/chrome/browser/media/r...
File chrome/browser/media/router/media_router_mojo_impl.cc (right):

https://codereview.chromium.org/1821823002/diff/100001/chrome/browser/media/r...
chrome/browser/media/router/media_router_mojo_impl.cc:90: #if defined(OS_WIN)
Please put this into its own function in an anonymous namespace, i.e.
CanFirewallUseLocalPorts()

Also, I would not be opposed to putting it into its own file as well to keep the
include list for this file smaller (maybe in media_route_provider_util.cc).

https://codereview.chromium.org/1821823002/diff/100001/chrome/browser/media/r...
chrome/browser/media/router/media_router_mojo_impl.cc:99: #endif
What if !defined(OS_WIN)?  Should we set the flag to true so we can ensure mDNS
is enabled on startup?

https://codereview.chromium.org/1821823002/diff/100001/chrome/browser/media/r...
chrome/browser/media/router/media_router_mojo_impl.cc:176: if
(is_mdns_discovery_enabled_) {
Did you mean !is_mdns_discovery_enabled_?

Nit: Remove { }

https://codereview.chromium.org/1821823002/diff/100001/chrome/browser/media/r...
chrome/browser/media/router/media_router_mojo_impl.cc:429: if
(media_route_provider_) {
What if is_mdns_discovery_enabled_ is already true?

Nit: Remove { } but leave newline after if.

[imcheng, 2016-03-24 01:17:20]

https://codereview.chromium.org/1821823002/diff/100001/chrome/browser/media/r...
File chrome/browser/media/router/media_router_mojo_impl.cc (right):

https://codereview.chromium.org/1821823002/diff/100001/chrome/browser/media/r...
chrome/browser/media/router/media_router_mojo_impl.cc:91: base::FilePath
exe_path;
Ideally this check should happen in FILE thread, see:
https://bugs.chromium.org/p/chromium/issues/detail?id=593167#c68

That would mean is_mdns_discovery_enabled_ won't be set to true on construction
time, but I think that is fine.

https://codereview.chromium.org/1821823002/diff/100001/chrome/browser/media/r...
chrome/browser/media/router/media_router_mojo_impl.cc:176: if
(is_mdns_discovery_enabled_) {
On 2016/03/24 00:04:41, mark a. foltz wrote:
> Did you mean !is_mdns_discovery_enabled_?
> 
> Nit: Remove { }

I think this is correct. |is_mdns_discovery_enabled_| really means
|should_enable_mdns_discovery_|. Perhaps it should be renamed as such.

https://codereview.chromium.org/1821823002/diff/100001/chrome/browser/media/r...
chrome/browser/media/router/media_router_mojo_impl.cc:177:
media_route_provider_->EnableMdnsDiscovery();
Since we already enable mdns discovery on the extension by default on
non-Windows, should this code be Windows only?

https://codereview.chromium.org/1821823002/diff/100001/chrome/browser/media/r...
File chrome/browser/media/router/media_router_mojo_impl.h (right):

https://codereview.chromium.org/1821823002/diff/100001/chrome/browser/media/r...
chrome/browser/media/router/media_router_mojo_impl.h:388: bool
is_mdns_discovery_enabled_ = false;
This should be true for non-Windows.

https://codereview.chromium.org/1821823002/diff/100001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/media_router/media_router_ui.cc (right):

https://codereview.chromium.org/1821823002/diff/100001/chrome/browser/ui/webu...
chrome/browser/ui/webui/media_router/media_router_ui.cc:183:
router_->EnableMdnsDiscovery();
Prefer to move this to InitCommon

[btolsch, 2016-03-24 01:44:04]

Responding to comments before the next patch, especially to get more input from
Derek. Thanks!

https://codereview.chromium.org/1821823002/diff/100001/chrome/browser/media/a...
File chrome/browser/media/android/router/media_router_android.cc (right):

https://codereview.chromium.org/1821823002/diff/100001/chrome/browser/media/a...
chrome/browser/media/android/router/media_router_android.cc:248:
NOTIMPLEMENTED();
On 2016/03/24 00:04:41, mark a. foltz wrote:
> I don't think this is necessary, you have an implementation below?
> Unless it's a bug to call this at all in Android.

The original design called for this to only be in MediaRouterMojoImpl to avoid
having any code in Android, but testing made that difficult for now. So yes I
think this shouldn't be called on Android.

I'll let Derek have the final answer to whether this is appropriate.

https://codereview.chromium.org/1821823002/diff/100001/chrome/browser/media/a...
File chrome/browser/media/android/router/media_router_android.h (right):

https://codereview.chromium.org/1821823002/diff/100001/chrome/browser/media/a...
chrome/browser/media/android/router/media_router_android.h:68: 
On 2016/03/23 22:04:45, apacible wrote:
> nit: remove extra line here so it's obvious these two functions are from the
> MediaRouter implementation.

Done.

https://codereview.chromium.org/1821823002/diff/100001/chrome/browser/media/r...
File chrome/browser/media/router/media_router_mojo_impl.cc (right):

https://codereview.chromium.org/1821823002/diff/100001/chrome/browser/media/r...
chrome/browser/media/router/media_router_mojo_impl.cc:32: 
On 2016/03/23 22:04:45, apacible wrote:
> nit: remove newline.

Done.

https://codereview.chromium.org/1821823002/diff/100001/chrome/browser/media/r...
chrome/browser/media/router/media_router_mojo_impl.cc:90: #if defined(OS_WIN)
On 2016/03/24 00:04:41, mark a. foltz wrote:
> Please put this into its own function in an anonymous namespace, i.e.
> CanFirewallUseLocalPorts()
> 
> Also, I would not be opposed to putting it into its own file as well to keep
the
> include list for this file smaller (maybe in media_route_provider_util.cc).

Done.

https://codereview.chromium.org/1821823002/diff/100001/chrome/browser/media/r...
chrome/browser/media/router/media_router_mojo_impl.cc:91: base::FilePath
exe_path;
On 2016/03/24 01:17:19, imcheng wrote:
> Ideally this check should happen in FILE thread, see:
> https://bugs.chromium.org/p/chromium/issues/detail?id=593167#c68
> 
> That would mean is_mdns_discovery_enabled_ won't be set to true on
construction
> time, but I think that is fine.

How do I run this from a file thread?

https://codereview.chromium.org/1821823002/diff/100001/chrome/browser/media/r...
chrome/browser/media/router/media_router_mojo_impl.cc:99: #endif
On 2016/03/24 00:04:41, mark a. foltz wrote:
> What if !defined(OS_WIN)?  Should we set the flag to true so we can ensure
mDNS
> is enabled on startup?

If !defined(OS_WIN) then the extension will already have enabled mDNS.

https://codereview.chromium.org/1821823002/diff/100001/chrome/browser/media/r...
chrome/browser/media/router/media_router_mojo_impl.cc:176: if
(is_mdns_discovery_enabled_) {
On 2016/03/24 01:17:19, imcheng wrote:
> On 2016/03/24 00:04:41, mark a. foltz wrote:
> > Did you mean !is_mdns_discovery_enabled_?
> > 
> > Nit: Remove { }
> 
> I think this is correct. |is_mdns_discovery_enabled_| really means
> |should_enable_mdns_discovery_|. Perhaps it should be renamed as such.

I think this (and the renaming) would be correct if the extension weren't saving
its mDNS flag on suspension. Now that it is saving the flag itself and doesn't
need to be reminded every time it re-registers with the MR, I think the flag
could keep its name and this could become !is_mdns_discovery_enabled_. What do
you think?

https://codereview.chromium.org/1821823002/diff/100001/chrome/browser/media/r...
chrome/browser/media/router/media_router_mojo_impl.cc:177:
media_route_provider_->EnableMdnsDiscovery();
On 2016/03/24 01:17:19, imcheng wrote:
> Since we already enable mdns discovery on the extension by default on
> non-Windows, should this code be Windows only?

Yes, this could also be Windows-only. Done.

https://codereview.chromium.org/1821823002/diff/100001/chrome/browser/media/r...
chrome/browser/media/router/media_router_mojo_impl.cc:429: if
(media_route_provider_) {
On 2016/03/24 00:04:41, mark a. foltz wrote:
> What if is_mdns_discovery_enabled_ is already true?
> 
> Nit: Remove { } but leave newline after if.

I think I wrote this because the extension wasn't saving the mDNS flag on
suspension so this was also being called when the extension wakes up to restore
the flag. Now, however, the extension is saving that flag, so I don't think this
is necessary.

https://codereview.chromium.org/1821823002/diff/100001/chrome/browser/media/r...
File chrome/browser/media/router/media_router_mojo_impl.h (right):

https://codereview.chromium.org/1821823002/diff/100001/chrome/browser/media/r...
chrome/browser/media/router/media_router_mojo_impl.h:101: 
On 2016/03/23 22:04:45, apacible wrote:
> nit: same comment on removing newlines.

Done.

https://codereview.chromium.org/1821823002/diff/100001/chrome/browser/media/r...
chrome/browser/media/router/media_router_mojo_impl.h:388: bool
is_mdns_discovery_enabled_ = false;
On 2016/03/24 01:17:19, imcheng wrote:
> This should be true for non-Windows.

Done.

https://codereview.chromium.org/1821823002/diff/100001/chrome/browser/media/r...
File chrome/browser/media/router/test_helper.h (right):

https://codereview.chromium.org/1821823002/diff/100001/chrome/browser/media/r...
chrome/browser/media/router/test_helper.h:153: 
On 2016/03/23 22:04:45, apacible wrote:
> nit: remove newline.

Done.

https://codereview.chromium.org/1821823002/diff/100001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/media_router/media_router_ui.cc (right):

https://codereview.chromium.org/1821823002/diff/100001/chrome/browser/ui/webu...
chrome/browser/ui/webui/media_router/media_router_ui.cc:183:
router_->EnableMdnsDiscovery();
On 2016/03/24 01:17:20, imcheng wrote:
> Prefer to move this to InitCommon

Done.

[imcheng, 2016-03-24 02:29:31]

https://codereview.chromium.org/1821823002/diff/100001/chrome/browser/media/r...
File chrome/browser/media/router/media_router_mojo_impl.cc (right):

https://codereview.chromium.org/1821823002/diff/100001/chrome/browser/media/r...
chrome/browser/media/router/media_router_mojo_impl.cc:91: base::FilePath
exe_path;
On 2016/03/24 01:44:03, btolsch wrote:
> On 2016/03/24 01:17:19, imcheng wrote:
> > Ideally this check should happen in FILE thread, see:
> > https://bugs.chromium.org/p/chromium/issues/detail?id=593167#c68
> > 
> > That would mean is_mdns_discovery_enabled_ won't be set to true on
> construction
> > time, but I think that is fine.
> 
> How do I run this from a file thread?

You can use BrowserThread::PostTask, see example here:
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext...

Also, once you find out that CanUseLocalPorts is true, you should post a task to
enqueue a request to enable mdns on the extension on the UI thread.

https://codereview.chromium.org/1821823002/diff/100001/chrome/browser/media/r...
chrome/browser/media/router/media_router_mojo_impl.cc:176: if
(is_mdns_discovery_enabled_) {
On 2016/03/24 01:44:03, btolsch wrote:
> On 2016/03/24 01:17:19, imcheng wrote:
> > On 2016/03/24 00:04:41, mark a. foltz wrote:
> > > Did you mean !is_mdns_discovery_enabled_?
> > > 
> > > Nit: Remove { }
> > 
> > I think this is correct. |is_mdns_discovery_enabled_| really means
> > |should_enable_mdns_discovery_|. Perhaps it should be renamed as such.
> 
> I think this (and the renaming) would be correct if the extension weren't
saving
> its mDNS flag on suspension. Now that it is saving the flag itself and doesn't
> need to be reminded every time it re-registers with the MR, I think the flag
> could keep its name and this could become !is_mdns_discovery_enabled_. What do
> you think?

Actually, it does need to be reminded on every wakeup, because extension updates
will wipe out the mdns_enabled flag on the extension. So the solution here is to
always call EnableMdnsDiscovery() on wakeup.

[btolsch, 2016-03-24 21:32:26]

I responded to comments and moved the firewall function into a separate file. 
However, now I'm seeing a problem with tests on Windows related to
BrowserThread::PostTask and something not being initialized.

+wez in cause he has time to look at the thread issue.

https://codereview.chromium.org/1821823002/diff/100001/chrome/browser/media/r...
File chrome/browser/media/router/media_router_mojo_impl.cc (right):

https://codereview.chromium.org/1821823002/diff/100001/chrome/browser/media/r...
chrome/browser/media/router/media_router_mojo_impl.cc:91: base::FilePath
exe_path;
On 2016/03/24 02:29:31, imcheng wrote:
> On 2016/03/24 01:44:03, btolsch wrote:
> > On 2016/03/24 01:17:19, imcheng wrote:
> > > Ideally this check should happen in FILE thread, see:
> > > https://bugs.chromium.org/p/chromium/issues/detail?id=593167#c68
> > > 
> > > That would mean is_mdns_discovery_enabled_ won't be set to true on
> > construction
> > > time, but I think that is fine.
> > 
> > How do I run this from a file thread?
> 
> You can use BrowserThread::PostTask, see example here:
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext...
> 
> Also, once you find out that CanUseLocalPorts is true, you should post a task
to
> enqueue a request to enable mdns on the extension on the UI thread.

Done but currently this is making tests on Windows fail, possibly because it is
using BrowserThread::PostTask without some sort of required initialization.
Specifically ThreadTaskRunnerHandle doesn't seem to ever be initialized, but I'm
not sure how to go about fixing this.

https://codereview.chromium.org/1821823002/diff/100001/chrome/browser/media/r...
chrome/browser/media/router/media_router_mojo_impl.cc:429: if
(media_route_provider_) {
On 2016/03/24 01:44:03, btolsch wrote:
> On 2016/03/24 00:04:41, mark a. foltz wrote:
> > What if is_mdns_discovery_enabled_ is already true?
> > 
> > Nit: Remove { } but leave newline after if.
> 
> I think I wrote this because the extension wasn't saving the mDNS flag on
> suspension so this was also being called when the extension wakes up to
restore
> the flag. Now, however, the extension is saving that flag, so I don't think
this
> is necessary.

Per Derek's comment about extension upgrade, this is still necessary.

[mark a. foltz, 2016-03-24 22:59:33]

Getting there.  The test failures look like a failure to initialize a task
runner for the FILE thread.  Try adding the following to the test class; I don't
think it should affect the other MessageLoops that are used in the test.  I'd
eventually like get rid of the manual message loop management there.

https://code.google.com/p/chromium/codesearch#chromium/src/content/public/tes...

https://codereview.chromium.org/1821823002/diff/140001/chrome/browser/media/r...
File chrome/browser/media/router/media_route_provider_util.cc (right):

https://codereview.chromium.org/1821823002/diff/140001/chrome/browser/media/r...
chrome/browser/media/router/media_route_provider_util.cc:11: #include
"chrome/installer/util/browser_distribution.h"
Do you need to declare any additional deps= in the .gn/.gyp files for the build/
or installer/ libraries?

https://codereview.chromium.org/1821823002/diff/140001/chrome/browser/media/r...
File chrome/browser/media/router/media_route_provider_util.h (right):

https://codereview.chromium.org/1821823002/diff/140001/chrome/browser/media/r...
chrome/browser/media/router/media_route_provider_util.h:12: #if defined(OS_WIN)
If you name these media_router_provider_util_win.{h,cc}, then they will only be
included in the Windows build.

https://gyp.gsrc.io/docs/UserDocumentation.md#Add-new-source-files

https://codereview.chromium.org/1821823002/diff/140001/chrome/browser/media/r...
chrome/browser/media/router/media_route_provider_util.h:13: void
CanFirewallUseLocalPorts(MediaRouterMojoImpl* router);
It's not a good pattern to pass a pointer to the caller call back on it
directly.  Instead have the caller pass a base::Closure and have this method
call .Run() in it.

From an API point of view, I would have passed a base::Callback<void(bool)> and
have the caller decide how to handle the positive or negative result (versus
requiring the negative case be a no-op).  But I don't feel strongly since this
is only used in one place.

https://codereview.chromium.org/1821823002/diff/140001/chrome/browser/media/r...
File chrome/browser/media/router/media_router.mojom (right):

https://codereview.chromium.org/1821823002/diff/140001/chrome/browser/media/r...
chrome/browser/media/router/media_router.mojom:285: // Enables mDNS discovery.
No-ops if mDNS discovery is already enabled.
Nit: s/no-ops/no-op/

https://codereview.chromium.org/1821823002/diff/140001/chrome/browser/media/r...
File chrome/browser/media/router/media_router_mojo_impl.cc (right):

https://codereview.chromium.org/1821823002/diff/140001/chrome/browser/media/r...
chrome/browser/media/router/media_router_mojo_impl.cc:417: return
should_enable_mdns_discovery_;
This can be inlined into the .h

https://codereview.chromium.org/1821823002/diff/140001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/media_router/media_router_ui.cc (right):

https://codereview.chromium.org/1821823002/diff/140001/chrome/browser/ui/webu...
chrome/browser/ui/webui/media_router/media_router_ui.cc:255: if
(!router_->should_enable_mdns_discovery()) {
IMO, this is confusing to read.

One solution:

1. The router could keep track of whether EnableMdnsDiscovery has already been
called on the MRPM by adding an internal flag
2. The method below is renamed to EnsureMdnsDiscoveryEnabled() (no-op if the
flag in #1 is true)
3. Just call EnsureMdnsDiscoveryEnabled() here.

[mark a. foltz, 2016-03-24 23:02:10]

https://codereview.chromium.org/1821823002/diff/140001/chrome/browser/media/r...
File chrome/browser/media/router/media_route_provider_util.h (right):

https://codereview.chromium.org/1821823002/diff/140001/chrome/browser/media/r...
chrome/browser/media/router/media_route_provider_util.h:13: void
CanFirewallUseLocalPorts(MediaRouterMojoImpl* router);
On 2016/03/24 at 22:59:33, mark a. foltz wrote:
> It's not a good pattern to pass a pointer to the caller call back on it
directly.  Instead have the caller pass a base::Closure and have this method
call .Run() in it.

Correction:  Have this method post a task to run it :)

> From an API point of view, I would have passed a base::Callback<void(bool)>
and have the caller decide how to handle the positive or negative result (versus
requiring the negative case be a no-op).  But I don't feel strongly since this
is only used in one place.

[btolsch, 2016-03-25 04:40:50]

TestBrowserThreadBundle fixed the original problem once it was put in the right
place (not MediaRouterMojoImplTest) but it also conflicted with the existing
MessageLoop in MediaRouterMojoTest. To fix it, I changed it to a RunLoop that
resets after every Run*() call. PTAL, thanks!

https://codereview.chromium.org/1821823002/diff/140001/chrome/browser/media/r...
File chrome/browser/media/router/media_route_provider_util.cc (right):

https://codereview.chromium.org/1821823002/diff/140001/chrome/browser/media/r...
chrome/browser/media/router/media_route_provider_util.cc:11: #include
"chrome/installer/util/browser_distribution.h"
On 2016/03/24 22:59:32, mark a. foltz wrote:
> Do you need to declare any additional deps= in the .gn/.gyp files for the
build/
> or installer/ libraries?

I didn't think so because nothing has been failing to build/link. How can I tell
if I need to add something?

https://codereview.chromium.org/1821823002/diff/140001/chrome/browser/media/r...
File chrome/browser/media/router/media_route_provider_util.h (right):

https://codereview.chromium.org/1821823002/diff/140001/chrome/browser/media/r...
chrome/browser/media/router/media_route_provider_util.h:12: #if defined(OS_WIN)
On 2016/03/24 22:59:32, mark a. foltz wrote:
> If you name these media_router_provider_util_win.{h,cc}, then they will only
be
> included in the Windows build.
> 
> https://gyp.gsrc.io/docs/UserDocumentation.md#Add-new-source-files
> 
> 

Done but should I leave the include in media_router_mojo_impl.cc or guard that
with another #if defined(OS_WIN)?

https://codereview.chromium.org/1821823002/diff/140001/chrome/browser/media/r...
chrome/browser/media/router/media_route_provider_util.h:13: void
CanFirewallUseLocalPorts(MediaRouterMojoImpl* router);
On 2016/03/24 22:59:33, mark a. foltz wrote:
> It's not a good pattern to pass a pointer to the caller call back on it
> directly.  Instead have the caller pass a base::Closure and have this method
> call .Run() in it.
> 
> From an API point of view, I would have passed a base::Callback<void(bool)>
and
> have the caller decide how to handle the positive or negative result (versus
> requiring the negative case be a no-op).  But I don't feel strongly since this
> is only used in one place.

Done.

https://codereview.chromium.org/1821823002/diff/140001/chrome/browser/media/r...
File chrome/browser/media/router/media_router.mojom (right):

https://codereview.chromium.org/1821823002/diff/140001/chrome/browser/media/r...
chrome/browser/media/router/media_router.mojom:285: // Enables mDNS discovery.
No-ops if mDNS discovery is already enabled.
On 2016/03/24 22:59:33, mark a. foltz wrote:
> Nit: s/no-ops/no-op/

Done.

https://codereview.chromium.org/1821823002/diff/140001/chrome/browser/media/r...
File chrome/browser/media/router/media_router_mojo_impl.cc (right):

https://codereview.chromium.org/1821823002/diff/140001/chrome/browser/media/r...
chrome/browser/media/router/media_router_mojo_impl.cc:417: return
should_enable_mdns_discovery_;
On 2016/03/24 22:59:33, mark a. foltz wrote:
> This can be inlined into the .h

This is a virtual and can't be defined inline.

https://codereview.chromium.org/1821823002/diff/140001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/media_router/media_router_ui.cc (right):

https://codereview.chromium.org/1821823002/diff/140001/chrome/browser/ui/webu...
chrome/browser/ui/webui/media_router/media_router_ui.cc:255: if
(!router_->should_enable_mdns_discovery()) {
On 2016/03/24 22:59:33, mark a. foltz wrote:
> IMO, this is confusing to read.
> 
> One solution:
> 
> 1. The router could keep track of whether EnableMdnsDiscovery has already been
> called on the MRPM by adding an internal flag
> 2. The method below is renamed to EnsureMdnsDiscoveryEnabled() (no-op if the
> flag in #1 is true)
> 3. Just call EnsureMdnsDiscoveryEnabled() here.
> 
> 
>  

I thought about putting a comment here since that if reads pretty badly, but I
like this better.

[imcheng, 2016-03-25 06:26:30]

https://codereview.chromium.org/1821823002/diff/220001/chrome/browser/media/r...
File chrome/browser/media/router/media_route_provider_util_win.cc (right):

https://codereview.chromium.org/1821823002/diff/220001/chrome/browser/media/r...
chrome/browser/media/router/media_route_provider_util_win.cc:18: void
DoCanFirewallUseLocalPorts(base::Callback<void(bool)> callback) {
const ref input

https://codereview.chromium.org/1821823002/diff/220001/chrome/browser/media/r...
chrome/browser/media/router/media_route_provider_util_win.cc:29:
base::Bind(std::move(callback), can_use_local_ports));
is the std::move necessary?

https://codereview.chromium.org/1821823002/diff/220001/chrome/browser/media/r...
chrome/browser/media/router/media_route_provider_util_win.cc:34: void
CanFirewallUseLocalPorts(base::Callback<void(bool)> callback) {
const ref input

https://codereview.chromium.org/1821823002/diff/220001/chrome/browser/media/r...
chrome/browser/media/router/media_route_provider_util_win.cc:37:
base::Bind(&DoCanFirewallUseLocalPorts, std::move(callback)));
is std::move necessary?

https://codereview.chromium.org/1821823002/diff/220001/chrome/browser/media/r...
File chrome/browser/media/router/media_route_provider_util_win.h (right):

https://codereview.chromium.org/1821823002/diff/220001/chrome/browser/media/r...
chrome/browser/media/router/media_route_provider_util_win.h:9: #include
"chrome/browser/media/router/media_router_mojo_impl.h"
rm this include

https://codereview.chromium.org/1821823002/diff/220001/chrome/browser/media/r...
chrome/browser/media/router/media_route_provider_util_win.h:13: void
CanFirewallUseLocalPorts(base::Callback<void(bool)> callback);
please add some comments for this function.

https://codereview.chromium.org/1821823002/diff/220001/chrome/browser/media/r...
chrome/browser/media/router/media_route_provider_util_win.h:13: void
CanFirewallUseLocalPorts(base::Callback<void(bool)> callback);
use const ref for input

https://codereview.chromium.org/1821823002/diff/220001/chrome/browser/media/r...
File chrome/browser/media/router/media_router.gypi (right):

https://codereview.chromium.org/1821823002/diff/220001/chrome/browser/media/r...
chrome/browser/media/router/media_router.gypi:53:
'media_route_provider_util_win.cc',
I think these files should be in 'media_router_win_sources', then in the
media_router target in media_router.gyp and BUILD.gn, include it if os == win.

https://codereview.chromium.org/1821823002/diff/220001/chrome/browser/media/r...
File chrome/browser/media/router/media_router_mojo_impl.cc (right):

https://codereview.chromium.org/1821823002/diff/220001/chrome/browser/media/r...
chrome/browser/media/router/media_router_mojo_impl.cc:424:
media_route_provider_->EnableMdnsDiscovery();
So when this is called, it is possible that the MR extension is not loaded (with
exception being RegisterMediaRouteProvider). You will need to queue a task and
wake up the extension. See
RegisterMediaSinksObserver for example.

https://codereview.chromium.org/1821823002/diff/220001/chrome/browser/media/r...
File chrome/browser/media/router/media_router_mojo_impl.h (right):

https://codereview.chromium.org/1821823002/diff/220001/chrome/browser/media/r...
chrome/browser/media/router/media_router_mojo_impl.h:392: bool
should_enable_mdns_discovery_ = true;
I would prefer that we define this value to be false for Windows here rather
than in the constructor.

[btolsch, 2016-03-25 18:57:04]

I addressed Derek's comments on the callback and build process, as well as other
small things. PTAL, thanks!

https://codereview.chromium.org/1821823002/diff/220001/chrome/browser/media/r...
File chrome/browser/media/router/media_route_provider_util_win.cc (right):

https://codereview.chromium.org/1821823002/diff/220001/chrome/browser/media/r...
chrome/browser/media/router/media_route_provider_util_win.cc:18: void
DoCanFirewallUseLocalPorts(base::Callback<void(bool)> callback) {
On 2016/03/25 06:26:30, imcheng wrote:
> const ref input

Done.

https://codereview.chromium.org/1821823002/diff/220001/chrome/browser/media/r...
chrome/browser/media/router/media_route_provider_util_win.cc:29:
base::Bind(std::move(callback), can_use_local_ports));
On 2016/03/25 06:26:30, imcheng wrote:
> is the std::move necessary?

A base::Callback is basically a pointer and a shared pointer. So passing by
value and moving is similar to passing by const ref and copying. I don't know
whether base::Bind takes advantage of rvalues of the function call though, and
since you don't like it I'll change it.

https://codereview.chromium.org/1821823002/diff/220001/chrome/browser/media/r...
chrome/browser/media/router/media_route_provider_util_win.cc:34: void
CanFirewallUseLocalPorts(base::Callback<void(bool)> callback) {
On 2016/03/25 06:26:30, imcheng wrote:
> const ref input

Done.

https://codereview.chromium.org/1821823002/diff/220001/chrome/browser/media/r...
chrome/browser/media/router/media_route_provider_util_win.cc:37:
base::Bind(&DoCanFirewallUseLocalPorts, std::move(callback)));
On 2016/03/25 06:26:30, imcheng wrote:
> is std::move necessary?

See other comment.

https://codereview.chromium.org/1821823002/diff/220001/chrome/browser/media/r...
File chrome/browser/media/router/media_route_provider_util_win.h (right):

https://codereview.chromium.org/1821823002/diff/220001/chrome/browser/media/r...
chrome/browser/media/router/media_route_provider_util_win.h:9: #include
"chrome/browser/media/router/media_router_mojo_impl.h"
On 2016/03/25 06:26:30, imcheng wrote:
> rm this include

Done.

https://codereview.chromium.org/1821823002/diff/220001/chrome/browser/media/r...
chrome/browser/media/router/media_route_provider_util_win.h:13: void
CanFirewallUseLocalPorts(base::Callback<void(bool)> callback);
On 2016/03/25 06:26:30, imcheng wrote:
> use const ref for input

Done.

https://codereview.chromium.org/1821823002/diff/220001/chrome/browser/media/r...
chrome/browser/media/router/media_route_provider_util_win.h:13: void
CanFirewallUseLocalPorts(base::Callback<void(bool)> callback);
On 2016/03/25 06:26:30, imcheng wrote:
> please add some comments for this function.

Done.

https://codereview.chromium.org/1821823002/diff/220001/chrome/browser/media/r...
File chrome/browser/media/router/media_router.gypi (right):

https://codereview.chromium.org/1821823002/diff/220001/chrome/browser/media/r...
chrome/browser/media/router/media_router.gypi:53:
'media_route_provider_util_win.cc',
On 2016/03/25 06:26:30, imcheng wrote:
> I think these files should be in 'media_router_win_sources', then in the
> media_router target in media_router.gyp and BUILD.gn, include it if os == win.

Done.

https://codereview.chromium.org/1821823002/diff/220001/chrome/browser/media/r...
File chrome/browser/media/router/media_router_mojo_impl.cc (right):

https://codereview.chromium.org/1821823002/diff/220001/chrome/browser/media/r...
chrome/browser/media/router/media_router_mojo_impl.cc:424:
media_route_provider_->EnableMdnsDiscovery();
On 2016/03/25 06:26:30, imcheng wrote:
> So when this is called, it is possible that the MR extension is not loaded
(with
> exception being RegisterMediaRouteProvider). You will need to queue a task and
> wake up the extension. See
> RegisterMediaSinksObserver for example.

Done.

https://codereview.chromium.org/1821823002/diff/220001/chrome/browser/media/r...
File chrome/browser/media/router/media_router_mojo_impl.h (right):

https://codereview.chromium.org/1821823002/diff/220001/chrome/browser/media/r...
chrome/browser/media/router/media_router_mojo_impl.h:392: bool
should_enable_mdns_discovery_ = true;
On 2016/03/25 06:26:30, imcheng wrote:
> I would prefer that we define this value to be false for Windows here rather
> than in the constructor.

Done.

[mark a. foltz, 2016-03-25 23:14:17]

https://codereview.chromium.org/1821823002/diff/240001/chrome/browser/media/r...
File chrome/browser/media/router/media_router.gypi (right):

https://codereview.chromium.org/1821823002/diff/240001/chrome/browser/media/r...
chrome/browser/media/router/media_router.gypi:58: 'media_router_win_sources': [
This shouldn't be necessary.  The build system will filter out these files for
non-Windows builds.

https://codereview.chromium.org/1821823002/diff/240001/chrome/browser/media/r...
File chrome/browser/media/router/media_router.h (right):

https://codereview.chromium.org/1821823002/diff/240001/chrome/browser/media/r...
chrome/browser/media/router/media_router.h:158: virtual bool
should_enable_mdns_discovery() const = 0;
But the method sounds like mdns will be enabled in the future?  Consider naming
this is_mdns_enabled()

https://codereview.chromium.org/1821823002/diff/240001/chrome/browser/media/r...
chrome/browser/media/router/media_router.h:162: virtual void
EnsureMdnsDiscoveryEnabled() = 0;
I don't feel great about exposing this API at the MediaRouter level.  It's
really an API to trigger a specific codepath on a specific platform (Windows) in
response to something happening in the UI.

Maybe there's a slightly more general way to think about this flow, like naming
it OnMediaSinkSelection() or OnUserInteraction(), which is implemented by
enabling mDNS if necessary.  

I don't want to derail this into a bikeshed, but consider adding a TODO() to
make this API more generic if we find other use cases that follow the same
pattern.

https://codereview.chromium.org/1821823002/diff/240001/chrome/browser/media/r...
File chrome/browser/media/router/media_router.mojom (right):

https://codereview.chromium.org/1821823002/diff/240001/chrome/browser/media/r...
chrome/browser/media/router/media_router.mojom:288: EnableMdnsDiscovery();
You'll need to update media_router_bindings.js to set the JS callback passed
from the MRPM.

https://codereview.chromium.org/1821823002/diff/240001/chrome/browser/media/r...
File chrome/browser/media/router/media_router_mojo_impl.cc (right):

https://codereview.chromium.org/1821823002/diff/240001/chrome/browser/media/r...
chrome/browser/media/router/media_router_mojo_impl.cc:85: #if defined(OS_WIN)
Prefer to check initial vale of should_ here instead of #ifdef

https://codereview.chromium.org/1821823002/diff/240001/chrome/browser/media/r...
chrome/browser/media/router/media_router_mojo_impl.cc:88:
base::Unretained(this)));
There's two design issues here (first is mandatory to fix).

- Unless you can guarantee that the task posted to the FILE thread will be
cancelled before |this| is destroyed, it could create a use after free when the
browser context is being shut down.  Option (1) is to store the callback in a
member and explicitly cancel it in the dtor.  Option (2) is to add a
WeakPtrFactory to the MediaRouterMojoImpl and use that to generate a weak
pointer to pass to the callback.  I generally prefer (1) 

- In principle, this check needs to be done only once per browser process (we're
not trying to handle firewall changes at runtime).  Since each active browser
profile will instantiate its own MediaRouterMojoImpl, there will be some extra
checks for users with multiple profiles.  Not a big deal since few users do
that, but maybe worth noting.  (One fix would be to move this check to
MediaRouterFactory (?) where it is cached and passed to each instance, but that
adds complexity for a little bit of performance gain.)

https://codereview.chromium.org/1821823002/diff/240001/chrome/browser/media/r...
chrome/browser/media/router/media_router_mojo_impl.cc:166: #if defined(OS_WIN)
Why only for Windows? Are we assuming that the MRPM has already enabled MDNS? 
Please add a comment explaining this.

https://codereview.chromium.org/1821823002/diff/240001/chrome/browser/media/r...
chrome/browser/media/router/media_router_mojo_impl.cc:168:
media_route_provider_->EnableMdnsDiscovery();
There's a code path L149-158 where the event page is not yet awake when the
registration request is processed.  In that case, this code won't be run.

https://codereview.chromium.org/1821823002/diff/240001/chrome/browser/media/r...
chrome/browser/media/router/media_router_mojo_impl.cc:421:
SetWakeReason(MediaRouteProviderWakeReason::ENABLE_MDNS_DISCOVERY);
I'm not sure what this method should do when should_ == true.  How do you
prevent Enable...() from being called multiple times?

https://codereview.chromium.org/1821823002/diff/240001/chrome/browser/media/r...
chrome/browser/media/router/media_router_mojo_impl.cc:850:
media_route_provider_->EnableMdnsDiscovery();
Another task could have already called this.

https://codereview.chromium.org/1821823002/diff/240001/chrome/browser/media/r...
chrome/browser/media/router/media_router_mojo_impl.cc:962: if
(firewall_can_use_local_ports) {
Nit: Remove { }

[btolsch, 2016-03-26 10:53:59]

I addressed Mark's latest comments and added a test. Unfortunately the test is
littered with #if defined(OS_WIN) since MediaRouterMojoImpl triggers different
calls on Windows. PTAL, thanks!

https://codereview.chromium.org/1821823002/diff/240001/chrome/browser/media/r...
File chrome/browser/media/router/media_router.gypi (right):

https://codereview.chromium.org/1821823002/diff/240001/chrome/browser/media/r...
chrome/browser/media/router/media_router.gypi:58: 'media_router_win_sources': [
On 2016/03/25 23:14:16, mark a. foltz wrote:
> This shouldn't be necessary.  The build system will filter out these files for
> non-Windows builds.

Ok, conflicting advice. I moved these back to media_router_non_android_sources.

https://codereview.chromium.org/1821823002/diff/240001/chrome/browser/media/r...
File chrome/browser/media/router/media_router.h (right):

https://codereview.chromium.org/1821823002/diff/240001/chrome/browser/media/r...
chrome/browser/media/router/media_router.h:158: virtual bool
should_enable_mdns_discovery() const = 0;
On 2016/03/25 23:14:16, mark a. foltz wrote:
> But the method sounds like mdns will be enabled in the future?  Consider
naming
> this is_mdns_enabled()

Due to other changes, it turns out this is no longer needed so I removed it.
This also slightly cleans up the Android and test code that was forced to
"implement" these methods.

https://codereview.chromium.org/1821823002/diff/240001/chrome/browser/media/r...
chrome/browser/media/router/media_router.h:162: virtual void
EnsureMdnsDiscoveryEnabled() = 0;
On 2016/03/25 23:14:16, mark a. foltz wrote:
> I don't feel great about exposing this API at the MediaRouter level.  It's
> really an API to trigger a specific codepath on a specific platform (Windows)
in
> response to something happening in the UI.
> 
> Maybe there's a slightly more general way to think about this flow, like
naming
> it OnMediaSinkSelection() or OnUserInteraction(), which is implemented by
> enabling mDNS if necessary.  
> 
> I don't want to derail this into a bikeshed, but consider adding a TODO() to
> make this API more generic if we find other use cases that follow the same
> pattern.

I think that since should_enable_mdns_discovery() was removed it is simple to
rename this to OnUserInteraction() now.

https://codereview.chromium.org/1821823002/diff/240001/chrome/browser/media/r...
File chrome/browser/media/router/media_router.mojom (right):

https://codereview.chromium.org/1821823002/diff/240001/chrome/browser/media/r...
chrome/browser/media/router/media_router.mojom:288: EnableMdnsDiscovery();
On 2016/03/25 23:14:16, mark a. foltz wrote:
> You'll need to update media_router_bindings.js to set the JS callback passed
> from the MRPM.

Done.

https://codereview.chromium.org/1821823002/diff/240001/chrome/browser/media/r...
File chrome/browser/media/router/media_router_mojo_impl.cc (right):

https://codereview.chromium.org/1821823002/diff/240001/chrome/browser/media/r...
chrome/browser/media/router/media_router_mojo_impl.cc:85: #if defined(OS_WIN)
On 2016/03/25 23:14:16, mark a. foltz wrote:
> Prefer to check initial vale of should_ here instead of #ifdef

There is nothing for this function call to link to on other platforms. I could
add a stub that just calls the callback with true but I think that obfuscates
things more than the #if. Let me know how you feel about that.

https://codereview.chromium.org/1821823002/diff/240001/chrome/browser/media/r...
chrome/browser/media/router/media_router_mojo_impl.cc:88:
base::Unretained(this)));
On 2016/03/25 23:14:16, mark a. foltz wrote:
> There's two design issues here (first is mandatory to fix).
> 
> - Unless you can guarantee that the task posted to the FILE thread will be
> cancelled before |this| is destroyed, it could create a use after free when
the
> browser context is being shut down.  Option (1) is to store the callback in a
> member and explicitly cancel it in the dtor.  Option (2) is to add a
> WeakPtrFactory to the MediaRouterMojoImpl and use that to generate a weak
> pointer to pass to the callback.  I generally prefer (1) 
> 
> - In principle, this check needs to be done only once per browser process
(we're
> not trying to handle firewall changes at runtime).  Since each active browser
> profile will instantiate its own MediaRouterMojoImpl, there will be some extra
> checks for users with multiple profiles.  Not a big deal since few users do
> that, but maybe worth noting.  (One fix would be to move this check to
> MediaRouterFactory (?) where it is cached and passed to each instance, but
that
> adds complexity for a little bit of performance gain.)

MediaRouterMojoImpl already has a WeakPtrFactory for other similar callback
cases. I will reuse that to implement (2).

https://codereview.chromium.org/1821823002/diff/240001/chrome/browser/media/r...
chrome/browser/media/router/media_router_mojo_impl.cc:166: #if defined(OS_WIN)
On 2016/03/25 23:14:16, mark a. foltz wrote:
> Why only for Windows? Are we assuming that the MRPM has already enabled MDNS? 
> Please add a comment explaining this.

Yes, the extension is also aware of whether it's on Windows or not and enables
mDNS accordingly. I added a comment here. I think it would be cleaner but less
efficient to remove that logic from the extension. I can leave a TODO to
consider that if you think it would be worthwhile.

https://codereview.chromium.org/1821823002/diff/240001/chrome/browser/media/r...
chrome/browser/media/router/media_router_mojo_impl.cc:168:
media_route_provider_->EnableMdnsDiscovery();
On 2016/03/25 23:14:16, mark a. foltz wrote:
> There's a code path L149-158 where the event page is not yet awake when the
> registration request is processed.  In that case, this code won't be run.

Doesn't this path get retried once the extension wakes up though? So this will
execute once the extension is awake, which is the same as queueing it. Am I
missing something?

https://codereview.chromium.org/1821823002/diff/240001/chrome/browser/media/r...
chrome/browser/media/router/media_router_mojo_impl.cc:421:
SetWakeReason(MediaRouteProviderWakeReason::ENABLE_MDNS_DISCOVERY);
On 2016/03/25 23:14:16, mark a. foltz wrote:
> I'm not sure what this method should do when should_ == true.  How do you
> prevent Enable...() from being called multiple times?

I'm not preventing it from being called multiple times since calling it multiple
times isn't a correctness issue. I added another state variable to indicate
whether we have already told the extension since it has been registered. If this
is too much noise then I think the only option is to leave it like this.

https://codereview.chromium.org/1821823002/diff/240001/chrome/browser/media/r...
chrome/browser/media/router/media_router_mojo_impl.cc:850:
media_route_provider_->EnableMdnsDiscovery();
On 2016/03/25 23:14:16, mark a. foltz wrote:
> Another task could have already called this.

See the comment above.

https://codereview.chromium.org/1821823002/diff/240001/chrome/browser/media/r...
chrome/browser/media/router/media_router_mojo_impl.cc:962: if
(firewall_can_use_local_ports) {
On 2016/03/25 23:14:16, mark a. foltz wrote:
> Nit: Remove { }

Done.

[mark a. foltz, 2016-03-29 00:48:20]

LGTM % minor comments, thanks for patience w/multiple rounds of review.  I think
imcheng@ is offline this week so don't wait for his approval.

IIUC the code path with delayed initialization of mDNS is only used on Windows. 
I would be okay with surrounding the relevant private methods, members, and
tests with #if defined(OS_WIN) so we don't have to think about how this code
behaves in the non-Windows case.  If it gets really ugly we can think about
creating media_router_mojo_impl_win.cc but I don't think that's necessary yet.

https://codereview.chromium.org/1821823002/diff/320001/chrome/browser/media/r...
File chrome/browser/media/router/media_route_provider_util_win.cc (right):

https://codereview.chromium.org/1821823002/diff/320001/chrome/browser/media/r...
chrome/browser/media/router/media_route_provider_util_win.cc:19: base::FilePath
exe_path;
Add a DCHECK_CURRENTLY_ON(content::BrowserThread::FILE) at the top of the fn

https://codereview.chromium.org/1821823002/diff/320001/chrome/browser/media/r...
chrome/browser/media/router/media_route_provider_util_win.cc:21: if
(base::PathService::Get(base::FILE_EXE, &exe_path)) {
If the firewall check fails to run at all, it might be good to have a way to
know that.

Consider having early returns after LOG(WARNING) if these preconditions fail
(base::PathService::Get, firewall_manager).

I don't know these implementations in detail; if it looks like these are very
rare/extremely broken situations, then maybe not worth it.

https://codereview.chromium.org/1821823002/diff/320001/chrome/browser/media/r...
File chrome/browser/media/router/media_router.h (right):

https://codereview.chromium.org/1821823002/diff/320001/chrome/browser/media/r...
chrome/browser/media/router/media_router.h:160: virtual void OnUserInteraction()
= 0;
Sorry to bikeshed, but often user initiated actions on the Web are called "user
gestures", so maybe OnUserGesture().  I don't feel strongly though.

https://codereview.chromium.org/1821823002/diff/320001/chrome/browser/media/r...
File chrome/browser/media/router/media_router_mojo_impl.cc (right):

https://codereview.chromium.org/1821823002/diff/320001/chrome/browser/media/r...
chrome/browser/media/router/media_router_mojo_impl.cc:856: void
MediaRouterMojoImpl::EnsureMdnsDiscoveryEnabled() {
This code should only be called on Windows, so I have a preference to surround
it with #if defined(OS_WIN).  To make it easier to read, put this method
alongside EnableMdnsFromFirewallCheck.

https://codereview.chromium.org/1821823002/diff/320001/chrome/browser/media/r...
File chrome/browser/media/router/media_router_mojo_impl.h (right):

https://codereview.chromium.org/1821823002/diff/320001/chrome/browser/media/r...
chrome/browser/media/router/media_router_mojo_impl.h:326: // Ensures that mDNS
discovery is enabled in the MRPM extension. This can be
Surround the mDNS functions with #if defined(OS_WIN)?
I don't see them used at all if !OS_WIN.

https://codereview.chromium.org/1821823002/diff/320001/chrome/browser/media/r...
chrome/browser/media/router/media_router_mojo_impl.h:334: void
EnableMdnsFromFirewallCheck(bool firewall_can_use_local_ports);
Perhaps name it OnFirewallCheckComplete?

https://codereview.chromium.org/1821823002/diff/320001/chrome/browser/media/r...
chrome/browser/media/router/media_router_mojo_impl.h:400: // |false| to |true|.
|is_mdns_enabled_| is set to |false| in
Maybe qualify the last sentence with "On Windows, ..."

Also |is_mdns_enabled_| is already set to false here.  Is it set again in
RegisterMediaRouteProvider?

https://codereview.chromium.org/1821823002/diff/320001/chrome/browser/media/r...
chrome/browser/media/router/media_router_mojo_impl.h:407: bool is_mdns_enabled_
= true;
Do these need to be defined at all if !OS_WIN?

https://codereview.chromium.org/1821823002/diff/320001/chrome/browser/media/r...
File chrome/browser/media/router/media_router_mojo_impl_unittest.cc (right):

https://codereview.chromium.org/1821823002/diff/320001/chrome/browser/media/r...
chrome/browser/media/router/media_router_mojo_impl_unittest.cc:1497:
TEST_F(MediaRouterMojoExtensionTest, EnableMdnsAfterEachRegister) {
How about surrounding the entire test with #if defined(OS_WIN)?

[btolsch, 2016-03-29 07:48:43]

The CQ bit was checked by btolsch@chromium.org

[btolsch, 2016-03-29 07:48:44]

The patchset sent to the CQ was uploaded after l-g-t-m from
apacible@chromium.org, amp@chromium.org, mfoltz@chromium.org
Link to the patchset: https://codereview.chromium.org/1821823002/#ps360001
(title: "Semicolon for Windows code")

[commit-bot: I haz the power, 2016-03-29 07:48:52]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1821823002/360001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1821823002/360001

[commit-bot: I haz the power, 2016-03-29 08:36:02]

Committed patchset #19 (id:360001)

[commit-bot: I haz the power, 2016-03-29 08:37:08]

Description was changed from

==========
[Media Router] Conditionally enable mDNS on Windows.

This change enables mDNS on Windows only when the user is in a context related
to the Media Router. Previously, a firewall prompt could be triggered on browser
startup which is confusing.

BUG=593167
R=apacible@chromium.org,amp@chromium.org
==========

to

==========
[Media Router] Conditionally enable mDNS on Windows.

This change enables mDNS on Windows only when the user is in a context related
to the Media Router. Previously, a firewall prompt could be triggered on browser
startup which is confusing.

BUG=593167
R=apacible@chromium.org,amp@chromium.org

Committed: https://crrev.com/74d51922f5dc2516ce51cb49a36c0b4180ada30e
Cr-Commit-Position: refs/heads/master@{#383689}
==========

[commit-bot: I haz the power, 2016-03-29 08:37:10]

Patchset 19 (id:??) landed as
https://crrev.com/74d51922f5dc2516ce51cb49a36c0b4180ada30e
Cr-Commit-Position: refs/heads/master@{#383689}