arc: Add InstanceHelper::GetInstanceForMethod()

components/arc/instance_holder.h

Index: components/arc/instance_holder.h
diff --git a/components/arc/instance_holder.h b/components/arc/instance_holder.h
index 4fb5b37023eac07e6303f0c9928c0e8752c8f547..dc5480d52b4c1eed442833d40e016325ab9a6ca9 100644
--- a/components/arc/instance_holder.h
+++ b/components/arc/instance_holder.h
@@ -42,6 +42,38 @@ class InstanceHolder {
   T* instance() const { return instance_; }

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

What about renaming this to GetInstanceForVersionZero() or something to
explicitly tell developers that they are getting a version-unchecked
|instance_|?

If you agree, adding GetInstanceForVersionZero() now but removing instance()
later (with a TODO) is fine.

[Luis Héctor Chávez, 2016/09/17 03:52:14]

Resolved to keep.

   uint32_t version() const { return version_; }
 
+  // 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) {

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

Providing two versions might not work as expected. I guess many will just use
this unary version for less typing. How about just removing the unary version so
we can always get better log messages?

[Luis Héctor Chávez, 2016/09/17 01:56:11]

If we go this route, I prefer the following:

GetInstanceForMethod(const char* method_name, uint32_t min_version);
GetInstanceForMethod(const char* method_name) { return
GetInstanceForMethod(method_name, 0); }

That's a little bit more explicit, consistent and less typing to boot. WDYT?

[Yusuke Sato, 2016/09/17 02:42:53]

We can still remove instance() (or at least rename it to instance_for_testing())
later, correct? If so, SGTM w/ two requests:

* Please change |method_name| to |method_name_for_logging| for the first one,
and to |method_name_with_min_version_0_for_logging| for the second one. 
* Please document that the second one should only be used when calling a method
with MinVersion=0.

[Luis Héctor Chávez, 2016/09/17 03:16:34]

i'd still like to keep it for the few cases where it doesn't make sense to log:
methods like the one you added that are doing their own validation, and
OnInstanceReady callbacks, where it's guaranteed that the instance() will be
available.

The intention is that for every place that is currently logging *something*, or
doing null/version checks, we use GetInterfaceForMethod(). otherwise, the
instance() is still available.

Param name change sounds good, but let me choose something less verbose for the
version zero case.

[Yusuke Sato, 2016/09/17 03:22:06]

Sure, thanks for the explanation. All you mentioned sgtm.

[Luis Héctor Chávez, 2016/09/17 03:52:14]

Done (hooray for regex!)

+    if (!instance_) {
+      VLOG(1) << __PRETTY_FUNCTION__ << " Instance not available.";
+      return nullptr;
+    }
+    if (version_ < min_version) {
+      VLOG(1) << __PRETTY_FUNCTION__ << " 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) {
+    if (!instance_) {

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

If we add GetInstanceForVersionZero(), we can/should probably add DCHECK_LT(0U,
min_version) << method_name; here.

[Luis Héctor Chávez, 2016/09/17 03:52:14]

Done.

+      VLOG(1) << __PRETTY_FUNCTION__ << " Instance not available to call "
+              << method_name << ".";
+      return nullptr;
+    }
+    if (version_ < min_version) {
+      VLOG(1) << __PRETTY_FUNCTION__ << " 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.