arc: Revamp the ArcBridgeService interface

components/arc/arc_bridge_service.h

Index: components/arc/arc_bridge_service.h
diff --git a/components/arc/arc_bridge_service.h b/components/arc/arc_bridge_service.h
index 3d719694314ee0219463b873144255e23136cbde..40ea1f4921ccdbd5452420b3040029c6e7bf24a8 100644
--- a/components/arc/arc_bridge_service.h
+++ b/components/arc/arc_bridge_service.h
@@ -75,84 +75,124 @@ class ArcBridgeService : public mojom::ArcBridgeHost {
     // Called whenever ARC's availability has changed for this system.
     virtual void OnAvailableChanged(bool available) {}
 
-    // Called whenever the ARC app interface state changes.
-    virtual void OnAppInstanceReady() {}
-    virtual void OnAppInstanceClosed() {}
-
-    // Called whenever the ARC audio interface state changes.
-    virtual void OnAudioInstanceReady() {}
-    virtual void OnAudioInstanceClosed() {}
-
-    // Called whenever the ARC auth interface state changes.
-    virtual void OnAuthInstanceReady() {}
-    virtual void OnAuthInstanceClosed() {}
-
-    // Called whenever ARC Bluetooth instance is ready.
-    virtual void OnBluetoothInstanceReady() {}
-    virtual void OnBluetoothInstanceClosed() {}
-
-    // Called whenever the ARC clipboard interface state changes.
-    virtual void OnClipboardInstanceReady() {}
-    virtual void OnClipboardInstanceClosed() {}
-
-    // Called whenever the ARC crash collector interface state changes.
-    virtual void OnCrashCollectorInstanceReady() {}
-    virtual void OnCrashCollectorInstanceClosed() {}
-
-    // Called whenever the ARC file system interface state changes.
-    virtual void OnFileSystemInstanceReady() {}
-    virtual void OnFileSystemInstanceClosed() {}
-
-    // Called whenever the ARC IME interface state changes.
-    virtual void OnImeInstanceReady() {}
-    virtual void OnImeInstanceClosed() {}
-
-    // Called whenever the ARC intent helper interface state changes.
-    virtual void OnIntentHelperInstanceReady() {}
-    virtual void OnIntentHelperInstanceClosed() {}
-
-    // Called whenever the ARC metrics interface state changes.
-    virtual void OnMetricsInstanceReady() {}
-    virtual void OnMetricsInstanceClosed() {}
-
-    // Called whenever the ARC notification interface state changes.
-    virtual void OnNotificationsInstanceReady() {}
-    virtual void OnNotificationsInstanceClosed() {}
-
-    // Called whenever the ARC net interface state changes.
-    virtual void OnNetInstanceReady() {}
-    virtual void OnNetInstanceClosed() {}
-
-    // Called whenever the ARC OBB mounter interface state changes.
-    virtual void OnObbMounterInstanceReady() {}
-    virtual void OnObbMounterInstanceClosed() {}
-
-    // Called whenever the ARC policy interface state changes.
-    virtual void OnPolicyInstanceReady() {}
-    virtual void OnPolicyInstanceClosed() {}
-
-    // Called whenever the ARC power interface state changes.
-    virtual void OnPowerInstanceReady() {}
-    virtual void OnPowerInstanceClosed() {}
-
-    // Called whenever the ARC process interface state changes.
-    virtual void OnProcessInstanceReady() {}
-    virtual void OnProcessInstanceClosed() {}
-
-    // Called whenever the ARC storage manager interface state changes.
-    virtual void OnStorageManagerInstanceReady() {}
-    virtual void OnStorageManagerInstanceClosed() {}
+   protected:
+    virtual ~Observer() {}
+  };
 
-    // Called whenever the ARC video interface state changes.
-    virtual void OnVideoInstanceReady() {}
-    virtual void OnVideoInstanceClosed() {}
+  // Notifies about connection events for individual instances.
+  template <typename T>

[hidehiko, 2016/07/11 05:24:53]

Could you comment that T needs to be a mojo interface?

[Luis Héctor Chávez, 2016/07/11 17:13:34]

Done.

+  class InstanceObserver {

[hidehiko, 2016/07/11 05:24:53]

InstanceObserver and InstanceHolder looks enough big. So, could you move this
out from this class?

Optional: Introducing a new file may be another option?

[Luis Héctor Chávez, 2016/07/11 17:13:34]

Done.

+   public:
+    // Called once the instance is ready. It is guaranteed that |instance| will
+    // not be null.
+    virtual void OnInstanceReady(T* instance, uint32_t version) {}
 
-    // Called whenever the ARC window manager interface state changes.
-    virtual void OnWindowManagerInstanceReady() {}
-    virtual void OnWindowManagerInstanceClosed() {}
+    // The parameter is always nullptr, but is required to be able to
+    // distinguish between implementations of InstanceObserver for different
+    // types.
+    virtual void OnInstanceClosed(T*) {}

[hidehiko, 2016/07/11 05:24:53]

Could you get rid of T*?

I'm guessing that it is supposed to inherit two ore more InstanceObserver
interface for a class, e.g.

class A : public InstanceObserver<T1>, public InstanceObserver<T2> ... {
};

etc. Though, in that case, I think it looks better to split the class into two
or more.

[Luis Héctor Chávez, 2016/07/11 17:13:34]

I can't :/ The metrics service tries needs to wait for both the metrics and
process instances.

 
    protected:
-    virtual ~Observer() {}
+    virtual ~InstanceObserver() {}

[hidehiko, 2016/07/11 05:24:53]

nit: s/{}/= default/

[Luis Héctor Chávez, 2016/07/11 17:13:34]

Done.

+  };
+
+  template <typename T>
+  class InstanceHolder {
+   public:
+    InstanceHolder() : weak_factory_(this) {}
+
+    // Gets the Mojo interface for all the instance services. This will return
+    // nullptr if that particular service is not ready yet. Use an
+    // InstanceObserver if you want to be notified when this is ready. This can
+    // only be called on the thread that this class was created on.
+    T* instance() const { return raw_ptr_; }

[hidehiko, 2016/07/11 05:24:53]

To access the raw pointer of T, there are two ways:

accessing InstanceHolder::instance(), or kee pthe ptr passed via
InstanceObserver::OnInstanceReady().
Actually, currently those are mixed in use. Which is the preferable way?

[Luis Héctor Chávez, 2016/07/11 17:13:34]

I'd prefer to not keep the ptr since it is more error-prone (since the user
needs to be diligent to reset the ptr to nullptr in the OnInstanceClosed()).
Having said that, keeping the pointer does save some boilerplate :/

Let's discuss this offline to see if it is possible to add this to the best
practices doc.

+    uint32_t version() const { return version_; }
+
+    // Adds or removes observers. This can only be called on the thread that
+    // this class was created on. RemoveObserver does nothing if |observer| is
+    // not in the list.
+    void AddObserver(InstanceObserver<T>* observer) {
+      DCHECK(thread_checker_.CalledOnValidThread());
+      observer_list_.AddObserver(observer);
+
+      if (instance())
+        observer->OnInstanceReady(instance(), version());
+    }
+
+    void RemoveObserver(InstanceObserver<T>* observer) {
+      DCHECK(thread_checker_.CalledOnValidThread());
+      observer_list_.RemoveObserver(observer);
+    }
+
+    // Called when the channel is closed.
+    void CloseChannel() {
+      if (!ptr_)
+        return;
+
+      ptr_.reset();
+      raw_ptr_ = nullptr;
+      version_ = 0;
+      if (observer_list_.might_have_observers()) {

[hidehiko, 2016/07/11 05:24:53]

FOR_EACH_OBSERVER(InstanceObserver<T>, observer_list_, OnInstanceClosed());

should work?

[Luis Héctor Chávez, 2016/07/11 17:13:34]

It doesn't since it needs the 'typename' since InstanceObserver<T> cannot be
resolved to a type otherwise. I can try to fix the FOR_EACH_OBSERVER, but that
should be done in a different change.

[Luis Héctor Chávez, 2016/07/11 20:10:44]

Filed https://bugs.chromium.org/p/chromium/issues/detail?id=627209 and will
follow-up separately. Will amend this change if the fix lands first.

+        typename base::ObserverList<InstanceObserver<T>>::Iterator it(
+            &observer_list_);
+        InstanceObserver<T>* obs;
+        while ((obs = it.GetNext()) != nullptr)
+          obs->OnInstanceClosed(instance());
+      }
+    }
+
+    // Sets the interface pointer to |ptr|, once the version is determined. This
+    // will eventually invoke SetInstance(), which will notify the observers.
+    void OnInstanceReady(mojo::InterfacePtr<T> ptr) {
+      temporary_ptr_ = std::move(ptr);
+      temporary_ptr_.QueryVersion(base::Bind(&InstanceHolder<T>::OnVersionReady,
+                                             weak_factory_.GetWeakPtr()));
+    }
+
+    // This method is not intended to be called directly. Normally it is called
+    // by OnInstanceReady once the version of the instance is determined, but it
+    // is also exposed so that tests can directly inject a raw pointer+version
+    // combination.
+    void SetInstance(T* raw_ptr, uint32_t raw_version = T::Version_) {
+      raw_ptr_ = raw_ptr;
+      version_ = raw_version;
+      if (observer_list_.might_have_observers()) {

[hidehiko, 2016/07/11 05:24:53]

Ditto for FOR_EACH_OBSERVER.

[Luis Héctor Chávez, 2016/07/11 17:13:34]

Acknowledged.

+        typename base::ObserverList<InstanceObserver<T>>::Iterator it(
+            &observer_list_);
+        InstanceObserver<T>* obs;
+        while ((obs = it.GetNext()) != nullptr)
+          obs->OnInstanceReady(instance(), version());
+      }
+    }
+
+   private:
+    void OnVersionReady(uint32_t version) {
+      ptr_ = std::move(temporary_ptr_);
+      ptr_.set_connection_error_handler(base::Bind(
+          &InstanceHolder<T>::CloseChannel, weak_factory_.GetWeakPtr()));
+      SetInstance(ptr_.get(), version);
+    }
+
+    // These two are copies of the contents of ptr_. They are provided here just
+    // so that tests can provide non-mojo implementations.
+    T* raw_ptr_ = nullptr;
+    uint32_t version_ = 0;
+
+    mojo::InterfacePtr<T> ptr_;
+
+    // Temporary Mojo interfaces.  After a Mojo interface pointer has been
+    // received from the other endpoint, we still need to asynchronously query
+    // its version.  While that is going on, we should still return nullptr on
+    // the instance() function.
+    // To keep the instance() functions being trivial, store the instance
+    // pointer in a temporary variable to avoid losing its reference.
+    mojo::InterfacePtr<T> temporary_ptr_;
+
+    base::ThreadChecker thread_checker_;
+    base::ObserverList<InstanceObserver<T>> observer_list_;
+    base::WeakPtrFactory<InstanceHolder<T>> weak_factory_;

[hidehiko, 2016/07/11 05:24:53]

Could you comment that this needs to be the last member, in order to cancel all
inflight callback with weak_ptr generated by this factory before destructing any
of other members?

[Luis Héctor Chávez, 2016/07/11 17:13:34]

Sure

+
+    DISALLOW_COPY_AND_ASSIGN(InstanceHolder<T>);
   };
 
   ~ArcBridgeService() override;
@@ -185,72 +225,38 @@ class ArcBridgeService : public mojom::ArcBridgeHost {
   void AddObserver(Observer* observer);
   void RemoveObserver(Observer* observer);
 
-  // Gets the Mojo interface for all the instance services. This will return
-  // nullptr if that particular service is not ready yet. Use an Observer if
-  // you want to be notified when this is ready. This can only be called on the
-  // thread that this class was created on.
-  mojom::AppInstance* app_instance() { return app_ptr_.get(); }
-  mojom::AudioInstance* audio_instance() { return audio_ptr_.get(); }
-  mojom::AuthInstance* auth_instance() { return auth_ptr_.get(); }
-  mojom::BluetoothInstance* bluetooth_instance() {
-    return bluetooth_ptr_.get();
-  }
-  mojom::ClipboardInstance* clipboard_instance() {
-    return clipboard_ptr_.get();
-  }
-  mojom::CrashCollectorInstance* crash_collector_instance() {
-    return crash_collector_ptr_.get();
-  }
-  mojom::FileSystemInstance* file_system_instance() {
-    return file_system_ptr_.get();
+  InstanceHolder<mojom::AppInstance>* app() { return &app_; }
+  InstanceHolder<mojom::AudioInstance>* audio() { return &audio_; }
+  InstanceHolder<mojom::AuthInstance>* auth() { return &auth_; }
+  InstanceHolder<mojom::BluetoothInstance>* bluetooth() { return &bluetooth_; }
+  InstanceHolder<mojom::ClipboardInstance>* clipboard() { return &clipboard_; }
+  InstanceHolder<mojom::CrashCollectorInstance>* crash_collector() {
+    return &crash_collector_;
   }
-  mojom::ImeInstance* ime_instance() { return ime_ptr_.get(); }
-  mojom::IntentHelperInstance* intent_helper_instance() {
-    return intent_helper_ptr_.get();
+  InstanceHolder<mojom::FileSystemInstance>* file_system() {
+    return &file_system_;
   }
-  mojom::MetricsInstance* metrics_instance() { return metrics_ptr_.get(); }
-  mojom::NetInstance* net_instance() { return net_ptr_.get(); }
-  mojom::NotificationsInstance* notifications_instance() {
-    return notifications_ptr_.get();
+  InstanceHolder<mojom::ImeInstance>* ime() { return &ime_; }
+  InstanceHolder<mojom::IntentHelperInstance>* intent_helper() {
+    return &intent_helper_;
   }
-  mojom::ObbMounterInstance* obb_mounter_instance() {
-    return obb_mounter_ptr_.get();
+  InstanceHolder<mojom::MetricsInstance>* metrics() { return &metrics_; }
+  InstanceHolder<mojom::NetInstance>* net() { return &net_; }
+  InstanceHolder<mojom::NotificationsInstance>* notifications() {
+    return &notifications_;
   }
-  mojom::PolicyInstance* policy_instance() { return policy_ptr_.get(); }
-  mojom::PowerInstance* power_instance() { return power_ptr_.get(); }
-  mojom::ProcessInstance* process_instance() { return process_ptr_.get(); }
-  mojom::StorageManagerInstance* storage_manager_instance() {
-    return storage_manager_ptr_.get();
-  }
-  mojom::VideoInstance* video_instance() { return video_ptr_.get(); }
-  mojom::WindowManagerInstance* window_manager_instance() {
-    return window_manager_ptr_.get();
-  }
-
-  int32_t app_version() const { return app_ptr_.version(); }
-  int32_t audio_version() const { return audio_ptr_.version(); }
-  int32_t bluetooth_version() const { return bluetooth_ptr_.version(); }
-  int32_t auth_version() const { return auth_ptr_.version(); }
-  int32_t clipboard_version() const { return clipboard_ptr_.version(); }
-  int32_t crash_collector_version() const {
-    return crash_collector_ptr_.version();
+  InstanceHolder<mojom::ObbMounterInstance>* obb_mounter() {
+    return &obb_mounter_;
   }
-  int32_t file_system_version() const { return file_system_ptr_.version(); }
-  int32_t ime_version() const { return ime_ptr_.version(); }
-  int32_t intent_helper_version() const { return intent_helper_ptr_.version(); }
-  int32_t metrics_version() const { return metrics_ptr_.version(); }
-  int32_t net_version() const { return net_ptr_.version(); }
-  int32_t notifications_version() const { return notifications_ptr_.version(); }
-  int32_t obb_mounter_version() const { return obb_mounter_ptr_.version(); }
-  int32_t policy_version() const { return policy_ptr_.version(); }
-  int32_t power_version() const { return power_ptr_.version(); }
-  int32_t process_version() const { return process_ptr_.version(); }
-  int32_t storage_manager_version() const {
-    return storage_manager_ptr_.version();
+  InstanceHolder<mojom::PolicyInstance>* policy() { return &policy_; }
+  InstanceHolder<mojom::PowerInstance>* power() { return &power_; }
+  InstanceHolder<mojom::ProcessInstance>* process() { return &process_; }
+  InstanceHolder<mojom::StorageManagerInstance>* storage_manager() {
+    return &storage_manager_;
   }
-  int32_t video_version() const { return video_ptr_.version(); }
-  int32_t window_manager_version() const {
-    return window_manager_ptr_.version();
+  InstanceHolder<mojom::VideoInstance>* video() { return &video_; }
+  InstanceHolder<mojom::WindowManagerInstance>* window_manager() {
+    return &window_manager_;
   }
 
   // ArcHost:
@@ -312,94 +318,26 @@ class ArcBridgeService : public mojom::ArcBridgeHost {
   FRIEND_TEST_ALL_PREFIXES(ArcBridgeTest, ShutdownMidStartup);
   FRIEND_TEST_ALL_PREFIXES(ArcBridgeTest, Restart);
 
-  // Called when one of the individual channels is closed.
-  void CloseAppChannel();
-  void CloseAudioChannel();
-  void CloseAuthChannel();
-  void CloseBluetoothChannel();
-  void CloseClipboardChannel();
-  void CloseCrashCollectorChannel();
-  void CloseFileSystemChannel();
-  void CloseImeChannel();
-  void CloseIntentHelperChannel();
-  void CloseMetricsChannel();
-  void CloseNetChannel();
-  void CloseNotificationsChannel();
-  void CloseObbMounterChannel();
-  void ClosePolicyChannel();
-  void ClosePowerChannel();
-  void CloseProcessChannel();
-  void CloseStorageManagerChannel();
-  void CloseVideoChannel();
-  void CloseWindowManagerChannel();
-
-  // Callbacks for QueryVersion.
-  void OnAppVersionReady(uint32_t version);
-  void OnAudioVersionReady(uint32_t version);
-  void OnAuthVersionReady(uint32_t version);
-  void OnBluetoothVersionReady(uint32_t version);
-  void OnClipboardVersionReady(uint32_t version);
-  void OnCrashCollectorVersionReady(uint32_t version);
-  void OnFileSystemVersionReady(uint32_t version);
-  void OnImeVersionReady(uint32_t version);
-  void OnIntentHelperVersionReady(uint32_t version);
-  void OnMetricsVersionReady(uint32_t version);
-  void OnNetVersionReady(uint32_t version);
-  void OnNotificationsVersionReady(uint32_t version);
-  void OnObbMounterVersionReady(uint32_t version);
-  void OnPolicyVersionReady(uint32_t version);
-  void OnPowerVersionReady(uint32_t version);
-  void OnProcessVersionReady(uint32_t version);
-  void OnStorageManagerVersionReady(uint32_t version);
-  void OnVideoVersionReady(uint32_t version);
-  void OnWindowManagerVersionReady(uint32_t version);
-
-  // Mojo interfaces.
-  mojom::AppInstancePtr app_ptr_;
-  mojom::AudioInstancePtr audio_ptr_;
-  mojom::AuthInstancePtr auth_ptr_;
-  mojom::BluetoothInstancePtr bluetooth_ptr_;
-  mojom::ClipboardInstancePtr clipboard_ptr_;
-  mojom::CrashCollectorInstancePtr crash_collector_ptr_;
-  mojom::FileSystemInstancePtr file_system_ptr_;
-  mojom::ImeInstancePtr ime_ptr_;
-  mojom::IntentHelperInstancePtr intent_helper_ptr_;
-  mojom::MetricsInstancePtr metrics_ptr_;
-  mojom::NetInstancePtr net_ptr_;
-  mojom::NotificationsInstancePtr notifications_ptr_;
-  mojom::ObbMounterInstancePtr obb_mounter_ptr_;
-  mojom::PolicyInstancePtr policy_ptr_;
-  mojom::PowerInstancePtr power_ptr_;
-  mojom::ProcessInstancePtr process_ptr_;
-  mojom::StorageManagerInstancePtr storage_manager_ptr_;
-  mojom::VideoInstancePtr video_ptr_;
-  mojom::WindowManagerInstancePtr window_manager_ptr_;
-
-  // Temporary Mojo interfaces.  After a Mojo interface pointer has been
-  // received from the other endpoint, we still need to asynchronously query
-  // its version.  While that is going on, we should still return nullptr on
-  // the xxx_instance() functions.
-  // To keep the xxx_instance() functions being trivial, store the instance
-  // pointer in a temporary variable to avoid losing its reference.
-  mojom::AppInstancePtr temporary_app_ptr_;
-  mojom::AudioInstancePtr temporary_audio_ptr_;
-  mojom::AuthInstancePtr temporary_auth_ptr_;
-  mojom::BluetoothInstancePtr temporary_bluetooth_ptr_;
-  mojom::ClipboardInstancePtr temporary_clipboard_ptr_;
-  mojom::CrashCollectorInstancePtr temporary_crash_collector_ptr_;
-  mojom::FileSystemInstancePtr temporary_file_system_ptr_;
-  mojom::ImeInstancePtr temporary_ime_ptr_;
-  mojom::IntentHelperInstancePtr temporary_intent_helper_ptr_;
-  mojom::MetricsInstancePtr temporary_metrics_ptr_;
-  mojom::NetInstancePtr temporary_net_ptr_;
-  mojom::NotificationsInstancePtr temporary_notifications_ptr_;
-  mojom::ObbMounterInstancePtr temporary_obb_mounter_ptr_;
-  mojom::PolicyInstancePtr temporary_policy_ptr_;
-  mojom::PowerInstancePtr temporary_power_ptr_;
-  mojom::ProcessInstancePtr temporary_process_ptr_;
-  mojom::StorageManagerInstancePtr temporary_storage_manager_ptr_;
-  mojom::VideoInstancePtr temporary_video_ptr_;
-  mojom::WindowManagerInstancePtr temporary_window_manager_ptr_;
+  // Instance holders.
+  InstanceHolder<mojom::AppInstance> app_;
+  InstanceHolder<mojom::AudioInstance> audio_;
+  InstanceHolder<mojom::AuthInstance> auth_;
+  InstanceHolder<mojom::BluetoothInstance> bluetooth_;
+  InstanceHolder<mojom::ClipboardInstance> clipboard_;
+  InstanceHolder<mojom::CrashCollectorInstance> crash_collector_;
+  InstanceHolder<mojom::FileSystemInstance> file_system_;
+  InstanceHolder<mojom::ImeInstance> ime_;
+  InstanceHolder<mojom::IntentHelperInstance> intent_helper_;
+  InstanceHolder<mojom::MetricsInstance> metrics_;
+  InstanceHolder<mojom::NetInstance> net_;
+  InstanceHolder<mojom::NotificationsInstance> notifications_;
+  InstanceHolder<mojom::ObbMounterInstance> obb_mounter_;
+  InstanceHolder<mojom::PolicyInstance> policy_;
+  InstanceHolder<mojom::PowerInstance> power_;
+  InstanceHolder<mojom::ProcessInstance> process_;
+  InstanceHolder<mojom::StorageManagerInstance> storage_manager_;
+  InstanceHolder<mojom::VideoInstance> video_;
+  InstanceHolder<mojom::WindowManagerInstance> window_manager_;
 
   base::ObserverList<Observer> observer_list_;