Issue 9538002: Create Login UI for Demo User.

Issue 9538002: Create Login UI for Demo User. (Closed)

Created:
8 years, 9 months ago by rkc

Modified:
8 years, 9 months ago

Reviewers:
xiyuan, Nikita (slow), pastarmovj, Evan Stade

CC:
chromium-reviews, nkostylev+watch_chromium.org, mihaip+watch_chromium.org, Aaron Boodman, arv (Not doing code reviews), stevenjb+watch_chromium.org, davemoore+watch_chromium.org, zel

Base URL:
svn://svn.chromium.org/chrome/trunk/src

Visibility:
Public.

More Reviews

Description

Create Login UI for Demo User. This CL creates the Login UI for the demo user. Additionally it creates the kiosk_mode_helper. The UI is not final and will undergo UI review iterations before it is finalized. The current page is 'close' to what the final page will look like but the image and text are still just placeholders. R=nkostylev@chromium.org,estade@chromium.org,xiyuan@chromium.org BUG=chromium-os:26043 TEST=Tested the login UI and logging in with the --enable-kiosk-mode flag. Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=124333

Patch Set 1 #

Total comments: 16

Patch Set 2 : . #

Patch Set 3 : License. #

Patch Set 4 : image removed. #

Total comments: 14

Created: 8 years, 9 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+305 lines, -35 lines)			Patch
M	chrome/app/generated_resources.grd	View	1 2	1 chunk	+3 lines, -0 lines	0 comments	Download
M	chrome/browser/browser_resources.grd	View	1 2	1 chunk	+1 line, -0 lines	0 comments	Download
M	chrome/browser/chromeos/chrome_browser_main_chromeos.cc	View	1	3 chunks	+4 lines, -2 lines	0 comments	Download
A	chrome/browser/chromeos/kiosk_mode/kiosk_mode_helper.h	View	1	1 chunk	+59 lines, -0 lines	6 comments	Download
A	chrome/browser/chromeos/kiosk_mode/kiosk_mode_helper.cc	View	1 2 3	1 chunk	+70 lines, -0 lines	0 comments	Download
M	chrome/browser/chromeos/kiosk_mode/kiosk_mode_screensaver.h	View	1	1 chunk	+3 lines, -0 lines	0 comments	Download
M	chrome/browser/chromeos/kiosk_mode/kiosk_mode_screensaver.cc	View	1	4 chunks	+30 lines, -27 lines	4 comments	Download
M	chrome/browser/chromeos/login/webui_login_display.h	View		1 chunk	+1 line, -0 lines	0 comments	Download
M	chrome/browser/chromeos/login/webui_login_display.cc	View		1 chunk	+6 lines, -0 lines	0 comments	Download
M	chrome/browser/chromeos/login/webui_login_display_host.cc	View	1	2 chunks	+3 lines, -0 lines	0 comments	Download
M	chrome/browser/chromeos/ui/screensaver_extension_dialog.cc	View		2 chunks	+12 lines, -4 lines	4 comments	Download
A	chrome/browser/resources/chromeos/login/demo_user_login.css	View		1 chunk	+41 lines, -0 lines	0 comments	Download
A	chrome/browser/resources/chromeos/login/demo_user_login.html	View	1	1 chunk	+19 lines, -0 lines	0 comments	Download
A	chrome/browser/resources/chromeos/login/demo_user_login.js	View		1 chunk	+25 lines, -0 lines	0 comments	Download
M	chrome/browser/ui/webui/chromeos/login/oobe_ui.cc	View	1 2	3 chunks	+5 lines, -2 lines	0 comments	Download
M	chrome/browser/ui/webui/chromeos/login/signin_screen_handler.h	View		2 chunks	+6 lines, -0 lines	0 comments	Download
M	chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc	View	1 2 3	4 chunks	+15 lines, -0 lines	0 comments	Download
M	chrome/chrome_browser.gypi	View	1 2 3	1 chunk	+2 lines, -0 lines	0 comments	Download

Messages

Total messages: 9 (0 generated)

Expand Messages | Collapse Messages

Nikita (slow)

lgtm with nits http://codereview.chromium.org/9538002/diff/1/chrome/browser/chromeos/kiosk_mode/kiosk_mode_screensaver.cc File chrome/browser/chromeos/kiosk_mode/kiosk_mode_screensaver.cc (right): http://codereview.chromium.org/9538002/diff/1/chrome/browser/chromeos/kiosk_mode/kiosk_mode_screensaver.cc#newcode51 chrome/browser/chromeos/kiosk_mode/kiosk_mode_screensaver.cc:51: chromeos::DBusThreadManager::Get()-> I've realized that you would ...

8 years, 9 months ago (2012-02-29 12:21:55 UTC) #2

xiyuan

LGTM with nits https://chromiumcodereview.appspot.com/9538002/diff/1/chrome/browser/chromeos/kiosk_mode/kiosk_mode_screensaver.cc File chrome/browser/chromeos/kiosk_mode/kiosk_mode_screensaver.cc (right): https://chromiumcodereview.appspot.com/9538002/diff/1/chrome/browser/chromeos/kiosk_mode/kiosk_mode_screensaver.cc#newcode21 chrome/browser/chromeos/kiosk_mode/kiosk_mode_screensaver.cc:21: Init(); nit: Could we have a ...

8 years, 9 months ago (2012-02-29 18:01:12 UTC) #3

rkc

https://chromiumcodereview.appspot.com/9538002/diff/1/chrome/browser/chromeos/kiosk_mode/kiosk_mode_screensaver.cc File chrome/browser/chromeos/kiosk_mode/kiosk_mode_screensaver.cc (right): https://chromiumcodereview.appspot.com/9538002/diff/1/chrome/browser/chromeos/kiosk_mode/kiosk_mode_screensaver.cc#newcode21 chrome/browser/chromeos/kiosk_mode/kiosk_mode_screensaver.cc:21: Init(); On 2012/02/29 18:01:12, xiyuan wrote: > nit: Could ...

8 years, 9 months ago (2012-02-29 20:57:48 UTC) #4

https://chromiumcodereview.appspot.com/9538002/diff/1/chrome/browser/chromeos...
File chrome/browser/chromeos/kiosk_mode/kiosk_mode_screensaver.cc (right):

https://chromiumcodereview.appspot.com/9538002/diff/1/chrome/browser/chromeos...
chrome/browser/chromeos/kiosk_mode/kiosk_mode_screensaver.cc:21: Init();
On 2012/02/29 18:01:12, xiyuan wrote:
> nit: Could we have a better name? On first look, "Init" confuses me with the
> Initialize method on KioskModHelper. How about change it to something else,
e.g.
> "Setup".

Done.

https://chromiumcodereview.appspot.com/9538002/diff/1/chrome/browser/chromeos...
chrome/browser/chromeos/kiosk_mode/kiosk_mode_screensaver.cc:23:
chromeos::KioskModeHelper::Get()->Initialize(
On 2012/02/29 18:01:12, xiyuan wrote:
> nit: suggest to enclose this with {} since it's more than one line.

Done.

https://chromiumcodereview.appspot.com/9538002/diff/1/chrome/browser/chromeos...
chrome/browser/chromeos/kiosk_mode/kiosk_mode_screensaver.cc:51:
chromeos::DBusThreadManager::Get()->
On 2012/02/29 12:21:55, Nikita Kostylev wrote:
> I've realized that you would also need a patch for PowerManager. If Chromebook
> stays idle at the login screen it will power off after some time. In Kiosk
mode
> we want to disable such behavior.

That is correct. I need to speak to Benson about this. The disable_idle_suspend
option still dims the screen, which is also something we want to disable for
this.

I'll address this in a forthcoming CL and changes to power manager.

https://chromiumcodereview.appspot.com/9538002/diff/1/chrome/browser/chromeos...
chrome/browser/chromeos/kiosk_mode/kiosk_mode_screensaver.cc:73: // the logout
dialog if it's still up.
On 2012/02/29 12:21:55, Nikita Kostylev wrote:
> nit: logout dialog?

Done.

https://chromiumcodereview.appspot.com/9538002/diff/1/chrome/browser/chromeos...
chrome/browser/chromeos/kiosk_mode/kiosk_mode_screensaver.cc:81: // Before
anything else, close the logout dialog to prevent restart
On 2012/02/29 12:21:55, Nikita Kostylev wrote:
> nit: logout dialog?

Done.

https://chromiumcodereview.appspot.com/9538002/diff/1/chrome/browser/chromeos...
chrome/browser/chromeos/kiosk_mode/kiosk_mode_screensaver.cc:102: if
(!g_kiosk_mode_screensaver) {
On 2012/02/29 12:21:55, Nikita Kostylev wrote:
> On the second thought delete of a NULL pointer is a noop so you may as well
> remove this is clause and remove check on shutdown path to simplify.

This actually really helped me with debugging already :) I'm inclined to leave
it in just to catch any weird places where Shutdown gets called without
Initialize, which should never really happen.

https://chromiumcodereview.appspot.com/9538002/diff/1/chrome/browser/resource...
File chrome/browser/resources/chromeos/login/demo_user_login.html (right):

https://chromiumcodereview.appspot.com/9538002/diff/1/chrome/browser/resource...
chrome/browser/resources/chromeos/login/demo_user_login.html:8: <body
bgcolor="#000000">
On 2012/02/29 18:01:12, xiyuan wrote:
> bgcolor should be in css. And you probably don't need this since your #page is
> width:100% and height:100%.

Done.

https://chromiumcodereview.appspot.com/9538002/diff/1/chrome/browser/ui/webui...
File chrome/browser/ui/webui/chromeos/login/oobe_ui.cc (right):

https://chromiumcodereview.appspot.com/9538002/diff/1/chrome/browser/ui/webui...
chrome/browser/ui/webui/chromeos/login/oobe_ui.cc:94: if
(chromeos::KioskModeHelper::Get()->IsKioskModeEnabled())
On 2012/02/29 12:21:55, Nikita Kostylev wrote:
> This will also be shown in case we decide to show a lock screen from demo user
> but that won't happen, right?

Correct. There is another CL I am working on that replaces the screen locker
with the auto-logout on idle.

pastarmovj

Sorry for being late and uninvited review but we were in flight on Friday. Please ...

8 years, 9 months ago (2012-03-05 15:22:25 UTC) #5

rkc

I'll address all the style fixes in the next CL for kiosk mode. http://codereview.chromium.org/9538002/diff/10002/chrome/browser/chromeos/kiosk_mode/kiosk_mode_helper.h File ...

8 years, 9 months ago (2012-03-05 23:16:13 UTC) #6

Evan Stade

is there a particular set of files I am intended to review, or should I ...

8 years, 9 months ago (2012-03-05 23:17:02 UTC) #7

rkc

On 2012/03/05 23:17:02, Evan Stade wrote: > is there a particular set of files I ...

8 years, 9 months ago (2012-03-05 23:18:40 UTC) #8

rkc

8 years, 9 months ago (2012-03-16 00:17:33 UTC) #9

Addressed pastarmovj@'s review comments in this CL,
http://codereview.chromium.org/9701084/ (patch set 2)

http://codereview.chromium.org/9538002/diff/10002/chrome/browser/chromeos/kio...
File chrome/browser/chromeos/kiosk_mode/kiosk_mode_helper.h (right):

http://codereview.chromium.org/9538002/diff/10002/chrome/browser/chromeos/kio...
chrome/browser/chromeos/kiosk_mode/kiosk_mode_helper.h:43: // Getters for
various Kiosk Mode values.
On 2012/03/05 15:22:25, pastarmovj wrote:
> I'd rather comment what those are. Also please change the measurement unit for
> the timeout policy to be ms because this is what is already established for
all
> other timing policies.

Changed to use base::TimeDelta instead.

http://codereview.chromium.org/9538002/diff/10002/chrome/browser/chromeos/kio...
chrome/browser/chromeos/kiosk_mode/kiosk_mode_helper.h:56: 
On 2012/03/05 15:22:25, pastarmovj wrote:
> Too many empty lines.

Done.

http://codereview.chromium.org/9538002/diff/10002/chrome/browser/chromeos/kio...
File chrome/browser/chromeos/kiosk_mode/kiosk_mode_screensaver.cc (right):

http://codereview.chromium.org/9538002/diff/10002/chrome/browser/chromeos/kio...
chrome/browser/chromeos/kiosk_mode/kiosk_mode_screensaver.cc:22: else
On 2012/03/05 15:22:25, pastarmovj wrote:
> curlies here because and above.

Done.

http://codereview.chromium.org/9538002/diff/10002/chrome/browser/chromeos/kio...
chrome/browser/chromeos/kiosk_mode/kiosk_mode_screensaver.cc:85:
chromeos::KioskModeHelper::Get()->GetScreensaverTimeout() * 1000);}
On 2012/03/05 15:22:25, pastarmovj wrote:
> Please put the } on a new line.

Code doesn't exist anymore.
Done.

http://codereview.chromium.org/9538002/diff/10002/chrome/browser/chromeos/ui/...
File chrome/browser/chromeos/ui/screensaver_extension_dialog.cc (right):

http://codereview.chromium.org/9538002/diff/10002/chrome/browser/chromeos/ui/...
chrome/browser/chromeos/ui/screensaver_extension_dialog.cc:63:
base::Bind(&ScreensaverExtensionDialog::LoadExtension,
On 2012/03/05 15:22:25, pastarmovj wrote:
> Too much indentation.

Done.

http://codereview.chromium.org/9538002/diff/10002/chrome/browser/chromeos/ui/...
chrome/browser/chromeos/ui/screensaver_extension_dialog.cc:79: LOG(ERROR) <<
"Could not load screensaver extension from: " <<
On 2012/03/05 15:22:25, pastarmovj wrote:
> The last << goes on the next line and aligns with the previous <<.

Done.

Expand Messages | Collapse Messages