Get rid of RefCounted for ActivityIconLoader.

Get rid of RefCounted for ActivityIconLoader.

ActivityIconLoader was RefCounted.
However, all its access are on UI thread, and well managed.

This CL gets rid of RefCounted. Now ActivityIconLoader is a part of
ArcIntentHelperBridge. Its exposed method GetActivityIcons() is proxied by ArcIntentHelperBridge, and the class is put in internal namespace, in order to avoid accidental access from outside of components/arc/intent_helper.

BUG=n/a
TEST=Ran trybots.

Review-Url: https://codereview.chromium.org/2655233007
Cr-Commit-Position: refs/heads/master@{#447200}
Committed: https://chromium.googlesource.com/chromium/src/+/ccdfe0541dd3f6e7c85fc774772e3ff880730216

[hidehiko, 2017-01-27 15:12:01]

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

[commit-bot: I haz the power, 2017-01-27 15:12:20]

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, 2017-01-27 15:44:26]

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

[commit-bot: I haz the power, 2017-01-27 15:44:27]

Dry run: This issue passed the CQ dry run.

[hidehiko, 2017-01-27 16:04:05]

hidehiko@chromium.org changed reviewers:
+ afakhry@chromium.org, kinaba@chromium.org, yusukes@chromium.org

[hidehiko, 2017-01-27 16:04:06]

This is just for clean up. PTAL.

yusukes@: design, c/b/c/arc, components/arc.
kinaba@: */file_manager/*
afakhry@: c/b/task_manager

https://codereview.chromium.org/2655233007/diff/1/chrome/browser/chromeos/ext...
File chrome/browser/chromeos/extensions/file_manager/event_router.cc (right):

https://codereview.chromium.org/2655233007/diff/1/chrome/browser/chromeos/ext...
chrome/browser/chromeos/extensions/file_manager/event_router.cc:42: #include
"components/arc/intent_helper/arc_intent_helper_bridge.h"
Note: Was missing, although this was indirectly included.

https://codereview.chromium.org/2655233007/diff/1/components/arc/arc_service_...
File components/arc/arc_service_manager.h (right):

https://codereview.chromium.org/2655233007/diff/1/components/arc/arc_service_...
components/arc/arc_service_manager.h:19: #include "components/arc/arc_service.h"
Note: this is needed because AddServiceInternal() is actually called in the
inline method, which requires std::unique_ptr instantiation and destruction.
Specifically, its dtor requires ArcService's dtor.

https://codereview.chromium.org/2655233007/diff/1/components/arc/intent_helpe...
File components/arc/intent_helper/arc_intent_helper_bridge.h (right):

https://codereview.chromium.org/2655233007/diff/1/components/arc/intent_helpe...
components/arc/intent_helper/arc_intent_helper_bridge.h:60: ActivityIconLoader*
icon_loader() { return &icon_loader_; }
Note: or, alternatively we may want to provide GetActivityIcon() in this class,
which just proxies to ActivityIconLoader.
Then, external classes just can call ArcIntentHelperBridge::GetActivityIcon
directly, and ActivityIconLoader can be encapsulated.

The concern is, then, we would probably need many "using" to alias signatures in
ActivityIconLoader.

Yusuke, WDYT?

[Yusuke Sato, 2017-01-27 18:21:18]

lgtm with comments

https://codereview.chromium.org/2655233007/diff/1/components/arc/intent_helpe...
File components/arc/intent_helper/activity_icon_loader.cc (right):

https://codereview.chromium.org/2655233007/diff/1/components/arc/intent_helpe...
components/arc/intent_helper/activity_icon_loader.cc:54: // static
remove

https://codereview.chromium.org/2655233007/diff/1/components/arc/intent_helpe...
File components/arc/intent_helper/activity_icon_loader.h (right):

https://codereview.chromium.org/2655233007/diff/1/components/arc/intent_helpe...
components/arc/intent_helper/activity_icon_loader.h:113:
base::WeakPtrFactory<ActivityIconLoader> weak_ptr_factory_;
nit: Can you add the usual comment '// this member has to be the last blah blah'
here? I know it's redundant for you but maybe useful for others.

https://codereview.chromium.org/2655233007/diff/1/components/arc/intent_helpe...
components/arc/intent_helper/activity_icon_loader.h:114:
DISALLOW_COPY_AND_ASSIGN(ActivityIconLoader);
nit: space between L113/114?

https://codereview.chromium.org/2655233007/diff/1/components/arc/intent_helpe...
File components/arc/intent_helper/arc_intent_helper_bridge.h (right):

https://codereview.chromium.org/2655233007/diff/1/components/arc/intent_helpe...
components/arc/intent_helper/arc_intent_helper_bridge.h:60: ActivityIconLoader*
icon_loader() { return &icon_loader_; }
Just in case, can you mention that the returned pointer shouldn't be held in the
caller's class? (another idea is to explicitly return an weakptr but it's
probably overkill.)

[afakhry, 2017-01-28 01:08:44]

https://codereview.chromium.org/2655233007/diff/1/chrome/browser/task_manager...
File chrome/browser/task_manager/providers/arc/arc_process_task.cc (left):

https://codereview.chromium.org/2655233007/diff/1/chrome/browser/task_manager...
chrome/browser/task_manager/providers/arc/arc_process_task.cc:52:
scoped_refptr<arc::ActivityIconLoader> GetIconLoader() {
You could still keep this function:

arc::ActivityIconLoader* GetIconLoader() {
  auto* intent_helper_bridge =
      arc::ArcServiceManager::
      GetGlobalService<arc::ArcIntentHelperBridge>();
  if (!intent_helper_bridge)
    return nullptr;

  return intent_helper_bridge->icon_loader();
}

And then the code in StartIconLoading() remains the same.

[kinaba, 2017-01-30 01:07:16]

*/file_manager/* lgtm

[hidehiko, 2017-01-30 08:11:04]

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

[commit-bot: I haz the power, 2017-01-30 08:11:15]

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

[hidehiko, 2017-01-30 08:12:27]

Description was changed from

==========
Get rid of RefCounted for ActivityIconLoader.

ActivityIconLoader was RefCounted.
However, all its access are on UI thread, and well managed.

This CL gets rid of RefCounted. Now ActivityIconLoader is a part of
ArcIntentHelperBridge. All its access from external classes
are done via ArcServiceManager::GetGlobalService<ArcIntentHelperBridge>().

BUG=n/a
TEST=Ran trybots.
==========

to

==========
Get rid of RefCounted for ActivityIconLoader.

ActivityIconLoader was RefCounted.
However, all its access are on UI thread, and well managed.

This CL gets rid of RefCounted. Now ActivityIconLoader is a part of
ArcIntentHelperBridge. Its exposed method GetActivityIcons() is proxied by
ArcIntentHelperBridge, and the class is put in internal namespace, in order to
avoid accidental access from outside of components/arc/intent_helper.

BUG=n/a
TEST=Ran trybots.
==========

[commit-bot: I haz the power, 2017-01-30 08:59:54]

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

[commit-bot: I haz the power, 2017-01-30 08:59:55]

Dry run: This issue passed the CQ dry run.

[hidehiko, 2017-01-30 17:04:03]

Thank you for review.
PTAL afakhry@.

Yusuke, for better lifetime management in client,
I'd like to make a bit more refactoring. Could you take another look?

Best regards,
- hidehiko

https://codereview.chromium.org/2655233007/diff/1/chrome/browser/task_manager...
File chrome/browser/task_manager/providers/arc/arc_process_task.cc (left):

https://codereview.chromium.org/2655233007/diff/1/chrome/browser/task_manager...
chrome/browser/task_manager/providers/arc/arc_process_task.cc:52:
scoped_refptr<arc::ActivityIconLoader> GetIconLoader() {
On 2017/01/28 01:08:43, afakhry wrote:
> You could still keep this function:
> 
> arc::ActivityIconLoader* GetIconLoader() {
>   auto* intent_helper_bridge =
>       arc::ArcServiceManager::
>       GetGlobalService<arc::ArcIntentHelperBridge>();
>   if (!intent_helper_bridge)
>     return nullptr;
> 
>   return intent_helper_bridge->icon_loader();
> }
> 
> And then the code in StartIconLoading() remains the same.

Hmm... Could you elaborate what's the goal to keep this utility?
IMHO, because very similar functions are defined in anonymous namespace of
multiple files, I do not think keeping it here is a good idea.

Providing a utility by arc namespace is another option, but it looks redundant
to me because icon_loader() is always alive if ArcIntentHelperBridge is alive.

(Also considering Yusuke's review comment about lifetime of icon_loader,)
I introduced a ArcIntentHelperBridge::GetActivityIcons, which is just a proxy to
ActivityIconLoader::GetActivityIcons, so that clients do not need worried about
its lifetime.

WDYT?

https://codereview.chromium.org/2655233007/diff/1/components/arc/intent_helpe...
File components/arc/intent_helper/activity_icon_loader.cc (right):

https://codereview.chromium.org/2655233007/diff/1/components/arc/intent_helpe...
components/arc/intent_helper/activity_icon_loader.cc:54: // static
On 2017/01/27 18:21:17, Yusuke Sato wrote:
> remove

Done.

https://codereview.chromium.org/2655233007/diff/1/components/arc/intent_helpe...
File components/arc/intent_helper/activity_icon_loader.h (right):

https://codereview.chromium.org/2655233007/diff/1/components/arc/intent_helpe...
components/arc/intent_helper/activity_icon_loader.h:113:
base::WeakPtrFactory<ActivityIconLoader> weak_ptr_factory_;
On 2017/01/27 18:21:17, Yusuke Sato wrote:
> nit: Can you add the usual comment '// this member has to be the last blah
blah'
> here? I know it's redundant for you but maybe useful for others.

Done.

https://codereview.chromium.org/2655233007/diff/1/components/arc/intent_helpe...
components/arc/intent_helper/activity_icon_loader.h:114:
DISALLOW_COPY_AND_ASSIGN(ActivityIconLoader);
On 2017/01/27 18:21:17, Yusuke Sato wrote:
> nit: space between L113/114?

Done.

https://codereview.chromium.org/2655233007/diff/1/components/arc/intent_helpe...
File components/arc/intent_helper/arc_intent_helper_bridge.h (right):

https://codereview.chromium.org/2655233007/diff/1/components/arc/intent_helpe...
components/arc/intent_helper/arc_intent_helper_bridge.h:60: ActivityIconLoader*
icon_loader() { return &icon_loader_; }
On 2017/01/27 18:21:17, Yusuke Sato wrote:
> Just in case, can you mention that the returned pointer shouldn't be held in
the
> caller's class? (another idea is to explicitly return an weakptr but it's
> probably overkill.)

Instead, I removed this method, in the new PS.

https://codereview.chromium.org/2655233007/diff/40001/components/arc/intent_h...
File components/arc/intent_helper/arc_intent_helper_bridge.cc (left):

https://codereview.chromium.org/2655233007/diff/40001/components/arc/intent_h...
components/arc/intent_helper/arc_intent_helper_bridge.cc:133: bool
ArcIntentHelperBridge::IsIntentHelperAvailable(GetResult* out_error_code) {
Merged into GetInstanceForRequestActivityIcons() in activity_icon_loader.cc.

The goal to do it in this CL is to remove GetResult from ArcIntenthelperBridge,
because its name is conflicting with ActivityIconLoader::GetResult.

[afakhry, 2017-01-30 17:11:26]

c/b/task_manager lgtm.

[Yusuke Sato, 2017-01-30 20:38:26]

lgtm

https://codereview.chromium.org/2655233007/diff/40001/components/arc/intent_h...
File components/arc/intent_helper/activity_icon_loader.h (left):

https://codereview.chromium.org/2655233007/diff/40001/components/arc/intent_h...
components/arc/intent_helper/activity_icon_loader.h:15: #include
"base/memory/ref_counted.h"
refptr still in use at L37?

[hidehiko, 2017-01-31 05:54:08]

Thank you for review. Submitting.

https://codereview.chromium.org/2655233007/diff/40001/components/arc/intent_h...
File components/arc/intent_helper/activity_icon_loader.h (left):

https://codereview.chromium.org/2655233007/diff/40001/components/arc/intent_h...
components/arc/intent_helper/activity_icon_loader.h:15: #include
"base/memory/ref_counted.h"
On 2017/01/30 20:38:26, Yusuke Sato wrote:
> refptr still in use at L37?

Good catch. Revived.

[hidehiko, 2017-01-31 06:01:33]

The CQ bit was checked by hidehiko@chromium.org

[hidehiko, 2017-01-31 06:01:33]

The patchset sent to the CQ was uploaded after l-g-t-m from kinaba@chromium.org,
afakhry@chromium.org, yusukes@chromium.org
Link to the patchset: https://codereview.chromium.org/2655233007/#ps80001
(title: "address comments")

[commit-bot: I haz the power, 2017-01-31 06:01:54]

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, 2017-01-31 06:49:09]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1485842493284160,
"parent_rev": "c790e2177832fc573c5c17861ec8edad6a3ef566", "commit_rev":
"ccdfe0541dd3f6e7c85fc774772e3ff880730216"}

[commit-bot: I haz the power, 2017-01-31 06:50:12]

Description was changed from

==========
Get rid of RefCounted for ActivityIconLoader.

ActivityIconLoader was RefCounted.
However, all its access are on UI thread, and well managed.

This CL gets rid of RefCounted. Now ActivityIconLoader is a part of
ArcIntentHelperBridge. Its exposed method GetActivityIcons() is proxied by
ArcIntentHelperBridge, and the class is put in internal namespace, in order to
avoid accidental access from outside of components/arc/intent_helper.

BUG=n/a
TEST=Ran trybots.
==========

to

==========
Get rid of RefCounted for ActivityIconLoader.

ActivityIconLoader was RefCounted.
However, all its access are on UI thread, and well managed.

This CL gets rid of RefCounted. Now ActivityIconLoader is a part of
ArcIntentHelperBridge. Its exposed method GetActivityIcons() is proxied by
ArcIntentHelperBridge, and the class is put in internal namespace, in order to
avoid accidental access from outside of components/arc/intent_helper.

BUG=n/a
TEST=Ran trybots.

Review-Url: https://codereview.chromium.org/2655233007
Cr-Commit-Position: refs/heads/master@{#447200}
Committed:
https://chromium.googlesource.com/chromium/src/+/ccdfe0541dd3f6e7c85fc774772e...
==========

[commit-bot: I haz the power, 2017-01-31 06:50:13]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/ccdfe0541dd3f6e7c85fc774772e...