arc: Add InstanceHelper::GetInstanceForMethod()

arc: Add InstanceHelper::GetInstanceForMethod()

Many developers have added utility functions to check if the many Mojo
instances are ready and with the correct version, which seems redundant.
This change adds a function to do that (and log failures).

BUG=647853
TEST=cheets_SettingsBridge

Committed: https://crrev.com/3faadf3336fe654e2cde8f16daacd136585026a1
Cr-Commit-Position: refs/heads/master@{#419471}

[Luis Héctor Chávez, 2016-09-16 21:37:14]

The CQ bit was checked by lhchavez@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-16 21:37:43]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-16 22:23:42]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-16 22:23:43]

Dry run: This issue passed the CQ dry run.

[Luis Héctor Chávez, 2016-09-16 22:54:36]

Description was changed from

==========
arc: Add InstanceHelper::GetInstanceForVersion()

Many developers have added utility functions to check if the many Mojo
instances are ready and with the correct version, which seems redundant.
This change adds a function to do that (and log failures).

BUG=None
TEST=cheets_SettingsBridge
==========

to

==========
arc: Add InstanceHelper::GetInstanceForVersion()

Many developers have added utility functions to check if the many Mojo
instances are ready and with the correct version, which seems redundant.
This change adds a function to do that (and log failures).

BUG=647853
TEST=cheets_SettingsBridge
==========

[Luis Héctor Chávez, 2016-09-16 22:55:40]

lhchavez@chromium.org changed reviewers:
+ afakhry@chromium.org, xiyuan@chromium.org, yusukes@chromium.org

[Luis Héctor Chávez, 2016-09-16 22:55:41]

PTAL this small cleanup

afakhry: chrome/browser/task_manager/
xiyuan: chrome/browser/ui
yusukes: */arc

[xiyuan, 2016-09-16 22:58:59]

c/b/ui lgtm

[Yusuke Sato, 2016-09-16 23:58:49]

nice!

https://codereview.chromium.org/2347293002/diff/20001/chrome/browser/task_man...
File chrome/browser/task_manager/providers/arc/arc_process_task.cc (right):

https://codereview.chromium.org/2347293002/diff/20001/chrome/browser/task_man...
chrome/browser/task_manager/providers/arc/arc_process_task.cc:1: // Copyright
2015 The Chromium Authors. All rights reserved.
Can you also modify the version() check in
ArcIntentHelperBridge::GetIntentHelperInstanceWithErrorCode()?

https://codereview.chromium.org/2347293002/diff/20001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc (right):

https://codereview.chromium.org/2347293002/diff/20001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc:357: auto* app_instance =
app_instance_holder_->GetInstanceForVersion(
is * necessary here?

(ok, confirmed this with Luis. In Chromium, it is encouraged (or enforced?) to
use * where applicable. crbug.com/554600)

https://codereview.chromium.org/2347293002/diff/20001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/arc/arc_app_utils.cc (right):

https://codereview.chromium.org/2347293002/diff/20001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/arc/arc_app_utils.cc:34: constexpr int kMinVersion =
0;
uint32_t (line 34-41)

https://codereview.chromium.org/2347293002/diff/20001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/arc/arc_app_utils.cc:54: arc::mojom::AppInstance*
GetAppInstance(int required_version,
What about changing the type to uint32_t while you're at it?

https://codereview.chromium.org/2347293002/diff/20001/components/arc/bluetoot...
File components/arc/bluetooth/arc_bluetooth_bridge.cc (right):

https://codereview.chromium.org/2347293002/diff/20001/components/arc/bluetoot...
components/arc/bluetooth/arc_bluetooth_bridge.cc:347:
arc_bridge_service()->bluetooth()->instance()->OnLEDeviceFound(
btle_instance->OnLEDevice...(

https://codereview.chromium.org/2347293002/diff/20001/components/arc/bluetoot...
components/arc/bluetooth/arc_bluetooth_bridge.cc:473:
arc_bridge_service()->bluetooth()->instance()->OnSearchComplete(
same

https://codereview.chromium.org/2347293002/diff/20001/components/arc/bluetoot...
components/arc/bluetooth/arc_bluetooth_bridge.cc:538:
arc_bridge_service()->bluetooth()->instance()->OnGattNotify(
same

https://codereview.chromium.org/2347293002/diff/20001/components/arc/ime/arc_...
File components/arc/ime/arc_ime_bridge_impl.cc (right):

https://codereview.chromium.org/2347293002/diff/20001/components/arc/ime/arc_...
components/arc/ime/arc_ime_bridge_impl.cc:19: constexpr uint32_t
kMinInstanceVersion = 0;
You didn't introduce version zero checks for BT, so this is a bit inconsistent,
I think.

https://codereview.chromium.org/2347293002/diff/20001/components/arc/ime/arc_...
components/arc/ime/arc_ime_bridge_impl.cc:118: kMinInstanceVersion,
"SendInsertText");
s/Send// ?

https://codereview.chromium.org/2347293002/diff/20001/components/arc/instance...
File components/arc/instance_holder.h (right):

https://codereview.chromium.org/2347293002/diff/20001/components/arc/instance...
components/arc/instance_holder.h:43: uint32_t version() const { return version_;
}
Can we remove this now? If not, what's preventing us from doing so?

https://codereview.chromium.org/2347293002/diff/20001/components/arc/instance...
components/arc/instance_holder.h:62: T* GetInstanceForVersion(uint32_t
min_version, const char* method_name) {
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?

https://codereview.chromium.org/2347293002/diff/20001/components/arc/net/arc_...
File components/arc/net/arc_net_host_impl.cc (right):

https://codereview.chromium.org/2347293002/diff/20001/components/arc/net/arc_...
components/arc/net/arc_net_host_impl.cc:31: constexpr uint32_t
kDefaultMinInstanceVersion = 0;
same, remove?

https://codereview.chromium.org/2347293002/diff/20001/components/arc/net/arc_...
components/arc/net/arc_net_host_impl.cc:33: constexpr uint32_t
kDeviceListChangedMinInstanceVersion = 3;
I think this should be renamed to kWifiEnabledStateChanged.

https://codereview.chromium.org/2347293002/diff/20001/components/arc/net/arc_...
components/arc/net/arc_net_host_impl.cc:608: kDefaultMinInstanceVersion,
"GetDefaultNetwork");
* Not sure why kDefaultMinInstanceVersion is used here. I believe this should be
kDefaultNetworkChangedMinInstanceVersion.
* Also, the second param should be "DefaultNetworkChanged"?

https://codereview.chromium.org/2347293002/diff/20001/components/arc/net/arc_...
components/arc/net/arc_net_host_impl.cc:618: auto* net_instance =
arc_bridge_service()->net()->GetInstanceForVersion(
I think you can move this to line 624.

https://codereview.chromium.org/2347293002/diff/20001/components/arc/net/arc_...
components/arc/net/arc_net_host_impl.cc:640:
kDeviceListChangedMinInstanceVersion, "DeviceListChanged");
      kWifiEnabledStateChanged, "WifiEnabledStateChanged");

https://codereview.chromium.org/2347293002/diff/20001/components/arc/storage_...
File components/arc/storage_manager/arc_storage_manager.cc (right):

https://codereview.chromium.org/2347293002/diff/20001/components/arc/storage_...
components/arc/storage_manager/arc_storage_manager.cc:56: kMinInstanceVersion,
"GetApplicationSize");
Application*s*

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

https://codereview.chromium.org/2347293002/diff/20001/chrome/browser/task_man...
File chrome/browser/task_manager/providers/arc/arc_process_task.cc (right):

https://codereview.chromium.org/2347293002/diff/20001/chrome/browser/task_man...
chrome/browser/task_manager/providers/arc/arc_process_task.cc:1: // Copyright
2015 The Chromium Authors. All rights reserved.
On 2016/09/16 23:58:48, Yusuke Sato wrote:
> Can you also modify the version() check in
> ArcIntentHelperBridge::GetIntentHelperInstanceWithErrorCode()?

Discussed offline. I'll keep the code as-is because
GetIntentHelperInstanceWithErrorCode needs to disambiguate between not-ready and
not-supported cases to return the correct error code.

https://codereview.chromium.org/2347293002/diff/20001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc (right):

https://codereview.chromium.org/2347293002/diff/20001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc:357: auto* app_instance =
app_instance_holder_->GetInstanceForVersion(
On 2016/09/16 23:58:48, Yusuke Sato wrote:
> is * necessary here?
> 
> (ok, confirmed this with Luis. In Chromium, it is encouraged (or enforced?) to
> use * where applicable. crbug.com/554600)

Acknowledged.

https://codereview.chromium.org/2347293002/diff/20001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/arc/arc_app_utils.cc (right):

https://codereview.chromium.org/2347293002/diff/20001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/arc/arc_app_utils.cc:34: constexpr int kMinVersion =
0;
On 2016/09/16 23:58:48, Yusuke Sato wrote:
> uint32_t (line 34-41)

Good catch, done.

https://codereview.chromium.org/2347293002/diff/20001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/arc/arc_app_utils.cc:54: arc::mojom::AppInstance*
GetAppInstance(int required_version,
On 2016/09/16 23:58:48, Yusuke Sato wrote:
> What about changing the type to uint32_t while you're at it?

Done.

https://codereview.chromium.org/2347293002/diff/20001/components/arc/bluetoot...
File components/arc/bluetooth/arc_bluetooth_bridge.cc (right):

https://codereview.chromium.org/2347293002/diff/20001/components/arc/bluetoot...
components/arc/bluetooth/arc_bluetooth_bridge.cc:347:
arc_bridge_service()->bluetooth()->instance()->OnLEDeviceFound(
On 2016/09/16 23:58:48, Yusuke Sato wrote:
> btle_instance->OnLEDevice...(

Done.

https://codereview.chromium.org/2347293002/diff/20001/components/arc/bluetoot...
components/arc/bluetooth/arc_bluetooth_bridge.cc:473:
arc_bridge_service()->bluetooth()->instance()->OnSearchComplete(
On 2016/09/16 23:58:48, Yusuke Sato wrote:
> same

Done.

https://codereview.chromium.org/2347293002/diff/20001/components/arc/bluetoot...
components/arc/bluetooth/arc_bluetooth_bridge.cc:538:
arc_bridge_service()->bluetooth()->instance()->OnGattNotify(
On 2016/09/16 23:58:48, Yusuke Sato wrote:
> same

Done.

https://codereview.chromium.org/2347293002/diff/20001/components/arc/ime/arc_...
File components/arc/ime/arc_ime_bridge_impl.cc (right):

https://codereview.chromium.org/2347293002/diff/20001/components/arc/ime/arc_...
components/arc/ime/arc_ime_bridge_impl.cc:19: constexpr uint32_t
kMinInstanceVersion = 0;
On 2016/09/16 23:58:48, Yusuke Sato wrote:
> You didn't introduce version zero checks for BT, so this is a bit
inconsistent,
> I think.

Added version zero checks for BT.

https://codereview.chromium.org/2347293002/diff/20001/components/arc/ime/arc_...
components/arc/ime/arc_ime_bridge_impl.cc:118: kMinInstanceVersion,
"SendInsertText");
On 2016/09/16 23:58:48, Yusuke Sato wrote:
> s/Send// ?

Done.

https://codereview.chromium.org/2347293002/diff/20001/components/arc/instance...
File components/arc/instance_holder.h (right):

https://codereview.chromium.org/2347293002/diff/20001/components/arc/instance...
components/arc/instance_holder.h:43: uint32_t version() const { return version_;
}
On 2016/09/16 23:58:48, Yusuke Sato wrote:
> Can we remove this now? If not, what's preventing us from doing so?

GetIntentHelperInstanceWithErrorCode() still needs it to disambiguate.

https://codereview.chromium.org/2347293002/diff/20001/components/arc/instance...
components/arc/instance_holder.h:62: T* GetInstanceForVersion(uint32_t
min_version, const char* method_name) {
On 2016/09/16 23:58:48, Yusuke Sato wrote:
> 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?

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).

https://codereview.chromium.org/2347293002/diff/20001/components/arc/net/arc_...
File components/arc/net/arc_net_host_impl.cc (right):

https://codereview.chromium.org/2347293002/diff/20001/components/arc/net/arc_...
components/arc/net/arc_net_host_impl.cc:31: constexpr uint32_t
kDefaultMinInstanceVersion = 0;
On 2016/09/16 23:58:49, Yusuke Sato wrote:
> same, remove?

Acknowledged.

https://codereview.chromium.org/2347293002/diff/20001/components/arc/net/arc_...
components/arc/net/arc_net_host_impl.cc:33: constexpr uint32_t
kDeviceListChangedMinInstanceVersion = 3;
On 2016/09/16 23:58:49, Yusuke Sato wrote:
> I think this should be renamed to kWifiEnabledStateChanged.

Done.

https://codereview.chromium.org/2347293002/diff/20001/components/arc/net/arc_...
components/arc/net/arc_net_host_impl.cc:608: kDefaultMinInstanceVersion,
"GetDefaultNetwork");
On 2016/09/16 23:58:49, Yusuke Sato wrote:
> * Not sure why kDefaultMinInstanceVersion is used here. I believe this should
be
> kDefaultNetworkChangedMinInstanceVersion.
> * Also, the second param should be "DefaultNetworkChanged"?
> 

The thing is that there are two "optional" parameters to this function: the
version (default to 0), and the method name (default to nullptr).  I didn't want
to create 4 very similar functions, so adding the version=0 was a compromise.

I can also move the version to a template parameter (with a default of 0 so it
can be omitted), but i'm not sure if that's more or less readable.

https://codereview.chromium.org/2347293002/diff/20001/components/arc/net/arc_...
components/arc/net/arc_net_host_impl.cc:618: auto* net_instance =
arc_bridge_service()->net()->GetInstanceForVersion(
On 2016/09/16 23:58:49, Yusuke Sato wrote:
> I think you can move this to line 624.

Done.

https://codereview.chromium.org/2347293002/diff/20001/components/arc/net/arc_...
components/arc/net/arc_net_host_impl.cc:640:
kDeviceListChangedMinInstanceVersion, "DeviceListChanged");
On 2016/09/16 23:58:49, Yusuke Sato wrote:
>       kWifiEnabledStateChanged, "WifiEnabledStateChanged");
> 

Done.

https://codereview.chromium.org/2347293002/diff/20001/components/arc/storage_...
File components/arc/storage_manager/arc_storage_manager.cc (right):

https://codereview.chromium.org/2347293002/diff/20001/components/arc/storage_...
components/arc/storage_manager/arc_storage_manager.cc:56: kMinInstanceVersion,
"GetApplicationSize");
On 2016/09/16 23:58:49, Yusuke Sato wrote:
> Application*s*

Done.

[afakhry, 2016-09-17 00:40:49]

task_manager lgtm

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

https://codereview.chromium.org/2347293002/diff/20001/components/arc/bluetoot...
File components/arc/bluetooth/arc_bluetooth_bridge.cc (right):

https://codereview.chromium.org/2347293002/diff/20001/components/arc/bluetoot...
components/arc/bluetooth/arc_bluetooth_bridge.cc:1884: bool
ArcBluetoothBridge::HasBluetoothInstance() const {
nice!

https://codereview.chromium.org/2347293002/diff/20001/components/arc/ime/arc_...
File components/arc/ime/arc_ime_bridge_impl.cc (right):

https://codereview.chromium.org/2347293002/diff/20001/components/arc/ime/arc_...
components/arc/ime/arc_ime_bridge_impl.cc:19: constexpr uint32_t
kMinInstanceVersion = 0;
On 2016/09/17 00:30:53, Luis Héctor Chávez wrote:
> On 2016/09/16 23:58:48, Yusuke Sato wrote:
> > You didn't introduce version zero checks for BT, so this is a bit
> inconsistent,
> > I think.
> 
> Added version zero checks for BT.

Ok, so I think you're saying that we should always use GetInstanceForVersion()
even when the minimum required version is 0, correct?

Assuming so, that seems like a good practice as using the raw instance()
function is more error prone.

https://codereview.chromium.org/2347293002/diff/20001/components/arc/instance...
File components/arc/instance_holder.h (right):

https://codereview.chromium.org/2347293002/diff/20001/components/arc/instance...
components/arc/instance_holder.h:62: T* GetInstanceForVersion(uint32_t
min_version, const char* method_name) {
On 2016/09/17 00:30:53, Luis Héctor Chávez wrote:
> On 2016/09/16 23:58:48, Yusuke Sato wrote:
> > 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?
> 
> 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).

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

https://codereview.chromium.org/2347293002/diff/100001/components/arc/instanc...
File components/arc/instance_holder.h (right):

https://codereview.chromium.org/2347293002/diff/100001/components/arc/instanc...
components/arc/instance_holder.h:42: T* instance() const { return instance_; }
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.

https://codereview.chromium.org/2347293002/diff/100001/components/arc/instanc...
components/arc/instance_holder.h:48: T* GetInstanceForVersion(uint32_t
min_version) {
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?

https://codereview.chromium.org/2347293002/diff/100001/components/arc/instanc...
components/arc/instance_holder.h:63: if (!instance_) {
If we add GetInstanceForVersionZero(), we can/should probably add DCHECK_LT(0U,
min_version) << method_name; here.

https://codereview.chromium.org/2347293002/diff/100001/components/arc/net/arc...
File components/arc/net/arc_net_host_impl.cc (right):

https://codereview.chromium.org/2347293002/diff/100001/components/arc/net/arc...
components/arc/net/arc_net_host_impl.cc:579: kDefaultMinInstanceVersion,
"ScanCompleted");
nit: ScanCompleted has [MinVersion=1] but version 0 is queries here.

https://codereview.chromium.org/2347293002/diff/100001/components/arc/net/arc...
components/arc/net/arc_net_host_impl.cc:608: kDefaultMinInstanceVersion,
"DefaultNetworkChanged");
Ok, then could you document why kDefaultMinInstanceVersion is used here?

https://codereview.chromium.org/2347293002/diff/100001/components/arc/net/arc...
components/arc/net/arc_net_host_impl.cc:620:
net_instance->DefaultNetworkChanged(nullptr, nullptr);
nit: this won't compile :)

I meant something like:

 if (!network) {
   VLOG(1) << "No ...";
   auto* net_instance = ...;
   if (net_instance)
     net_instance->DefaultNetworkChanged(...);
   return;
 }

https://codereview.chromium.org/2347293002/diff/100001/components/arc/net/arc...
components/arc/net/arc_net_host_impl.cc:625:
kDefaultNetworkChangedMinInstanceVersion, "DefaultNetworkChanged");
..and then, why this one was okay to use
kDefaultNetworkChangedMinInstanceVersion?

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

https://codereview.chromium.org/2347293002/diff/100001/components/arc/instanc...
File components/arc/instance_holder.h (right):

https://codereview.chromium.org/2347293002/diff/100001/components/arc/instanc...
components/arc/instance_holder.h:48: T* GetInstanceForVersion(uint32_t
min_version) {
On 2016/09/17 01:38:56, Yusuke Sato wrote:
> 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?

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?

https://codereview.chromium.org/2347293002/diff/100001/components/arc/net/arc...
File components/arc/net/arc_net_host_impl.cc (right):

https://codereview.chromium.org/2347293002/diff/100001/components/arc/net/arc...
components/arc/net/arc_net_host_impl.cc:579: kDefaultMinInstanceVersion,
"ScanCompleted");
On 2016/09/17 01:38:57, Yusuke Sato wrote:
> nit: ScanCompleted has [MinVersion=1] but version 0 is queries here.

Done.

https://codereview.chromium.org/2347293002/diff/100001/components/arc/net/arc...
components/arc/net/arc_net_host_impl.cc:608: kDefaultMinInstanceVersion,
"DefaultNetworkChanged");
On 2016/09/17 01:38:57, Yusuke Sato wrote:
> Ok, then could you document why kDefaultMinInstanceVersion is used here?

Oh there was a bug here! This should have been
kDefaultNetworkChangedMinInstanceVersion.

https://codereview.chromium.org/2347293002/diff/100001/components/arc/net/arc...
components/arc/net/arc_net_host_impl.cc:620:
net_instance->DefaultNetworkChanged(nullptr, nullptr);
On 2016/09/17 01:38:57, Yusuke Sato wrote:
> nit: this won't compile :)
> 
> I meant something like:
> 
>  if (!network) {
>    VLOG(1) << "No ...";
>    auto* net_instance = ...;
>    if (net_instance)
>      net_instance->DefaultNetworkChanged(...);
>    return;
>  }

Yeah, realized this too late :P Done.

https://codereview.chromium.org/2347293002/diff/100001/components/arc/net/arc...
components/arc/net/arc_net_host_impl.cc:625:
kDefaultNetworkChangedMinInstanceVersion, "DefaultNetworkChanged");
On 2016/09/17 01:38:57, Yusuke Sato wrote:
> ..and then, why this one was okay to use
> kDefaultNetworkChangedMinInstanceVersion?

Both should use it.

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

https://codereview.chromium.org/2347293002/diff/100001/components/arc/instanc...
File components/arc/instance_holder.h (right):

https://codereview.chromium.org/2347293002/diff/100001/components/arc/instanc...
components/arc/instance_holder.h:48: T* GetInstanceForVersion(uint32_t
min_version) {
On 2016/09/17 01:56:11, Luis Héctor Chávez wrote:
> On 2016/09/17 01:38:56, Yusuke Sato wrote:
> > 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?
> 
> 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?

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:35]

https://codereview.chromium.org/2347293002/diff/100001/components/arc/instanc...
File components/arc/instance_holder.h (right):

https://codereview.chromium.org/2347293002/diff/100001/components/arc/instanc...
components/arc/instance_holder.h:48: T* GetInstanceForVersion(uint32_t
min_version) {
On 2016/09/17 02:42:53, Yusuke Sato wrote:
> On 2016/09/17 01:56:11, Luis Héctor Chávez wrote:
> > On 2016/09/17 01:38:56, Yusuke Sato wrote:
> > > 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?
> > 
> > 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?
> 
> 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.

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]

https://codereview.chromium.org/2347293002/diff/100001/components/arc/instanc...
File components/arc/instance_holder.h (right):

https://codereview.chromium.org/2347293002/diff/100001/components/arc/instanc...
components/arc/instance_holder.h:48: T* GetInstanceForVersion(uint32_t
min_version) {
On 2016/09/17 03:16:34, Luis Héctor Chávez wrote:
> On 2016/09/17 02:42:53, Yusuke Sato wrote:
> > On 2016/09/17 01:56:11, Luis Héctor Chávez wrote:
> > > On 2016/09/17 01:38:56, Yusuke Sato wrote:
> > > > 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?
> > > 
> > > 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?
> > 
> > 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.
> 
> 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.

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

[Luis Héctor Chávez, 2016-09-17 03:45:43]

Description was changed from

==========
arc: Add InstanceHelper::GetInstanceForVersion()

Many developers have added utility functions to check if the many Mojo
instances are ready and with the correct version, which seems redundant.
This change adds a function to do that (and log failures).

BUG=647853
TEST=cheets_SettingsBridge
==========

to

==========
arc: Add InstanceHelper::GetInstanceForMethod()

Many developers have added utility functions to check if the many Mojo
instances are ready and with the correct version, which seems redundant.
This change adds a function to do that (and log failures).

BUG=647853
TEST=cheets_SettingsBridge
==========

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

The CQ bit was checked by lhchavez@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-17 03:52:12]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

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

https://codereview.chromium.org/2347293002/diff/100001/components/arc/instanc...
File components/arc/instance_holder.h (right):

https://codereview.chromium.org/2347293002/diff/100001/components/arc/instanc...
components/arc/instance_holder.h:42: T* instance() const { return instance_; }
On 2016/09/17 01:38:56, Yusuke Sato wrote:
> 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.

Resolved to keep.

https://codereview.chromium.org/2347293002/diff/100001/components/arc/instanc...
components/arc/instance_holder.h:48: T* GetInstanceForVersion(uint32_t
min_version) {
On 2016/09/17 03:22:06, Yusuke Sato wrote:
> On 2016/09/17 03:16:34, Luis Héctor Chávez wrote:
> > On 2016/09/17 02:42:53, Yusuke Sato wrote:
> > > On 2016/09/17 01:56:11, Luis Héctor Chávez wrote:
> > > > On 2016/09/17 01:38:56, Yusuke Sato wrote:
> > > > > 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?
> > > > 
> > > > 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?
> > > 
> > > 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.
> > 
> > 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.
> 
> Sure, thanks for the explanation. All you mentioned sgtm.

Done (hooray for regex!)

https://codereview.chromium.org/2347293002/diff/100001/components/arc/instanc...
components/arc/instance_holder.h:63: if (!instance_) {
On 2016/09/17 01:38:57, Yusuke Sato wrote:
> If we add GetInstanceForVersionZero(), we can/should probably add
DCHECK_LT(0U,
> min_version) << method_name; here.

Done.

[Yusuke Sato, 2016-09-17 04:28:05]

thanks! lgtm

https://codereview.chromium.org/2347293002/diff/160001/components/arc/instanc...
File components/arc/instance_holder.h (right):

https://codereview.chromium.org/2347293002/diff/160001/components/arc/instanc...
components/arc/instance_holder.h:51: DCHECK_LT(0u, min_version) << T::Name_ <<
"::" << method_name_for_logging;
nit: you can't have this check anymore since line 68 passes 0u in.

[commit-bot: I haz the power, 2016-09-17 04:31:47]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-17 04:31:48]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Luis Héctor Chávez, 2016-09-17 13:35:11]

The CQ bit was checked by lhchavez@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-17 13:35:23]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-17 14:13:52]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-17 14:13:53]

Dry run: This issue passed the CQ dry run.

[Luis Héctor Chávez, 2016-09-19 15:33:11]

https://codereview.chromium.org/2347293002/diff/160001/components/arc/instanc...
File components/arc/instance_holder.h (right):

https://codereview.chromium.org/2347293002/diff/160001/components/arc/instanc...
components/arc/instance_holder.h:51: DCHECK_LT(0u, min_version) << T::Name_ <<
"::" << method_name_for_logging;
On 2016/09/17 04:28:05, Yusuke Sato wrote:
> nit: you can't have this check anymore since line 68 passes 0u in.

Done.

[Luis Héctor Chávez, 2016-09-19 15:33:18]

The CQ bit was checked by lhchavez@chromium.org

[Luis Héctor Chávez, 2016-09-19 15:33:19]

The patchset sent to the CQ was uploaded after l-g-t-m from xiyuan@chromium.org,
afakhry@chromium.org, yusukes@chromium.org
Link to the patchset: https://codereview.chromium.org/2347293002/#ps180001
(title: "No more DCHECK")

[commit-bot: I haz the power, 2016-09-19 15:33:50]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-19 16:00:11]

Description was changed from

==========
arc: Add InstanceHelper::GetInstanceForMethod()

Many developers have added utility functions to check if the many Mojo
instances are ready and with the correct version, which seems redundant.
This change adds a function to do that (and log failures).

BUG=647853
TEST=cheets_SettingsBridge
==========

to

==========
arc: Add InstanceHelper::GetInstanceForMethod()

Many developers have added utility functions to check if the many Mojo
instances are ready and with the correct version, which seems redundant.
This change adds a function to do that (and log failures).

BUG=647853
TEST=cheets_SettingsBridge
==========

[commit-bot: I haz the power, 2016-09-19 16:00:12]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2016-09-19 16:03:09]

Description was changed from

==========
arc: Add InstanceHelper::GetInstanceForMethod()

Many developers have added utility functions to check if the many Mojo
instances are ready and with the correct version, which seems redundant.
This change adds a function to do that (and log failures).

BUG=647853
TEST=cheets_SettingsBridge
==========

to

==========
arc: Add InstanceHelper::GetInstanceForMethod()

Many developers have added utility functions to check if the many Mojo
instances are ready and with the correct version, which seems redundant.
This change adds a function to do that (and log failures).

BUG=647853
TEST=cheets_SettingsBridge

Committed: https://crrev.com/3faadf3336fe654e2cde8f16daacd136585026a1
Cr-Commit-Position: refs/heads/master@{#419471}
==========

[commit-bot: I haz the power, 2016-09-19 16:03:10]

Patchset 10 (id:??) landed as
https://crrev.com/3faadf3336fe654e2cde8f16daacd136585026a1
Cr-Commit-Position: refs/heads/master@{#419471}