arc: Add Arc Kiosk app service and ability to launch kiosk apps.

arc: Add Arc Kiosk app service and ability to launch kiosk apps.

ArcKioskAppService is started for ARC++ kiosk sessions and keeps track of ARC++ state until it's ready to start kiosk app.
When all conditions are met, ArcKioskAppLauncher is created and it launches corresponding Android kiosk app, tracks it's status and pins to fullscreen when the app window is opened.

BUG=634497
Committed: https://crrev.com/000b1184a0fc089cdb20919ae255ec2dcf0c65d6
Cr-Commit-Position: refs/heads/master@{#432866}

[Sergey Poromov, 2016-11-15 16:22:48]

poromov@chromium.org changed reviewers:
+ bartfab@chromium.org, nkostylev@chromium.org, peletskyi@chromium.org,
stevenjb@chromium.org

[Sergey Poromov, 2016-11-15 18:08:35]

Hi Nikita,
please review changes in the following files:
chrome/browser/chromeos/app_mode_arc/*

[Sergey Poromov, 2016-11-15 18:09:20]

Hi Steven,
please review changes in the following files:
chrome/browser/ui/app_list/arc/*
chrome/browser/ui/ash/launcher/*

[stevenjb, 2016-11-15 18:54:18]

https://codereview.chromium.org/2496903003/diff/40001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/arc/arc_app_list_prefs.h (right):

https://codereview.chromium.org/2496903003/diff/40001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/arc/arc_app_list_prefs.h:379:
chromeos::ArcKioskAppService* kiosk_app_service_;
Please initialize this. (Initialize both of these to nullptr in the header to be
consistent with other inline initialization above).

[Sergey Poromov, 2016-11-15 21:15:38]

https://codereview.chromium.org/2496903003/diff/40001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/arc/arc_app_list_prefs.h (right):

https://codereview.chromium.org/2496903003/diff/40001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/arc/arc_app_list_prefs.h:379:
chromeos::ArcKioskAppService* kiosk_app_service_;
On 2016/11/15 18:54:18, stevenjb wrote:
> Please initialize this. (Initialize both of these to nullptr in the header to
be
> consistent with other inline initialization above).

Done.

[stevenjb, 2016-11-15 21:19:37]

https://codereview.chromium.org/2496903003/diff/40001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/arc/arc_app_list_prefs.h (right):

https://codereview.chromium.org/2496903003/diff/40001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/arc/arc_app_list_prefs.h:379:
chromeos::ArcKioskAppService* kiosk_app_service_;
On 2016/11/15 21:15:38, Sergey Poromov wrote:
> On 2016/11/15 18:54:18, stevenjb wrote:
> > Please initialize this. (Initialize both of these to nullptr in the header
to
> be
> > consistent with other inline initialization above).
> 
> Done.

You didn't remove the sync_service initialization in arc_app_list_prefs.cc.

[Sergey Poromov, 2016-11-15 21:25:14]

On 2016/11/15 21:19:37, stevenjb wrote:
>
https://codereview.chromium.org/2496903003/diff/40001/chrome/browser/ui/app_l...
> File chrome/browser/ui/app_list/arc/arc_app_list_prefs.h (right):
> 
>
https://codereview.chromium.org/2496903003/diff/40001/chrome/browser/ui/app_l...
> chrome/browser/ui/app_list/arc/arc_app_list_prefs.h:379:
> chromeos::ArcKioskAppService* kiosk_app_service_;
> On 2016/11/15 21:15:38, Sergey Poromov wrote:
> > On 2016/11/15 18:54:18, stevenjb wrote:
> > > Please initialize this. (Initialize both of these to nullptr in the header
> to
> > be
> > > consistent with other inline initialization above).
> > 
> > Done.
> 
> You didn't remove the sync_service initialization in arc_app_list_prefs.cc.

Done. Thanks.

[stevenjb, 2016-11-15 22:58:11]

lgtm w/ additional nit

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

https://codereview.chromium.org/2496903003/diff/80001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc:695: kiosk_app_service_ =
nullptr;
nit: The if isn't needed in this case (sorry, missed this the first time
through)

[Sergey Poromov, 2016-11-15 23:06:32]

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

https://codereview.chromium.org/2496903003/diff/80001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc:695: kiosk_app_service_ =
nullptr;
On 2016/11/15 22:58:11, stevenjb wrote:
> nit: The if isn't needed in this case (sorry, missed this the first time
> through)

Done.

[Nikita (slow), 2016-11-16 15:33:20]

On 2016/11/15 18:08:35, Sergey Poromov wrote:
> Hi Nikita,
> please review changes in the following files:
> chrome/browser/chromeos/app_mode_arc/*

lgtm with nits

[Nikita (slow), 2016-11-16 15:33:28]

https://codereview.chromium.org/2496903003/diff/100001/chrome/browser/chromeo...
File chrome/browser/chromeos/app_mode/arc/arc_kiosk_app_launcher.cc (right):

https://codereview.chromium.org/2496903003/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/app_mode/arc/arc_kiosk_app_launcher.cc:75:
ash::wm::PinWindow(window, true);
nit: true /* trusted */

https://codereview.chromium.org/2496903003/diff/100001/chrome/browser/chromeo...
File chrome/browser/chromeos/app_mode/arc/arc_kiosk_app_launcher.h (right):

https://codereview.chromium.org/2496903003/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/app_mode/arc/arc_kiosk_app_launcher.h:42: bool
CheckWindow(aura::Window* const window);
nit: method comment

https://codereview.chromium.org/2496903003/diff/100001/chrome/browser/chromeo...
File chrome/browser/chromeos/app_mode/arc/arc_kiosk_app_service.cc (right):

https://codereview.chromium.org/2496903003/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/app_mode/arc/arc_kiosk_app_service.cc:98:
profile_->GetPrefs()->GetBoolean(prefs::kArcPolicyCompliant)) {
nit: Same comment about policy compliance.
On ARC++ managing app will most likely always return that we're compliant.

https://codereview.chromium.org/2496903003/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/app_mode/arc/arc_kiosk_app_service.cc:115: //
TODO(poromov@): Choose appropriate app id to launch
nit: bug #?

https://codereview.chromium.org/2496903003/diff/100001/chrome/browser/chromeo...
File chrome/browser/chromeos/app_mode/arc/arc_kiosk_app_service_factory.cc
(right):

https://codereview.chromium.org/2496903003/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/app_mode/arc/arc_kiosk_app_service_factory.cc:18:
GetInstance()->GetServiceForBrowserContext(context, true));
nit: true /* .. */

[Sergey Poromov, 2016-11-16 15:54:09]

https://codereview.chromium.org/2496903003/diff/100001/chrome/browser/chromeo...
File chrome/browser/chromeos/app_mode/arc/arc_kiosk_app_launcher.cc (right):

https://codereview.chromium.org/2496903003/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/app_mode/arc/arc_kiosk_app_launcher.cc:75:
ash::wm::PinWindow(window, true);
On 2016/11/16 15:33:28, Nikita (slow) wrote:
> nit: true /* trusted */

Done.

https://codereview.chromium.org/2496903003/diff/100001/chrome/browser/chromeo...
File chrome/browser/chromeos/app_mode/arc/arc_kiosk_app_launcher.h (right):

https://codereview.chromium.org/2496903003/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/app_mode/arc/arc_kiosk_app_launcher.h:42: bool
CheckWindow(aura::Window* const window);
On 2016/11/16 15:33:28, Nikita (slow) wrote:
> nit: method comment

Done.

https://codereview.chromium.org/2496903003/diff/100001/chrome/browser/chromeo...
File chrome/browser/chromeos/app_mode/arc/arc_kiosk_app_service.cc (right):

https://codereview.chromium.org/2496903003/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/app_mode/arc/arc_kiosk_app_service.cc:98:
profile_->GetPrefs()->GetBoolean(prefs::kArcPolicyCompliant)) {
On 2016/11/16 15:33:28, Nikita (slow) wrote:
> nit: Same comment about policy compliance.
> On ARC++ managing app will most likely always return that we're compliant.
> 

This pref tracks whether ARC++ reported or not that it's compliant.
Also there might be cases when applying policies failed and that means that the
app shouldn't be started or even stopped if running.

https://codereview.chromium.org/2496903003/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/app_mode/arc/arc_kiosk_app_service.cc:115: //
TODO(poromov@): Choose appropriate app id to launch
On 2016/11/16 15:33:28, Nikita (slow) wrote:
> nit: bug #?

Done.

https://codereview.chromium.org/2496903003/diff/100001/chrome/browser/chromeo...
File chrome/browser/chromeos/app_mode/arc/arc_kiosk_app_service_factory.cc
(right):

https://codereview.chromium.org/2496903003/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/app_mode/arc/arc_kiosk_app_service_factory.cc:18:
GetInstance()->GetServiceForBrowserContext(context, true));
On 2016/11/16 15:33:28, Nikita (slow) wrote:
> nit: true /* .. */

Done.

[Luis Héctor Chávez, 2016-11-16 16:37:11]

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

[Luis Héctor Chávez, 2016-11-16 16:37:12]

drive-by

Can you also add a bit more description to the commit message?

https://codereview.chromium.org/2496903003/diff/120001/chrome/browser/chromeo...
File chrome/browser/chromeos/app_mode/arc/arc_kiosk_app_launcher.h (right):

https://codereview.chromium.org/2496903003/diff/120001/chrome/browser/chromeo...
chrome/browser/chromeos/app_mode/arc/arc_kiosk_app_launcher.h:43: bool
CheckWindow(aura::Window* const window);
You need to add the pinning part of the intention to the name. Otherwise the
code in arc_kiosk_app_launcher.cc:37 looks super weird.

How about PinAppWindow or something like that?

https://codereview.chromium.org/2496903003/diff/120001/chrome/browser/chromeo...
chrome/browser/chromeos/app_mode/arc/arc_kiosk_app_launcher.h:46: std::string
app_id_;
nit: const std::string app_id_;

https://codereview.chromium.org/2496903003/diff/120001/chrome/browser/chromeo...
chrome/browser/chromeos/app_mode/arc/arc_kiosk_app_launcher.h:47:
ArcAppListPrefs* prefs_;
nit: ArcAppListPrefs* const prefs_;

https://codereview.chromium.org/2496903003/diff/120001/chrome/browser/chromeo...
File chrome/browser/chromeos/app_mode/arc/arc_kiosk_app_service.cc (right):

https://codereview.chromium.org/2496903003/diff/120001/chrome/browser/chromeo...
chrome/browser/chromeos/app_mode/arc/arc_kiosk_app_service.cc:17:
ArcKioskAppService* ArcKioskAppService::Create(Profile* profile,
nit: this needs a // static comment before. Same for ::Get.

https://codereview.chromium.org/2496903003/diff/120001/chrome/browser/chromeo...
chrome/browser/chromeos/app_mode/arc/arc_kiosk_app_service.cc:29: if (app_id ==
app_id_) {
nit: elide braces

https://codereview.chromium.org/2496903003/diff/120001/chrome/browser/chromeo...
chrome/browser/chromeos/app_mode/arc/arc_kiosk_app_service.cc:56: activity ==
app_info_->activity)
nit: you cannot elide braces if the condition spans more than one line.

https://codereview.chromium.org/2496903003/diff/120001/chrome/browser/chromeo...
chrome/browser/chromeos/app_mode/arc/arc_kiosk_app_service.cc:87: if (prefs_) {
nit: elide braces, since you've been doing it in the rest of the file. same
below.

https://codereview.chromium.org/2496903003/diff/120001/chrome/browser/chromeo...
chrome/browser/chromeos/app_mode/arc/arc_kiosk_app_service.cc:96: app_info_ =
std::move(prefs_->GetApp(app_id_));
pref_->GetApp(...) is an rvalue. std::move() should only be used with lvalues
(i.e. std::move(variable_name)).

https://codereview.chromium.org/2496903003/diff/120001/chrome/browser/chromeo...
chrome/browser/chromeos/app_mode/arc/arc_kiosk_app_service.cc:111: if (app) {
nit:

if (!app)
  return std::string();

// Rest of the function.

https://codereview.chromium.org/2496903003/diff/120001/chrome/browser/chromeo...
chrome/browser/chromeos/app_mode/arc/arc_kiosk_app_service.cc:114: if
(!app_ids.empty()) {
nit:

if (app_ids.empty())
  return std::string();

return std::string(*app_ids.begin());

https://codereview.chromium.org/2496903003/diff/120001/chrome/browser/chromeo...
File chrome/browser/chromeos/app_mode/arc/arc_kiosk_app_service.h (right):

https://codereview.chromium.org/2496903003/diff/120001/chrome/browser/chromeo...
chrome/browser/chromeos/app_mode/arc/arc_kiosk_app_service.h:50: void
MaybeStartOrStopKioskApp();
Consider renaming it "PreconditionsChanged" to reduce the uncertainty in the
name, and add a small comment referring to the documentation block at the
beginning of the class.

https://codereview.chromium.org/2496903003/diff/120001/chrome/browser/chromeo...
File chrome/browser/chromeos/app_mode/arc/arc_kiosk_app_service_factory.cc
(right):

https://codereview.chromium.org/2496903003/diff/120001/chrome/browser/chromeo...
chrome/browser/chromeos/app_mode/arc/arc_kiosk_app_service_factory.cc:33:
ArcKioskAppServiceFactory::~ArcKioskAppServiceFactory() {}
nit: = default

https://codereview.chromium.org/2496903003/diff/120001/chrome/browser/ui/app_...
File chrome/browser/ui/app_list/arc/arc_app_list_prefs.h (right):

https://codereview.chromium.org/2496903003/diff/120001/chrome/browser/ui/app_...
chrome/browser/ui/app_list/arc/arc_app_list_prefs.h:379:
chromeos::ArcKioskAppService* kiosk_app_service_ = nullptr;
Please add a comment about the purpose of this variable. As the code stands, it
looks like it's not used at all. The side effects of this are non-obvious.

[Sergey Poromov, 2016-11-16 17:41:46]

Description was changed from

==========
arc: Add Arc Kiosk app service and ability to launch kiosk apps.

BUG=634497
==========

to

==========
arc: Add Arc Kiosk app service and ability to launch kiosk apps.

ArcKioskAppService is started for ARC++ kiosk sessions and keeps track of ARC++
state until it's ready to start kiosk app. When all conditions are met,
ArcKioskAppLauncher is created and it launches corresponding Android kiosk app,
tracks it's status and pins to fullscreen when the app window is opened.

BUG=634497
==========

[Sergey Poromov, 2016-11-16 17:42:00]

Description was changed from

==========
arc: Add Arc Kiosk app service and ability to launch kiosk apps.

ArcKioskAppService is started for ARC++ kiosk sessions and keeps track of ARC++
state until it's ready to start kiosk app. When all conditions are met,
ArcKioskAppLauncher is created and it launches corresponding Android kiosk app,
tracks it's status and pins to fullscreen when the app window is opened.

BUG=634497
==========

to

==========
arc: Add Arc Kiosk app service and ability to launch kiosk apps.

ArcKioskAppService is started for ARC++ kiosk sessions and keeps track of ARC++
state until it's ready to start kiosk app. 
When all conditions are met, ArcKioskAppLauncher is created and it launches
corresponding Android kiosk app, tracks it's status and pins to fullscreen when
the app window is opened.

BUG=634497
==========

[Sergey Poromov, 2016-11-16 17:42:14]

https://codereview.chromium.org/2496903003/diff/120001/chrome/browser/chromeo...
File chrome/browser/chromeos/app_mode/arc/arc_kiosk_app_launcher.h (right):

https://codereview.chromium.org/2496903003/diff/120001/chrome/browser/chromeo...
chrome/browser/chromeos/app_mode/arc/arc_kiosk_app_launcher.h:43: bool
CheckWindow(aura::Window* const window);
On 2016/11/16 16:37:12, Luis Héctor Chávez wrote:
> You need to add the pinning part of the intention to the name. Otherwise the
> code in arc_kiosk_app_launcher.cc:37 looks super weird.
> 
> How about PinAppWindow or something like that?

It both checks and pins the window if needed, so CheckAndPinWindow is better for
now. And the method is quite small to split in several ones.

https://codereview.chromium.org/2496903003/diff/120001/chrome/browser/chromeo...
chrome/browser/chromeos/app_mode/arc/arc_kiosk_app_launcher.h:46: std::string
app_id_;
On 2016/11/16 16:37:12, Luis Héctor Chávez wrote:
> nit: const std::string app_id_;

Done.

https://codereview.chromium.org/2496903003/diff/120001/chrome/browser/chromeo...
chrome/browser/chromeos/app_mode/arc/arc_kiosk_app_launcher.h:47:
ArcAppListPrefs* prefs_;
On 2016/11/16 16:37:11, Luis Héctor Chávez wrote:
> nit: ArcAppListPrefs* const prefs_;

Done.

https://codereview.chromium.org/2496903003/diff/120001/chrome/browser/chromeo...
File chrome/browser/chromeos/app_mode/arc/arc_kiosk_app_service.cc (right):

https://codereview.chromium.org/2496903003/diff/120001/chrome/browser/chromeo...
chrome/browser/chromeos/app_mode/arc/arc_kiosk_app_service.cc:17:
ArcKioskAppService* ArcKioskAppService::Create(Profile* profile,
On 2016/11/16 16:37:12, Luis Héctor Chávez wrote:
> nit: this needs a // static comment before. Same for ::Get.

Done.

https://codereview.chromium.org/2496903003/diff/120001/chrome/browser/chromeo...
chrome/browser/chromeos/app_mode/arc/arc_kiosk_app_service.cc:29: if (app_id ==
app_id_) {
On 2016/11/16 16:37:12, Luis Héctor Chávez wrote:
> nit: elide braces

Done.

https://codereview.chromium.org/2496903003/diff/120001/chrome/browser/chromeo...
chrome/browser/chromeos/app_mode/arc/arc_kiosk_app_service.cc:56: activity ==
app_info_->activity)
On 2016/11/16 16:37:12, Luis Héctor Chávez wrote:
> nit: you cannot elide braces if the condition spans more than one line.

Done.

https://codereview.chromium.org/2496903003/diff/120001/chrome/browser/chromeo...
chrome/browser/chromeos/app_mode/arc/arc_kiosk_app_service.cc:87: if (prefs_) {
On 2016/11/16 16:37:12, Luis Héctor Chávez wrote:
> nit: elide braces, since you've been doing it in the rest of the file. same
> below.

Done.

https://codereview.chromium.org/2496903003/diff/120001/chrome/browser/chromeo...
chrome/browser/chromeos/app_mode/arc/arc_kiosk_app_service.cc:96: app_info_ =
std::move(prefs_->GetApp(app_id_));
On 2016/11/16 16:37:12, Luis Héctor Chávez wrote:
> pref_->GetApp(...) is an rvalue. std::move() should only be used with lvalues
> (i.e. std::move(variable_name)).

Done.

https://codereview.chromium.org/2496903003/diff/120001/chrome/browser/chromeo...
chrome/browser/chromeos/app_mode/arc/arc_kiosk_app_service.cc:111: if (app) {
On 2016/11/16 16:37:12, Luis Héctor Chávez wrote:
> nit:
> 
> if (!app)
>   return std::string();
> 
> // Rest of the function.

Done.

https://codereview.chromium.org/2496903003/diff/120001/chrome/browser/chromeo...
chrome/browser/chromeos/app_mode/arc/arc_kiosk_app_service.cc:114: if
(!app_ids.empty()) {
On 2016/11/16 16:37:12, Luis Héctor Chávez wrote:
> nit:
> 
> if (app_ids.empty())
>   return std::string();
> 
> return std::string(*app_ids.begin());

Done.

https://codereview.chromium.org/2496903003/diff/120001/chrome/browser/chromeo...
File chrome/browser/chromeos/app_mode/arc/arc_kiosk_app_service.h (right):

https://codereview.chromium.org/2496903003/diff/120001/chrome/browser/chromeo...
chrome/browser/chromeos/app_mode/arc/arc_kiosk_app_service.h:50: void
MaybeStartOrStopKioskApp();
On 2016/11/16 16:37:12, Luis Héctor Chávez wrote:
> Consider renaming it "PreconditionsChanged" to reduce the uncertainty in the
> name, and add a small comment referring to the documentation block at the
> beginning of the class.

Done.

https://codereview.chromium.org/2496903003/diff/120001/chrome/browser/chromeo...
File chrome/browser/chromeos/app_mode/arc/arc_kiosk_app_service_factory.cc
(right):

https://codereview.chromium.org/2496903003/diff/120001/chrome/browser/chromeo...
chrome/browser/chromeos/app_mode/arc/arc_kiosk_app_service_factory.cc:33:
ArcKioskAppServiceFactory::~ArcKioskAppServiceFactory() {}
On 2016/11/16 16:37:12, Luis Héctor Chávez wrote:
> nit: = default

Done.

https://codereview.chromium.org/2496903003/diff/120001/chrome/browser/ui/app_...
File chrome/browser/ui/app_list/arc/arc_app_list_prefs.h (right):

https://codereview.chromium.org/2496903003/diff/120001/chrome/browser/ui/app_...
chrome/browser/ui/app_list/arc/arc_app_list_prefs.h:379:
chromeos::ArcKioskAppService* kiosk_app_service_ = nullptr;
On 2016/11/16 16:37:12, Luis Héctor Chávez wrote:
> Please add a comment about the purpose of this variable. As the code stands,
it
> looks like it's not used at all. The side effects of this are non-obvious.

Added comment.

[Luis Héctor Chávez, 2016-11-16 17:48:46]

https://codereview.chromium.org/2496903003/diff/140001/chrome/browser/chromeo...
File chrome/browser/chromeos/app_mode/arc/arc_kiosk_app_launcher.cc (right):

https://codereview.chromium.org/2496903003/diff/140001/chrome/browser/chromeo...
chrome/browser/chromeos/app_mode/arc/arc_kiosk_app_launcher.cc:69: bool
ArcKioskAppLauncher::CheckWindow(aura::Window* const window) {
Can you run trybots? This does not compile.

https://codereview.chromium.org/2496903003/diff/140001/chrome/browser/chromeo...
File chrome/browser/chromeos/app_mode/arc/arc_kiosk_app_service.cc (right):

https://codereview.chromium.org/2496903003/diff/140001/chrome/browser/chromeo...
chrome/browser/chromeos/app_mode/arc/arc_kiosk_app_service.cc:113: if
(app_ids.empty()) {
nit: elide braces

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

https://codereview.chromium.org/2496903003/diff/140001/chrome/browser/ui/app_...
chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc:671: kiosk_app_service_ =
chromeos::ArcKioskAppService::Get(profile_);
Also mention that this launches the kiosk app.

[Sergey Poromov, 2016-11-16 18:02:34]

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

[commit-bot: I haz the power, 2016-11-16 18:03:03]

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

[Sergey Poromov, 2016-11-16 18:03:20]

https://codereview.chromium.org/2496903003/diff/140001/chrome/browser/chromeo...
File chrome/browser/chromeos/app_mode/arc/arc_kiosk_app_launcher.cc (right):

https://codereview.chromium.org/2496903003/diff/140001/chrome/browser/chromeo...
chrome/browser/chromeos/app_mode/arc/arc_kiosk_app_launcher.cc:69: bool
ArcKioskAppLauncher::CheckWindow(aura::Window* const window) {
On 2016/11/16 17:48:46, Luis Héctor Chávez wrote:
> Can you run trybots? This does not compile.

Acknowledged.

https://codereview.chromium.org/2496903003/diff/140001/chrome/browser/chromeo...
File chrome/browser/chromeos/app_mode/arc/arc_kiosk_app_service.cc (right):

https://codereview.chromium.org/2496903003/diff/140001/chrome/browser/chromeo...
chrome/browser/chromeos/app_mode/arc/arc_kiosk_app_service.cc:113: if
(app_ids.empty()) {
On 2016/11/16 17:48:46, Luis Héctor Chávez wrote:
> nit: elide braces

Done.

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

https://codereview.chromium.org/2496903003/diff/140001/chrome/browser/ui/app_...
chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc:671: kiosk_app_service_ =
chromeos::ArcKioskAppService::Get(profile_);
On 2016/11/16 17:48:46, Luis Héctor Chávez wrote:
> Also mention that this launches the kiosk app.

It's mentioned in header file and as well comment above implies it.

[Sergey Poromov, 2016-11-16 18:03:36]

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

[commit-bot: I haz the power, 2016-11-16 18:04: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, 2016-11-16 18:49:26]

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

[commit-bot: I haz the power, 2016-11-16 18:49:27]

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

[Sergey Poromov, 2016-11-16 22:13:43]

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

[commit-bot: I haz the power, 2016-11-16 22:14:52]

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

[commit-bot: I haz the power, 2016-11-16 23:40:14]

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

[commit-bot: I haz the power, 2016-11-16 23:40:15]

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

[Sergey Poromov, 2016-11-17 11:48:29]

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

[commit-bot: I haz the power, 2016-11-17 11:48:43]

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

[commit-bot: I haz the power, 2016-11-17 14:10:20]

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

[commit-bot: I haz the power, 2016-11-17 14:10:21]

Dry run: This issue passed the CQ dry run.

[Sergey Poromov, 2016-11-17 14:23:35]

The CQ bit was checked by poromov@chromium.org

[Sergey Poromov, 2016-11-17 14:23:35]

The patchset sent to the CQ was uploaded after l-g-t-m from
stevenjb@chromium.org, nkostylev@chromium.org
Link to the patchset: https://codereview.chromium.org/2496903003/#ps200001
(title: "rebase")

[commit-bot: I haz the power, 2016-11-17 14:23:49]

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

[commit-bot: I haz the power, 2016-11-17 14:28:55]

Description was changed from

==========
arc: Add Arc Kiosk app service and ability to launch kiosk apps.

ArcKioskAppService is started for ARC++ kiosk sessions and keeps track of ARC++
state until it's ready to start kiosk app. 
When all conditions are met, ArcKioskAppLauncher is created and it launches
corresponding Android kiosk app, tracks it's status and pins to fullscreen when
the app window is opened.

BUG=634497
==========

to

==========
arc: Add Arc Kiosk app service and ability to launch kiosk apps.

ArcKioskAppService is started for ARC++ kiosk sessions and keeps track of ARC++
state until it's ready to start kiosk app. 
When all conditions are met, ArcKioskAppLauncher is created and it launches
corresponding Android kiosk app, tracks it's status and pins to fullscreen when
the app window is opened.

BUG=634497
==========

[commit-bot: I haz the power, 2016-11-17 14:28:56]

Committed patchset #11 (id:200001)

[commit-bot: I haz the power, 2016-11-17 14:31:17]

Description was changed from

==========
arc: Add Arc Kiosk app service and ability to launch kiosk apps.

ArcKioskAppService is started for ARC++ kiosk sessions and keeps track of ARC++
state until it's ready to start kiosk app. 
When all conditions are met, ArcKioskAppLauncher is created and it launches
corresponding Android kiosk app, tracks it's status and pins to fullscreen when
the app window is opened.

BUG=634497
==========

to

==========
arc: Add Arc Kiosk app service and ability to launch kiosk apps.

ArcKioskAppService is started for ARC++ kiosk sessions and keeps track of ARC++
state until it's ready to start kiosk app.
When all conditions are met, ArcKioskAppLauncher is created and it launches
corresponding Android kiosk app, tracks it's status and pins to fullscreen when
the app window is opened.

BUG=634497

Committed: https://crrev.com/000b1184a0fc089cdb20919ae255ec2dcf0c65d6
Cr-Commit-Position: refs/heads/master@{#432866}
==========

[commit-bot: I haz the power, 2016-11-17 14:31:18]

Patchset 11 (id:??) landed as
https://crrev.com/000b1184a0fc089cdb20919ae255ec2dcf0c65d6
Cr-Commit-Position: refs/heads/master@{#432866}