Move more utility functions to arc_util.

Move more utility functions to arc_util.

ArcSessionManager also as static utility methods
which are not directly related to the class.
This CL extracts them into arc_util.
IsArcAllowedForProfile() is still in chrome/browser/chromeos/arc,
because of its dependency.
Others are in components/arc/.

This is preparation to split state machine inside arc_session_manager.
Also, it's time to add unittest for each.

BUG=657687
BUG=b/31079732
TEST=Ran bots.
TBR=benwells@chromium.org

Review-Url: https://codereview.chromium.org/2642783003
Cr-Commit-Position: refs/heads/master@{#447223}
Committed: https://chromium.googlesource.com/chromium/src/+/0b75bce63b6fc6a882337f92bb8780ba703efeb8

[hidehiko, 2017-01-18 17:41:01]

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

[commit-bot: I haz the power, 2017-01-18 17:41:19]

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-18 18:24:14]

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

[commit-bot: I haz the power, 2017-01-18 18:24:15]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[hidehiko, 2017-01-19 05:47:32]

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

[commit-bot: I haz the power, 2017-01-19 05:47: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, 2017-01-19 06:46:31]

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

[commit-bot: I haz the power, 2017-01-19 06:46:32]

Dry run: This issue passed the CQ dry run.

[hidehiko, 2017-01-19 06:57:25]

hidehiko@chromium.org changed reviewers:
+ afakhry@chromium.org, benwells@chromium.org, kinaba@chromium.org,
lhchavez@chromium.org, skuhne@chromium.org, xiyuan@chromium.org,
yusukes@chromium.org

[hidehiko, 2017-01-19 06:57:26]

PTAL.
- lhchavez@, yusukes@: c/b/c/arc, components/arc
- kinaba@: c/b/c/extentions/file_manager, c/b/c/file_manager
- xiyuan@: other c/b/c files, c/b/ui/app_list, c/b/ui/webui
- afakhry@: c/b/task_manager
- skuhne@: c/b/ui/ash/launcher
- benwells@: c/b/ui/extensions

Just FYI:
- derat@: note_taking_helper
- nya@, oka@: file_manager/volume_manager changes.
- khmel@: app_list related changes.

c/b/c/arc and components/arc are core parts of the change, and other files
should be boilerplate changes along with function location moving.

Thank you for review in advance,
- hidehiko

https://codereview.chromium.org/2642783003/diff/20001/components/arc/intent_h...
File components/arc/intent_helper/arc_intent_helper_bridge.cc (right):

https://codereview.chromium.org/2642783003/diff/20001/components/arc/intent_h...
components/arc/intent_helper/arc_intent_helper_bridge.cc:136: if
(!arc::util::IsArcEnabled()) {
FYI to yusukes@.

IIUC, this check is not up-to-date.
Nowadays on ChromeOS, ArcServiceManager instance should be created regardless of
--enable-arc flag. So, we should drop this check.
Also, if it is ok to merge the VLOG message at L141 into L151, we can simplify
the code.

Yusuke, WDYT? If it is ok for you, I'll make the change in a following CL.

[Shuhei Takahashi, 2017-01-19 07:08:07]

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

[Shuhei Takahashi, 2017-01-19 07:08:08]

QQ: Do we need util:: namespace for those functions? All functions in arc_util.h
looks very fundamental to ARC and IMO worth putting it directly under arc::.

[hidehiko, 2017-01-19 07:40:37]

On 2017/01/19 07:08:08, Shuhei Takahashi wrote:
> QQ: Do we need util:: namespace for those functions? All functions in
arc_util.h
> looks very fundamental to ARC and IMO worth putting it directly under arc::.

Thank you for suggestion and offline chat, and it's actually an option.
However, in ARC related code, checking whether ARC is enabled or not is done in
various ways, and those functions have similar (or same) names. To avoid
confusing situation, I think it's clearer to put it in some namespace.

Some more notes:
- The main goal is to reduce the dependency from arc_session_manager to chrome/.
It is for moving arc_session_manager to components/arc, for cleaning code
dependency.
- Actually, "enable" has several (conceptually different) meaning in ARC,
unfortunately. Ideally, we should avoid it, but we need more time to discuss.
- Ideally, unifying checking ways is a good option. To avoid breakage, we need
to do it very carefully, and not that easy now, unfortunately.
- Having a class with static methods is another option (common practice in
Chrome). But in this case, due to dependency, we cannot put the functions into
one file (c/b/c/arc/arc_util and components/arc/arc_util). So, I chose namespace
to make them group.
- Welcome better names ;-).

[Shuhei Takahashi, 2017-01-19 08:29:10]

On 2017/01/19 07:40:37, hidehiko wrote:
> On 2017/01/19 07:08:08, Shuhei Takahashi wrote:
> > QQ: Do we need util:: namespace for those functions? All functions in
> arc_util.h
> > looks very fundamental to ARC and IMO worth putting it directly under arc::.
> 
> Thank you for suggestion and offline chat, and it's actually an option.
> However, in ARC related code, checking whether ARC is enabled or not is done
in
> various ways, and those functions have similar (or same) names. To avoid
> confusing situation, I think it's clearer to put it in some namespace.

Thanks for the explanation. To be clearer, I understand your concern is that, if
IsArcEnabled() is called in arc::SomeClass::some_function(), it is a bit
difficult to see if it calls arc::IsArcEnabled() or
arc::SomeClass::IsArcEnabled(), and it makes sense.


> Some more notes:
> - The main goal is to reduce the dependency from arc_session_manager to
chrome/.
> It is for moving arc_session_manager to components/arc, for cleaning code
> dependency.
> - Actually, "enable" has several (conceptually different) meaning in ARC,
> unfortunately. Ideally, we should avoid it, but we need more time to discuss.
> - Ideally, unifying checking ways is a good option. To avoid breakage, we need
> to do it very carefully, and not that easy now, unfortunately.
> - Having a class with static methods is another option (common practice in
> Chrome). But in this case, due to dependency, we cannot put the functions into
> one file (c/b/c/arc/arc_util and components/arc/arc_util). So, I chose
namespace
> to make them group.
> - Welcome better names ;-).

[Mr4D (OOO till 08-26), 2017-01-19 14:54:03]

c/b/ui/launcher lgtm

[afakhry, 2017-01-19 16:55:01]

c/b/task_manager lgtm

[Luis Héctor Chávez, 2017-01-19 18:09:16]

https://codereview.chromium.org/2642783003/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/BUILD.gn (right):

https://codereview.chromium.org/2642783003/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/BUILD.gn:1460: "arc/arc_util_unittest.cc",
:D

https://codereview.chromium.org/2642783003/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/arc/arc_util.cc (right):

https://codereview.chromium.org/2642783003/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/arc_util.cc:21: VLOG(1) << "Arc is not enabled.";
nit: s/Arc/ARC/g

https://codereview.chromium.org/2642783003/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/arc/arc_util.h (right):

https://codereview.chromium.org/2642783003/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/arc_util.h:10: // chrome/, so this file contains
such utilities.
nit: mention that including chrome/ from components/ is disallowed by DEPS.

https://codereview.chromium.org/2642783003/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/arc_util.h:15: namespace util {
I think I agree with nya@'s original comment of the util namespace being
superfluous. Only 23 *_util.h files in the codebase define an util namespace,
the vast majority (640 of them) do not.

All the utility functions currently start with "IsArc", so that's enough of a
"namespace" to prevent collisions/confusion IMO.

https://codereview.chromium.org/2642783003/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/arc_util.h:17: // Returns true if Arc is allowed to
run for the given profile.
nit: s/Arc/ARC/

https://codereview.chromium.org/2642783003/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/arc/arc_util_unittest.cc (right):

https://codereview.chromium.org/2642783003/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/arc_util_unittest.cc:134: // false for Legacy
sueprvised user.
nit: s/sueprvised/supervised/

https://codereview.chromium.org/2642783003/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/arc_util_unittest.cc:176: // TODO(hidehiko): Add
test for Ephemral users. There seems no way to easily
nit: s/Ephemral/Ephemeral/

https://codereview.chromium.org/2642783003/diff/20001/chrome/browser/ui/exten...
File chrome/browser/ui/extensions/app_launch_params.cc (right):

https://codereview.chromium.org/2642783003/diff/20001/chrome/browser/ui/exten...
chrome/browser/ui/extensions/app_launch_params.cc:38: if
(arc::util::IsArcAllowedForProfile(profile)) {
nit: maybe elide braces? Not sure how that plays with the comment in L42. Up to
you.

https://codereview.chromium.org/2642783003/diff/20001/components/arc/arc_util.h
File components/arc/arc_util.h (right):

https://codereview.chromium.org/2642783003/diff/20001/components/arc/arc_util...
components/arc/arc_util.h:20: // Checks if OptIn verification was disabled by
switch in command line.
nit: s/OptIn/opt-in/

https://codereview.chromium.org/2642783003/diff/20001/components/arc/arc_util...
components/arc/arc_util.h:21: bool IsOptInVerificationDisabled();
nit: IsArcOptInVerificationDisabled()? That'll bring consistency with the rest
of the functions.

[xiyuan, 2017-01-19 19:12:47]

c/b/c, c/b/ui/app_list, c/b/ui/webui lgtm

https://codereview.chromium.org/2642783003/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/arc/arc_util_unittest.cc (right):

https://codereview.chromium.org/2642783003/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/arc_util_unittest.cc:177: // simulate ephemeral
user.
If we make FakeUserManager::AreEphemeralUsersEnabled [1] configurable and return
true, the next new user login would go through the ephemeral user code path.

[1]
https://cs.chromium.org/chromium/src/components/user_manager/fake_user_manage...

[khmel, 2017-01-19 19:18:53]

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

[khmel, 2017-01-19 19:18:54]

https://codereview.chromium.org/2642783003/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/arc/arc_util_unittest.cc (right):

https://codereview.chromium.org/2642783003/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/arc_util_unittest.cc:177: // simulate ephemeral
user.
On 2017/01/19 19:12:46, xiyuan wrote:
> If we make FakeUserManager::AreEphemeralUsersEnabled [1] configurable and
return
> true, the next new user login would go through the ephemeral user code path.
> 
> [1]
>
https://cs.chromium.org/chromium/src/components/user_manager/fake_user_manage...

We already have this configurable:
https://cs.chromium.org/chromium/src/chrome/browser/chromeos/login/users/fake...

And it is used in Arc browser test.

[xiyuan, 2017-01-19 19:21:51]

https://codereview.chromium.org/2642783003/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/arc/arc_util_unittest.cc (right):

https://codereview.chromium.org/2642783003/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/arc_util_unittest.cc:177: // simulate ephemeral
user.
On 2017/01/19 19:18:54, khmel wrote:
> On 2017/01/19 19:12:46, xiyuan wrote:
> > If we make FakeUserManager::AreEphemeralUsersEnabled [1] configurable and
> return
> > true, the next new user login would go through the ephemeral user code path.
> > 
> > [1]
> >
>
https://cs.chromium.org/chromium/src/components/user_manager/fake_user_manage...
> 
> We already have this configurable:
>
https://cs.chromium.org/chromium/src/chrome/browser/chromeos/login/users/fake...
> 
> And it is used in Arc browser test.
> 

So we need to implement that in FakeUserManager under /components too. This test
is under /components and could not use FakeChromeUserManager from /chrome

[Yusuke Sato, 2017-01-19 22:39:31]

https://codereview.chromium.org/2642783003/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/arc/arc_session_manager.h (right):

https://codereview.chromium.org/2642783003/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/arc_session_manager.h:132: bool IsAllowed() const;
I feel the function name is too similar to IsAllowedForProfile() in
c/b/c/a/arc_util.h while this one only checks if profile_ is null. Can't we
rename this to something like IsPrimaryUserProfileReady() and update the comment
at L131?

Doing this in a separate CL is fine with a TODO.

https://codereview.chromium.org/2642783003/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/arc_session_manager.h:160: bool IsArcManaged()
const;
* Please document the function. When typically should we call this?
* Can we have a longer name for this, like IsArcEnabledPreferenceManaged()?

Doing this in a separate CL is fine with a TODO.

https://codereview.chromium.org/2642783003/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/arc_session_manager.h:161: bool IsArcEnabled()
const;
So we still have the function with exactly the same name as c/a/arc_util.h's
(but this one checks the pref in the profile_ rather than the command line.) Can
you at very least add function documents (on when to call this and when not) to
these the function and possibly rename it?

Doing this in a separate CL is fine with a TODO.

https://codereview.chromium.org/2642783003/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/arc/arc_util.cc (right):

https://codereview.chromium.org/2642783003/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/arc_util.cc:35: // IsPrimaryProfile can return true
for an incognito profile corresponding
This type of comment looks useful. Can you add comments to L47, 54, and 61 too?
(especially the last two.)

https://codereview.chromium.org/2642783003/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/arc/arc_util.h (right):

https://codereview.chromium.org/2642783003/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/arc_util.h:17: // Returns true if Arc is allowed to
run for the given profile.
Can we have a little bit more documentation? Explaining when the function
returns false (in a non-comprehensive way) might be useful.

Please also mention that passing nullptr to the function is safe.

https://codereview.chromium.org/2642783003/diff/20001/components/arc/arc_util.cc
File components/arc/arc_util.cc (right):

https://codereview.chromium.org/2642783003/diff/20001/components/arc/arc_util...
components/arc/arc_util.cc:17: const base::Feature
kArcEnabledFeature{"EnableARC",
This is for finch experiment right? A short comment why this is needed in
addition to switches::kEnableArc might be helpful.

https://codereview.chromium.org/2642783003/diff/20001/components/arc/arc_util.h
File components/arc/arc_util.h (right):

https://codereview.chromium.org/2642783003/diff/20001/components/arc/arc_util...
components/arc/arc_util.h:7: 
I still feel we need a very lengthy document on how to use these utility
functions here (b/33667791). Can you add a TODO with that bug# if you agree?

https://codereview.chromium.org/2642783003/diff/20001/components/arc/arc_util...
components/arc/arc_util.h:11: // Returns true if ARC has been enabled through a
commandline switch.
Could you provide a bit more documents? What does "enabled through a command
line" actually mean to developers? How and when the command line switch is
typically set?

I know it's populated by session_manager based on CrOS' (libchromeos-ui
package's?) USE flag, and can be overridden by /etc/chorme_dev.conf, but I guess
it's unclear for most of ARC devs.

Also, when typically do we want to call the utility function?

https://codereview.chromium.org/2642783003/diff/20001/components/arc/arc_util...
components/arc/arc_util.h:12: bool IsArcEnabled();
Name collision. ArcSessionManager has the same function but does a different
thing. Would it be possible to give better names to both? Currently "enabled"
has two different meaning.

https://codereview.chromium.org/2642783003/diff/20001/components/arc/arc_util...
components/arc/arc_util.h:14: // Returns true if ARC is available on the current
board.
When typically do we want to call the utility function? My understanding is that
except for a few rare occasions, developers should NOT call this at all.

https://codereview.chromium.org/2642783003/diff/20001/components/arc/arc_util...
components/arc/arc_util.h:20: // Checks if OptIn verification was disabled by
switch in command line.
Can you mention when typically this returns true? (like when
/etc/chrome_dev.conf is blah blah, maybe)

https://codereview.chromium.org/2642783003/diff/20001/components/arc/arc_util...
components/arc/arc_util.h:22: 
Having 'void EnableArcForTesting()' (or a better name which is consistent with
L12) might be helpful to hide the command line switch from developers. At least
chrome/browser/sync/test/integration/sync_arc_package_helper.cc does the
following:

  base::CommandLine::ForCurrentProcess()->AppendSwitch(
      chromeos::switches::kEnableArc);

https://codereview.chromium.org/2642783003/diff/20001/components/arc/intent_h...
File components/arc/intent_helper/arc_intent_helper_bridge.cc (right):

https://codereview.chromium.org/2642783003/diff/20001/components/arc/intent_h...
components/arc/intent_helper/arc_intent_helper_bridge.cc:136: if
(!arc::util::IsArcEnabled()) {
That's fine but I also think we can remove the function itself by now along with
the enums. I can do that if needed.

[kinaba, 2017-01-20 01:53:22]

*file_manager* lgtm

[hidehiko, 2017-01-20 10:00:33]

On 2017/01/20 01:53:22, kinaba wrote:
> *file_manager* lgtm

Thank you all for your quick review.

Luis, Yusuke, thank you very much for comments.
I totally agree with most of them.

But I'm really worried about inlining util:: into arc:: directly *now*,
because of the "enabled" name conflicting, as Yusuke pointed in the review
comment.
Actually, ARC in Chrome has two "enabled" meaning;
* --enable-arc command line flag
* arc.enabled preference
and these are conceptually totally different.

So, let's resolve it first. b/34480289.
Considering that inlining needs to touch most of the files,
let me leave this CL for now. I'll update ASAP when the conflicting is resolved
(and I hope we can get agreement quickly).

[hidehiko, 2017-01-24 17:05:37]

Patchset #3 (id:40001) has been deleted

[hidehiko, 2017-01-24 17:33:28]

Patchset #4 (id:80001) has been deleted

[hidehiko, 2017-01-24 17:41:50]

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

[commit-bot: I haz the power, 2017-01-24 17:42:14]

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-24 17:44:47]

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

[commit-bot: I haz the power, 2017-01-24 17:44:49]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[hidehiko, 2017-01-24 17:46:12]

Luis, Yusuke, thank you for the discussion.
Now name conflicting is being resolved, I rebased on top of the CL which is
under review to you.
Let me work on flag clean up related task in following CLs, separately.

PTAL.

https://codereview.chromium.org/2642783003/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/BUILD.gn (right):

https://codereview.chromium.org/2642783003/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/BUILD.gn:1460: "arc/arc_util_unittest.cc",
On 2017/01/19 18:09:15, Luis Héctor Chávez wrote:
> :D

;-)

https://codereview.chromium.org/2642783003/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/arc/arc_session_manager.h (right):

https://codereview.chromium.org/2642783003/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/arc_session_manager.h:132: bool IsAllowed() const;
On 2017/01/19 22:39:31, Yusuke Sato wrote:
> I feel the function name is too similar to IsAllowedForProfile() in
> c/b/c/a/arc_util.h while this one only checks if profile_ is null. Can't we
> rename this to something like IsPrimaryUserProfileReady() and update the
comment
> at L131?
> 
> Doing this in a separate CL is fine with a TODO.

Sure. Added TODO.

https://codereview.chromium.org/2642783003/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/arc_session_manager.h:160: bool IsArcManaged()
const;
On 2017/01/19 22:39:31, Yusuke Sato wrote:
> * Please document the function. When typically should we call this?
> * Can we have a longer name for this, like IsArcEnabledPreferenceManaged()?
> 
> Doing this in a separate CL is fine with a TODO.

This is for existing code. So let me add TODO.

https://codereview.chromium.org/2642783003/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/arc_session_manager.h:161: bool IsArcEnabled()
const;
On 2017/01/19 22:39:31, Yusuke Sato wrote:
> So we still have the function with exactly the same name as c/a/arc_util.h's
> (but this one checks the pref in the profile_ rather than the command line.)
Can
> you at very least add function documents (on when to call this and when not)
to
> these the function and possibly rename it?
> 
> Doing this in a separate CL is fine with a TODO.

Similarly, added TODO.

https://codereview.chromium.org/2642783003/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/arc/arc_util.cc (right):

https://codereview.chromium.org/2642783003/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/arc_util.cc:21: VLOG(1) << "Arc is not enabled.";
On 2017/01/19 18:09:15, Luis Héctor Chávez wrote:
> nit: s/Arc/ARC/g

Done.

https://codereview.chromium.org/2642783003/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/arc_util.cc:35: // IsPrimaryProfile can return true
for an incognito profile corresponding
On 2017/01/19 22:39:31, Yusuke Sato wrote:
> This type of comment looks useful. Can you add comments to L47, 54, and 61
too?
> (especially the last two.)

Added to L54. However, as for L47 and L61, I'm not very sure about its
background because I neither write it nor review it, and I just moved the code
here.
So, let me add TODO for its archaeology.

https://codereview.chromium.org/2642783003/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/arc/arc_util.h (right):

https://codereview.chromium.org/2642783003/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/arc_util.h:10: // chrome/, so this file contains
such utilities.
On 2017/01/19 18:09:15, Luis Héctor Chávez wrote:
> nit: mention that including chrome/ from components/ is disallowed by DEPS.

Done.

https://codereview.chromium.org/2642783003/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/arc_util.h:15: namespace util {
On 2017/01/19 18:09:15, Luis Héctor Chávez wrote:
> I think I agree with nya@'s original comment of the util namespace being
> superfluous. Only 23 *_util.h files in the codebase define an util namespace,
> the vast majority (640 of them) do not.
> 
> All the utility functions currently start with "IsArc", so that's enough of a
> "namespace" to prevent collisions/confusion IMO.

Ok, so name conflicting should be resolved now. Removed util namespace.

https://codereview.chromium.org/2642783003/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/arc_util.h:17: // Returns true if Arc is allowed to
run for the given profile.
On 2017/01/19 18:09:15, Luis Héctor Chávez wrote:
> nit: s/Arc/ARC/

I just moved the comment ;-). Anyway, fixed.

https://codereview.chromium.org/2642783003/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/arc_util.h:17: // Returns true if Arc is allowed to
run for the given profile.
On 2017/01/19 22:39:31, Yusuke Sato wrote:
> Can we have a little bit more documentation? Explaining when the function
> returns false (in a non-comprehensive way) might be useful.
> 
> Please also mention that passing nullptr to the function is safe.

Done.

https://codereview.chromium.org/2642783003/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/arc/arc_util_unittest.cc (right):

https://codereview.chromium.org/2642783003/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/arc_util_unittest.cc:134: // false for Legacy
sueprvised user.
On 2017/01/19 18:09:15, Luis Héctor Chávez wrote:
> nit: s/sueprvised/supervised/

Done.

https://codereview.chromium.org/2642783003/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/arc_util_unittest.cc:176: // TODO(hidehiko): Add
test for Ephemral users. There seems no way to easily
On 2017/01/19 18:09:15, Luis Héctor Chávez wrote:
> nit: s/Ephemral/Ephemeral/

Done.

https://codereview.chromium.org/2642783003/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/arc_util_unittest.cc:177: // simulate ephemeral
user.
On 2017/01/19 19:21:51, xiyuan wrote:
> On 2017/01/19 19:18:54, khmel wrote:
> > On 2017/01/19 19:12:46, xiyuan wrote:
> > > If we make FakeUserManager::AreEphemeralUsersEnabled [1] configurable and
> > return
> > > true, the next new user login would go through the ephemeral user code
path.
> > > 
> > > [1]
> > >
> >
>
https://cs.chromium.org/chromium/src/components/user_manager/fake_user_manage...
> > 
> > We already have this configurable:
> >
>
https://cs.chromium.org/chromium/src/chrome/browser/chromeos/login/users/fake...
> > 
> > And it is used in Arc browser test.
> > 
> 
> So we need to implement that in FakeUserManager under /components too. This
test
> is under /components and could not use FakeChromeUserManager from /chrome

xiyuan@, khmel@, thanks for the info.
I'll give it a try. Considering this is not the regression, and I think I need
more work, let me work on it in another CL.

https://codereview.chromium.org/2642783003/diff/20001/chrome/browser/ui/exten...
File chrome/browser/ui/extensions/app_launch_params.cc (right):

https://codereview.chromium.org/2642783003/diff/20001/chrome/browser/ui/exten...
chrome/browser/ui/extensions/app_launch_params.cc:38: if
(arc::util::IsArcAllowedForProfile(profile)) {
On 2017/01/19 18:09:15, Luis Héctor Chávez wrote:
> nit: maybe elide braces? Not sure how that plays with the comment in L42. Up
to
> you.

I couldn't find any guideline, and I think I've rarely seen dropping braces in
chromium if there is else clause (though, I do not have stats...). For now, let
me keep this as is.

https://codereview.chromium.org/2642783003/diff/20001/components/arc/arc_util.cc
File components/arc/arc_util.cc (right):

https://codereview.chromium.org/2642783003/diff/20001/components/arc/arc_util...
components/arc/arc_util.cc:17: const base::Feature
kArcEnabledFeature{"EnableARC",
On 2017/01/19 22:39:31, Yusuke Sato wrote:
> This is for finch experiment right? A short comment why this is needed in
> addition to switches::kEnableArc might be helpful. 

Done.

https://codereview.chromium.org/2642783003/diff/20001/components/arc/arc_util.h
File components/arc/arc_util.h (right):

https://codereview.chromium.org/2642783003/diff/20001/components/arc/arc_util...
components/arc/arc_util.h:7: 
On 2017/01/19 22:39:31, Yusuke Sato wrote:
> I still feel we need a very lengthy document on how to use these utility
> functions here (b/33667791). Can you add a TODO with that bug# if you agree?

Addressed in base CL.

https://codereview.chromium.org/2642783003/diff/20001/components/arc/arc_util...
components/arc/arc_util.h:11: // Returns true if ARC has been enabled through a
commandline switch.
On 2017/01/19 22:39:31, Yusuke Sato wrote:
> Could you provide a bit more documents? What does "enabled through a command
> line" actually mean to developers? How and when the command line switch is
> typically set?
> 
> I know it's populated by session_manager based on CrOS' (libchromeos-ui
> package's?) USE flag, and can be overridden by /etc/chorme_dev.conf, but I
guess
> it's unclear for most of ARC devs.
> 
> Also, when typically do we want to call the utility function?

Added more documentation in another CL.

As for detailed comments about relation to command line flags, let me work on it
in the CL to clean up the flags.

https://codereview.chromium.org/2642783003/diff/20001/components/arc/arc_util...
components/arc/arc_util.h:12: bool IsArcEnabled();
On 2017/01/19 22:39:31, Yusuke Sato wrote:
> Name collision. ArcSessionManager has the same function but does a different
> thing. Would it be possible to give better names to both? Currently "enabled"
> has two different meaning.

Now, it should be resolved.

https://codereview.chromium.org/2642783003/diff/20001/components/arc/arc_util...
components/arc/arc_util.h:14: // Returns true if ARC is available on the current
board.
On 2017/01/19 22:39:31, Yusuke Sato wrote:
> When typically do we want to call the utility function? My understanding is
that
> except for a few rare occasions, developers should NOT call this at all.

Acknowledged.

https://codereview.chromium.org/2642783003/diff/20001/components/arc/arc_util...
components/arc/arc_util.h:20: // Checks if OptIn verification was disabled by
switch in command line.
On 2017/01/19 18:09:15, Luis Héctor Chávez wrote:
> nit: s/OptIn/opt-in/

Done.

https://codereview.chromium.org/2642783003/diff/20001/components/arc/arc_util...
components/arc/arc_util.h:20: // Checks if OptIn verification was disabled by
switch in command line.
On 2017/01/19 22:39:31, Yusuke Sato wrote:
> Can you mention when typically this returns true? (like when
> /etc/chrome_dev.conf is blah blah, maybe)

Hmmm, I guess rather than mentioning commandline flags about
/etc/chrome_dev.conf, the purpose looks important? WDYT?

https://codereview.chromium.org/2642783003/diff/20001/components/arc/arc_util...
components/arc/arc_util.h:21: bool IsOptInVerificationDisabled();
On 2017/01/19 18:09:15, Luis Héctor Chávez wrote:
> nit: IsArcOptInVerificationDisabled()? That'll bring consistency with the rest
> of the functions.

Done.

https://codereview.chromium.org/2642783003/diff/20001/components/arc/arc_util...
components/arc/arc_util.h:22: 
On 2017/01/19 22:39:31, Yusuke Sato wrote:
> Having 'void EnableArcForTesting()' (or a better name which is consistent with
> L12) might be helpful to hide the command line switch from developers. At
least
> chrome/browser/sync/test/integration/sync_arc_package_helper.cc does the
> following:
> 
>   base::CommandLine::ForCurrentProcess()->AppendSwitch(
>       chromeos::switches::kEnableArc);

As written in another CL's TODO comment, let me work on this with cleaning up
flags.

[hidehiko, 2017-01-24 17:47:56]

Hmm... Try looks not working well with rebasing on top of under review CL for
some reason. I'll re-run after the dependent CL is landed.

[Yusuke Sato, 2017-01-24 20:36:59]

lgtm, thanks!

https://codereview.chromium.org/2642783003/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/arc/arc_session_manager.h (right):

https://codereview.chromium.org/2642783003/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/arc_session_manager.h:161: bool IsArcEnabled()
const;
On 2017/01/24 17:46:11, hidehiko wrote:
> On 2017/01/19 22:39:31, Yusuke Sato wrote:
> > So we still have the function with exactly the same name as c/a/arc_util.h's
> > (but this one checks the pref in the profile_ rather than the command line.)
> Can
> > you at very least add function documents (on when to call this and when not)
> to
> > these the function and possibly rename it?
> > 
> > Doing this in a separate CL is fine with a TODO.
> 
> Similarly, added TODO.

Thanks, and please fix these TODOs as part of b/3448028. The current situation
is really confusing until these TODOs are properly addressed.

https://codereview.chromium.org/2642783003/diff/20001/components/arc/arc_util.h
File components/arc/arc_util.h (right):

https://codereview.chromium.org/2642783003/diff/20001/components/arc/arc_util...
components/arc/arc_util.h:22: 
On 2017/01/24 17:46:12, hidehiko wrote:
> On 2017/01/19 22:39:31, Yusuke Sato wrote:
> > Having 'void EnableArcForTesting()' (or a better name which is consistent
with
> > L12) might be helpful to hide the command line switch from developers. At
> least
> > chrome/browser/sync/test/integration/sync_arc_package_helper.cc does the
> > following:
> > 
> >   base::CommandLine::ForCurrentProcess()->AppendSwitch(
> >       chromeos::switches::kEnableArc);
> 
> As written in another CL's TODO comment, let me work on this with cleaning up
> flags.

Eh.. where is the TODO? I think I'm lost..

https://codereview.chromium.org/2642783003/diff/100001/chrome/browser/chromeo...
File chrome/browser/chromeos/BUILD.gn (right):

https://codereview.chromium.org/2642783003/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/BUILD.gn:1: # Copyright 2014 The Chromium Authors. All
rights reserved.
The CL description is outdated. It still mentions util::. Please update.

https://codereview.chromium.org/2642783003/diff/100001/chrome/browser/chromeo...
File chrome/browser/chromeos/arc/arc_util.cc (right):

https://codereview.chromium.org/2642783003/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/arc/arc_util.cc:58: // TODO(hidehiko): Comment what
situation this condition wants to support.
Then, probably TODO(hidehiko|xxx) would be better. (xxx == the original author)

https://codereview.chromium.org/2642783003/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/arc/arc_util.cc:75: // TODO(hidehiko): Comment what
situation this condition wants to support.
same

https://codereview.chromium.org/2642783003/diff/100001/components/arc/arc_util.h
File components/arc/arc_util.h (right):

https://codereview.chromium.org/2642783003/diff/100001/components/arc/arc_uti...
components/arc/arc_util.h:28: // In most cases, this is for testing purpose.
"this is disabled" ?

[Luis Héctor Chávez, 2017-01-26 17:58:33]

lgtm

[hidehiko, 2017-01-31 07:42:24]

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

[commit-bot: I haz the power, 2017-01-31 07:42:31]

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-31 09:31:37]

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

[commit-bot: I haz the power, 2017-01-31 09:31:38]

Dry run: This issue passed the CQ dry run.

[hidehiko, 2017-01-31 13:06:22]

Description was changed from

==========
Extract static utility functions into arc::util namespace.

ArcBridgeService and ArcSessionManager have static utility
methods which are not directly related to each class.

This CL extracts them into arc::util namespace.
Most functions should be put in components/arc/arc_util,
but some function highly depending on chrome/ code is put into
chrome/browser/chromeos/arc_util.

This is preparation to split state machine inside arc_session_manager.
Also, it's time to add unittest for each.

BUG=657687
BUG=b/31079732
TEST=Ran bots.
==========

to

==========
Move more utility functions to arc_util.

ArcSessionManager also as static utility methods
which are not directly related to the class.
This CL extracts them into arc_util.
IsArcAllowedForProfile() is still in chrome/browser/chromeos/arc,
because of its dependency.
Others are in components/arc/.

This is preparation to split state machine inside arc_session_manager.
Also, it's time to add unittest for each.

BUG=657687
BUG=b/31079732
TEST=Ran bots.
==========

[hidehiko, 2017-01-31 13:06:40]

Description was changed from

==========
Move more utility functions to arc_util.

ArcSessionManager also as static utility methods
which are not directly related to the class.
This CL extracts them into arc_util.
IsArcAllowedForProfile() is still in chrome/browser/chromeos/arc,
because of its dependency.
Others are in components/arc/.

This is preparation to split state machine inside arc_session_manager.
Also, it's time to add unittest for each.

BUG=657687
BUG=b/31079732
TEST=Ran bots.
==========

to

==========
Move more utility functions to arc_util.

ArcSessionManager also as static utility methods
which are not directly related to the class.
This CL extracts them into arc_util.
IsArcAllowedForProfile() is still in chrome/browser/chromeos/arc,
because of its dependency.
Others are in components/arc/.

This is preparation to split state machine inside arc_session_manager.
Also, it's time to add unittest for each.

BUG=657687
BUG=b/31079732
TEST=Ran bots.
TBR=benwells@chromium.org
==========

[hidehiko, 2017-01-31 13:07:51]

Thank you for review, all.
Landing with TBR=benwells@, following the second item of
http://dev.chromium.org/developers/owners-files#TOC-When-to-use-To-Be-Reviewe...

https://codereview.chromium.org/2642783003/diff/100001/chrome/browser/chromeo...
File chrome/browser/chromeos/BUILD.gn (right):

https://codereview.chromium.org/2642783003/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/BUILD.gn:1: # Copyright 2014 The Chromium Authors. All
rights reserved.
On 2017/01/24 20:36:58, Yusuke Sato wrote:
> The CL description is outdated. It still mentions util::. Please update.

Done.

https://codereview.chromium.org/2642783003/diff/100001/chrome/browser/chromeo...
File chrome/browser/chromeos/arc/arc_util.cc (right):

https://codereview.chromium.org/2642783003/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/arc/arc_util.cc:58: // TODO(hidehiko): Comment what
situation this condition wants to support.
On 2017/01/24 20:36:59, Yusuke Sato wrote:
> Then, probably TODO(hidehiko|xxx) would be better. (xxx == the original
author)

Finding original author means I need to go deeper till original CL which
includes the motivation. So done archaeology.

https://codereview.chromium.org/2642783003/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/arc/arc_util.cc:75: // TODO(hidehiko): Comment what
situation this condition wants to support.
On 2017/01/24 20:36:59, Yusuke Sato wrote:
> same

Done.

https://codereview.chromium.org/2642783003/diff/100001/components/arc/arc_util.h
File components/arc/arc_util.h (right):

https://codereview.chromium.org/2642783003/diff/100001/components/arc/arc_uti...
components/arc/arc_util.h:28: // In most cases, this is for testing purpose.
On 2017/01/24 20:36:59, Yusuke Sato wrote:
> "this is disabled" ?

Done.

[hidehiko, 2017-01-31 13:08:06]

The CQ bit was checked by hidehiko@chromium.org

[hidehiko, 2017-01-31 13:08:06]

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

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

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 13:13:05]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1485868086379790,
"parent_rev": "eaedd4e6b81145755d8953a8d8929093ccb0e683", "commit_rev":
"0b75bce63b6fc6a882337f92bb8780ba703efeb8"}

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

Description was changed from

==========
Move more utility functions to arc_util.

ArcSessionManager also as static utility methods
which are not directly related to the class.
This CL extracts them into arc_util.
IsArcAllowedForProfile() is still in chrome/browser/chromeos/arc,
because of its dependency.
Others are in components/arc/.

This is preparation to split state machine inside arc_session_manager.
Also, it's time to add unittest for each.

BUG=657687
BUG=b/31079732
TEST=Ran bots.
TBR=benwells@chromium.org
==========

to

==========
Move more utility functions to arc_util.

ArcSessionManager also as static utility methods
which are not directly related to the class.
This CL extracts them into arc_util.
IsArcAllowedForProfile() is still in chrome/browser/chromeos/arc,
because of its dependency.
Others are in components/arc/.

This is preparation to split state machine inside arc_session_manager.
Also, it's time to add unittest for each.

BUG=657687
BUG=b/31079732
TEST=Ran bots.
TBR=benwells@chromium.org

Review-Url: https://codereview.chromium.org/2642783003
Cr-Commit-Position: refs/heads/master@{#447223}
Committed:
https://chromium.googlesource.com/chromium/src/+/0b75bce63b6fc6a882337f92bb87...
==========

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

Committed patchset #6 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/0b75bce63b6fc6a882337f92bb87...

[tsergeant, 2017-01-31 23:53:41]

A revert of this CL (patchset #6 id:140001) has been created in
https://codereview.chromium.org/2668153002/ by tsergeant@chromium.org.

The reason for reverting is: The new test
ChromeArcUtilTest.IsArcAllowedForProfile_SupervisedUserFlow fails on the builder
Linux Chromium OS ASan LSan Tests (1).

See example failure:
https://luci-milo.appspot.com/buildbot/chromium.memory/Linux%20Chromium%20OS%....