Bind open firewall ports to visible application windows.

extensions/browser/api/socket/app_firewall_hole_manager.h

Index: extensions/browser/api/socket/app_firewall_hole_manager.h
diff --git a/extensions/browser/api/socket/app_firewall_hole_manager.h b/extensions/browser/api/socket/app_firewall_hole_manager.h
new file mode 100644
index 0000000000000000000000000000000000000000..cd24cbf8a4b464685f00b692701638bdf05a545b
--- /dev/null
+++ b/extensions/browser/api/socket/app_firewall_hole_manager.h
@@ -0,0 +1,94 @@
+// Copyright 2015 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef EXTENSIONS_BROWSER_API_SOCKET_APP_FIREWALL_HOLE_MANAGER_H_
+#define EXTENSIONS_BROWSER_API_SOCKET_APP_FIREWALL_HOLE_MANAGER_H_
+
+#include <map>
+
+#include "base/scoped_observer.h"
+#include "chromeos/network/firewall_hole.h"
+#include "extensions/browser/app_window/app_window_registry.h"
+
+namespace content {
+class BrowserContext;
+}
+
+namespace extensions {
+
+class AppFirewallHoleManager;
+
+// Represents an open port in the system firewall that will be opened and closed
+// automatically when the application has a visible window or not. The hole is
+// closed on destruction.
+class AppFirewallHole {
+ public:
+  typedef chromeos::FirewallHole::PortType PortType;
+
+  ~AppFirewallHole();
+
+  PortType type() const { return type_; }
+  uint16_t port() const { return port_; }
+  const std::string& extension_id() const { return extension_id_; }
+
+ private:
+  friend class AppFirewallHoleManager;
+
+  AppFirewallHole(AppFirewallHoleManager* manager,
+                  PortType type,
+                  uint16_t port,
+                  const std::string& extension_id);
+
+  void SetVisible(bool app_visible);
+  void OnFirewallHoleOpened(scoped_ptr<chromeos::FirewallHole> firewall_hole);
+
+  PortType type_;
+  uint16_t port_;
+  std::string extension_id_;
+  bool app_visible_ = false;
+
+  AppFirewallHoleManager* manager_;

[rpaquay, 2015/03/23 18:29:07]

nit: comment why it is ok to have raw ptr?

[Reilly Grant (use Gerrit), 2015/03/23 20:09:31]

Done.

[Reilly Grant (use Gerrit), 2015/03/23 20:09:31]

Done.

+
+  // This will hold the FirewallHole object if one is opened.
+  scoped_ptr<chromeos::FirewallHole> firewall_hole_;
+
+  base::WeakPtrFactory<AppFirewallHole> weak_factory_;
+};
+
+// Tracks ports in the system firewall opened by an application so that they
+// may be automatically opened and closed only when the application has a
+// visible window.
+class AppFirewallHoleManager : public KeyedService,
+                               public AppWindowRegistry::Observer {
+ public:
+  explicit AppFirewallHoleManager(content::BrowserContext* context);
+  ~AppFirewallHoleManager() override;
+
+  // Returns the instance for a given browser context, or NULL if none.
+  static AppFirewallHoleManager* Get(content::BrowserContext* context);
+
+  // Takes ownership of the AppFirewallHole and will open a port on the system
+  // firewall if the associated application is currently visible.
+  AppFirewallHole* Open(AppFirewallHole::PortType type,

[rpaquay, 2015/03/23 18:29:07]

Why don't we return a scoped_ptr<AppFirewallHole,
BrowserThread::DeleteOnUIThread> here instead of a raw pointer?

From what I understand of the code, the ownership is an implementation detail,
as AppFirewallHole unregister itselfs when the desctructor is called. So, from a
consumer perspective, AppFirewallHoleManager is more a factory than a "manager":
you call into a method to get an instance (as scoped_ptr) that you are
responsible to release when you are done.

(Unless I am missing something of course :))

[Reilly Grant (use Gerrit), 2015/03/23 20:09:31]

I've switched it to a regular scoped_ptr. I want to keep the BrowserThread
dependency out of this class. It's easy to do the conversion in socket_api.cc.
While in general I agree with this comment. The class does straddle the boundary
between manager and factory there are two reasons why I think manager is more
appropriate:

1) It is the owner of all AppFirewallHole objects, while factories typically
just construct them.
2) I refuse the name a class AppFirewallHoleFactoryFactory.

[rpaquay, 2015/03/23 21:04:57]

Yes, yes, agreed, I didn't mean to imply you should change the name, I was just
continuing my train of thought wrt to using scoped_ptr for return value.

+                        uint16_t port,
+                        const std::string& extension_id);
+
+ private:
+  friend class AppFirewallHole;
+
+  void Close(AppFirewallHole* hole);
+
+  // AppWindowRegistry::Observer
+  void OnAppWindowRemoved(AppWindow* app_window) override;
+  void OnAppWindowHidden(AppWindow* app_window) override;
+  void OnAppWindowShown(AppWindow* app_window, bool was_hidden) override;
+
+  content::BrowserContext* context_;
+  ScopedObserver<AppWindowRegistry, AppWindowRegistry::Observer> observer_;
+  std::multimap<std::string, AppFirewallHole*> tracked_holes_;
+};
+
+}  // namespace extensions
+
+#endif  // EXTENSIONS_BROWSER_API_SOCKET_APP_FIREWALL_HOLE_MANAGER_H_