Always use arc::InstanceHolder<T>::GetInstanceForMethod

Always use arc::InstanceHolder<T>::GetInstanceForMethod

Previously, both arc::InstanceHolder<T>::instance() with some manual
version checks (or without any version checks by mistake) and
arc::InstanceHolder<T>::GetInstanceForMethod() are used for getting
an instance pointer. This CL removes the former so that the relatively
unsafe code won't be used as a template in the future.

Details:

* Use GetInstanceForMethod() even for Init (because it is not always
  true that Init is with MinVersion=0). Add missing version checks
  so that such code won't be copied when adding a new ARC service.
* Add nullptr checks to all OnInstanceReady functions for consistency.
* Change the return type of instance() from T* to bool and rename it
  to HasInstance() because all remaining instance() usages are inside
  if statement.
* Remove version() to prevent developers from using the unary version
  of GetInstanceForMethod() and version().
* Change the log level for the version mismatch log in
  GetInstanceForMethod() from VLOG(2) to LOG(ERROR) because most of
  the existing code I removed in favor of GetInstanceForMethod's
  logging used LOG(ERROR).
* Use uint32_t for min versions.
* Add #includes to remove IWYU lint warnings

BUG=647853
TEST=try
TEST=ARC still starts without any unexpected error logs.
TBR=dmazzoni@chromium.org

Committed: https://crrev.com/34ceeb456810e5f24f2f233e30d98423385cb614
Cr-Commit-Position: refs/heads/master@{#421113}

[Yusuke Sato, 2016-09-21 03:47:41]

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

[commit-bot: I haz the power, 2016-09-21 03:48:00]

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-21 04:03:24]

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

[commit-bot: I haz the power, 2016-09-21 04:03:26]

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

[Yusuke Sato, 2016-09-21 04:23:15]

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

[commit-bot: I haz the power, 2016-09-21 04:23:35]

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-21 04:45:56]

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

[commit-bot: I haz the power, 2016-09-21 04:45:57]

Dry run: This issue passed the CQ dry run.

[Yusuke Sato, 2016-09-21 07:06:34]

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

[commit-bot: I haz the power, 2016-09-21 07:06:52]

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-21 07:31:17]

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

[commit-bot: I haz the power, 2016-09-21 07:31:18]

Dry run: This issue passed the CQ dry run.

[Yusuke Sato, 2016-09-21 18:10:36]

Description was changed from

==========
wip, poc

BUG=
==========

to

==========
Always use arc::InstanceHolder<T>::GetInstanceForMethod

Previously, both arc::InstanceHolder<T>::instance() with some manual
version checks (or without any version checks by mistake) and
arc::InstanceHolder<T>::GetInstanceForMethod() are used for getting
an instance pointer. This CL removes the former so that the relatively
unsafe code won't be used as a template in the future.

Details:

* Use GetInstanceForMethod() even for Init (because it is not always
  true that Init is with MinVersion=0). Add missing version checks
  so that such code won't be copied when adding a new ARC service.
* Add nullptr checks to all OnInstanceReady functions for consistency.
* Change the return type of instance() from T* to bool and rename it
  to HasInstance() because all remaining instance() usages are inside
  if statement.
* Rename version() to GetVersionForLogging() to prevent developers
  from using the unary version of GetInstanceForMethod() and
  version().
* Change the log level for the version mismatch log in
  GetInstanceForMethod() from VLOG(2) to LOG(ERROR) because most of
  the existing code I removed in favor of GetInstanceForMethod's
  logging used LOG(ERROR).
* Use uint32_t for min versions.
* Add #includes to remove IWYU lint warnings

BUG=647853
TEST=try
TEST=ARC still starts without any unexpected error logs.
==========

[Yusuke Sato, 2016-09-21 18:27:47]

yusukes@chromium.org changed reviewers:
+ lhchavez@chromium.org

[Yusuke Sato, 2016-09-21 18:27:48]

Tried to detect instance() calls without proper version checks, and actually
found many instances. Changed them to GetInstanceForMethod calls so that such
(mildly) unsafe code won't be copy-and-pasted when someone defines a new ARC
service. PTAL.

https://codereview.chromium.org/2357053002/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/arc/arc_auth_service.cc (right):

https://codereview.chromium.org/2357053002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/arc_auth_service.cc:233: if (!instance)
Previously, OnInstanceReady() methods were written in various style
(DCHECK(instance), if(!instance), if(!instance) w/ LOG(ERROR), if(!instance) w/
LOG(DFATAL), no null check, etc.) For now, I used 'if(!instance)' w/o logs for
all of them.

https://codereview.chromium.org/2357053002/diff/40001/chrome/browser/memory/t...
File chrome/browser/memory/tab_manager_delegate_chromeos.h (left):

https://codereview.chromium.org/2357053002/diff/40001/chrome/browser/memory/t...
chrome/browser/memory/tab_manager_delegate_chromeos.h:67: //
InstanceHolder<arc::mojom::ProcessInstance>::Observer overrides.
This doesn't seem to be necessary.

https://codereview.chromium.org/2357053002/diff/40001/chrome/browser/memory/t...
chrome/browser/memory/tab_manager_delegate_chromeos.h:179: // Holds a weak
pointer to arc::mojom::ProcessInstance.
same

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

https://codereview.chromium.org/2357053002/diff/40001/components/arc/instance...
components/arc/instance_holder.h:52: T* GetInstanceForMethod(const std::string&
method_name_for_logging,
Changed to string to disallow nullptr.

[Yusuke Sato, 2016-09-22 22:07:31]

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

[commit-bot: I haz the power, 2016-09-22 22:08:04]

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

[Yusuke Sato, 2016-09-22 22:26:28]

https://codereview.chromium.org/2357053002/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/arc/arc_auth_service.cc (right):

https://codereview.chromium.org/2357053002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/arc_auth_service.cc:233: if (!instance)
On 2016/09/21 18:27:48, Yusuke Sato wrote:
> Previously, OnInstanceReady() methods were written in various style
> (DCHECK(instance), if(!instance), if(!instance) w/ LOG(ERROR), if(!instance)
w/
> LOG(DFATAL), no null check, etc.) For now, I used 'if(!instance)' w/o logs for
> all of them.
> 

Changed to CHECK() based on your feedback. PTAL.

[commit-bot: I haz the power, 2016-09-22 22:33:46]

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

[commit-bot: I haz the power, 2016-09-22 22:33:48]

Dry run: This issue passed the CQ dry run.

[Luis Héctor Chávez, 2016-09-23 22:51:04]

Thanks for doing this :D

https://codereview.chromium.org/2357053002/diff/60001/chrome/browser/chromeos...
File chrome/browser/chromeos/arc/arc_navigation_throttle.cc (right):

https://codereview.chromium.org/2357053002/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/arc_navigation_throttle.cc:234: "HandleUrl",
kMinVersionForHandleUrl);
nit: Given that version 7 was added more than three months ago, I think in this
case it's safe to just check for AddPreferredPackage.

https://codereview.chromium.org/2357053002/diff/60001/chrome/browser/memory/t...
File chrome/browser/memory/tab_manager_delegate_chromeos.h (left):

https://codereview.chromium.org/2357053002/diff/60001/chrome/browser/memory/t...
chrome/browser/memory/tab_manager_delegate_chromeos.h:180:
arc::mojom::ProcessInstance* arc_process_instance_;
oh! good catch!

https://codereview.chromium.org/2357053002/diff/60001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/arc/arc_package_syncable_service.cc (right):

https://codereview.chromium.org/2357053002/diff/60001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/arc/arc_package_syncable_service.cc:438: if (!prefs_
|| !instance) {
You should check |prefs_| before L436.

https://codereview.chromium.org/2357053002/diff/60001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/arc/arc_package_syncable_service.cc:457: if (!prefs_
|| !instance) {
ditto.

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

https://codereview.chromium.org/2357053002/diff/60001/components/arc/instance...
components/arc/instance_holder.h:42: bool HasInstance() const { return
instance_; }
nit: has_instance()

https://codereview.chromium.org/2357053002/diff/60001/components/arc/instance...
components/arc/instance_holder.h:46: uint32_t GetVersionForLogging() const {
return version_; }
This one is fine since we want to discourage its use.

https://codereview.chromium.org/2357053002/diff/60001/components/arc/intent_h...
File components/arc/intent_helper/link_handler_model_impl.cc (right):

https://codereview.chromium.org/2357053002/diff/60001/components/arc/intent_h...
components/arc/intent_helper/link_handler_model_impl.cc:60:
"RequestUrlHandlerList", kMinInstanceVersion);
This is MinVersion=4.

[Luis Héctor Chávez, 2016-09-23 23:22:45]

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

https://codereview.chromium.org/2357053002/diff/60001/components/arc/instance...
components/arc/instance_holder.h:46: uint32_t GetVersionForLogging() const {
return version_; }
On 2016/09/23 22:51:04, Luis Héctor Chávez wrote:
> This one is fine since we want to discourage its use.

Oh wait, it's not being used anywhere! Let's get rid of it :) That way nobody
will even have the temptation.

[Yusuke Sato, 2016-09-23 23:37:10]

Description was changed from

==========
Always use arc::InstanceHolder<T>::GetInstanceForMethod

Previously, both arc::InstanceHolder<T>::instance() with some manual
version checks (or without any version checks by mistake) and
arc::InstanceHolder<T>::GetInstanceForMethod() are used for getting
an instance pointer. This CL removes the former so that the relatively
unsafe code won't be used as a template in the future.

Details:

* Use GetInstanceForMethod() even for Init (because it is not always
  true that Init is with MinVersion=0). Add missing version checks
  so that such code won't be copied when adding a new ARC service.
* Add nullptr checks to all OnInstanceReady functions for consistency.
* Change the return type of instance() from T* to bool and rename it
  to HasInstance() because all remaining instance() usages are inside
  if statement.
* Rename version() to GetVersionForLogging() to prevent developers
  from using the unary version of GetInstanceForMethod() and
  version().
* Change the log level for the version mismatch log in
  GetInstanceForMethod() from VLOG(2) to LOG(ERROR) because most of
  the existing code I removed in favor of GetInstanceForMethod's
  logging used LOG(ERROR).
* Use uint32_t for min versions.
* Add #includes to remove IWYU lint warnings

BUG=647853
TEST=try
TEST=ARC still starts without any unexpected error logs.
==========

to

==========
Always use arc::InstanceHolder<T>::GetInstanceForMethod

Previously, both arc::InstanceHolder<T>::instance() with some manual
version checks (or without any version checks by mistake) and
arc::InstanceHolder<T>::GetInstanceForMethod() are used for getting
an instance pointer. This CL removes the former so that the relatively
unsafe code won't be used as a template in the future.

Details:

* Use GetInstanceForMethod() even for Init (because it is not always
  true that Init is with MinVersion=0). Add missing version checks
  so that such code won't be copied when adding a new ARC service.
* Add nullptr checks to all OnInstanceReady functions for consistency.
* Change the return type of instance() from T* to bool and rename it
  to HasInstance() because all remaining instance() usages are inside
  if statement.
* Remove version() to prevent developers from using the unary version
  of GetInstanceForMethod() and version().
* Change the log level for the version mismatch log in
  GetInstanceForMethod() from VLOG(2) to LOG(ERROR) because most of
  the existing code I removed in favor of GetInstanceForMethod's
  logging used LOG(ERROR).
* Use uint32_t for min versions.
* Add #includes to remove IWYU lint warnings

BUG=647853
TEST=try
TEST=ARC still starts without any unexpected error logs.
==========

[Yusuke Sato, 2016-09-24 00:14:59]

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

[Yusuke Sato, 2016-09-24 00:15:23]

Thanks for the review! PTAL.

https://codereview.chromium.org/2357053002/diff/60001/chrome/browser/chromeos...
File chrome/browser/chromeos/arc/arc_navigation_throttle.cc (right):

https://codereview.chromium.org/2357053002/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/arc_navigation_throttle.cc:234: "HandleUrl",
kMinVersionForHandleUrl);
On 2016/09/23 22:51:03, Luis Héctor Chávez wrote:
> nit: Given that version 7 was added more than three months ago, I think in
this
> case it's safe to just check for AddPreferredPackage.

Yes that's true, but in terms of safety, probably 90% of version checks we have
right now are unnecessary by now.

My feeling is that removing or consolidating version checks based on practical
safety is somewhat complicated, and "always check, period" type of simpler
guideline would probably work better. Based on this, I'd keep two version checks
here to encourage developers to always check versions. WDYT?

https://codereview.chromium.org/2357053002/diff/60001/chrome/browser/memory/t...
File chrome/browser/memory/tab_manager_delegate_chromeos.h (left):

https://codereview.chromium.org/2357053002/diff/60001/chrome/browser/memory/t...
chrome/browser/memory/tab_manager_delegate_chromeos.h:180:
arc::mojom::ProcessInstance* arc_process_instance_;
On 2016/09/23 22:51:03, Luis Héctor Chávez wrote:
> oh! good catch!

Acknowledged.

https://codereview.chromium.org/2357053002/diff/60001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/arc/arc_package_syncable_service.cc (right):

https://codereview.chromium.org/2357053002/diff/60001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/arc/arc_package_syncable_service.cc:438: if (!prefs_
|| !instance) {
On 2016/09/23 22:51:04, Luis Héctor Chávez wrote:
> You should check |prefs_| before L436.

Done, thanks!

https://codereview.chromium.org/2357053002/diff/60001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/arc/arc_package_syncable_service.cc:457: if (!prefs_
|| !instance) {
On 2016/09/23 22:51:04, Luis Héctor Chávez wrote:
> ditto.

Done.

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

https://codereview.chromium.org/2357053002/diff/60001/components/arc/instance...
components/arc/instance_holder.h:42: bool HasInstance() const { return
instance_; }
On 2016/09/23 22:51:04, Luis Héctor Chávez wrote:
> nit: has_instance()

Done.

https://codereview.chromium.org/2357053002/diff/60001/components/arc/instance...
components/arc/instance_holder.h:46: uint32_t GetVersionForLogging() const {
return version_; }
On 2016/09/23 23:22:45, Luis Héctor Chávez wrote:
> On 2016/09/23 22:51:04, Luis Héctor Chávez wrote:
> > This one is fine since we want to discourage its use.
> 
> Oh wait, it's not being used anywhere! Let's get rid of it :) That way nobody
> will even have the temptation.

Ok, removed.

https://codereview.chromium.org/2357053002/diff/60001/components/arc/instance...
components/arc/instance_holder.h:46: uint32_t GetVersionForLogging() const {
return version_; }
On 2016/09/23 22:51:04, Luis Héctor Chávez wrote:
> This one is fine since we want to discourage its use.

Acknowledged.

https://codereview.chromium.org/2357053002/diff/60001/components/arc/intent_h...
File components/arc/intent_helper/link_handler_model_impl.cc (right):

https://codereview.chromium.org/2357053002/diff/60001/components/arc/intent_h...
components/arc/intent_helper/link_handler_model_impl.cc:60:
"RequestUrlHandlerList", kMinInstanceVersion);
On 2016/09/23 22:51:04, Luis Héctor Chávez wrote:
> This is MinVersion=4.

There are two similarly-named functions (oops..), and this one is with
MinVersion=2, I believe.

  // Requests a list of packages that can handle the URL.
  [MinVersion=2] RequestUrlHandlerList@3(string url)
      => (array<UrlHandlerInfo> handlers);

  // Requests a list of packages that can handle the list of files.
  [MinVersion=4] RequestUrlListHandlerList@6(array<UrlWithMimeType> urls)
      => (array<UrlHandlerInfo> handlers);

[commit-bot: I haz the power, 2016-09-24 00:15:36]

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

[dcheng, 2016-09-24 00:25:25]

https://codereview.chromium.org/2357053002/diff/80001/chrome/browser/chromeos...
File chrome/browser/chromeos/arc/arc_auth_service.cc (right):

https://codereview.chromium.org/2357053002/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/arc_auth_service.cc:233: CHECK(instance);
We should prefer DCHECK() over CHECK() to document things that should "never
happen". CHECK() is for debugging things in crash reports or for preventing
security bugs, neither of which appears to be the case here.

[dcheng, 2016-09-24 00:27:12]

dcheng@chromium.org changed reviewers:
+ dcheng@chromium.org

[dcheng, 2016-09-24 00:27:13]

https://codereview.chromium.org/2357053002/diff/80001/chrome/browser/chromeos...
File chrome/browser/chromeos/arc/arc_auth_service.cc (right):

https://codereview.chromium.org/2357053002/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/arc_auth_service.cc:233: CHECK(instance);
On 2016/09/24 00:25:25, dcheng wrote:
> We should prefer DCHECK() over CHECK() to document things that should "never
> happen". CHECK() is for debugging things in crash reports or for preventing
> security bugs, neither of which appears to be the case here.

See
https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md...
for more information.

[commit-bot: I haz the power, 2016-09-24 00:40:46]

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

[commit-bot: I haz the power, 2016-09-24 00:40:50]

Dry run: This issue passed the CQ dry run.

[Yusuke Sato, 2016-09-24 00:49:08]

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

[commit-bot: I haz the power, 2016-09-24 00:49:40]

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

[Yusuke Sato, 2016-09-24 00:50:54]

https://codereview.chromium.org/2357053002/diff/80001/chrome/browser/chromeos...
File chrome/browser/chromeos/arc/arc_auth_service.cc (right):

https://codereview.chromium.org/2357053002/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/arc_auth_service.cc:233: CHECK(instance);
On 2016/09/24 00:25:25, dcheng wrote:
> We should prefer DCHECK() over CHECK() to document things that should "never
> happen". CHECK() is for debugging things in crash reports or for preventing
> security bugs, neither of which appears to be the case here.

Done.

https://codereview.chromium.org/2357053002/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/arc_auth_service.cc:233: CHECK(instance);
On 2016/09/24 00:27:13, dcheng wrote:
> On 2016/09/24 00:25:25, dcheng wrote:
> > We should prefer DCHECK() over CHECK() to document things that should "never
> > happen". CHECK() is for debugging things in crash reports or for preventing
> > security bugs, neither of which appears to be the case here.
> 
> See
>
https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md...
> for more information.

Acknowledged.

[commit-bot: I haz the power, 2016-09-24 01:14:05]

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

[commit-bot: I haz the power, 2016-09-24 01:14:06]

Dry run: This issue passed the CQ dry run.

[Luis Héctor Chávez, 2016-09-24 02:00:00]

lgtm

https://codereview.chromium.org/2357053002/diff/60001/chrome/browser/chromeos...
File chrome/browser/chromeos/arc/arc_navigation_throttle.cc (right):

https://codereview.chromium.org/2357053002/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/arc_navigation_throttle.cc:234: "HandleUrl",
kMinVersionForHandleUrl);
On 2016/09/24 00:15:22, Yusuke Sato wrote:
> On 2016/09/23 22:51:03, Luis Héctor Chávez wrote:
> > nit: Given that version 7 was added more than three months ago, I think in
> this
> > case it's safe to just check for AddPreferredPackage.
> 
> Yes that's true, but in terms of safety, probably 90% of version checks we
have
> right now are unnecessary by now.
> 
> My feeling is that removing or consolidating version checks based on practical
> safety is somewhat complicated, and "always check, period" type of simpler
> guideline would probably work better. Based on this, I'd keep two version
checks
> here to encourage developers to always check versions. WDYT?

Fair enough.

[Yusuke Sato, 2016-09-26 05:12:27]

yusukes@chromium.org changed reviewers:
+ dmazzoni@chromium.org, georgesak@chromium.org, pavely@chromium.org,
xiyuan@chromium.org

[Yusuke Sato, 2016-09-26 05:12:28]

+more owners. Owners, this is a cleanup for ARC. Please take a look.

dmazzoni@
chrome/browser/speech/

georgesak@
chrome/browser/memory/

pavely@
chrome/browser/sync/test/integration/

xiyuan@
chrome/browser/ui/app_list/arc/
chrome/browser/chromeos/file_manager/

Thanks!

[xiyuan, 2016-09-26 16:20:43]

https://codereview.chromium.org/2357053002/diff/100001/chrome/browser/ui/app_...
File chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc (right):

https://codereview.chromium.org/2357053002/diff/100001/chrome/browser/ui/app_...
chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc:331: if (!app_instance)
nit: Can this be a DCHECK, same as L324 ? Or change L324 to an "if" ?

https://codereview.chromium.org/2357053002/diff/100001/chrome/browser/ui/app_...
File chrome/browser/ui/app_list/arc/arc_package_syncable_service.cc (right):

https://codereview.chromium.org/2357053002/diff/100001/chrome/browser/ui/app_...
chrome/browser/ui/app_list/arc/arc_package_syncable_service.cc:445: VLOG(2) <<
"Request to install package when bridge service is not ready: "
GetInstanceForMethod would generate a log message. Do we still need this? If we
do, the message should be updated.

https://codereview.chromium.org/2357053002/diff/100001/chrome/browser/ui/app_...
chrome/browser/ui/app_list/arc/arc_package_syncable_service.cc:470: VLOG(2) <<
"Request to uninstall package when bridge service is not ready: "
Remove or update the message.

[Yusuke Sato, 2016-09-26 16:43:23]

https://codereview.chromium.org/2357053002/diff/100001/chrome/browser/ui/app_...
File chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc (right):

https://codereview.chromium.org/2357053002/diff/100001/chrome/browser/ui/app_...
chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc:331: if (!app_instance)
On 2016/09/26 16:20:42, xiyuan wrote:
> nit: Can this be a DCHECK, same as L324 ? Or change L324 to an "if" ? 

Line 322 requests version 0 instance which should always succeed (because of
has_instance() test at line 309 has already passed) hence line 324 uses DCHECK.
However, line 329 requests more specific (newer) version of the instance which
may still fail. When it fails, I think logging and returning would be the
least-worst thing to do.

Added more code comments to clarify.

https://codereview.chromium.org/2357053002/diff/100001/chrome/browser/ui/app_...
File chrome/browser/ui/app_list/arc/arc_package_syncable_service.cc (right):

https://codereview.chromium.org/2357053002/diff/100001/chrome/browser/ui/app_...
chrome/browser/ui/app_list/arc/arc_package_syncable_service.cc:445: VLOG(2) <<
"Request to install package when bridge service is not ready: "
On 2016/09/26 16:20:43, xiyuan wrote:
> GetInstanceForMethod would generate a log message. Do we still need this? If
we
> do, the message should be updated.

Done. Removed the comment.

https://codereview.chromium.org/2357053002/diff/100001/chrome/browser/ui/app_...
chrome/browser/ui/app_list/arc/arc_package_syncable_service.cc:470: VLOG(2) <<
"Request to uninstall package when bridge service is not ready: "
On 2016/09/26 16:20:43, xiyuan wrote:
> Remove or update the message.

Done.

[xiyuan, 2016-09-26 16:45:54]

lgtm

c/b/ui/app_list/arc/
c/b/chromeos/file_manager/

https://codereview.chromium.org/2357053002/diff/100001/chrome/browser/ui/app_...
File chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc (right):

https://codereview.chromium.org/2357053002/diff/100001/chrome/browser/ui/app_...
chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc:331: if (!app_instance)
On 2016/09/26 16:43:22, Yusuke Sato wrote:
> On 2016/09/26 16:20:42, xiyuan wrote:
> > nit: Can this be a DCHECK, same as L324 ? Or change L324 to an "if" ? 
> 
> Line 322 requests version 0 instance which should always succeed (because of
> has_instance() test at line 309 has already passed) hence line 324 uses
DCHECK.
> However, line 329 requests more specific (newer) version of the instance which
> may still fail. When it fails, I think logging and returning would be the
> least-worst thing to do.
> 
> Added more code comments to clarify.

Okay. Thanks for the clarification and comments.

[Yusuke Sato, 2016-09-26 16:53:01]

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

[commit-bot: I haz the power, 2016-09-26 16:53:43]

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

[Georges Khalil, 2016-09-26 17:02:21]

lgtm for chrome/browser/memory/

[commit-bot: I haz the power, 2016-09-26 17:22:14]

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

[commit-bot: I haz the power, 2016-09-26 17:22:15]

Dry run: This issue passed the CQ dry run.

[pavely, 2016-09-26 18:01:09]

chrome/browser/sync/test/integration/ lgtm

[Eric Caruso, 2016-09-26 18:11:20]

components/arc/power/ lgtm

[Yusuke Sato, 2016-09-26 19:35:12]

Thanks all. Dominic, please take a look at chrome/browser/speech/.

[Yusuke Sato, 2016-09-27 05:10:05]

Description was changed from

==========
Always use arc::InstanceHolder<T>::GetInstanceForMethod

Previously, both arc::InstanceHolder<T>::instance() with some manual
version checks (or without any version checks by mistake) and
arc::InstanceHolder<T>::GetInstanceForMethod() are used for getting
an instance pointer. This CL removes the former so that the relatively
unsafe code won't be used as a template in the future.

Details:

* Use GetInstanceForMethod() even for Init (because it is not always
  true that Init is with MinVersion=0). Add missing version checks
  so that such code won't be copied when adding a new ARC service.
* Add nullptr checks to all OnInstanceReady functions for consistency.
* Change the return type of instance() from T* to bool and rename it
  to HasInstance() because all remaining instance() usages are inside
  if statement.
* Remove version() to prevent developers from using the unary version
  of GetInstanceForMethod() and version().
* Change the log level for the version mismatch log in
  GetInstanceForMethod() from VLOG(2) to LOG(ERROR) because most of
  the existing code I removed in favor of GetInstanceForMethod's
  logging used LOG(ERROR).
* Use uint32_t for min versions.
* Add #includes to remove IWYU lint warnings

BUG=647853
TEST=try
TEST=ARC still starts without any unexpected error logs.
==========

to

==========
Always use arc::InstanceHolder<T>::GetInstanceForMethod

Previously, both arc::InstanceHolder<T>::instance() with some manual
version checks (or without any version checks by mistake) and
arc::InstanceHolder<T>::GetInstanceForMethod() are used for getting
an instance pointer. This CL removes the former so that the relatively
unsafe code won't be used as a template in the future.

Details:

* Use GetInstanceForMethod() even for Init (because it is not always
  true that Init is with MinVersion=0). Add missing version checks
  so that such code won't be copied when adding a new ARC service.
* Add nullptr checks to all OnInstanceReady functions for consistency.
* Change the return type of instance() from T* to bool and rename it
  to HasInstance() because all remaining instance() usages are inside
  if statement.
* Remove version() to prevent developers from using the unary version
  of GetInstanceForMethod() and version().
* Change the log level for the version mismatch log in
  GetInstanceForMethod() from VLOG(2) to LOG(ERROR) because most of
  the existing code I removed in favor of GetInstanceForMethod's
  logging used LOG(ERROR).
* Use uint32_t for min versions.
* Add #includes to remove IWYU lint warnings

BUG=647853
TEST=try
TEST=ARC still starts without any unexpected error logs.
TBR=dmazzoni@chromium.org
==========

[Yusuke Sato, 2016-09-27 05:11:18]

Let me land this with TBR=dmazzoni as this is not really a TTS change but just
an ARC cleanup.

[Yusuke Sato, 2016-09-27 05:11:30]

The CQ bit was checked by yusukes@chromium.org

[Yusuke Sato, 2016-09-27 05:11:30]

The patchset sent to the CQ was uploaded after l-g-t-m from
lhchavez@chromium.org, xiyuan@chromium.org
Link to the patchset: https://codereview.chromium.org/2357053002/#ps140001
(title: "rebase, no code change")

[commit-bot: I haz the power, 2016-09-27 05:11:46]

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-27 05:17:24]

Description was changed from

==========
Always use arc::InstanceHolder<T>::GetInstanceForMethod

Previously, both arc::InstanceHolder<T>::instance() with some manual
version checks (or without any version checks by mistake) and
arc::InstanceHolder<T>::GetInstanceForMethod() are used for getting
an instance pointer. This CL removes the former so that the relatively
unsafe code won't be used as a template in the future.

Details:

* Use GetInstanceForMethod() even for Init (because it is not always
  true that Init is with MinVersion=0). Add missing version checks
  so that such code won't be copied when adding a new ARC service.
* Add nullptr checks to all OnInstanceReady functions for consistency.
* Change the return type of instance() from T* to bool and rename it
  to HasInstance() because all remaining instance() usages are inside
  if statement.
* Remove version() to prevent developers from using the unary version
  of GetInstanceForMethod() and version().
* Change the log level for the version mismatch log in
  GetInstanceForMethod() from VLOG(2) to LOG(ERROR) because most of
  the existing code I removed in favor of GetInstanceForMethod's
  logging used LOG(ERROR).
* Use uint32_t for min versions.
* Add #includes to remove IWYU lint warnings

BUG=647853
TEST=try
TEST=ARC still starts without any unexpected error logs.
TBR=dmazzoni@chromium.org
==========

to

==========
Always use arc::InstanceHolder<T>::GetInstanceForMethod

Previously, both arc::InstanceHolder<T>::instance() with some manual
version checks (or without any version checks by mistake) and
arc::InstanceHolder<T>::GetInstanceForMethod() are used for getting
an instance pointer. This CL removes the former so that the relatively
unsafe code won't be used as a template in the future.

Details:

* Use GetInstanceForMethod() even for Init (because it is not always
  true that Init is with MinVersion=0). Add missing version checks
  so that such code won't be copied when adding a new ARC service.
* Add nullptr checks to all OnInstanceReady functions for consistency.
* Change the return type of instance() from T* to bool and rename it
  to HasInstance() because all remaining instance() usages are inside
  if statement.
* Remove version() to prevent developers from using the unary version
  of GetInstanceForMethod() and version().
* Change the log level for the version mismatch log in
  GetInstanceForMethod() from VLOG(2) to LOG(ERROR) because most of
  the existing code I removed in favor of GetInstanceForMethod's
  logging used LOG(ERROR).
* Use uint32_t for min versions.
* Add #includes to remove IWYU lint warnings

BUG=647853
TEST=try
TEST=ARC still starts without any unexpected error logs.
TBR=dmazzoni@chromium.org
==========

[commit-bot: I haz the power, 2016-09-27 05:17:26]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-09-27 05:19:31]

Description was changed from

==========
Always use arc::InstanceHolder<T>::GetInstanceForMethod

Previously, both arc::InstanceHolder<T>::instance() with some manual
version checks (or without any version checks by mistake) and
arc::InstanceHolder<T>::GetInstanceForMethod() are used for getting
an instance pointer. This CL removes the former so that the relatively
unsafe code won't be used as a template in the future.

Details:

* Use GetInstanceForMethod() even for Init (because it is not always
  true that Init is with MinVersion=0). Add missing version checks
  so that such code won't be copied when adding a new ARC service.
* Add nullptr checks to all OnInstanceReady functions for consistency.
* Change the return type of instance() from T* to bool and rename it
  to HasInstance() because all remaining instance() usages are inside
  if statement.
* Remove version() to prevent developers from using the unary version
  of GetInstanceForMethod() and version().
* Change the log level for the version mismatch log in
  GetInstanceForMethod() from VLOG(2) to LOG(ERROR) because most of
  the existing code I removed in favor of GetInstanceForMethod's
  logging used LOG(ERROR).
* Use uint32_t for min versions.
* Add #includes to remove IWYU lint warnings

BUG=647853
TEST=try
TEST=ARC still starts without any unexpected error logs.
TBR=dmazzoni@chromium.org
==========

to

==========
Always use arc::InstanceHolder<T>::GetInstanceForMethod

Previously, both arc::InstanceHolder<T>::instance() with some manual
version checks (or without any version checks by mistake) and
arc::InstanceHolder<T>::GetInstanceForMethod() are used for getting
an instance pointer. This CL removes the former so that the relatively
unsafe code won't be used as a template in the future.

Details:

* Use GetInstanceForMethod() even for Init (because it is not always
  true that Init is with MinVersion=0). Add missing version checks
  so that such code won't be copied when adding a new ARC service.
* Add nullptr checks to all OnInstanceReady functions for consistency.
* Change the return type of instance() from T* to bool and rename it
  to HasInstance() because all remaining instance() usages are inside
  if statement.
* Remove version() to prevent developers from using the unary version
  of GetInstanceForMethod() and version().
* Change the log level for the version mismatch log in
  GetInstanceForMethod() from VLOG(2) to LOG(ERROR) because most of
  the existing code I removed in favor of GetInstanceForMethod's
  logging used LOG(ERROR).
* Use uint32_t for min versions.
* Add #includes to remove IWYU lint warnings

BUG=647853
TEST=try
TEST=ARC still starts without any unexpected error logs.
TBR=dmazzoni@chromium.org

Committed: https://crrev.com/34ceeb456810e5f24f2f233e30d98423385cb614
Cr-Commit-Position: refs/heads/master@{#421113}
==========

[commit-bot: I haz the power, 2016-09-27 05:19:33]

Patchset 8 (id:??) landed as
https://crrev.com/34ceeb456810e5f24f2f233e30d98423385cb614
Cr-Commit-Position: refs/heads/master@{#421113}