ppapi: PPB_VpnProvider: Implement Service Helper

content/public/browser/pepper_vpn_provider_service_helper.h

+// Copyright 2016 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 CONTENT_PUBLIC_BROWSER_PEPPER_VPN_PROVIDER_SERVICE_HELPER_H_
+#define CONTENT_PUBLIC_BROWSER_PEPPER_VPN_PROVIDER_SERVICE_HELPER_H_

[ncarter (slow), 2016/05/17 19:07:36]

In general, content/public should have one class per file, and the filename
should match up to the class name (i.e., VpnProviderServiceHelper).

For the delegate interfaces, if they are kept as top-level classes in the
content namespace, they should be in their own file. Alternatively, you can use
a nested class with a shorter name to keep things in this one file (e.g.
VpnProviderServiceHelper::Delegate).

Example delegates in this directory:
https://code.google.com/p/chromium/codesearch#search/&q=Delegate%20file:conte...

[adrian.belgun, 2016/06/10 14:41:07]

Done.

+
+#include <map>
+#include <memory>
+#include <vector>
+
+#include "base/callback.h"
+#include "base/memory/weak_ptr.h"
+#include "content/common/content_export.h"
+
+namespace base {
+template <typename T>
+struct DefaultSingletonTraits;
+}
+
+namespace content {
+class BrowserContext;
+
+class CONTENT_EXPORT VpnMessageFilterDelegate {

[ncarter (slow), 2016/05/17 19:07:35]

Document these:
  Which threads will these methods be called on?
  What must the implementor do?

[adrian.belgun, 2016/06/10 14:41:07]

Done.

+ public:
+  virtual ~VpnMessageFilterDelegate();

[ncarter (slow), 2016/05/17 19:07:36]

This class is called MessageFilterDelegate, but the API doesn't seem to really
be doing filtering, it's about sending data. Is there a better name?

[adrian.belgun, 2016/06/10 14:41:07]

Done. Renamed to PepperVpnProviderResourceHostProxy. the object that implements
this will only forward the calls to the PepperVpnProviderResourceHost object.

+
+  virtual void SendOnPacketReceived(const std::vector<char>& data) = 0;

[ncarter (slow), 2016/05/17 19:07:36]

It looks like this Delegate interface will be implemented by
content/browser/renderer_host/pepper/pepper_vpn_provider_message_filter_chromeos
? If so, then there is no reason for it to be part of content/public:
content/public is only for classes that need to be visible to the content
embedder.

For naming schemes, I typically expect the 'Foo' in FooDelegate to identify the
class that calls the delegate, not the class that implements the Delegate
interface. As in, "Foo delegates handling of an operation to its FooDelegate."

Also, once you are move this to content/browser/renderer_host you may not need a
delegate relationship (or a Register method) at all, since the two impl classes
might be able to see each other directly, and you would have the option of
tightly coupling them.

[adrian.belgun, 2016/06/10 14:41:07]

Done. Split functionalty to Impl. 
Cannot avoid Register due to content/browser/renderer_host/pepper is dependent
on "enable_plugins" being set.

+  virtual void SendOnUnbind() = 0;
+};
+
+class CONTENT_EXPORT VpnServiceDelegate {

[ncarter (slow), 2016/05/17 19:07:36]

I see that the VpnService in extensions will actually implement the
VpnServiceDelegate. so this does belong in content/public.

Is this really best described as a Delegate? What does this class actually
represent, in this API? It looks like it represents something that can bind a
configuration and send packets. So does it actually represent something like a
tunnel/channel/connection/link/route, something like that?

[adrian.belgun, 2016/06/10 14:41:07]

Done. Renamed to VpnServiceProxy. The object that implements this will only
forward the calls to a VpnService object.

+ public:
+  virtual ~VpnServiceDelegate();
+
+  using SuccessCallback = base::Closure;
+  using StringCallback = base::Callback<void(const std::string& result)>;

[ncarter (slow), 2016/05/17 19:07:36]

This looks unused?

[adrian.belgun, 2016/06/10 14:41:07]

Done.

+  using FailureCallback =
+      base::Callback<void(const std::string& error_name,
+                          const std::string& error_message)>;
+
+  virtual void Bind(const std::string& extension_id,
+                    const std::string& configuration_id,
+                    const std::string& configuration_name,
+                    const SuccessCallback& success,
+                    const FailureCallback& failure) = 0;
+  virtual void SendPacket(const std::string& extension_id,
+                          const std::vector<char>& data,
+                          const SuccessCallback& success,
+                          const FailureCallback& failure) = 0;
+};
+

[ncarter (slow), 2016/05/17 19:07:36]

content/public, being the public api of this module, requires documentation.

Each class here should have a comment describing what it's for, and how it's
meant to be used. Please mention threading behavior. Methods should have
documentation too, that gives one an overall sense of how what behaviors need to
be implemented by content embedder, or what behaviors the content/
implementation must provide.

[adrian.belgun, 2016/06/10 14:41:07]

Done.

+class CONTENT_EXPORT VpnProviderServiceHelper {
+ public:
+  using SuccessCallback = base::Closure;
+  using StringCallback = base::Callback<void(const std::string& result)>;
+  using FailureCallback =
+      base::Callback<void(const std::string& error_name,
+                          const std::string& error_message)>;
+
+  static VpnProviderServiceHelper* GetInstance();

[ncarter (slow), 2016/05/17 19:07:36]

VpnProviderServiceHelper ought to be a pure virtual interface with no data
members, implemented by a VpnProviderServiceHelperImpl in content/browser
(possibly content/browser/renderer_host/pepper).

content/public rules are documented here:
https://www.chromium.org/developers/content-module/content-api . See where it
mentions pure virtual interfaces.

Some examples of this pattern elsewhere, for reference:
https://code.google.com/p/chromium/codesearch#search/&q=file:content.public%2...

[adrian.belgun, 2016/06/10 14:41:07]

Done.

+
+  // These functions must get called on the UI thread
+  void RegisterDelegate(const std::string& extension_id,

[ncarter (slow), 2016/05/17 19:07:36]

extensions don't exist at the content/ layer, so this is almost certainly not
the right way to express this. I can help you figure out the right way to
express it. What is the goal of these extension_ids?

note that extension_id are never used in content (what hits you see below are
mistakes):
https://code.google.com/p/chromium/codesearch#search/&q=extension_id%20file:s...

[adrian.belgun, 2016/06/10 14:41:07]

Done. Replaced with host_id. The extension id is used for matching the Pepper
plugin with the extension that created the extension.

+                        std::unique_ptr<VpnMessageFilterDelegate> delegate);
+  void RegisterDelegate(content::BrowserContext* context,
+                        std::unique_ptr<VpnServiceDelegate> delegate);

[ncarter (slow), 2016/05/17 19:07:36]

Is there only one of these per BrowserContext? Or can multiple delegates be
registered?

Another possible way to structure the registration handshake, would for content
to call out to the embedder via ContentBrowserClient interface, and have the
embedder return an instance that way. Sort of like this:
https://code.google.com/p/chromium/codesearch#chromium/src/content/public/bro...

I'm not sure yet if that's a good idea in this case, it depends on the overall
behavior, which I don't quite understand yet.

[adrian.belgun, 2016/06/10 14:41:07]

Yes. There is one VpnService per browser context. Currently leaving it as is.
For the next patch set I will look into this.

+  void Bind(const std::string& extension_id,
+            const std::string& configuration_id,
+            const std::string& configuration_name,
+            int render_process_id,
+            const SuccessCallback& success,
+            const FailureCallback& failure);
+  void SendPacket(const std::string& extension_id,
+                  const std::vector<char>& data,
+                  int render_process_id,
+                  SuccessCallback success,
+                  FailureCallback failure);
+  void OnUnbind(const std::string& extension_id);
+  void OnPacketReceived(const std::string& extension_id,
+                        const std::vector<char>& data);

[ncarter (slow), 2016/05/17 19:07:36]

Are all of these methods going to be called outside of content? Only expose in
content/public/ the ones that really need to be called by the consumer (which I
think is vpn_service.cc, in the extensions module?). The rest can be Impl-only
methods.

[adrian.belgun, 2016/06/10 14:41:07]

Done.

+
+ private:
+  VpnProviderServiceHelper();
+  ~VpnProviderServiceHelper();
+  friend struct base::DefaultSingletonTraits<VpnProviderServiceHelper>;
+
+  // These functions must get called on the IO thread
+  void OnUnbindIO(const std::string& extension_id);
+  void OnPacketReceivedIO(const std::string& extension_id,
+                          const std::vector<char>& data);
+
+  std::map<std::string, std::unique_ptr<VpnMessageFilterDelegate>>
+      extension_to_host_map_;
+  std::map<content::BrowserContext*, std::unique_ptr<VpnServiceDelegate>>
+      context_to_service_map_;
+
+  base::WeakPtrFactory<VpnProviderServiceHelper> weak_factory_;
+
+  DISALLOW_COPY_AND_ASSIGN(VpnProviderServiceHelper);
+};
+
+}  // namespace content
+
+#endif  // CONTENT_PUBLIC_BROWSER_PEPPER_VPN_PROVIDER_SERVICE_HELPER_H_