Migrate --enable-arc and --arc-available part 1.

Migrate --enable-arc and --arc-available part 1.

This CL removes ArcBridgeService::GetEnabled/GetAvailable,
instead instroduces one utility function IsArcAvailable()
to cover all those usages.

GetAvailable() returns whether ARC is installed or not,
regardless of whether it can run or not.
Its usage should be limited to finch only,
because users cannot run ARC actually.
New function IsArcAvailable() returns whether ARC can run
on the device (equivalent to GetEnabled(), but avoid "enabled" name
due to the conflict of arc.enabled preference).

The only usage of GetAvailable() was for the internal APIs,
which should be simply replaced by IsArcAvailable().

BUG=b/34480289, b/34478891
TEST=Ran bots.
TBR=skuhne@chromium.org, benwells@chromium.org

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

[hidehiko, 2017-01-24 09:07:05]

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

[commit-bot: I haz the power, 2017-01-24 09:07:12]

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-24 10:57:35]

hidehiko@chromium.org changed reviewers:
+ benwells@chromium.org, lhchavez@chromium.org, rkc@chromium.org,
xiyuan@chromium.org, yusukes@chromium.org

[hidehiko, 2017-01-24 10:57:36]

PTAL.

yusukes@, lhchavez@: c/b/c/arc, components/arc, overall design.
rkc@: related APIs as original author.
xiyuan@: other files under c/b/c/.
afakhry@: c/b/task_manager
benwells@: c/b/ui/extensions

Thank you for review in advance.
- hidehiko

https://codereview.chromium.org/2648213004/diff/1/chrome/browser/chromeos/arc...
File chrome/browser/chromeos/arc/arc_session_manager.cc (right):

https://codereview.chromium.org/2648213004/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_session_manager.cc:72: bool
g_disallow_for_testing = false;
Luis, Yusuke:
this is for testing of info_private_api.
It requires for IsArcAvailable() to return true, but for IsAllowedForProfile()
to return false.

Also, the testing framework does not seem to support with Incognito, Legacy
Supervised, nor secondary profile.

If you know better approach, please let me know.

An alternative idea is introducing a internal (test-only) preference, e.g.
arc.disallowed_for_testing.
Though, I couldn't find such a use case, so I just avoided it.

Any comments are welcome.

https://codereview.chromium.org/2648213004/diff/1/chrome/browser/chromeos/ext...
File chrome/browser/chromeos/extensions/info_private_apitest.cc (right):

https://codereview.chromium.org/2648213004/diff/1/chrome/browser/chromeos/ext...
chrome/browser/chromeos/extensions/info_private_apitest.cc:35: //
TODO(hidehiko): Introduce a utility to let IsArcAvailable() return true
Note: As many other tests do something similar to this, let me work on this in a
following CL.

https://codereview.chromium.org/2648213004/diff/1/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/session/chrome_session_manager.cc (right):

https://codereview.chromium.org/2648213004/diff/1/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/session/chrome_session_manager.cc:108:
arc::ArcServiceLauncher::Get()->OnPrimaryUserProfilePrepared(user_profile);
Note: because ArcServiceLauncher instance is always created regardless of
IsArcAvailable(), we should be able to call this unconditionally.

Note that, in case of that IsArcAvailable() returns false,
ArcSessionManager::OnPrimaryUserProfilePrepared() handles it.

https://codereview.chromium.org/2648213004/diff/1/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/session/user_session_manager.cc (right):

https://codereview.chromium.org/2648213004/diff/1/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/session/user_session_manager.cc:1188:
arc::ArcServiceLauncher::Get()->OnPrimaryUserProfilePrepared(profile);
Ditto for chrome_session_manager.cc.

https://codereview.chromium.org/2648213004/diff/1/chrome/browser/ui/extension...
File chrome/browser/ui/extensions/app_launch_params.cc (right):

https://codereview.chromium.org/2648213004/diff/1/chrome/browser/ui/extension...
chrome/browser/ui/extensions/app_launch_params.cc:37: // TODO(b/34478891):
Remove this from app launch.
Note: According to the discussion on the bug, we can remove whole this section.
But let me work on it in another CL to focus on ABS::GetAvailable() removal in
this CL.

https://codereview.chromium.org/2648213004/diff/1/components/arc/arc_util_uni...
File components/arc/arc_util_unittest.cc (right):

https://codereview.chromium.org/2648213004/diff/1/components/arc/arc_util_uni...
components/arc/arc_util_unittest.cc:36: TEST_F(ArcUtilTest,
IsArcAvailable_Installed) {
Note: these sub-test are named after the discussion of the new flag.

[commit-bot: I haz the power, 2017-01-24 11:04:51]

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

[commit-bot: I haz the power, 2017-01-24 11:04:52]

Dry run: This issue passed the CQ dry run.

[hidehiko, 2017-01-24 11:08:49]

hidehiko@chromium.org changed reviewers:
+ afakhry@chromium.org

[hidehiko, 2017-01-24 11:08:49]

Oops, I accidentally dropped afakhry@ from R=...

PTAL.

[Luis Héctor Chávez, 2017-01-24 17:00:37]

https://codereview.chromium.org/2648213004/diff/1/chrome/browser/chromeos/arc...
File chrome/browser/chromeos/arc/arc_session_manager.cc (right):

https://codereview.chromium.org/2648213004/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_session_manager.cc:72: bool
g_disallow_for_testing = false;
On 2017/01/24 10:57:36, hidehiko wrote:
> Luis, Yusuke:
> this is for testing of info_private_api.
> It requires for IsArcAvailable() to return true, but for IsAllowedForProfile()
> to return false.
> 
> Also, the testing framework does not seem to support with Incognito, Legacy
> Supervised, nor secondary profile.
> 
> If you know better approach, please let me know.
> 
> An alternative idea is introducing a internal (test-only) preference, e.g.
> arc.disallowed_for_testing.
> Though, I couldn't find such a use case, so I just avoided it.
> 
> Any comments are welcome.

I think I prefer having this explicitly rather than a preference, since the
latter usually has lower discoverability (at least in code search it's almost
always one more click) and overhead.

https://codereview.chromium.org/2648213004/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_session_manager.cc:161: VLOG(1) << "Arc is not
enabled.";
nit: not your fault, but can you s/Arc/ARC/?

https://codereview.chromium.org/2648213004/diff/1/chrome/browser/chromeos/ext...
File chrome/browser/chromeos/extensions/info_private_apitest.cc (right):

https://codereview.chromium.org/2648213004/diff/1/chrome/browser/chromeos/ext...
chrome/browser/chromeos/extensions/info_private_apitest.cc:84:
IN_PROC_BROWSER_TEST_F(ChromeOSInfoPrivateTest, ArcAvailable) {
This name is confusing. Can you change it to ArcAvailableDisallowed?

https://codereview.chromium.org/2648213004/diff/1/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/session/chrome_session_manager.cc (right):

https://codereview.chromium.org/2648213004/diff/1/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/session/chrome_session_manager.cc:108:
arc::ArcServiceLauncher::Get()->OnPrimaryUserProfilePrepared(user_profile);
On 2017/01/24 10:57:36, hidehiko wrote:
> Note: because ArcServiceLauncher instance is always created regardless of
> IsArcAvailable(), we should be able to call this unconditionally.
> 
> Note that, in case of that IsArcAvailable() returns false,
> ArcSessionManager::OnPrimaryUserProfilePrepared() handles it.

Please note this in a comment.

Also, I believe the motivation behind the check was to avoid
instantiating/initializing stuff unless it was necessary, so I'd vote to keep it
unless it's causing some complication elsewhere.

https://codereview.chromium.org/2648213004/diff/1/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/session/user_session_manager.cc (right):

https://codereview.chromium.org/2648213004/diff/1/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/session/user_session_manager.cc:1188:
arc::ArcServiceLauncher::Get()->OnPrimaryUserProfilePrepared(profile);
On 2017/01/24 10:57:36, hidehiko wrote:
> Ditto for chrome_session_manager.cc.

Ditto.

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

https://codereview.chromium.org/2648213004/diff/1/components/arc/arc_util.h#n...
components/arc/arc_util.h:14: // Returns true if the current device installs ARC
and can run it.
nit: "if the current device has ARC installed and has the capability of running
it."

https://codereview.chromium.org/2648213004/diff/1/components/arc/arc_util.h#n...
components/arc/arc_util.h:20: // ArcServiceManager, ArcServiceLauncher) are
instanciated regardless of this
nit: s/instanciated/instantiated/

[afakhry, 2017-01-24 17:31:31]

task_manager_impl.cc lgtm

[xiyuan, 2017-01-24 17:52:38]

https://codereview.chromium.org/2648213004/diff/1/chrome/browser/chromeos/ext...
File chrome/browser/chromeos/extensions/info_private_apitest.cc (left):

https://codereview.chromium.org/2648213004/diff/1/chrome/browser/chromeos/ext...
chrome/browser/chromeos/extensions/info_private_apitest.cc:96: "arc enabled"))
Is this test case no longer needed? If so, also update
chromeos_info_private/extended/background.js.

[hidehiko, 2017-01-24 18:05:59]

Thank you for review. PTAL.

https://codereview.chromium.org/2648213004/diff/1/chrome/browser/chromeos/arc...
File chrome/browser/chromeos/arc/arc_session_manager.cc (right):

https://codereview.chromium.org/2648213004/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_session_manager.cc:161: VLOG(1) << "Arc is not
enabled.";
On 2017/01/24 17:00:37, Luis Héctor Chávez wrote:
> nit: not your fault, but can you s/Arc/ARC/?

Done.

https://codereview.chromium.org/2648213004/diff/1/chrome/browser/chromeos/ext...
File chrome/browser/chromeos/extensions/info_private_apitest.cc (left):

https://codereview.chromium.org/2648213004/diff/1/chrome/browser/chromeos/ext...
chrome/browser/chromeos/extensions/info_private_apitest.cc:96: "arc enabled"))
On 2017/01/24 17:52:38, xiyuan wrote:
> Is this test case no longer needed? If so, also update
> chromeos_info_private/extended/background.js.

Good catch. Done.

https://codereview.chromium.org/2648213004/diff/1/chrome/browser/chromeos/ext...
File chrome/browser/chromeos/extensions/info_private_apitest.cc (right):

https://codereview.chromium.org/2648213004/diff/1/chrome/browser/chromeos/ext...
chrome/browser/chromeos/extensions/info_private_apitest.cc:84:
IN_PROC_BROWSER_TEST_F(ChromeOSInfoPrivateTest, ArcAvailable) {
On 2017/01/24 17:00:37, Luis Héctor Chávez wrote:
> This name is confusing. Can you change it to ArcAvailableDisallowed?

Done.

https://codereview.chromium.org/2648213004/diff/1/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/session/chrome_session_manager.cc (right):

https://codereview.chromium.org/2648213004/diff/1/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/session/chrome_session_manager.cc:108:
arc::ArcServiceLauncher::Get()->OnPrimaryUserProfilePrepared(user_profile);
On 2017/01/24 17:00:37, Luis Héctor Chávez wrote:
> On 2017/01/24 10:57:36, hidehiko wrote:
> > Note: because ArcServiceLauncher instance is always created regardless of
> > IsArcAvailable(), we should be able to call this unconditionally.
> > 
> > Note that, in case of that IsArcAvailable() returns false,
> > ArcSessionManager::OnPrimaryUserProfilePrepared() handles it.
> 
> Please note this in a comment.
> 
> Also, I believe the motivation behind the check was to avoid
> instantiating/initializing stuff unless it was necessary, so I'd vote to keep
it
> unless it's causing some complication elsewhere.

I prefer the consistency, here.
ArcService instances are initialized regardless of the IsArcAvailable(), so I
think it makes sense to call OnPrimaryUserProfilePreapred() as well, where some
services are initialized in the function.

So, the diff in a case IsArcAvailable() returns false then is whether Shutdown()
is called or not, in ArcSessionManager. However, if IsArcAvailable() returns
false, no-one can starts arc so it should be almost no-op. Thus, I do not think
we have much benefit by culling the call.

WDYT?

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

https://codereview.chromium.org/2648213004/diff/1/components/arc/arc_util.h#n...
components/arc/arc_util.h:14: // Returns true if the current device installs ARC
and can run it.
On 2017/01/24 17:00:37, Luis Héctor Chávez wrote:
> nit: "if the current device has ARC installed and has the capability of
running
> it."

Done.

https://codereview.chromium.org/2648213004/diff/1/components/arc/arc_util.h#n...
components/arc/arc_util.h:20: // ArcServiceManager, ArcServiceLauncher) are
instanciated regardless of this
On 2017/01/24 17:00:37, Luis Héctor Chávez wrote:
> nit: s/instanciated/instantiated/

Done.

[Yusuke Sato, 2017-01-24 19:02:45]

https://codereview.chromium.org/2648213004/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/extensions/info_private_apitest.cc (right):

https://codereview.chromium.org/2648213004/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/extensions/info_private_apitest.cc:35: //
TODO(hidehiko): Introduce a utility to let IsArcAvailable() return true
Is this difficult to implement now?

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

https://codereview.chromium.org/2648213004/diff/40001/chrome/browser/ui/exten...
chrome/browser/ui/extensions/app_launch_params.cc:41: } else if
(arc::IsArcAvailable()) {
nit: no }{

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

https://codereview.chromium.org/2648213004/diff/40001/components/arc/arc_util...
components/arc/arc_util.cc:25: // TODO(hidehiko): Unify --enable-arc and
--arc-available flags.
What about adding something like this?

 // If switches::kEnableArc is set, the device is officially supported to run
 // ARC. If it is not, but switches::kArcAvailable is set, ARC is installed
 // but is not allowed to run unless |kEnableArcFeature| is true.

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

https://codereview.chromium.org/2648213004/diff/40001/components/arc/arc_util...
components/arc/arc_util.h:14: // Returns true if the current device has ARC
installed and has the capability
nit: the term "capability" seems ambiguous to me. What about this:

 // Returns true if ARC is installed and the current device is
 // officially supported to run ARC.

You can mention Finch in .cc side if you want.

https://codereview.chromium.org/2648213004/diff/40001/components/arc/arc_util...
File components/arc/arc_util_unittest.cc (right):

https://codereview.chromium.org/2648213004/diff/40001/components/arc/arc_util...
components/arc/arc_util_unittest.cc:18: class ArcUtilTest : public testing::Test
{
nit: put this in an anonymous namespace

https://codereview.chromium.org/2648213004/diff/40001/components/arc/arc_util...
components/arc/arc_util_unittest.cc:36: TEST_F(ArcUtilTest,
IsArcAvailable_Installed) {
nit: is it common to have underscores in a test name?

https://codereview.chromium.org/2648213004/diff/40001/components/arc/arc_util...
components/arc/arc_util_unittest.cc:48:
feature_list.InitFromCommandLine("EnableARC", "");
* kEnableARC[] please.
* Most of us are not familiar with ScopedFeatureList. A comment like '// Enable
the experiment.' would help.

https://codereview.chromium.org/2648213004/diff/40001/components/arc/arc_util...
components/arc/arc_util_unittest.cc:53: feature_list.InitFromCommandLine("",
"EnableARC");
// Disable the experiment.

(here and elsewhere)

https://codereview.chromium.org/2648213004/diff/40001/components/arc/arc_util...
components/arc/arc_util_unittest.cc:76: TEST_F(ArcUtilTest,
IsArcAvailable_OfficialSupport) {
same

[xiyuan, 2017-01-24 19:22:41]

lgtm for my part

[Rahul Chaturvedi, 2017-01-24 20:33:03]

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

[Rahul Chaturvedi, 2017-01-24 20:33:04]

https://codereview.chromium.org/2648213004/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/extensions/info_private_apitest.cc (left):

https://codereview.chromium.org/2648213004/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/extensions/info_private_apitest.cc:93:
IN_PROC_BROWSER_TEST_F(ChromeOSInfoPrivateTest, ArcEnabled) {
If we're removing this test, how are we testing this functionality?

https://codereview.chromium.org/2648213004/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/extensions/info_private_apitest.cc (right):

https://codereview.chromium.org/2648213004/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/extensions/info_private_apitest.cc:35: //
TODO(hidehiko): Introduce a utility to let IsArcAvailable() return true
Please implement this as part of this CL.

[hidehiko, 2017-01-25 16:40:12]

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

[commit-bot: I haz the power, 2017-01-25 16:40:33]

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-25 17:40:28]

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

[commit-bot: I haz the power, 2017-01-25 17:40:59]

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-25 17:54:47]

Thank you for review. PTAL.

https://codereview.chromium.org/2648213004/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/extensions/info_private_apitest.cc (left):

https://codereview.chromium.org/2648213004/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/extensions/info_private_apitest.cc:93:
IN_PROC_BROWSER_TEST_F(ChromeOSInfoPrivateTest, ArcEnabled) {
On 2017/01/24 20:33:04, Rahul Chaturvedi wrote:
> If we're removing this test, how are we testing this functionality?

Oops. I misunderstood the code. Good catch. Restored now.

https://codereview.chromium.org/2648213004/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/extensions/info_private_apitest.cc (right):

https://codereview.chromium.org/2648213004/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/extensions/info_private_apitest.cc:35: //
TODO(hidehiko): Introduce a utility to let IsArcAvailable() return true
On 2017/01/24 20:33:04, Rahul Chaturvedi wrote:
> Please implement this as part of this CL.

Not technically difficult, but there are more files to be changed, so I was
worried about the increased size of CL.
# And, personally, I much prefer doing one thing per CL, if possible.

Anyway, I think "two reviewer have same comment" is a signal I should do now.
So, done.

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

https://codereview.chromium.org/2648213004/diff/40001/chrome/browser/ui/exten...
chrome/browser/ui/extensions/app_launch_params.cc:41: } else if
(arc::IsArcAvailable()) {
On 2017/01/24 19:02:44, Yusuke Sato wrote:
> nit: no }{

Hmm... I couldn't find a good example to omit in this case...
Any reference?

Anyway, I follow you and Luis's comment. Done.

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

https://codereview.chromium.org/2648213004/diff/40001/components/arc/arc_util...
components/arc/arc_util.cc:25: // TODO(hidehiko): Unify --enable-arc and
--arc-available flags.
On 2017/01/24 19:02:44, Yusuke Sato wrote:
> What about adding something like this?
> 
>  // If switches::kEnableArc is set, the device is officially supported to run
>  // ARC. If it is not, but switches::kArcAvailable is set, ARC is installed
>  // but is not allowed to run unless |kEnableArcFeature| is true.

Done.

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

https://codereview.chromium.org/2648213004/diff/40001/components/arc/arc_util...
components/arc/arc_util.h:14: // Returns true if the current device has ARC
installed and has the capability
On 2017/01/24 19:02:44, Yusuke Sato wrote:
> nit: the term "capability" seems ambiguous to me. What about this:
> 
>  // Returns true if ARC is installed and the current device is
>  // officially supported to run ARC.
> 
> You can mention Finch in .cc side if you want.
> 

Done.

https://codereview.chromium.org/2648213004/diff/40001/components/arc/arc_util...
File components/arc/arc_util_unittest.cc (right):

https://codereview.chromium.org/2648213004/diff/40001/components/arc/arc_util...
components/arc/arc_util_unittest.cc:18: class ArcUtilTest : public testing::Test
{
On 2017/01/24 19:02:44, Yusuke Sato wrote:
> nit: put this in an anonymous namespace

Can I keep it as is for consistency?
IIUC, most of ARC test fixture class are directly under the arc namespace.

https://codereview.chromium.org/2648213004/diff/40001/components/arc/arc_util...
components/arc/arc_util_unittest.cc:30:
std::unique_ptr<base::test::ScopedCommandLine> scoped_command_line_;
Note: it turned out that test framework actually restores command line flags per
test case, so we do not need it.

https://codereview.chromium.org/2648213004/diff/40001/components/arc/arc_util...
components/arc/arc_util_unittest.cc:36: TEST_F(ArcUtilTest,
IsArcAvailable_Installed) {
On 2017/01/24 19:02:44, Yusuke Sato wrote:
> nit: is it common to have underscores in a test name?

I think yes.
https://cs.chromium.org/search/?q=TEST_F%5C(.%2B_&sq=package:chromium&type=cs

about 1200 hits. Many are MAYBE_ and DISABLED_, but it actually contains real
'_' usages.

https://codereview.chromium.org/2648213004/diff/40001/components/arc/arc_util...
components/arc/arc_util_unittest.cc:48:
feature_list.InitFromCommandLine("EnableARC", "");
On 2017/01/24 19:02:44, Yusuke Sato wrote:
> * kEnableARC[] please.
> * Most of us are not familiar with ScopedFeatureList. A comment like '//
Enable
> the experiment.' would help.

Instead, I introduced a small wrapper. WDYT?

https://codereview.chromium.org/2648213004/diff/40001/components/arc/arc_util...
components/arc/arc_util_unittest.cc:53: feature_list.InitFromCommandLine("",
"EnableARC");
On 2017/01/24 19:02:44, Yusuke Sato wrote:
> // Disable the experiment.
> 
> (here and elsewhere)

Acknowledged.

https://codereview.chromium.org/2648213004/diff/40001/components/arc/arc_util...
components/arc/arc_util_unittest.cc:76: TEST_F(ArcUtilTest,
IsArcAvailable_OfficialSupport) {
On 2017/01/24 19:02:44, Yusuke Sato wrote:
> same

Acknowledged.

[Yusuke Sato, 2017-01-25 18:59:32]

https://codereview.chromium.org/2648213004/diff/40001/components/arc/arc_util...
File components/arc/arc_util_unittest.cc (right):

https://codereview.chromium.org/2648213004/diff/40001/components/arc/arc_util...
components/arc/arc_util_unittest.cc:18: class ArcUtilTest : public testing::Test
{
On 2017/01/25 17:54:47, hidehiko wrote:
> On 2017/01/24 19:02:44, Yusuke Sato wrote:
> > nit: put this in an anonymous namespace
> 
> Can I keep it as is for consistency?
> IIUC, most of ARC test fixture class are directly under the arc namespace.

Should we keep doing that?

https://google.github.io/styleguide/cppguide.html#Unnamed_Namespaces_and_Stat...
does not exclude _test.cc files.

[commit-bot: I haz the power, 2017-01-25 19:20:01]

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

[commit-bot: I haz the power, 2017-01-25 19:20:02]

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

[hidehiko, 2017-01-26 02:36:59]

https://codereview.chromium.org/2648213004/diff/40001/components/arc/arc_util...
File components/arc/arc_util_unittest.cc (right):

https://codereview.chromium.org/2648213004/diff/40001/components/arc/arc_util...
components/arc/arc_util_unittest.cc:18: class ArcUtilTest : public testing::Test
{
On 2017/01/25 18:59:31, Yusuke Sato wrote:
> On 2017/01/25 17:54:47, hidehiko wrote:
> > On 2017/01/24 19:02:44, Yusuke Sato wrote:
> > > nit: put this in an anonymous namespace
> > 
> > Can I keep it as is for consistency?
> > IIUC, most of ARC test fixture class are directly under the arc namespace.
> 
> Should we keep doing that?
> 
>
https://google.github.io/styleguide/cppguide.html#Unnamed_Namespaces_and_Stat...
> does not exclude _test.cc files.

I see. So,
- done in this file only.
- other tests under arc/, let's do it separately. Tracking bug: crbug.com/685464

[Yusuke Sato, 2017-01-26 02:41:46]

Thanks!

(btw, still lgtm, just in case.)

[hidehiko, 2017-01-26 06:20:21]

hidehiko@chromium.org changed reviewers:
+ bartfab@chromium.org, maxbogue@chromium.org, msw@chromium.org,
skuhne@chromium.org

[hidehiko, 2017-01-26 06:20:22]

Thank you for review, Yusuke.

Rahul, Luis, friendly ping?

R+=more owners for the added files at PS4.

xiyuan@: Could you take another look?
  c/b/ui/app_list/*, c/b/c/app_mode/*
bartfab@: c/b/policy
maxbogue@: c/b/sync
skuhne@: c/b/ui/ash
msw@: c/b/ui/views

[Rahul Chaturvedi, 2017-01-26 06:31:22]

https://codereview.chromium.org/2648213004/diff/110001/chrome/browser/chromeo...
File chrome/browser/chromeos/extensions/info_private_apitest.cc (right):

https://codereview.chromium.org/2648213004/diff/110001/chrome/browser/chromeo...
chrome/browser/chromeos/extensions/info_private_apitest.cc:83:
IN_PROC_BROWSER_TEST_F(ChromeOSInfoPrivateTest, ArcEnabled) {
I hate to nit-pick, but these tests will now always pass since we always enable
ARC++ as part of the test setup. Ideally we also want to test the situation
where ARC++ is *not* enabled. Earlier that was not getting tested but by adding
a simple check to the kiosk test above, or introducing a small test, we could
test it.

This functionality has broken before, hence my reluctance to let this go without
every single test case being tested. We minimum we need to test for,
ARC++ not enabled
ARC++ enabled, but not available
ARC++ available

I understand that the first of these tests did not exist to start; but this
would be a good time to add it :)

[Rahul Chaturvedi, 2017-01-26 06:32:11]

In the interests of not blocking this across timezones, since I am OOO for the
next few days.

lgtm provided the additional test I requested is added.

[hidehiko, 2017-01-26 06:49:15]

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

[commit-bot: I haz the power, 2017-01-26 06:49:32]

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-26 06:51:08]

Thank you for review, Rahul!

https://codereview.chromium.org/2648213004/diff/110001/chrome/browser/chromeo...
File chrome/browser/chromeos/extensions/info_private_apitest.cc (right):

https://codereview.chromium.org/2648213004/diff/110001/chrome/browser/chromeo...
chrome/browser/chromeos/extensions/info_private_apitest.cc:83:
IN_PROC_BROWSER_TEST_F(ChromeOSInfoPrivateTest, ArcEnabled) {
On 2017/01/26 06:31:22, Rahul Chaturvedi wrote:
> I hate to nit-pick, but these tests will now always pass since we always
enable
> ARC++ as part of the test setup. Ideally we also want to test the situation
> where ARC++ is *not* enabled. Earlier that was not getting tested but by
adding
> a simple check to the kiosk test above, or introducing a small test, we could
> test it.
> 
> This functionality has broken before, hence my reluctance to let this go
without
> every single test case being tested. We minimum we need to test for,
> ARC++ not enabled
> ARC++ enabled, but not available
> ARC++ available
> 
> I understand that the first of these tests did not exist to start; but this
> would be a good time to add it :)

Sure, done.
Instead of "not enabled", I named "not available" after the actual value ;-).

[commit-bot: I haz the power, 2017-01-26 08:09:41]

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

[commit-bot: I haz the power, 2017-01-26 08:09:42]

Dry run: This issue passed the CQ dry run.

[bartfab (slow), 2017-01-26 14:18:28]

policy LGTM

[xiyuan, 2017-01-26 16:06:11]

c/b/ui/app_list/*, c/b/c/app_mode/* lgtm

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

sorry for the delay. lgtm

https://codereview.chromium.org/2648213004/diff/1/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/session/chrome_session_manager.cc (right):

https://codereview.chromium.org/2648213004/diff/1/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/session/chrome_session_manager.cc:108:
arc::ArcServiceLauncher::Get()->OnPrimaryUserProfilePrepared(user_profile);
On 2017/01/24 18:05:56, hidehiko wrote:
> On 2017/01/24 17:00:37, Luis Héctor Chávez wrote:
> > On 2017/01/24 10:57:36, hidehiko wrote:
> > > Note: because ArcServiceLauncher instance is always created regardless of
> > > IsArcAvailable(), we should be able to call this unconditionally.
> > > 
> > > Note that, in case of that IsArcAvailable() returns false,
> > > ArcSessionManager::OnPrimaryUserProfilePrepared() handles it.
> > 
> > Please note this in a comment.
> > 
> > Also, I believe the motivation behind the check was to avoid
> > instantiating/initializing stuff unless it was necessary, so I'd vote to
keep
> it
> > unless it's causing some complication elsewhere.
> 
> I prefer the consistency, here.
> ArcService instances are initialized regardless of the IsArcAvailable(), so I
> think it makes sense to call OnPrimaryUserProfilePreapred() as well, where
some
> services are initialized in the function.
> 
> So, the diff in a case IsArcAvailable() returns false then is whether
Shutdown()
> is called or not, in ArcSessionManager. However, if IsArcAvailable() returns
> false, no-one can starts arc so it should be almost no-op. Thus, I do not
think
> we have much benefit by culling the call.
> 
> WDYT?
> 

I still believe we should try to avoid even instantiating the services when ARC
is not available, but I also buy the consistency argument. Let's fix that in a
separate change.

https://codereview.chromium.org/2648213004/diff/130001/components/arc/arc_uti...
File components/arc/arc_util_unittest.cc (right):

https://codereview.chromium.org/2648213004/diff/130001/components/arc/arc_uti...
components/arc/arc_util_unittest.cc:47: // Not availble, by-default.
nit: available

[msw, 2017-01-26 18:22:44]

c/b/ui/views lgtm with a q

https://codereview.chromium.org/2648213004/diff/130001/chrome/browser/ui/view...
File chrome/browser/ui/views/arc_app_dialog_view_browsertest.cc (left):

https://codereview.chromium.org/2648213004/diff/130001/chrome/browser/ui/view...
chrome/browser/ui/views/arc_app_dialog_view_browsertest.cc:48:
session_manager->OnPrimaryUserProfilePrepared(profile_);
q: just checking, is this not needed anymore?

[maxbogue, 2017-01-26 22:36:06]

c/b/sync lgtm

[hidehiko, 2017-01-30 08:21:58]

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:22:17]

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-30 09:30:18]

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

[commit-bot: I haz the power, 2017-01-30 09:30:19]

Dry run: This issue passed the CQ dry run.

[hidehiko, 2017-01-30 13:14:05]

Description was changed from

==========
Migrate --enable-arc and --arc-available part 1.

This CL removes ArcBridgeService::GetEnabled/GetAvailable,
instead instroduces one utility function IsArcAvailable()
to cover all those usages.

GetAvailable() returns whether ARC is installed or not,
regardless of whether it can run or not.
Its usage should be limited to finch only,
because users cannot run ARC actually.
New function IsArcAvailable() returns whether ARC can run
on the device (equivalent to GetEnabled(), but avoid "enabled" name
due to the conflict of arc.enabled preference).

The only usage of GetAvailable() was for the internal APIs,
which should be simply replaced by IsArcAvailable().

BUG=b/34480289, b/34478891
TEST=Ran bots.
==========

to

==========
Migrate --enable-arc and --arc-available part 1.

This CL removes ArcBridgeService::GetEnabled/GetAvailable,
instead instroduces one utility function IsArcAvailable()
to cover all those usages.

GetAvailable() returns whether ARC is installed or not,
regardless of whether it can run or not.
Its usage should be limited to finch only,
because users cannot run ARC actually.
New function IsArcAvailable() returns whether ARC can run
on the device (equivalent to GetEnabled(), but avoid "enabled" name
due to the conflict of arc.enabled preference).

The only usage of GetAvailable() was for the internal APIs,
which should be simply replaced by IsArcAvailable().

BUG=b/34480289, b/34478891
TEST=Ran bots.
TBR=skuhne@chromium.org, benwells@chromium.org
==========

[hidehiko, 2017-01-30 13:15:18]

Following the second item of
http://dev.chromium.org/developers/owners-files#TOC-When-to-use-To-Be-Reviewe...

I'm sending this to CQ with TBR=skuhne, benwells.

Thank you very much for your review.

https://codereview.chromium.org/2648213004/diff/1/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/session/chrome_session_manager.cc (right):

https://codereview.chromium.org/2648213004/diff/1/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/session/chrome_session_manager.cc:108:
arc::ArcServiceLauncher::Get()->OnPrimaryUserProfilePrepared(user_profile);
On 2017/01/26 17:48:26, Luis Héctor Chávez wrote:
> On 2017/01/24 18:05:56, hidehiko wrote:
> > On 2017/01/24 17:00:37, Luis Héctor Chávez wrote:
> > > On 2017/01/24 10:57:36, hidehiko wrote:
> > > > Note: because ArcServiceLauncher instance is always created regardless
of
> > > > IsArcAvailable(), we should be able to call this unconditionally.
> > > > 
> > > > Note that, in case of that IsArcAvailable() returns false,
> > > > ArcSessionManager::OnPrimaryUserProfilePrepared() handles it.
> > > 
> > > Please note this in a comment.
> > > 
> > > Also, I believe the motivation behind the check was to avoid
> > > instantiating/initializing stuff unless it was necessary, so I'd vote to
> keep
> > it
> > > unless it's causing some complication elsewhere.
> > 
> > I prefer the consistency, here.
> > ArcService instances are initialized regardless of the IsArcAvailable(), so
I
> > think it makes sense to call OnPrimaryUserProfilePreapred() as well, where
> some
> > services are initialized in the function.
> > 
> > So, the diff in a case IsArcAvailable() returns false then is whether
> Shutdown()
> > is called or not, in ArcSessionManager. However, if IsArcAvailable() returns
> > false, no-one can starts arc so it should be almost no-op. Thus, I do not
> think
> > we have much benefit by culling the call.
> > 
> > WDYT?
> > 
> 
> I still believe we should try to avoid even instantiating the services when
ARC
> is not available, but I also buy the consistency argument. Let's fix that in a
> separate change.

Thanks. I created a tracking bug. crbug.com/686703.

https://codereview.chromium.org/2648213004/diff/130001/components/arc/arc_uti...
File components/arc/arc_util_unittest.cc (right):

https://codereview.chromium.org/2648213004/diff/130001/components/arc/arc_uti...
components/arc/arc_util_unittest.cc:47: // Not availble, by-default.
On 2017/01/26 17:48:26, Luis Héctor Chávez wrote:
> nit: available

Good catch. Done.

[hidehiko, 2017-01-30 13:15:28]

The CQ bit was checked by hidehiko@chromium.org

[hidehiko, 2017-01-30 13:15:28]

The patchset sent to the CQ was uploaded after l-g-t-m from
afakhry@chromium.org, steel@chromium.org, yusukes@chromium.org,
bartfab@chromium.org, xiyuan@chromium.org, lhchavez@chromium.org,
msw@chromium.org, maxbogue@chromium.org
Link to the patchset: https://codereview.chromium.org/2648213004/#ps170001
(title: "address comments.")

[commit-bot: I haz the power, 2017-01-30 13:15:38]

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-30 14:36:52]

CQ is committing da patch.

Bot data: {"patchset_id": 170001, "attempt_start_ts": 1485782128079080,
"parent_rev": "711b0a82a640608bf4e069b9398ec415e2b8b20a", "commit_rev":
"aab3deacd994b91cf50594577b4c8e0329cdfee1"}

[commit-bot: I haz the power, 2017-01-30 14:37:30]

Description was changed from

==========
Migrate --enable-arc and --arc-available part 1.

This CL removes ArcBridgeService::GetEnabled/GetAvailable,
instead instroduces one utility function IsArcAvailable()
to cover all those usages.

GetAvailable() returns whether ARC is installed or not,
regardless of whether it can run or not.
Its usage should be limited to finch only,
because users cannot run ARC actually.
New function IsArcAvailable() returns whether ARC can run
on the device (equivalent to GetEnabled(), but avoid "enabled" name
due to the conflict of arc.enabled preference).

The only usage of GetAvailable() was for the internal APIs,
which should be simply replaced by IsArcAvailable().

BUG=b/34480289, b/34478891
TEST=Ran bots.
TBR=skuhne@chromium.org, benwells@chromium.org
==========

to

==========
Migrate --enable-arc and --arc-available part 1.

This CL removes ArcBridgeService::GetEnabled/GetAvailable,
instead instroduces one utility function IsArcAvailable()
to cover all those usages.

GetAvailable() returns whether ARC is installed or not,
regardless of whether it can run or not.
Its usage should be limited to finch only,
because users cannot run ARC actually.
New function IsArcAvailable() returns whether ARC can run
on the device (equivalent to GetEnabled(), but avoid "enabled" name
due to the conflict of arc.enabled preference).

The only usage of GetAvailable() was for the internal APIs,
which should be simply replaced by IsArcAvailable().

BUG=b/34480289, b/34478891
TEST=Ran bots.
TBR=skuhne@chromium.org, benwells@chromium.org

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

[commit-bot: I haz the power, 2017-01-30 14:37:32]

Committed patchset #10 (id:170001) as
https://chromium.googlesource.com/chromium/src/+/aab3deacd994b91cf50594577b4c...