arc: Add splash screen for ARC++ Kiosk startup

arc: Add splash screen for ARC++ Kiosk startup

Implemented similar to App Launch Splash Screen that is used for Chrome Kiosk.

Splash screen is shown immediately after launching ARC++ Kiosk session
and destructed when app window is launched. This leads to no UI exposed,
except to sign in screen, splash screen and finally app window.

User can escape launching of the app by pressing Ctrl+Alt+S during the splash
screen.

Also fixes bug of restarting app few seconds after launch.

BUG=682778, 682780, 681111, 677310
TEST=Manual, Start ARC++ Kiosk sessions.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

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

[Sergey Poromov, 2017-01-24 18:03:36]

Description was changed from

==========
arc: Add splash screen for ARC++ Kiosk startup

Implemented similar to App Launch Splash Screen that is used for Chrome Kiosk.

Splash screen is shown immediately after launching ARC++ Kiosk session
and destructed when app window is launched. This leads to no UI exposed,
except to sign in screen, splash screen and finally app window.

User can escape launching of the app by pressing Ctrl+Alt+S during the splash
screen.

Also fixes bug of restarting app few seconds after launch.

BUG=682778,682780,	681111,677310
TEST=Manual, Start ARC++ Kiosk sessions.
==========

to

==========
arc: Add splash screen for ARC++ Kiosk startup

Implemented similar to App Launch Splash Screen that is used for Chrome Kiosk.

Splash screen is shown immediately after launching ARC++ Kiosk session
and destructed when app window is launched. This leads to no UI exposed,
except to sign in screen, splash screen and finally app window.

User can escape launching of the app by pressing Ctrl+Alt+S during the splash
screen.

Also fixes bug of restarting app few seconds after launch.

BUG=682778,682780,	681111,677310
TEST=Manual, Start ARC++ Kiosk sessions.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[Sergey Poromov, 2017-01-24 18:05:35]

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

[Sergey Poromov, 2017-01-24 18:05:52]

poromov@chromium.org changed reviewers:
+ nkostylev@chromium.org

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

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 18:15:56]

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

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

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

[Luis Héctor Chávez, 2017-01-24 18:40:22]

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

[Luis Héctor Chávez, 2017-01-24 18:40:25]

drive-by

https://codereview.chromium.org/2649103006/diff/1/chrome/browser/chromeos/app...
File chrome/browser/chromeos/app_mode/arc/arc_kiosk_app_launcher.h (right):

https://codereview.chromium.org/2649103006/diff/1/chrome/browser/chromeos/app...
chrome/browser/chromeos/app_mode/arc/arc_kiosk_app_launcher.h:27: virtual
~Delegate() {}
nit: = default

https://codereview.chromium.org/2649103006/diff/1/chrome/browser/chromeos/app...
chrome/browser/chromeos/app_mode/arc/arc_kiosk_app_launcher.h:61: Delegate*
delegate_;
nit: Delegate* const delegate_;

Also note who owns it.

https://codereview.chromium.org/2649103006/diff/1/chrome/browser/chromeos/app...
File chrome/browser/chromeos/app_mode/arc/arc_kiosk_app_service.cc (right):

https://codereview.chromium.org/2649103006/diff/1/chrome/browser/chromeos/app...
chrome/browser/chromeos/app_mode/arc/arc_kiosk_app_service.cc:28: delegate_ =
delegate;
nit: DCHECK(delegate);

Do you also want to DCHECK if |delegate_| was already set?

https://codereview.chromium.org/2649103006/diff/1/chrome/browser/chromeos/app...
chrome/browser/chromeos/app_mode/arc/arc_kiosk_app_service.cc:123:
app_launcher_.reset(new ArcKioskAppLauncher(
nit: app_launcher_ = base::MakeUnique<ArcKioskAppLauncher>(...);

https://codereview.chromium.org/2649103006/diff/1/chrome/browser/chromeos/app...
File chrome/browser/chromeos/app_mode/arc/arc_kiosk_app_service.h (right):

https://codereview.chromium.org/2649103006/diff/1/chrome/browser/chromeos/app...
chrome/browser/chromeos/app_mode/arc/arc_kiosk_app_service.h:43: virtual
~Delegate() {}
nit: = default

https://codereview.chromium.org/2649103006/diff/1/chrome/browser/chromeos/app...
chrome/browser/chromeos/app_mode/arc/arc_kiosk_app_service.h:92: Delegate*
delegate_ = nullptr;
Please note who owns this and its lifetime.

https://codereview.chromium.org/2649103006/diff/1/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/arc_kiosk_controller.cc (right):

https://codereview.chromium.org/2649103006/diff/1/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/arc_kiosk_controller.cc:34:
login_performer_.reset(new ChromeLoginPerformer(this));
nit: login_performer_ = base::MakeUnique<ChromeLoginPerformer>(this);

https://codereview.chromium.org/2649103006/diff/1/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/arc_kiosk_controller.cc:39:
ArcKioskAppService::Get(profile_)->SetDelegate(nullptr);
Can |profile_| be nullptr?

https://codereview.chromium.org/2649103006/diff/1/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/arc_kiosk_controller.h (right):

https://codereview.chromium.org/2649103006/diff/1/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/arc_kiosk_controller.h:12: #include
"chrome/browser/profiles/profile.h"
Can you remove this and fwd-declare it?

https://codereview.chromium.org/2649103006/diff/1/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/arc_kiosk_controller.h:14: #include
"chromeos/login/auth/user_context.h"
same

https://codereview.chromium.org/2649103006/diff/1/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/arc_kiosk_controller.h:15: #include
"components/signin/core/account_id/account_id.h"
same

https://codereview.chromium.org/2649103006/diff/1/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/arc_kiosk_controller.h:56: 
Please note who owns all of these raw pointers, and their lifetimes.

https://codereview.chromium.org/2649103006/diff/1/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/arc_kiosk_controller.h:57: LoginDisplayHost*
host_;
These three can be * const.

https://codereview.chromium.org/2649103006/diff/1/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/arc_kiosk_controller.h:60: ArcKioskAppService*
arc_kiosk_app_service_ = nullptr;
unused?

https://codereview.chromium.org/2649103006/diff/1/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/screens/arc_kiosk_splash_screen_actor.h
(right):

https://codereview.chromium.org/2649103006/diff/1/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/screens/arc_kiosk_splash_screen_actor.h:25:
virtual ~Delegate() {}
nit: = default

https://codereview.chromium.org/2649103006/diff/1/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/screens/arc_kiosk_splash_screen_actor.h:28:
virtual ~ArcKioskSplashScreenActor() {}
nit: = default

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

https://codereview.chromium.org/2649103006/diff/1/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/session/user_session_manager.cc:1283:
user_manager->IsLoggedInAsArcKioskApp())
nit: you cannot elide braces if the condition spans more than one line

https://codereview.chromium.org/2649103006/diff/1/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/ui/login_display_host.h (right):

https://codereview.chromium.org/2649103006/diff/1/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/ui/login_display_host.h:14: #include
"chrome/browser/chromeos/login/ui/login_display.h"
nit: remove and fwd-declare AccountId.

https://codereview.chromium.org/2649103006/diff/1/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/ui/login_display_host_impl.h (right):

https://codereview.chromium.org/2649103006/diff/1/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/ui/login_display_host_impl.h:18: #include
"chrome/browser/chromeos/login/arc_kiosk_controller.h"
nit: remove and fwd-declare ArcKioskController

https://codereview.chromium.org/2649103006/diff/1/chrome/browser/ui/webui/chr...
File chrome/browser/ui/webui/chromeos/login/arc_kiosk_splash_screen_handler.cc
(right):

https://codereview.chromium.org/2649103006/diff/1/chrome/browser/ui/webui/chr...
chrome/browser/ui/webui/chromeos/login/arc_kiosk_splash_screen_handler.cc:18:
const char kJsScreenPath[] = "login.ArcKioskSplashScreen";
nit: constexpr

https://codereview.chromium.org/2649103006/diff/1/chrome/browser/ui/webui/chr...
chrome/browser/ui/webui/chromeos/login/arc_kiosk_splash_screen_handler.cc:27:
ArcKioskSplashScreenHandler::~ArcKioskSplashScreenHandler() {}
nit: = default

https://codereview.chromium.org/2649103006/diff/1/chrome/browser/ui/webui/chr...
chrome/browser/ui/webui/chromeos/login/arc_kiosk_splash_screen_handler.cc:35: if
(show_on_init_) {
nit:

if (!show_on_init_)
  return;

https://codereview.chromium.org/2649103006/diff/1/chrome/browser/ui/webui/chr...
chrome/browser/ui/webui/chromeos/login/arc_kiosk_splash_screen_handler.cc:49:
base::DictionaryValue* app_info = new base::DictionaryValue();
prefer to use 

std::unique_ptr<base::DictionaryValue> app_info =
base::MakeUnique<base::DictionaryValue>();

and then std::move() it.

https://codereview.chromium.org/2649103006/diff/1/chrome/browser/ui/webui/chr...
chrome/browser/ui/webui/chromeos/login/arc_kiosk_splash_screen_handler.cc:61: if
(page_is_ready()) {
nit:

if (!page_is_ready())
  return;

https://codereview.chromium.org/2649103006/diff/1/chrome/browser/ui/webui/chr...
chrome/browser/ui/webui/chromeos/login/arc_kiosk_splash_screen_handler.cc:99: if
(delegate_)
nit:

if (!delegate_) {
  LOG(WARNING) << "No delegate set to handle cancel app launch";
  return;
}

https://codereview.chromium.org/2649103006/diff/1/chrome/browser/ui/webui/chr...
File chrome/browser/ui/webui/chromeos/login/arc_kiosk_splash_screen_handler.h
(right):

https://codereview.chromium.org/2649103006/diff/1/chrome/browser/ui/webui/chr...
chrome/browser/ui/webui/chromeos/login/arc_kiosk_splash_screen_handler.h:8:
#include <set>
unused?

https://codereview.chromium.org/2649103006/diff/1/chrome/browser/ui/webui/chr...
chrome/browser/ui/webui/chromeos/login/arc_kiosk_splash_screen_handler.h:38:
void PopulateAppInfo(base::DictionaryValue* out_info);
nit: fwd-declare base::DictionaryValue

[Sergey Poromov, 2017-01-25 14:28:19]

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

[commit-bot: I haz the power, 2017-01-25 14:28:34]

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

[Sergey Poromov, 2017-01-25 14:29:22]

https://codereview.chromium.org/2649103006/diff/1/chrome/browser/chromeos/app...
File chrome/browser/chromeos/app_mode/arc/arc_kiosk_app_launcher.h (right):

https://codereview.chromium.org/2649103006/diff/1/chrome/browser/chromeos/app...
chrome/browser/chromeos/app_mode/arc/arc_kiosk_app_launcher.h:27: virtual
~Delegate() {}
On 2017/01/24 18:40:24, Luis Héctor Chávez wrote:
> nit: = default

Done.

https://codereview.chromium.org/2649103006/diff/1/chrome/browser/chromeos/app...
chrome/browser/chromeos/app_mode/arc/arc_kiosk_app_launcher.h:61: Delegate*
delegate_;
On 2017/01/24 18:40:24, Luis Héctor Chávez wrote:
> nit: Delegate* const delegate_;
> 
> Also note who owns it.

Done.

https://codereview.chromium.org/2649103006/diff/1/chrome/browser/chromeos/app...
File chrome/browser/chromeos/app_mode/arc/arc_kiosk_app_service.cc (right):

https://codereview.chromium.org/2649103006/diff/1/chrome/browser/chromeos/app...
chrome/browser/chromeos/app_mode/arc/arc_kiosk_app_service.cc:28: delegate_ =
delegate;
On 2017/01/24 18:40:24, Luis Héctor Chávez wrote:
> nit: DCHECK(delegate);
> 
> Do you also want to DCHECK if |delegate_| was already set?

This |delegate| could be null to unregister it.
Not sure whether it's a good practice, but it's used in many places around the
code. I think because it doesn't require one more separated method to reset the
delegate.

https://codereview.chromium.org/2649103006/diff/1/chrome/browser/chromeos/app...
chrome/browser/chromeos/app_mode/arc/arc_kiosk_app_service.cc:123:
app_launcher_.reset(new ArcKioskAppLauncher(
On 2017/01/24 18:40:24, Luis Héctor Chávez wrote:
> nit: app_launcher_ = base::MakeUnique<ArcKioskAppLauncher>(...);

Done.

https://codereview.chromium.org/2649103006/diff/1/chrome/browser/chromeos/app...
File chrome/browser/chromeos/app_mode/arc/arc_kiosk_app_service.h (right):

https://codereview.chromium.org/2649103006/diff/1/chrome/browser/chromeos/app...
chrome/browser/chromeos/app_mode/arc/arc_kiosk_app_service.h:43: virtual
~Delegate() {}
On 2017/01/24 18:40:24, Luis Héctor Chávez wrote:
> nit: = default

Done.

https://codereview.chromium.org/2649103006/diff/1/chrome/browser/chromeos/app...
chrome/browser/chromeos/app_mode/arc/arc_kiosk_app_service.h:92: Delegate*
delegate_ = nullptr;
On 2017/01/24 18:40:24, Luis Héctor Chávez wrote:
> Please note who owns this and its lifetime.

Done.

https://codereview.chromium.org/2649103006/diff/1/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/arc_kiosk_controller.cc (right):

https://codereview.chromium.org/2649103006/diff/1/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/arc_kiosk_controller.cc:34:
login_performer_.reset(new ChromeLoginPerformer(this));
On 2017/01/24 18:40:24, Luis Héctor Chávez wrote:
> nit: login_performer_ = base::MakeUnique<ChromeLoginPerformer>(this);

Done.

https://codereview.chromium.org/2649103006/diff/1/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/arc_kiosk_controller.cc:39:
ArcKioskAppService::Get(profile_)->SetDelegate(nullptr);
On 2017/01/24 18:40:24, Luis Héctor Chávez wrote:
> Can |profile_| be nullptr?

Done.

https://codereview.chromium.org/2649103006/diff/1/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/arc_kiosk_controller.h (right):

https://codereview.chromium.org/2649103006/diff/1/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/arc_kiosk_controller.h:12: #include
"chrome/browser/profiles/profile.h"
On 2017/01/24 18:40:24, Luis Héctor Chávez wrote:
> Can you remove this and fwd-declare it?

Done.

https://codereview.chromium.org/2649103006/diff/1/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/arc_kiosk_controller.h:14: #include
"chromeos/login/auth/user_context.h"
On 2017/01/24 18:40:24, Luis Héctor Chávez wrote:
> same

Done.

https://codereview.chromium.org/2649103006/diff/1/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/arc_kiosk_controller.h:15: #include
"components/signin/core/account_id/account_id.h"
On 2017/01/24 18:40:24, Luis Héctor Chávez wrote:
> same

Done.

https://codereview.chromium.org/2649103006/diff/1/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/arc_kiosk_controller.h:56: 
On 2017/01/24 18:40:24, Luis Héctor Chávez wrote:
> Please note who owns all of these raw pointers, and their lifetimes.

Done.

https://codereview.chromium.org/2649103006/diff/1/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/arc_kiosk_controller.h:57: LoginDisplayHost*
host_;
On 2017/01/24 18:40:24, Luis Héctor Chávez wrote:
> These three can be * const.

Done.

https://codereview.chromium.org/2649103006/diff/1/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/arc_kiosk_controller.h:60: ArcKioskAppService*
arc_kiosk_app_service_ = nullptr;
On 2017/01/24 18:40:24, Luis Héctor Chávez wrote:
> unused?

Done.

https://codereview.chromium.org/2649103006/diff/1/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/screens/arc_kiosk_splash_screen_actor.h
(right):

https://codereview.chromium.org/2649103006/diff/1/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/screens/arc_kiosk_splash_screen_actor.h:25:
virtual ~Delegate() {}
On 2017/01/24 18:40:24, Luis Héctor Chávez wrote:
> nit: = default

Done.

https://codereview.chromium.org/2649103006/diff/1/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/screens/arc_kiosk_splash_screen_actor.h:28:
virtual ~ArcKioskSplashScreenActor() {}
On 2017/01/24 18:40:24, Luis Héctor Chávez wrote:
> nit: = default

Done.

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

https://codereview.chromium.org/2649103006/diff/1/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/session/user_session_manager.cc:1283:
user_manager->IsLoggedInAsArcKioskApp())
On 2017/01/24 18:40:24, Luis Héctor Chávez wrote:
> nit: you cannot elide braces if the condition spans more than one line

Done.

https://codereview.chromium.org/2649103006/diff/1/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/ui/login_display_host.h (right):

https://codereview.chromium.org/2649103006/diff/1/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/ui/login_display_host.h:14: #include
"chrome/browser/chromeos/login/ui/login_display.h"
On 2017/01/24 18:40:24, Luis Héctor Chávez wrote:
> nit: remove and fwd-declare AccountId.

Done.

https://codereview.chromium.org/2649103006/diff/1/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/ui/login_display_host_impl.h (right):

https://codereview.chromium.org/2649103006/diff/1/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/ui/login_display_host_impl.h:18: #include
"chrome/browser/chromeos/login/arc_kiosk_controller.h"
On 2017/01/24 18:40:24, Luis Héctor Chávez wrote:
> nit: remove and fwd-declare ArcKioskController

Done.

https://codereview.chromium.org/2649103006/diff/1/chrome/browser/ui/webui/chr...
File chrome/browser/ui/webui/chromeos/login/arc_kiosk_splash_screen_handler.cc
(right):

https://codereview.chromium.org/2649103006/diff/1/chrome/browser/ui/webui/chr...
chrome/browser/ui/webui/chromeos/login/arc_kiosk_splash_screen_handler.cc:18:
const char kJsScreenPath[] = "login.ArcKioskSplashScreen";
On 2017/01/24 18:40:24, Luis Héctor Chávez wrote:
> nit: constexpr

Done.

https://codereview.chromium.org/2649103006/diff/1/chrome/browser/ui/webui/chr...
chrome/browser/ui/webui/chromeos/login/arc_kiosk_splash_screen_handler.cc:27:
ArcKioskSplashScreenHandler::~ArcKioskSplashScreenHandler() {}
On 2017/01/24 18:40:24, Luis Héctor Chávez wrote:
> nit: = default

Done.

https://codereview.chromium.org/2649103006/diff/1/chrome/browser/ui/webui/chr...
chrome/browser/ui/webui/chromeos/login/arc_kiosk_splash_screen_handler.cc:35: if
(show_on_init_) {
On 2017/01/24 18:40:25, Luis Héctor Chávez wrote:
> nit:
> 
> if (!show_on_init_)
>   return;

Done.

https://codereview.chromium.org/2649103006/diff/1/chrome/browser/ui/webui/chr...
chrome/browser/ui/webui/chromeos/login/arc_kiosk_splash_screen_handler.cc:61: if
(page_is_ready()) {
On 2017/01/24 18:40:25, Luis Héctor Chávez wrote:
> nit:
> 
> if (!page_is_ready())
>   return;

Done.

https://codereview.chromium.org/2649103006/diff/1/chrome/browser/ui/webui/chr...
chrome/browser/ui/webui/chromeos/login/arc_kiosk_splash_screen_handler.cc:99: if
(delegate_)
On 2017/01/24 18:40:25, Luis Héctor Chávez wrote:
> nit:
> 
> if (!delegate_) {
>   LOG(WARNING) << "No delegate set to handle cancel app launch";
>   return;
> }

Done.

https://codereview.chromium.org/2649103006/diff/1/chrome/browser/ui/webui/chr...
File chrome/browser/ui/webui/chromeos/login/arc_kiosk_splash_screen_handler.h
(right):

https://codereview.chromium.org/2649103006/diff/1/chrome/browser/ui/webui/chr...
chrome/browser/ui/webui/chromeos/login/arc_kiosk_splash_screen_handler.h:8:
#include <set>
On 2017/01/24 18:40:25, Luis Héctor Chávez wrote:
> unused?

Done.

https://codereview.chromium.org/2649103006/diff/1/chrome/browser/ui/webui/chr...
chrome/browser/ui/webui/chromeos/login/arc_kiosk_splash_screen_handler.h:38:
void PopulateAppInfo(base::DictionaryValue* out_info);
On 2017/01/24 18:40:25, Luis Héctor Chávez wrote:
> nit: fwd-declare base::DictionaryValue

Done.

[Sergey Poromov, 2017-01-25 15:28:43]

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

[commit-bot: I haz the power, 2017-01-25 15:29:03]

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-25 16:29:21]

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

[commit-bot: I haz the power, 2017-01-25 16:29:22]

Dry run: This issue passed the CQ dry run.

[Sergey Poromov, 2017-01-25 16:54:38]

poromov@chromium.org changed reviewers:
+ achuith@chromium.org

[Sergey Poromov, 2017-01-25 16:54:39]

Nikita, Achuith: Please review the CL

[Luis Héctor Chávez, 2017-01-25 17:46:40]

https://codereview.chromium.org/2649103006/diff/80001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/arc_kiosk_controller.cc (right):

https://codereview.chromium.org/2649103006/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/login/arc_kiosk_controller.cc:21: const int
kArcKioskSplashScreenMinTimeMS = 3000;
nit:

constexpr base::TimeDelta kArcKioskSplashScreenMinTime =
base::TimeDelta::FromSeconds(3);

https://codereview.chromium.org/2649103006/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/login/arc_kiosk_controller.cc:109: const int64_t
time_taken_ms =
nit:

const base::TimeDelta delta = base::TimeTicks::Now() -
launch_splash_screen_time_;

https://codereview.chromium.org/2649103006/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/login/arc_kiosk_controller.cc:116:
splash_wait_timer_.Start(
I wonder if it's cleaner overall to unconditionally start the timer and point it
to another function that calls CleanUp() and SessionStarted() if the app has
launched first. then OnAppWindowLaunched would be

launched_ = true;
if (splash_wait_timer_.IsRunning())
  return;
TheNewFunctionThatCallsCleanupAndSessionStarted();

https://codereview.chromium.org/2649103006/diff/80001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/arc_kiosk_controller.h (right):

https://codereview.chromium.org/2649103006/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/login/arc_kiosk_controller.h:14: #include
"chromeos/login/auth/user_context.h"
you forgot to remove this and account_id.h

https://codereview.chromium.org/2649103006/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/login/arc_kiosk_controller.h:64:
ArcKioskSplashScreenActor* const arc_kiosk_splash_screen_actor_ = nullptr;
You shouldn't initialize this to nullptr here since you'll be initializing it in
the ctor.

https://codereview.chromium.org/2649103006/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/login/arc_kiosk_controller.h:73: int64_t
launch_splash_start_time_ = 0;
nit: base::Time launch_splash_screen_time_;

[Nikita (slow), 2017-01-25 19:18:04]

On 2017/01/25 16:54:39, Sergey Poromov wrote:
> Nikita, Achuith: Please review the CL

Achuith, can you please review this CL as a whole?

I can review some parts if you think my help is still needed.

[Sergey Poromov, 2017-01-26 17:08:53]

https://codereview.chromium.org/2649103006/diff/80001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/arc_kiosk_controller.cc (right):

https://codereview.chromium.org/2649103006/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/login/arc_kiosk_controller.cc:21: const int
kArcKioskSplashScreenMinTimeMS = 3000;
On 2017/01/25 17:46:39, Luis Héctor Chávez wrote:
> nit:
> 
> constexpr base::TimeDelta kArcKioskSplashScreenMinTime =
> base::TimeDelta::FromSeconds(3);

Done.

https://codereview.chromium.org/2649103006/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/login/arc_kiosk_controller.cc:109: const int64_t
time_taken_ms =
On 2017/01/25 17:46:40, Luis Héctor Chávez wrote:
> nit:
> 
> const base::TimeDelta delta = base::TimeTicks::Now() -
> launch_splash_screen_time_;

Done.

https://codereview.chromium.org/2649103006/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/login/arc_kiosk_controller.cc:116:
splash_wait_timer_.Start(
On 2017/01/25 17:46:40, Luis Héctor Chávez wrote:
> I wonder if it's cleaner overall to unconditionally start the timer and point
it
> to another function that calls CleanUp() and SessionStarted() if the app has
> launched first. then OnAppWindowLaunched would be
> 
> launched_ = true;
> if (splash_wait_timer_.IsRunning())
>   return;
> TheNewFunctionThatCallsCleanupAndSessionStarted();

Done.

https://codereview.chromium.org/2649103006/diff/80001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/arc_kiosk_controller.h (right):

https://codereview.chromium.org/2649103006/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/login/arc_kiosk_controller.h:14: #include
"chromeos/login/auth/user_context.h"
On 2017/01/25 17:46:40, Luis Héctor Chávez wrote:
> you forgot to remove this and account_id.h

Done.

https://codereview.chromium.org/2649103006/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/login/arc_kiosk_controller.h:64:
ArcKioskSplashScreenActor* const arc_kiosk_splash_screen_actor_ = nullptr;
On 2017/01/25 17:46:40, Luis Héctor Chávez wrote:
> You shouldn't initialize this to nullptr here since you'll be initializing it
in
> the ctor.

Done.

https://codereview.chromium.org/2649103006/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/login/arc_kiosk_controller.h:73: int64_t
launch_splash_start_time_ = 0;
On 2017/01/25 17:46:40, Luis Héctor Chávez wrote:
> nit: base::Time launch_splash_screen_time_;

Done.

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

https://codereview.chromium.org/2649103006/diff/100001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/arc_kiosk_controller.cc (right):

https://codereview.chromium.org/2649103006/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/login/arc_kiosk_controller.cc:22: constexpr
base::TimeDelta kArcKioskSplashScreenMinTimeMS =
Please drop the "in milliseconds" from the comment and the "MS" from the name.

https://codereview.chromium.org/2649103006/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/login/arc_kiosk_controller.cc:40:
launch_splash_start_time_ = base::TimeTicks::Now();
The idea was to start the timer here unconditionally:

splash_wait_timer_.Start(FROM_HERE,
                         base::TimeTicks::Now() + kArcKioskSplashScreenMinTime,
                         base::Bind(weak_ptr_factory_.GetWeakPtr(),
                                    &ArcKioskController::CloseSplashScreen));

https://codereview.chromium.org/2649103006/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/login/arc_kiosk_controller.cc:56: const base::TimeDelta
time_taken_ms =
same, drop the "_ms" suffix

https://codereview.chromium.org/2649103006/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/login/arc_kiosk_controller.cc:58: if (time_taken_ms <
kArcKioskSplashScreenMinTimeMS) {
and then this implementation would be

if (!launched_)
  return;
CleanUp();
session_manager::SessionManager::Get()->SessionStarted();

https://codereview.chromium.org/2649103006/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/login/arc_kiosk_controller.cc:124: 
and this implementation would be

launched_ = true;
if (splash_wait_timer_.IsRunning())
  return;
CloseSplashScreen();

https://codereview.chromium.org/2649103006/diff/100001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/arc_kiosk_controller.h (right):

https://codereview.chromium.org/2649103006/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/login/arc_kiosk_controller.h:20: }
nit:  // namespace base

https://codereview.chromium.org/2649103006/diff/100001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/screens/arc_kiosk_splash_screen_actor.h
(right):

https://codereview.chromium.org/2649103006/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/login/screens/arc_kiosk_splash_screen_actor.h:13: enum
ArcKioskState {
nit: enum class

And since you are now forced to use ArcKioskState:: prefix to refer to the
individual entries, you can drop the "ARC_KIOSK_STATE_" from the entries' names.

https://codereview.chromium.org/2649103006/diff/100001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/ui/login_display_host_impl.cc (right):

https://codereview.chromium.org/2649103006/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/login/ui/login_display_host_impl.cc:806:
arc_kiosk_controller_.reset(new ArcKioskController(this, GetOobeUI()));
nit: = base::MakeUnique<ArcKioskController>(...);

[Sergey Poromov, 2017-01-26 18:30:22]

https://codereview.chromium.org/2649103006/diff/100001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/arc_kiosk_controller.cc (right):

https://codereview.chromium.org/2649103006/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/login/arc_kiosk_controller.cc:22: constexpr
base::TimeDelta kArcKioskSplashScreenMinTimeMS =
On 2017/01/26 17:37:45, Luis Héctor Chávez wrote:
> Please drop the "in milliseconds" from the comment and the "MS" from the name.

Done.

https://codereview.chromium.org/2649103006/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/login/arc_kiosk_controller.cc:40:
launch_splash_start_time_ = base::TimeTicks::Now();
On 2017/01/26 17:37:45, Luis Héctor Chávez wrote:
> The idea was to start the timer here unconditionally:
> 
> splash_wait_timer_.Start(FROM_HERE,
>                          base::TimeTicks::Now() +
kArcKioskSplashScreenMinTime,
>                          base::Bind(weak_ptr_factory_.GetWeakPtr(),
>                                     &ArcKioskController::CloseSplashScreen));

Done.

https://codereview.chromium.org/2649103006/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/login/arc_kiosk_controller.cc:56: const base::TimeDelta
time_taken_ms =
On 2017/01/26 17:37:45, Luis Héctor Chávez wrote:
> same, drop the "_ms" suffix

Done.

https://codereview.chromium.org/2649103006/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/login/arc_kiosk_controller.cc:58: if (time_taken_ms <
kArcKioskSplashScreenMinTimeMS) {
On 2017/01/26 17:37:45, Luis Héctor Chávez wrote:
> and then this implementation would be
> 
> if (!launched_)
>   return;
> CleanUp();
> session_manager::SessionManager::Get()->SessionStarted();

Could it be that this method is launched by timer simultaneously with
OnAppWindowLaunched() ? Or they are always running in the same thread?

https://codereview.chromium.org/2649103006/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/login/arc_kiosk_controller.cc:124: 
On 2017/01/26 17:37:45, Luis Héctor Chávez wrote:
> and this implementation would be
> 
> launched_ = true;
> if (splash_wait_timer_.IsRunning())
>   return;
> CloseSplashScreen();

Done.

https://codereview.chromium.org/2649103006/diff/100001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/arc_kiosk_controller.h (right):

https://codereview.chromium.org/2649103006/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/login/arc_kiosk_controller.h:20: }
On 2017/01/26 17:37:45, Luis Héctor Chávez wrote:
> nit:  // namespace base

Done.

https://codereview.chromium.org/2649103006/diff/100001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/screens/arc_kiosk_splash_screen_actor.h
(right):

https://codereview.chromium.org/2649103006/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/login/screens/arc_kiosk_splash_screen_actor.h:13: enum
ArcKioskState {
On 2017/01/26 17:37:45, Luis Héctor Chávez wrote:
> nit: enum class
> 
> And since you are now forced to use ArcKioskState:: prefix to refer to the
> individual entries, you can drop the "ARC_KIOSK_STATE_" from the entries'
names.

Done.

https://codereview.chromium.org/2649103006/diff/100001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/ui/login_display_host_impl.cc (right):

https://codereview.chromium.org/2649103006/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/login/ui/login_display_host_impl.cc:806:
arc_kiosk_controller_.reset(new ArcKioskController(this, GetOobeUI()));
On 2017/01/26 17:37:45, Luis Héctor Chávez wrote:
> nit: = base::MakeUnique<ArcKioskController>(...);

Done.

[Luis Héctor Chávez, 2017-01-26 19:05:49]

https://codereview.chromium.org/2649103006/diff/100001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/arc_kiosk_controller.cc (right):

https://codereview.chromium.org/2649103006/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/login/arc_kiosk_controller.cc:58: if (time_taken_ms <
kArcKioskSplashScreenMinTimeMS) {
On 2017/01/26 18:30:22, Sergey Poromov wrote:
> On 2017/01/26 17:37:45, Luis Héctor Chávez wrote:
> > and then this implementation would be
> > 
> > if (!launched_)
> >   return;
> > CleanUp();
> > session_manager::SessionManager::Get()->SessionStarted();
> 
> Could it be that this method is launched by timer simultaneously with
> OnAppWindowLaunched() ? Or they are always running in the same thread?

Timer callbacks are always run in the same thread where they are .Start()ed (by
virtue of Timer being a wrapper around PostDelayedTask()), so there is no risk
of races.

https://codereview.chromium.org/2649103006/diff/120001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/arc_kiosk_controller.cc (right):

https://codereview.chromium.org/2649103006/diff/120001/chrome/browser/chromeo...
chrome/browser/chromeos/login/arc_kiosk_controller.cc:40:
splash_wait_timer_.Start(FROM_HERE, kArcKioskSplashScreenMinTime, this,
what about the base::Bind() part? That's important too.

Otherwise if the ArcKioskController object goes away and the callback is
invoked, you'll get a dangling pointer to |this|.

There's also the fact that this version of Start() is going away and should not
be used in new code:
https://cs.chromium.org/chromium/src/base/timer/timer.h?sq=package:chromium&r...

[Sergey Poromov, 2017-01-26 21:09:39]

https://codereview.chromium.org/2649103006/diff/120001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/arc_kiosk_controller.cc (right):

https://codereview.chromium.org/2649103006/diff/120001/chrome/browser/chromeo...
chrome/browser/chromeos/login/arc_kiosk_controller.cc:40:
splash_wait_timer_.Start(FROM_HERE, kArcKioskSplashScreenMinTime, this,
On 2017/01/26 19:05:49, Luis Héctor Chávez wrote:
> what about the base::Bind() part? That's important too.
> 
> Otherwise if the ArcKioskController object goes away and the callback is
> invoked, you'll get a dangling pointer to |this|.
> 
> There's also the fact that this version of Start() is going away and should
not
> be used in new code:
>
https://cs.chromium.org/chromium/src/base/timer/timer.h?sq=package:chromium&r...

Done. Thanks, I missed the part about Bind in your prev.comment.

[Sergey Poromov, 2017-01-26 22:24:54]

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

[commit-bot: I haz the power, 2017-01-26 22:25:47]

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-26 22:54:31]

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

[commit-bot: I haz the power, 2017-01-26 22:54:32]

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

[Sergey Poromov, 2017-01-26 23:16:30]

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

[commit-bot: I haz the power, 2017-01-26 23:17:12]

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

[Sergey Poromov, 2017-01-26 23:41:02]

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

[commit-bot: I haz the power, 2017-01-26 23: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-27 00:57:38]

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

[commit-bot: I haz the power, 2017-01-27 00:57:39]

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_...)

[Sergey Poromov, 2017-01-27 10:53:59]

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

[commit-bot: I haz the power, 2017-01-27 10:54:23]

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

[commit-bot: I haz the power, 2017-01-27 11:58:42]

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

[commit-bot: I haz the power, 2017-01-27 11:58:43]

Dry run: This issue passed the CQ dry run.

[achuithb, 2017-02-03 00:35:00]

lgtm with nits.

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

https://codereview.chromium.org/2649103006/diff/180001/chrome/browser/chromeo...
chrome/browser/chromeos/app_mode/arc/arc_kiosk_app_launcher.h:27: virtual
~Delegate() = default;
Let's also have a private section with DISALLOW_COPY_AND_ASSIGN?

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

https://codereview.chromium.org/2649103006/diff/180001/chrome/browser/chromeo...
chrome/browser/chromeos/app_mode/arc/arc_kiosk_app_service.h:46: };
DISALLOW_COPY_AND_ASSIGN

https://codereview.chromium.org/2649103006/diff/180001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/arc_kiosk_controller.cc (right):

https://codereview.chromium.org/2649103006/diff/180001/chrome/browser/chromeo...
chrome/browser/chromeos/login/arc_kiosk_controller.cc:36: DVLOG(1) << "Starting
ARC Kiosk...";
Does it make sense to log the account_id?

https://codereview.chromium.org/2649103006/diff/180001/chrome/browser/chromeo...
chrome/browser/chromeos/login/arc_kiosk_controller.cc:91:
chrome::AttemptUserExit();
Shouldn't the order be reversed here and below?

https://codereview.chromium.org/2649103006/diff/180001/chrome/browser/chromeo...
chrome/browser/chromeos/login/arc_kiosk_controller.cc:120: if
(splash_wait_timer_.IsRunning())
A comment here might be useful

https://codereview.chromium.org/2649103006/diff/180001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/arc_kiosk_controller.h (right):

https://codereview.chromium.org/2649103006/diff/180001/chrome/browser/chromeo...
chrome/browser/chromeos/login/arc_kiosk_controller.h:20: }  // namespace base
nit: drop comment

https://codereview.chromium.org/2649103006/diff/180001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/oobe_screen.cc (right):

https://codereview.chromium.org/2649103006/diff/180001/chrome/browser/chromeo...
chrome/browser/chromeos/login/oobe_screen.cc:47: "login",          //
SCREEN_SPECIAL_LOGIN
Why change this

https://codereview.chromium.org/2649103006/diff/180001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/screens/arc_kiosk_splash_screen_actor.h
(right):

https://codereview.chromium.org/2649103006/diff/180001/chrome/browser/chromeo...
chrome/browser/chromeos/login/screens/arc_kiosk_splash_screen_actor.h:25:
virtual ~Delegate() = default;
add private DISALLOW_COPY_AND_ASSIGN

https://codereview.chromium.org/2649103006/diff/180001/chrome/browser/chromeo...
chrome/browser/chromeos/login/screens/arc_kiosk_splash_screen_actor.h:38: };
Add private DISALLOW_COPY_AND_ASSIGN

https://codereview.chromium.org/2649103006/diff/180001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/chromeos/login/arc_kiosk_splash_screen_handler.cc
(right):

https://codereview.chromium.org/2649103006/diff/180001/chrome/browser/ui/webu...
chrome/browser/ui/webui/chromeos/login/arc_kiosk_splash_screen_handler.cc:24: } 
// namespace
Can drop this comment

https://codereview.chromium.org/2649103006/diff/180001/chrome/browser/ui/webu...
chrome/browser/ui/webui/chromeos/login/arc_kiosk_splash_screen_handler.cc:106: }
shouldn't you have a default with an assert?

https://codereview.chromium.org/2649103006/diff/180001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/chromeos/login/arc_kiosk_splash_screen_handler.h
(right):

https://codereview.chromium.org/2649103006/diff/180001/chrome/browser/ui/webu...
chrome/browser/ui/webui/chromeos/login/arc_kiosk_splash_screen_handler.h:27: //
BaseScreenHandler implementation:
Can you make everything below this private? I believe they are all accessed via
a base pointer

https://codereview.chromium.org/2649103006/diff/180001/chrome/browser/ui/webu...
chrome/browser/ui/webui/chromeos/login/arc_kiosk_splash_screen_handler.h:46:
ArcKioskSplashScreenHandler::Delegate* delegate_;
= nullptr;

https://codereview.chromium.org/2649103006/diff/180001/chrome/browser/ui/webu...
chrome/browser/ui/webui/chromeos/login/arc_kiosk_splash_screen_handler.h:47:
bool show_on_init_;
initialize this here rather than ctor

[Sergey Poromov, 2017-02-03 15:14:49]

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

[commit-bot: I haz the power, 2017-02-03 15:15:12]

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

[Sergey Poromov, 2017-02-03 15:15:37]

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

https://codereview.chromium.org/2649103006/diff/180001/chrome/browser/chromeo...
chrome/browser/chromeos/app_mode/arc/arc_kiosk_app_launcher.h:27: virtual
~Delegate() = default;
On 2017/02/03 00:35:00, achuithb wrote:
> Let's also have a private section with DISALLOW_COPY_AND_ASSIGN?

Done.

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

https://codereview.chromium.org/2649103006/diff/180001/chrome/browser/chromeo...
chrome/browser/chromeos/app_mode/arc/arc_kiosk_app_service.h:46: };
On 2017/02/03 00:35:00, achuithb wrote:
> DISALLOW_COPY_AND_ASSIGN

Done.

https://codereview.chromium.org/2649103006/diff/180001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/arc_kiosk_controller.cc (right):

https://codereview.chromium.org/2649103006/diff/180001/chrome/browser/chromeo...
chrome/browser/chromeos/login/arc_kiosk_controller.cc:36: DVLOG(1) << "Starting
ARC Kiosk...";
On 2017/02/03 00:35:00, achuithb wrote:
> Does it make sense to log the account_id?

Done.

https://codereview.chromium.org/2649103006/diff/180001/chrome/browser/chromeo...
chrome/browser/chromeos/login/arc_kiosk_controller.cc:91:
chrome::AttemptUserExit();
On 2017/02/03 00:35:00, achuithb wrote:
> Shouldn't the order be reversed here and below?

Done.
This was done as in AppLaunchController::OnLaunchFailed, however reverse order
looks more appropriate, right.
However, this should be not so important, as all classes from CleanUp() are
anyway deleted on user exit.

https://codereview.chromium.org/2649103006/diff/180001/chrome/browser/chromeo...
chrome/browser/chromeos/login/arc_kiosk_controller.cc:120: if
(splash_wait_timer_.IsRunning())
On 2017/02/03 00:35:00, achuithb wrote:
> A comment here might be useful

Done.

https://codereview.chromium.org/2649103006/diff/180001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/arc_kiosk_controller.h (right):

https://codereview.chromium.org/2649103006/diff/180001/chrome/browser/chromeo...
chrome/browser/chromeos/login/arc_kiosk_controller.h:20: }  // namespace base
On 2017/02/03 00:35:00, achuithb wrote:
> nit: drop comment

Done.

https://codereview.chromium.org/2649103006/diff/180001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/oobe_screen.cc (right):

https://codereview.chromium.org/2649103006/diff/180001/chrome/browser/chromeo...
chrome/browser/chromeos/login/oobe_screen.cc:47: "login",          //
SCREEN_SPECIAL_LOGIN
On 2017/02/03 00:35:00, achuithb wrote:
> Why change this

Oops, looks like presubmit checks did it. Will try to avoid it.

https://codereview.chromium.org/2649103006/diff/180001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/screens/arc_kiosk_splash_screen_actor.h
(right):

https://codereview.chromium.org/2649103006/diff/180001/chrome/browser/chromeo...
chrome/browser/chromeos/login/screens/arc_kiosk_splash_screen_actor.h:25:
virtual ~Delegate() = default;
On 2017/02/03 00:35:00, achuithb wrote:
> add private DISALLOW_COPY_AND_ASSIGN

Done.

https://codereview.chromium.org/2649103006/diff/180001/chrome/browser/chromeo...
chrome/browser/chromeos/login/screens/arc_kiosk_splash_screen_actor.h:38: };
On 2017/02/03 00:35:00, achuithb wrote:
> Add private DISALLOW_COPY_AND_ASSIGN

Done.

https://codereview.chromium.org/2649103006/diff/180001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/chromeos/login/arc_kiosk_splash_screen_handler.cc
(right):

https://codereview.chromium.org/2649103006/diff/180001/chrome/browser/ui/webu...
chrome/browser/ui/webui/chromeos/login/arc_kiosk_splash_screen_handler.cc:24: } 
// namespace
On 2017/02/03 00:35:00, achuithb wrote:
> Can drop this comment

Done.

https://codereview.chromium.org/2649103006/diff/180001/chrome/browser/ui/webu...
chrome/browser/ui/webui/chromeos/login/arc_kiosk_splash_screen_handler.cc:106: }
On 2017/02/03 00:35:00, achuithb wrote:
> shouldn't you have a default with an assert?

Done.

https://codereview.chromium.org/2649103006/diff/180001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/chromeos/login/arc_kiosk_splash_screen_handler.h
(right):

https://codereview.chromium.org/2649103006/diff/180001/chrome/browser/ui/webu...
chrome/browser/ui/webui/chromeos/login/arc_kiosk_splash_screen_handler.h:27: //
BaseScreenHandler implementation:
On 2017/02/03 00:35:00, achuithb wrote:
> Can you make everything below this private? I believe they are all accessed
via
> a base pointer

Done.

https://codereview.chromium.org/2649103006/diff/180001/chrome/browser/ui/webu...
chrome/browser/ui/webui/chromeos/login/arc_kiosk_splash_screen_handler.h:46:
ArcKioskSplashScreenHandler::Delegate* delegate_;
On 2017/02/03 00:35:00, achuithb wrote:
> = nullptr;

Done.

https://codereview.chromium.org/2649103006/diff/180001/chrome/browser/ui/webu...
chrome/browser/ui/webui/chromeos/login/arc_kiosk_splash_screen_handler.h:47:
bool show_on_init_;
On 2017/02/03 00:35:00, achuithb wrote:
> initialize this here rather than ctor

Done.

[commit-bot: I haz the power, 2017-02-03 15:37:12]

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

[commit-bot: I haz the power, 2017-02-03 15:37:13]

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

[Sergey Poromov, 2017-02-03 15:48:32]

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

[commit-bot: I haz the power, 2017-02-03 15:48:59]

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-02-03 16:13:09]

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

[commit-bot: I haz the power, 2017-02-03 16:13:10]

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

[Sergey Poromov, 2017-02-03 16:27:25]

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

[commit-bot: I haz the power, 2017-02-03 16:27:53]

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

[Sergey Poromov, 2017-02-03 17:14:38]

The CQ bit was checked by poromov@chromium.org

[Sergey Poromov, 2017-02-03 17:14:39]

The patchset sent to the CQ was uploaded after l-g-t-m from achuith@chromium.org
Link to the patchset: https://codereview.chromium.org/2649103006/#ps240001
(title: "achuithb@ comments")

[commit-bot: I haz the power, 2017-02-03 17:15:00]

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-02-03 21:38:36]

CQ is committing da patch.

Bot data: {"patchset_id": 240001, "attempt_start_ts": 1486142078960010,
"parent_rev": "9f3583163a9592f26a0b72ff702eee5d9b5f0378", "commit_rev":
"af6c8bbf83b31deb1a772193dff2ff2871f4fbd7"}

[commit-bot: I haz the power, 2017-02-03 21:39:15]

Description was changed from

==========
arc: Add splash screen for ARC++ Kiosk startup

Implemented similar to App Launch Splash Screen that is used for Chrome Kiosk.

Splash screen is shown immediately after launching ARC++ Kiosk session
and destructed when app window is launched. This leads to no UI exposed,
except to sign in screen, splash screen and finally app window.

User can escape launching of the app by pressing Ctrl+Alt+S during the splash
screen.

Also fixes bug of restarting app few seconds after launch.

BUG=682778,682780,	681111,677310
TEST=Manual, Start ARC++ Kiosk sessions.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
arc: Add splash screen for ARC++ Kiosk startup

Implemented similar to App Launch Splash Screen that is used for Chrome Kiosk.

Splash screen is shown immediately after launching ARC++ Kiosk session
and destructed when app window is launched. This leads to no UI exposed,
except to sign in screen, splash screen and finally app window.

User can escape launching of the app by pressing Ctrl+Alt+S during the splash
screen.

Also fixes bug of restarting app few seconds after launch.

BUG=682778,682780,	681111,677310
TEST=Manual, Start ARC++ Kiosk sessions.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

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

[commit-bot: I haz the power, 2017-02-03 21:39:17]

Committed patchset #13 (id:240001) as
https://chromium.googlesource.com/chromium/src/+/af6c8bbf83b31deb1a772193dff2...