arc: Add InstanceHelper::GetInstanceForMethod()

components/arc/instance_holder.h

 // 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 COMPONENTS_ARC_INSTANCE_HOLDER_H_
 #define COMPONENTS_ARC_INSTANCE_HOLDER_H_
 
 #include <utility>
 
 #include "base/bind.h"
@@ skipping 22 matching lines @@
     virtual ~Observer() = default;
   };
 
   InstanceHolder() = default;
 
   // 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.
   T* instance() const { return instance_; }
   uint32_t version() const { return version_; }

[Yusuke Sato, 2016/09/16 23:58:48]

Can we remove this now? If not, what's preventing us from doing so?

[Luis Héctor Chávez, 2016/09/17 00:30:53]

GetIntentHelperInstanceWithErrorCode() still needs it to disambiguate.

 
+  // Gets the Mojo interface for the instance, but only if its reported version
+  // is at least |min_version|. Returns nullptr if the instance is either not
+  // ready or does not have the requested version, and logs appropriately.
+  T* GetInstanceForVersion(uint32_t min_version) {
+    if (!instance_) {
+      VLOG(2) << __func__ << " Instance not available.";
+      return nullptr;
+    }
+    if (version_ < min_version) {
+      VLOG(2) << __func__ << " Instance version mismatch. Expected "
+              << min_version << " got " << version_;
+      return nullptr;
+    }
+    return instance_;
+  }
+
+  // Same as the above, but with an explicit method name for better logging.
+  T* GetInstanceForVersion(uint32_t min_version, const char* method_name) {

[Yusuke Sato, 2016/09/16 23:58:48]

Passing the method_name every time only for logging seems a bit tedious. I
already saw several typos during the review too. Can't we automagically query a
method name from |min_version| (or can't we modify the mojo C++ code generator
so that we can do it?) It'll be 1:N mapping and the logging will be a bit
verbose, though.

Or, querying a min version from a function pointer
(&mojom::...Instance::DoSomething) would be even better if at all possible. This
way, the mapping will be N:1.

wdyt?

[Luis Héctor Chávez, 2016/09/17 00:30:53]

We can't query the method name from |min_version| unfortunately (see the
bluetooth service, several methods were added in the same version). There's
nothing generated right now that we can use :( I can try some stuff out, but I'd
like to do that in a separate commit (if possible).

(There's also typeid, but I didn't pursue it since it seems to be unused in
Chromium).

[Yusuke Sato, 2016/09/17 01:38:56]

Acknowledged. I guess typeid() won't always work as Chromium is built with
-fno-rtti.

+    if (!instance_) {
+      VLOG(2) << __func__ << " Instance not available to call " << method_name
+              << ".";
+      return nullptr;
+    }
+    if (version_ < min_version) {
+      VLOG(2) << __func__ << " Instance version mismatch to call "
+              << method_name << ". Expected " << min_version << " got "
+              << version_;
+      return nullptr;
+    }
+    return instance_;
+  }
+
   // 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(Observer* observer) {
     DCHECK(thread_checker_.CalledOnValidThread());
     observer_list_.AddObserver(observer);
 
     if (instance())
       observer->OnInstanceReady();
   }
@@ skipping 32 matching lines @@
 
   base::ThreadChecker thread_checker_;
   base::ObserverList<Observer> observer_list_;
 
   DISALLOW_COPY_AND_ASSIGN(InstanceHolder<T>);
 };
 
 }  // namespace arc
 
 #endif  // COMPONENTS_ARC_INSTANCE_HOLDER_H_