Reland: chromeos: Convert ash VPNDelegate interface to mojo

ash/common/system/chromeos/network/vpn_list.h

Index: ash/common/system/chromeos/network/vpn_list.h
diff --git a/ash/common/system/chromeos/network/vpn_delegate.h b/ash/common/system/chromeos/network/vpn_list.h
similarity index 73%
rename from ash/common/system/chromeos/network/vpn_delegate.h
rename to ash/common/system/chromeos/network/vpn_list.h
index d16f0cac48314d6a75042feb9026680680a53df1..6c7b0383d0f516db899ccc428fc2b9f5f12a67a6 100644
--- a/ash/common/system/chromeos/network/vpn_delegate.h
+++ b/ash/common/system/chromeos/network/vpn_list.h
@@ -2,15 +2,17 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#ifndef ASH_COMMON_SYSTEM_CHROMEOS_NETWORK_VPN_DELEGATE_H_
-#define ASH_COMMON_SYSTEM_CHROMEOS_NETWORK_VPN_DELEGATE_H_
+#ifndef ASH_COMMON_SYSTEM_CHROMEOS_NETWORK_VPN_LIST_H_
+#define ASH_COMMON_SYSTEM_CHROMEOS_NETWORK_VPN_LIST_H_
 
 #include <string>
 #include <vector>
 
 #include "ash/ash_export.h"
+#include "ash/public/interfaces/vpn_list.mojom.h"
 #include "base/macros.h"
 #include "base/observer_list.h"
+#include "mojo/public/cpp/bindings/binding_set.h"
 
 namespace ash {
 
@@ -40,9 +42,7 @@ struct ASH_EXPORT VPNProvider {
 // list of VPN providers enabled in the primary user's profile. The delegate
 // furthermore allows the UI code to request that a VPN provider show its "add
 // network" dialog.
-// TODO(jamescook): Rename this to VpnList as part of mojo conversion because it
-// won't be a delegate anymore.
-class ASH_EXPORT VPNDelegate {
+class ASH_EXPORT VpnList : public mojom::VpnList {
  public:
   // An observer that is notified whenever the list of VPN providers enabled in
   // the primary user's profile changes.
@@ -57,8 +57,8 @@ class ASH_EXPORT VPNDelegate {
     DISALLOW_ASSIGN(Observer);
   };
 
-  VPNDelegate();
-  virtual ~VPNDelegate();
+  VpnList();
+  ~VpnList() override;
 
   const std::vector<VPNProvider>& vpn_providers() { return vpn_providers_; }
 
@@ -66,19 +66,15 @@ class ASH_EXPORT VPNDelegate {
   // primary user's profile, in addition to the built-in OpenVPN/L2TP provider.
   bool HaveThirdPartyVPNProviders() const;
 
-  // Requests that the third-party VPN provider identified by |extension_id|
-  // show its "add network" dialog. If |extension_id| is empty then the built-in
-  // VPN provider's dialog is shown.
-  virtual void ShowAddPage(const std::string& extension_id) = 0;
-
   void AddObserver(Observer* observer);
   void RemoveObserver(Observer* observer);
 
-  // Sets the list of extension-backed VPN providers. The list may be empty.
-  // Notifies observers. Public for testing.
-  // TODO(jamescook): Convert to mojo interface.
+  // Binds the mojom::VpnList interface to this object.
+  void BindRequest(mojom::VpnListRequest request);

[stevenjb, 2016/11/22 17:19:48]

Just for my edification: Why isn't this (and bindings_) part of mojom::VpnList?

[James Cook, 2016/11/22 17:37:30]

This method (and bindings_) can actually exist anywhere. This works as follows:
1. Some process (say, chrome) makes a mojo connection to ash.
2. Ash sees an inbound mojo connection request. The app runs an OnConnect
method. See
https://cs.chromium.org/chromium/src/ash/mus/window_manager_application.cc?dr...
3. OnConnect registers all the mojo interfaces that are allowed to be used by
that connection.
https://cs.chromium.org/chromium/src/ash/common/mojo_interface_factory.cc?dr&...
4. Later, when chrome requests to use the VpnList interface, ash needs to "bind"
that request to the code/class that implements the interface. The request comes
in on the IO thread and there's a base::Bind callback that binds the request to
an object instance.
https://cs.chromium.org/chromium/src/ash/common/mojo_interface_factory.cc?dr&...
5. The |binding_| stores the mapping of interface request to object instance. It
can live anywhere. Sometimes it is a BindingSet if there is the possibility that
more than one external client would connect to the same object instance.

In ash we store the bindings in the implementation classes themselves.

In chrome we sometimes do that, and sometimes store them elsewhere. See
https://cs.chromium.org/chromium/src/chrome/browser/chromeos/chrome_interface...

I'm not sure if one is necessarily better than the other.

+
+  // mojom::VpnList:
   void SetThirdPartyVpnProviders(
-      const std::vector<VPNProvider>& third_party_providers);
+      std::vector<mojom::ThirdPartyVpnProviderPtr> providers) override;
 
  private:
   // Notify observers that the list of VPN providers enabled in the primary
@@ -88,15 +84,18 @@ class ASH_EXPORT VPNDelegate {
   // Adds the built-in OpenVPN/L2TP provider to |vpn_providers_|.
   void AddBuiltInProvider();
 
+  // Bindings for the mojom::VpnList interface.
+  mojo::BindingSet<mojom::VpnList> bindings_;
+
   // Cache of VPN providers, including the built-in OpenVPN/L2TP provider and
   // other providers added by extensions in the primary user's profile.
   std::vector<VPNProvider> vpn_providers_;
 
   base::ObserverList<Observer> observer_list_;
 
-  DISALLOW_COPY_AND_ASSIGN(VPNDelegate);
+  DISALLOW_COPY_AND_ASSIGN(VpnList);
 };
 
 }  // namespace ash
 
-#endif  // ASH_COMMON_SYSTEM_CHROMEOS_NETWORK_VPN_DELEGATE_H_
+#endif  // ASH_COMMON_SYSTEM_CHROMEOS_NETWORK_VPN_LIST_H_