[Media Router] Conditionally enable mDNS on Windows.

chrome/browser/media/router/media_router_mojo_impl.cc

Index: chrome/browser/media/router/media_router_mojo_impl.cc
diff --git a/chrome/browser/media/router/media_router_mojo_impl.cc b/chrome/browser/media/router/media_router_mojo_impl.cc
index 54c50ee20c89b837ddffe385d4c6fe37d90b0fdd..4c17a0b4d40aaac695b7fdb438088d48b727fd56 100644
--- a/chrome/browser/media/router/media_router_mojo_impl.cc
+++ b/chrome/browser/media/router/media_router_mojo_impl.cc
@@ -8,12 +8,15 @@
 #include <utility>
 
 #include "base/bind.h"
+#include "base/files/file_path.h"
 #include "base/guid.h"
 #include "base/logging.h"
 #include "base/memory/scoped_vector.h"
 #include "base/observer_list.h"
+#include "base/path_service.h"
 #include "base/stl_util.h"
 #include "base/strings/stringprintf.h"
+#include "build/build_config.h"
 #include "chrome/browser/media/router/issues_observer.h"
 #include "chrome/browser/media/router/media_router_factory.h"
 #include "chrome/browser/media/router/media_router_metrics.h"
@@ -22,8 +25,11 @@
 #include "chrome/browser/media/router/media_sinks_observer.h"
 #include "chrome/browser/media/router/presentation_session_messages_observer.h"
 #include "chrome/browser/sessions/session_tab_helper.h"
+#include "chrome/installer/util/browser_distribution.h"
+#include "chrome/installer/util/firewall_manager_win.h"
 #include "extensions/browser/process_manager.h"
 
+

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

nit: remove newline.

[btolsch, 2016/03/24 01:44:03]

Done.

 #define DVLOG_WITH_INSTANCE(level) \
   DVLOG(level) << "MR #" << instance_id_ << ": "
 
@@ -81,6 +87,16 @@ MediaRouterMojoImpl::MediaRouterMojoImpl(
       current_wake_reason_(MediaRouteProviderWakeReason::TOTAL_COUNT),
       weak_factory_(this) {
   DCHECK(event_page_tracker_);
+#if defined(OS_WIN)

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

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).

[btolsch, 2016/03/24 01:44:03]

Done.

+  base::FilePath exe_path;

[imcheng, 2016/03/24 01:17:19]

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.

[btolsch, 2016/03/24 01:44:03]

How do I run this from a file thread?

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

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.

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

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.

+  if (base::PathService::Get(base::FILE_EXE, &exe_path)) {
+    auto firewall_manager = installer::FirewallManager::Create(
+        BrowserDistribution::GetDistribution(), exe_path);
+    if (firewall_manager) {
+      is_mdns_discovery_enabled_ = firewall_manager->CanUseLocalPorts();
+    }
+  }
+#endif

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

What if !defined(OS_WIN)?  Should we set the flag to true so we can ensure mDNS
is enabled on startup?

[btolsch, 2016/03/24 01:44:03]

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

 }
 
 MediaRouterMojoImpl::~MediaRouterMojoImpl() {
@@ -157,6 +173,9 @@ void MediaRouterMojoImpl::RegisterMediaRouteProvider(
   callback.Run(instance_id_);
   ExecutePendingRequests();
   wakeup_attempt_count_ = 0;
+  if (is_mdns_discovery_enabled_) {

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

Did you mean !is_mdns_discovery_enabled_?

Nit: Remove { }

[imcheng, 2016/03/24 01:17:19]

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

[btolsch, 2016/03/24 01:44:03]

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?

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

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.

+    media_route_provider_->EnableMdnsDiscovery();

[imcheng, 2016/03/24 01:17:19]

Since we already enable mdns discovery on the extension by default on
non-Windows, should this code be Windows only?

[btolsch, 2016/03/24 01:44:03]

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

+  }
 }
 
 void MediaRouterMojoImpl::OnIssue(const interfaces::IssuePtr issue) {
@@ -402,6 +421,17 @@ void MediaRouterMojoImpl::ClearIssue(const Issue::Id& issue_id) {
   issue_manager_.ClearIssue(issue_id);
 }
 
+bool MediaRouterMojoImpl::is_mdns_discovery_enabled() const {
+  return is_mdns_discovery_enabled_;
+}
+
+void MediaRouterMojoImpl::EnableMdnsDiscovery() {
+  if (media_route_provider_) {

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

What if is_mdns_discovery_enabled_ is already true?

Nit: Remove { } but leave newline after if.

[btolsch, 2016/03/24 01:44:03]

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.

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

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

+    media_route_provider_->EnableMdnsDiscovery();
+  }
+  is_mdns_discovery_enabled_ = true;
+}
+
 bool MediaRouterMojoImpl::RegisterMediaSinksObserver(
     MediaSinksObserver* observer) {
   DCHECK(thread_checker_.CalledOnValidThread());