Refactor kiosk app launch to be part of login screen UI flow.

Refactor kiosk app launch to be part of login screen UI flow.

The app launch UI no longer creates its own view and is no longer
a separate WebUI page. The splash screen is now a screen included
in the oobe and login flows.

BUG=269182
TEST=updated tests and manually tested on device

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=218461

[Tim Song, 2013-08-15 01:51:12]

This is my attempt at refactoring the kiosk app launch flow. One thing I need
some clarification on is the --force-app-mode flag. I removed StartupAppLauncher
in startup_browser_creator.cc, but it seems the app still gets launched anyways
with the flag set. Will this potentially cause problems?

Please take a look. I'll update the unit tests if the general approach is fine.

[xiyuan, 2013-08-15 18:19:42]

The approach seems fine. 

The change in startup_browser_creator.cc might have some problem.

https://codereview.chromium.org/22914008/diff/1/chrome/browser/chromeos/app_m...
File chrome/browser/chromeos/app_mode/kiosk_profile_loader.h (right):

https://codereview.chromium.org/22914008/diff/1/chrome/browser/chromeos/app_m...
chrome/browser/chromeos/app_mode/kiosk_profile_loader.h:31: };
nit: add a dtor
 protected:
  virtual ~Delegate() {}

https://codereview.chromium.org/22914008/diff/1/chrome/browser/chromeos/login...
File chrome/browser/chromeos/login/app_launch_controller.cc (right):

https://codereview.chromium.org/22914008/diff/1/chrome/browser/chromeos/login...
chrome/browser/chromeos/login/app_launch_controller.cc:73:
oobe_display_->GetAppLaunchSplashScreenActor();
nit: This can be part of the member initializer list.

https://codereview.chromium.org/22914008/diff/1/chrome/browser/chromeos/login...
chrome/browser/chromeos/login/app_launch_controller.cc:90: (new
KioskProfileLoader(KioskAppManager::Get(), app_id_, this))->Start();
I know I started this managing-its-own-life thing but now it makes more sense to
get rid of that and make AppLaunchController own KioskProfileLoader. Let's at
least put a TODO here.

https://codereview.chromium.org/22914008/diff/1/chrome/browser/chromeos/login...
File chrome/browser/chromeos/login/app_launch_controller.h (right):

https://codereview.chromium.org/22914008/diff/1/chrome/browser/chromeos/login...
chrome/browser/chromeos/login/app_launch_controller.h:66: void
OnProfileLoadFailed(KioskAppLaunchError::Error error);
nit: virtual ... OVERRIDE;

https://codereview.chromium.org/22914008/diff/1/chrome/browser/chromeos/login...
chrome/browser/chromeos/login/app_launch_controller.h:99: std::string app_id_;
nit: put back const if |app_id_| is not going to change during the life time of
this object.

https://codereview.chromium.org/22914008/diff/1/chrome/browser/chromeos/login...
File chrome/browser/chromeos/login/existing_user_controller.h (right):

https://codereview.chromium.org/22914008/diff/1/chrome/browser/chromeos/login...
chrome/browser/chromeos/login/existing_user_controller.h:98: virtual void
LaunchKioskApp(const std::string& app_id) OVERRIDE;
nit: LaunchKioskApp -> LoginAsKioskApp
Since we plan merge the login code path, it might make sense to have similar
names with the rest of the flows.

https://codereview.chromium.org/22914008/diff/1/chrome/browser/chromeos/login...
File chrome/browser/chromeos/login/login_display_host_impl.cc (right):

https://codereview.chromium.org/22914008/diff/1/chrome/browser/chromeos/login...
chrome/browser/chromeos/login/login_display_host_impl.cc:891: new
chromeos::LoginDisplayHostImpl(screen_bounds);
nit: chromeos:: is not needed.

https://codereview.chromium.org/22914008/diff/1/chrome/browser/chromeos/login...
File chrome/browser/chromeos/login/screens/app_launch_splash_screen_actor.h
(right):

https://codereview.chromium.org/22914008/diff/1/chrome/browser/chromeos/login...
chrome/browser/chromeos/login/screens/app_launch_splash_screen_actor.h:12: class
AppLaunchSplashScreenActor {
nit: Document the class.

https://codereview.chromium.org/22914008/diff/1/chrome/browser/chromeos/login...
chrome/browser/chromeos/login/screens/app_launch_splash_screen_actor.h:25: };
nit: add a dtor

https://codereview.chromium.org/22914008/diff/1/chrome/browser/resources/chro...
File chrome/browser/resources/chromeos/login/display_manager.js (right):

https://codereview.chromium.org/22914008/diff/1/chrome/browser/resources/chro...
chrome/browser/resources/chromeos/login/display_manager.js:221: }
nit: Remove enclosing {} for one liner.

https://codereview.chromium.org/22914008/diff/1/chrome/browser/resources/chro...
chrome/browser/resources/chromeos/login/display_manager.js:505: setFullScreen_:
function(screen) {
This could be done via CSS.

In oobe_screen.css:
.step.fullscreen {
  height: 100%;
  left: 0;
  right: 0;
  top: 0;
  width: 100%;
}

In screen_container.css:
#oobe.fullscreen #outer-container {
  height: 100%;
  width: 100%;
}

Then in updateScreenSize above, toggle "fullscreen" class on #oobe based on
whether the screen has "fullscreen" class. Something like
$('oobe').classList.toggle("fullscreen",
                           screen.classList.contains('fullscreen'));

https://codereview.chromium.org/22914008/diff/1/chrome/browser/resources/chro...
File chrome/browser/resources/chromeos/login/screen_app_launch_splash.js
(right):

https://codereview.chromium.org/22914008/diff/1/chrome/browser/resources/chro...
chrome/browser/resources/chromeos/login/screen_app_launch_splash.js:22:
disableTextSelectAndDrag();
Don't think we need this. Select and drag should be disabled on login screen
except on input fields.

https://codereview.chromium.org/22914008/diff/1/chrome/browser/resources/chro...
chrome/browser/resources/chromeos/login/screen_app_launch_splash.js:28:
shortcutEnabled != null ? !shortcutEnabled : true;
How could |shortcutEnabled| be null?

https://codereview.chromium.org/22914008/diff/1/chrome/browser/resources/chro...
chrome/browser/resources/chromeos/login/screen_app_launch_splash.js:29:
window.webkitRequestAnimationFrame(function() {});
This seems not needed.

https://codereview.chromium.org/22914008/diff/1/chrome/browser/resources/chro...
chrome/browser/resources/chromeos/login/screen_app_launch_splash.js:39:
disableTextSelectAndDrag(true, true);
Why do we need to do it here?

https://codereview.chromium.org/22914008/diff/1/chrome/browser/resources/chro...
chrome/browser/resources/chromeos/login/screen_app_launch_splash.js:65: },
nit: no ',' for the last property.

https://codereview.chromium.org/22914008/diff/1/chrome/browser/ui/startup/sta...
File chrome/browser/ui/startup/startup_browser_creator.cc (left):

https://codereview.chromium.org/22914008/diff/1/chrome/browser/ui/startup/sta...
chrome/browser/ui/startup/startup_browser_creator.cc:605: }
We would need to do something here to handle relaunching the app after chrome
crashed and restarted by the session manager.

https://codereview.chromium.org/22914008/diff/1/chrome/browser/ui/webui/chrom...
File chrome/browser/ui/webui/chrome_web_ui_controller_factory.cc (left):

https://codereview.chromium.org/22914008/diff/1/chrome/browser/ui/webui/chrom...
chrome/browser/ui/webui/chrome_web_ui_controller_factory.cc:343: if (url.host()
== chrome::kChromeUIAppLaunchHost)
nit: Don't forget to remove kChromeUIAppLaunchHost from url_constants and remove
app_launch_ui.cc (I saw the file is removed from gyp but not itself).

https://codereview.chromium.org/22914008/diff/1/chrome/browser/ui/webui/chrom...
File chrome/browser/ui/webui/chromeos/login/app_launch_splash_screen_handler.cc
(right):

https://codereview.chromium.org/22914008/diff/1/chrome/browser/ui/webui/chrom...
chrome/browser/ui/webui/chromeos/login/app_launch_splash_screen_handler.cc:45:
product_os_name));
nit: alignment

https://codereview.chromium.org/22914008/diff/1/chrome/browser/ui/webui/chrom...
chrome/browser/ui/webui/chromeos/login/app_launch_splash_screen_handler.cc:97:
l10n_util::GetStringUTF8(GetProgressMessageFromState(state_)));
nit: need enclosing {} since it's more than one line.

https://codereview.chromium.org/22914008/diff/1/chrome/browser/ui/webui/chrom...
chrome/browser/ui/webui/chromeos/login/app_launch_splash_screen_handler.cc:106:
base::DictionaryValue& out_info) {
nit: usually we use '*' if the function changes the argument.

https://codereview.chromium.org/22914008/diff/1/chrome/browser/ui/webui/chrom...
chrome/browser/ui/webui/chromeos/login/app_launch_splash_screen_handler.cc:155:
} // namespace chromeos
nit: two-space before //

https://codereview.chromium.org/22914008/diff/1/chrome/browser/ui/webui/chrom...
File chrome/browser/ui/webui/chromeos/login/app_launch_splash_screen_handler.h
(right):

https://codereview.chromium.org/22914008/diff/1/chrome/browser/ui/webui/chrom...
chrome/browser/ui/webui/chromeos/login/app_launch_splash_screen_handler.h:15: 
nit: nuke one empty line.

https://codereview.chromium.org/22914008/diff/1/chrome/browser/ui/webui/chrom...
chrome/browser/ui/webui/chromeos/login/app_launch_splash_screen_handler.h:21: 
nit: nuke the empty line

[Tim Song, 2013-08-16 19:07:59]

I resurrected the StartupAppLauncher and refactored the lifetime management of
both StartupAppLauncher and KioskProfileLoader. I also added a
app_launch_utils.cc file to handle a simple app launch as needed in
startup_browser_creator.cc.

I'm currently in the process of getting the tests to work.

https://codereview.chromium.org/22914008/diff/1/chrome/browser/chromeos/app_m...
File chrome/browser/chromeos/app_mode/kiosk_profile_loader.h (right):

https://codereview.chromium.org/22914008/diff/1/chrome/browser/chromeos/app_m...
chrome/browser/chromeos/app_mode/kiosk_profile_loader.h:31: };
On 2013/08/15 18:19:42, xiyuan wrote:
> nit: add a dtor
>  protected:
>   virtual ~Delegate() {}

Done.

https://codereview.chromium.org/22914008/diff/1/chrome/browser/chromeos/login...
File chrome/browser/chromeos/login/app_launch_controller.cc (right):

https://codereview.chromium.org/22914008/diff/1/chrome/browser/chromeos/login...
chrome/browser/chromeos/login/app_launch_controller.cc:73:
oobe_display_->GetAppLaunchSplashScreenActor();
On 2013/08/15 18:19:42, xiyuan wrote:
> nit: This can be part of the member initializer list.

Done.

https://codereview.chromium.org/22914008/diff/1/chrome/browser/chromeos/login...
chrome/browser/chromeos/login/app_launch_controller.cc:90: (new
KioskProfileLoader(KioskAppManager::Get(), app_id_, this))->Start();
On 2013/08/15 18:19:42, xiyuan wrote:
> I know I started this managing-its-own-life thing but now it makes more sense
to
> get rid of that and make AppLaunchController own KioskProfileLoader. Let's at
> least put a TODO here.

Done. Now using a scoped_ptr.

https://codereview.chromium.org/22914008/diff/1/chrome/browser/chromeos/login...
File chrome/browser/chromeos/login/app_launch_controller.h (right):

https://codereview.chromium.org/22914008/diff/1/chrome/browser/chromeos/login...
chrome/browser/chromeos/login/app_launch_controller.h:66: void
OnProfileLoadFailed(KioskAppLaunchError::Error error);
On 2013/08/15 18:19:42, xiyuan wrote:
> nit: virtual ... OVERRIDE;

Done.

https://codereview.chromium.org/22914008/diff/1/chrome/browser/chromeos/login...
chrome/browser/chromeos/login/app_launch_controller.h:99: std::string app_id_;
On 2013/08/15 18:19:42, xiyuan wrote:
> nit: put back const if |app_id_| is not going to change during the life time
of
> this object.

Done.

https://codereview.chromium.org/22914008/diff/1/chrome/browser/chromeos/login...
File chrome/browser/chromeos/login/existing_user_controller.h (right):

https://codereview.chromium.org/22914008/diff/1/chrome/browser/chromeos/login...
chrome/browser/chromeos/login/existing_user_controller.h:98: virtual void
LaunchKioskApp(const std::string& app_id) OVERRIDE;
On 2013/08/15 18:19:42, xiyuan wrote:
> nit: LaunchKioskApp -> LoginAsKioskApp
> Since we plan merge the login code path, it might make sense to have similar
> names with the rest of the flows.

Done.

https://codereview.chromium.org/22914008/diff/1/chrome/browser/chromeos/login...
File chrome/browser/chromeos/login/login_display_host_impl.cc (right):

https://codereview.chromium.org/22914008/diff/1/chrome/browser/chromeos/login...
chrome/browser/chromeos/login/login_display_host_impl.cc:891: new
chromeos::LoginDisplayHostImpl(screen_bounds);
On 2013/08/15 18:19:42, xiyuan wrote:
> nit: chromeos:: is not needed.

Done.

https://codereview.chromium.org/22914008/diff/1/chrome/browser/chromeos/login...
File chrome/browser/chromeos/login/screens/app_launch_splash_screen_actor.h
(right):

https://codereview.chromium.org/22914008/diff/1/chrome/browser/chromeos/login...
chrome/browser/chromeos/login/screens/app_launch_splash_screen_actor.h:12: class
AppLaunchSplashScreenActor {
On 2013/08/15 18:19:42, xiyuan wrote:
> nit: Document the class.

Done.

https://codereview.chromium.org/22914008/diff/1/chrome/browser/chromeos/login...
chrome/browser/chromeos/login/screens/app_launch_splash_screen_actor.h:25: };
On 2013/08/15 18:19:42, xiyuan wrote:
> nit: add a dtor

Done.

https://codereview.chromium.org/22914008/diff/1/chrome/browser/resources/chro...
File chrome/browser/resources/chromeos/login/display_manager.js (right):

https://codereview.chromium.org/22914008/diff/1/chrome/browser/resources/chro...
chrome/browser/resources/chromeos/login/display_manager.js:221: }
On 2013/08/15 18:19:42, xiyuan wrote:
> nit: Remove enclosing {} for one liner.

Done.

https://codereview.chromium.org/22914008/diff/1/chrome/browser/resources/chro...
chrome/browser/resources/chromeos/login/display_manager.js:505: setFullScreen_:
function(screen) {
On 2013/08/15 18:19:42, xiyuan wrote:
> This could be done via CSS.
> 
> In oobe_screen.css:
> .step.fullscreen {
>   height: 100%;
>   left: 0;
>   right: 0;
>   top: 0;
>   width: 100%;
> }
> 
> In screen_container.css:
> #oobe.fullscreen #outer-container {
>   height: 100%;
>   width: 100%;
> }
> 
> Then in updateScreenSize above, toggle "fullscreen" class on #oobe based on
> whether the screen has "fullscreen" class. Something like
> $('oobe').classList.toggle("fullscreen",
>                            screen.classList.contains('fullscreen'));

Good call. #outer-container is the parent of the #oobe, so I moved the
fullscreen to that element instead. It seems we still need the early return to
prevent all of the resizing logic below from running.

https://codereview.chromium.org/22914008/diff/1/chrome/browser/resources/chro...
File chrome/browser/resources/chromeos/login/screen_app_launch_splash.js
(right):

https://codereview.chromium.org/22914008/diff/1/chrome/browser/resources/chro...
chrome/browser/resources/chromeos/login/screen_app_launch_splash.js:22:
disableTextSelectAndDrag();
On 2013/08/15 18:19:42, xiyuan wrote:
> Don't think we need this. Select and drag should be disabled on login screen
> except on input fields.

Done. This is indeed the case.

https://codereview.chromium.org/22914008/diff/1/chrome/browser/resources/chro...
chrome/browser/resources/chromeos/login/screen_app_launch_splash.js:28:
shortcutEnabled != null ? !shortcutEnabled : true;
On 2013/08/15 18:19:42, xiyuan wrote:
> How could |shortcutEnabled| be null?

Done. We'll always pass in this field when we show the screen.

https://codereview.chromium.org/22914008/diff/1/chrome/browser/resources/chro...
chrome/browser/resources/chromeos/login/screen_app_launch_splash.js:29:
window.webkitRequestAnimationFrame(function() {});
On 2013/08/15 18:19:42, xiyuan wrote:
> This seems not needed.

Done.

https://codereview.chromium.org/22914008/diff/1/chrome/browser/resources/chro...
chrome/browser/resources/chromeos/login/screen_app_launch_splash.js:39:
disableTextSelectAndDrag(true, true);
On 2013/08/15 18:19:42, xiyuan wrote:
> Why do we need to do it here?

Done.

https://codereview.chromium.org/22914008/diff/1/chrome/browser/resources/chro...
chrome/browser/resources/chromeos/login/screen_app_launch_splash.js:65: },
On 2013/08/15 18:19:42, xiyuan wrote:
> nit: no ',' for the last property.

Done.

https://codereview.chromium.org/22914008/diff/1/chrome/browser/ui/startup/sta...
File chrome/browser/ui/startup/startup_browser_creator.cc (left):

https://codereview.chromium.org/22914008/diff/1/chrome/browser/ui/startup/sta...
chrome/browser/ui/startup/startup_browser_creator.cc:605: }
On 2013/08/15 18:19:42, xiyuan wrote:
> We would need to do something here to handle relaunching the app after chrome
> crashed and restarted by the session manager.

Done.

https://codereview.chromium.org/22914008/diff/1/chrome/browser/ui/webui/chrom...
File chrome/browser/ui/webui/chrome_web_ui_controller_factory.cc (left):

https://codereview.chromium.org/22914008/diff/1/chrome/browser/ui/webui/chrom...
chrome/browser/ui/webui/chrome_web_ui_controller_factory.cc:343: if (url.host()
== chrome::kChromeUIAppLaunchHost)
On 2013/08/15 18:19:42, xiyuan wrote:
> nit: Don't forget to remove kChromeUIAppLaunchHost from url_constants and
remove
> app_launch_ui.cc (I saw the file is removed from gyp but not itself).

Done. Nice catch. I also forgot to remove app_launch_view.cc/h

https://codereview.chromium.org/22914008/diff/1/chrome/browser/ui/webui/chrom...
File chrome/browser/ui/webui/chromeos/login/app_launch_splash_screen_handler.cc
(right):

https://codereview.chromium.org/22914008/diff/1/chrome/browser/ui/webui/chrom...
chrome/browser/ui/webui/chromeos/login/app_launch_splash_screen_handler.cc:45:
product_os_name));
On 2013/08/15 18:19:42, xiyuan wrote:
> nit: alignment

Done.

https://codereview.chromium.org/22914008/diff/1/chrome/browser/ui/webui/chrom...
chrome/browser/ui/webui/chromeos/login/app_launch_splash_screen_handler.cc:97:
l10n_util::GetStringUTF8(GetProgressMessageFromState(state_)));
On 2013/08/15 18:19:42, xiyuan wrote:
> nit: need enclosing {} since it's more than one line.

Done.

https://codereview.chromium.org/22914008/diff/1/chrome/browser/ui/webui/chrom...
chrome/browser/ui/webui/chromeos/login/app_launch_splash_screen_handler.cc:106:
base::DictionaryValue& out_info) {
On 2013/08/15 18:19:42, xiyuan wrote:
> nit: usually we use '*' if the function changes the argument.

Done.

https://codereview.chromium.org/22914008/diff/1/chrome/browser/ui/webui/chrom...
chrome/browser/ui/webui/chromeos/login/app_launch_splash_screen_handler.cc:155:
} // namespace chromeos
On 2013/08/15 18:19:42, xiyuan wrote:
> nit: two-space before //

Done.

https://codereview.chromium.org/22914008/diff/1/chrome/browser/ui/webui/chrom...
File chrome/browser/ui/webui/chromeos/login/app_launch_splash_screen_handler.h
(right):

https://codereview.chromium.org/22914008/diff/1/chrome/browser/ui/webui/chrom...
chrome/browser/ui/webui/chromeos/login/app_launch_splash_screen_handler.h:15: 
On 2013/08/15 18:19:42, xiyuan wrote:
> nit: nuke one empty line.

Done.

https://codereview.chromium.org/22914008/diff/1/chrome/browser/ui/webui/chrom...
chrome/browser/ui/webui/chromeos/login/app_launch_splash_screen_handler.h:21: 
On 2013/08/15 18:19:42, xiyuan wrote:
> nit: nuke the empty line

Done.

[xiyuan, 2013-08-19 17:40:29]

LGTM with nits

https://codereview.chromium.org/22914008/diff/32001/chrome/browser/chromeos/a...
File chrome/browser/chromeos/app_mode/app_launch_utils.h (right):

https://codereview.chromium.org/22914008/diff/32001/chrome/browser/chromeos/a...
chrome/browser/chromeos/app_mode/app_launch_utils.h:8: #include
"chrome/browser/profiles/profile.h"
nit: forward declaration in the header file is enough.

https://codereview.chromium.org/22914008/diff/32001/chrome/browser/chromeos/a...
File chrome/browser/chromeos/app_mode/kiosk_profile_loader.cc (right):

https://codereview.chromium.org/22914008/diff/32001/chrome/browser/chromeos/a...
chrome/browser/chromeos/app_mode/kiosk_profile_loader.cc:159: Delegate*
delegate)
nit: alignment

https://codereview.chromium.org/22914008/diff/32001/chrome/browser/chromeos/l...
File chrome/browser/chromeos/login/existing_user_controller.h (right):

https://codereview.chromium.org/22914008/diff/32001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/existing_user_controller.h:98: virtual void
LoginAsKioskApp(const std::string& app_id) OVERRIDE;
nit: Move it with other LoginAsxxx methods

https://codereview.chromium.org/22914008/diff/32001/chrome/browser/chromeos/l...
File chrome/browser/chromeos/login/login_display.h (right):

https://codereview.chromium.org/22914008/diff/32001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/login_display.h:104: virtual void
LoginAsKioskApp(const std::string& app_id) = 0;
nit: move it with other LoginAsxxx methods.

https://codereview.chromium.org/22914008/diff/32001/chrome/browser/chromeos/l...
File chrome/browser/chromeos/login/login_display_host_impl.cc (right):

https://codereview.chromium.org/22914008/diff/32001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/login_display_host_impl.cc:90: const char
kAppLaunchSplashURL[] = "chrome://oobe/login#app-launch-splash";
Use '#' to define display in login screen seems changed. You might need to
change this to "chrome://oobe/app-launch-splash" and update kKnownDisplayTypes
in oobe_ui.cc.

See https://chromiumcodereview.appspot.com/21004005

https://codereview.chromium.org/22914008/diff/32001/chrome/browser/resources/...
File chrome/browser/resources/chromeos/login/display_manager.js (right):

https://codereview.chromium.org/22914008/diff/32001/chrome/browser/resources/...
chrome/browser/resources/chromeos/login/display_manager.js:496: }
nit: Although we don't have code to return from a 'fullsreen' screen yet, but
it's good to have code after this "if" to clear 'fullscreen' just in case.

https://codereview.chromium.org/22914008/diff/32001/chrome/browser/ui/webui/c...
File chrome/browser/ui/webui/chromeos/login/app_launch_splash_screen_handler.h
(right):

https://codereview.chromium.org/22914008/diff/32001/chrome/browser/ui/webui/c...
chrome/browser/ui/webui/chromeos/login/app_launch_splash_screen_handler.h:36:
virtual void SetDelegate(AppLaunchSplashScreenHandler::Delegate* delegate)
OVERRIDE;
nit: wrap at 80 char col. In this case, we might need to wrap like this because
we don't want to put OVERRIDE on a line just by itself:
virtual void SetDelegate(
    AppLaunchSplashScreenHandler::Delegate* delegate) OVERRIDE;

[Tim Song, 2013-08-19 20:05:06]

https://codereview.chromium.org/22914008/diff/32001/chrome/browser/chromeos/a...
File chrome/browser/chromeos/app_mode/app_launch_utils.h (right):

https://codereview.chromium.org/22914008/diff/32001/chrome/browser/chromeos/a...
chrome/browser/chromeos/app_mode/app_launch_utils.h:8: #include
"chrome/browser/profiles/profile.h"
On 2013/08/19 17:40:29, xiyuan wrote:
> nit: forward declaration in the header file is enough.

Done.

https://codereview.chromium.org/22914008/diff/32001/chrome/browser/chromeos/a...
File chrome/browser/chromeos/app_mode/kiosk_profile_loader.cc (right):

https://codereview.chromium.org/22914008/diff/32001/chrome/browser/chromeos/a...
chrome/browser/chromeos/app_mode/kiosk_profile_loader.cc:159: Delegate*
delegate)
On 2013/08/19 17:40:29, xiyuan wrote:
> nit: alignment

Done.

https://codereview.chromium.org/22914008/diff/32001/chrome/browser/chromeos/l...
File chrome/browser/chromeos/login/existing_user_controller.h (right):

https://codereview.chromium.org/22914008/diff/32001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/existing_user_controller.h:98: virtual void
LoginAsKioskApp(const std::string& app_id) OVERRIDE;
On 2013/08/19 17:40:29, xiyuan wrote:
> nit: Move it with other LoginAsxxx methods

Done.

https://codereview.chromium.org/22914008/diff/32001/chrome/browser/chromeos/l...
File chrome/browser/chromeos/login/login_display.h (right):

https://codereview.chromium.org/22914008/diff/32001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/login_display.h:104: virtual void
LoginAsKioskApp(const std::string& app_id) = 0;
On 2013/08/19 17:40:29, xiyuan wrote:
> nit: move it with other LoginAsxxx methods.

Done.

https://codereview.chromium.org/22914008/diff/32001/chrome/browser/chromeos/l...
File chrome/browser/chromeos/login/login_display_host_impl.cc (right):

https://codereview.chromium.org/22914008/diff/32001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/login_display_host_impl.cc:90: const char
kAppLaunchSplashURL[] = "chrome://oobe/login#app-launch-splash";
On 2013/08/19 17:40:29, xiyuan wrote:
> Use '#' to define display in login screen seems changed. You might need to
> change this to "chrome://oobe/app-launch-splash" and update kKnownDisplayTypes
> in oobe_ui.cc.
> 
> See https://chromiumcodereview.appspot.com/21004005

Done. I added the app launch splash screen as a display type that loads assets
from the login flow.

https://codereview.chromium.org/22914008/diff/32001/chrome/browser/resources/...
File chrome/browser/resources/chromeos/login/display_manager.js (right):

https://codereview.chromium.org/22914008/diff/32001/chrome/browser/resources/...
chrome/browser/resources/chromeos/login/display_manager.js:496: }
On 2013/08/19 17:40:29, xiyuan wrote:
> nit: Although we don't have code to return from a 'fullsreen' screen yet, but
> it's good to have code after this "if" to clear 'fullscreen' just in case.

Done. The #('inner-container').style is being set below, and we also need to
clear these custom styles for the fullscreen styles to be applied.

https://codereview.chromium.org/22914008/diff/32001/chrome/browser/ui/webui/c...
File chrome/browser/ui/webui/chromeos/login/app_launch_splash_screen_handler.h
(right):

https://codereview.chromium.org/22914008/diff/32001/chrome/browser/ui/webui/c...
chrome/browser/ui/webui/chromeos/login/app_launch_splash_screen_handler.h:36:
virtual void SetDelegate(AppLaunchSplashScreenHandler::Delegate* delegate)
OVERRIDE;
On 2013/08/19 17:40:29, xiyuan wrote:
> nit: wrap at 80 char col. In this case, we might need to wrap like this
because
> we don't want to put OVERRIDE on a line just by itself:
> virtual void SetDelegate(
>     AppLaunchSplashScreenHandler::Delegate* delegate) OVERRIDE;

Done.

[Tim Song, 2013-08-19 20:12:15]

+sky

Please review: 
M chrome/browser/browser_resources.grd
M chrome/browser/ui/startup/startup_browser_creator.cc

Thank you.

[sky, 2013-08-19 22:04:39]

Said files LGTM

[commit-bot: I haz the power, 2013-08-19 22:52:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tengs@chromium.org/22914008/89001

[commit-bot: I haz the power, 2013-08-20 12:14:49]

Change committed as 218461