Screensaver at login screen.

Screensaver at login screen.
This CL adds code to show a screensaver on the login screen. The location of the screensaver and timeout for it to show up are currently hard coded but will be changed to pull from an enterprise policy once work on that end is completed.
The screensaver stays active only on the login screen, if any user logs on, we de-activate the screensaver for the rest of the session. Since logout causes a Chrome restart, we will be active on the login screen again.
If the screensaver crashes, we reload the extension and show ourselves again.

R=sky@chromium.org,xiyuan@chromium.org,yoz@chromium.org
BUG=chromium-os:26042
TEST=Tested that the screensaver comes up on the login screen, does not come up once we log in, reloads when I manually crash the screensaver extension process. Also verified that SelectFileDialog on ChromeOS works as intended.

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

[rkc, 2012-02-24 00:58:02]

On 2012/02/24 00:54:47, Rahul Chaturvedi wrote:

Added,
sky for the UI review.
xiyuan for the chromeos UI/extensions review
yoz for all the funky extension stuff :)

[sky, 2012-02-24 05:03:53]

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

https://chromiumcodereview.appspot.com/9455038/diff/1/chrome/browser/chromeos...
chrome/browser/chromeos/kiosk_mode/kiosk_mode_screensaver.cc:36:
power_manager->AddObserver(this);
What if this object is deleted before you get the notification?

https://chromiumcodereview.appspot.com/9455038/diff/1/chrome/browser/chromeos...
chrome/browser/chromeos/kiosk_mode/kiosk_mode_screensaver.cc:44: void
KioskModeScreensaverObserver::Observe(int type,
nit: wrap type onto the next line

https://chromiumcodereview.appspot.com/9455038/diff/1/chrome/browser/chromeos...
chrome/browser/chromeos/kiosk_mode/kiosk_mode_screensaver.cc:47: if (type ==
chrome::NOTIFICATION_LOGIN_USER_CHANGED) {
Since you only register one type, this should be a DCHECK.

https://chromiumcodereview.appspot.com/9455038/diff/1/chrome/browser/chromeos...
chrome/browser/chromeos/kiosk_mode/kiosk_mode_screensaver.cc:74: 
nit: only one newline here.

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

https://chromiumcodereview.appspot.com/9455038/diff/1/chrome/browser/chromeos...
chrome/browser/chromeos/kiosk_mode/kiosk_mode_screensaver.h:37: class
KioskModeScreensaver {
Why bother with an extra class here? Why not rename Observer to
KioskModeScreensaver?

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

https://chromiumcodereview.appspot.com/9455038/diff/1/chrome/browser/chromeos...
chrome/browser/chromeos/kiosk_mode/kiosk_mode_screensaver_unittest.cc:26:
KioskModeScreensaverTest() : ui_thread_(BrowserThread::UI, &message_loop_),
: on next line

https://chromiumcodereview.appspot.com/9455038/diff/1/chrome/browser/chromeos...
chrome/browser/chromeos/kiosk_mode/kiosk_mode_screensaver_unittest.cc:34:
BrowserThread::PostTask(BrowserThread::UI,
Document why this needs to be delayed.

https://chromiumcodereview.appspot.com/9455038/diff/1/chrome/browser/ui/views...
File chrome/browser/ui/views/extensions/extension_dialog.cc (right):

https://chromiumcodereview.appspot.com/9455038/diff/1/chrome/browser/ui/views...
chrome/browser/ui/views/extensions/extension_dialog.cc:137: void
ExtensionDialog::InitWindowFullscreen() {
Looks like you still have the browser in the one place that calls this. How come
you need to move it? I ask as it's best to get the rootwindow from an existing
window rather than like this.

https://chromiumcodereview.appspot.com/9455038/diff/1/chrome/browser/ui/views...
File chrome/browser/ui/views/screensaver_extension_dialog.cc (right):

https://chromiumcodereview.appspot.com/9455038/diff/1/chrome/browser/ui/views...
chrome/browser/ui/views/screensaver_extension_dialog.cc:59: std::string error;
If this needs to run on a specific thread, DCHECK that.
This is also error prone because you're using the same object on multiple
threads. I would rather see a standalone function that loads the extension and
callback with the loaded extension.

https://chromiumcodereview.appspot.com/9455038/diff/1/chrome/browser/ui/views...
chrome/browser/ui/views/screensaver_extension_dialog.cc:92: GURL
screensaver_url("chrome-extension://" +
Surely there must be constants for this sort of thing around?

https://chromiumcodereview.appspot.com/9455038/diff/1/chrome/browser/ui/views...
chrome/browser/ui/views/screensaver_extension_dialog.cc:114: if
(extension_dialog_)
Who deletes this class?

https://chromiumcodereview.appspot.com/9455038/diff/1/chrome/browser/ui/views...
chrome/browser/ui/views/screensaver_extension_dialog.cc:126:
MessageLoop::current()->PostTask(FROM_HERE,
Why does this need to be delayed?

https://chromiumcodereview.appspot.com/9455038/diff/1/chrome/browser/ui/views...
File chrome/browser/ui/views/screensaver_extension_dialog.h (right):

https://chromiumcodereview.appspot.com/9455038/diff/1/chrome/browser/ui/views...
chrome/browser/ui/views/screensaver_extension_dialog.h:1: // Copyright (c) 2011
The Chromium Authors. All rights reserved.
2012

https://chromiumcodereview.appspot.com/9455038/diff/1/chrome/browser/ui/views...
chrome/browser/ui/views/screensaver_extension_dialog.h:5: #ifndef
CHROME_BROWSER_UI_VIEWS_SCREENSAVER_EXTENSION_DIALOG_H_
Should this class exist in chrome/browser/chromeos some where?

https://chromiumcodereview.appspot.com/9455038/diff/1/chrome/browser/ui/views...
chrome/browser/ui/views/screensaver_extension_dialog.h:36: virtual void Show();
OVERRIDE on these two.

https://chromiumcodereview.appspot.com/9455038/diff/1/chrome/browser/ui/views...
chrome/browser/ui/views/screensaver_extension_dialog.h:43: explicit
ScreensaverExtensionDialog();
no explicit

[Yoyo Zhou, 2012-02-24 21:51:04]

http://codereview.chromium.org/9455038/diff/1/chrome/browser/ui/views/extensi...
File chrome/browser/ui/views/extensions/extension_dialog.cc (right):

http://codereview.chromium.org/9455038/diff/1/chrome/browser/ui/views/extensi...
chrome/browser/ui/views/extensions/extension_dialog.cc:133: return
manager->CreatePopupHost(url, browser);
browser is NULL here; it's clearer to make NULL be the argument.

http://codereview.chromium.org/9455038/diff/1/chrome/browser/ui/views/screens...
File chrome/browser/ui/views/screensaver_extension_dialog.cc (right):

http://codereview.chromium.org/9455038/diff/1/chrome/browser/ui/views/screens...
chrome/browser/ui/views/screensaver_extension_dialog.cc:41: if (g_instance)
So once the screensaver is closed, it will never appear again (even if no one
logs in)?

http://codereview.chromium.org/9455038/diff/1/chrome/browser/ui/views/screens...
chrome/browser/ui/views/screensaver_extension_dialog.cc:62:
screensaver_extension_ = extension_file_util::LoadExtension(
This should probably call Extension::Create, but see comment below about
ComponentLoader.

http://codereview.chromium.org/9455038/diff/1/chrome/browser/ui/views/screens...
chrome/browser/ui/views/screensaver_extension_dialog.cc:92: GURL
screensaver_url("chrome-extension://" +
On 2012/02/24 05:03:53, sky wrote:
> Surely there must be constants for this sort of thing around?

Use Extension::GetResourceURL for this.

http://codereview.chromium.org/9455038/diff/1/chrome/browser/ui/views/screens...
chrome/browser/ui/views/screensaver_extension_dialog.cc:94: "/background.html");
Why hardcode this? Also, there seems to be no reason it needs to be a background
page; it seems this is more of a packaged app.

http://codereview.chromium.org/9455038/diff/1/chrome/browser/ui/views/screens...
chrome/browser/ui/views/screensaver_extension_dialog.cc:96: Profile*
default_profile = ProfileManager::GetDefaultProfile();
Are you sure this will be the correct profile? I'm not familiar with the login
screen profile situation.

http://codereview.chromium.org/9455038/diff/1/chrome/browser/ui/views/screens...
chrome/browser/ui/views/screensaver_extension_dialog.cc:97: if
(default_profile->GetExtensionService()->AddExtension(
If this is a component extension it should use ComponentLoader. Perhaps even
ComponentLoader::Load.

[rkc, 2012-02-25 02:14:02]

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

https://chromiumcodereview.appspot.com/9455038/diff/1/chrome/browser/chromeos...
chrome/browser/chromeos/kiosk_mode/kiosk_mode_screensaver.cc:36:
power_manager->AddObserver(this);
On 2012/02/24 05:03:53, sky wrote:
> What if this object is deleted before you get the notification?

Very unlikely considering that this class lives for the lifetime of the Chrome
process. Still, added a destructor that removes our observer.

Done.

https://chromiumcodereview.appspot.com/9455038/diff/1/chrome/browser/chromeos...
chrome/browser/chromeos/kiosk_mode/kiosk_mode_screensaver.cc:44: void
KioskModeScreensaverObserver::Observe(int type,
On 2012/02/24 05:03:53, sky wrote:
> nit: wrap type onto the next line

Done.

https://chromiumcodereview.appspot.com/9455038/diff/1/chrome/browser/chromeos...
chrome/browser/chromeos/kiosk_mode/kiosk_mode_screensaver.cc:47: if (type ==
chrome::NOTIFICATION_LOGIN_USER_CHANGED) {
On 2012/02/24 05:03:53, sky wrote:
> Since you only register one type, this should be a DCHECK.

Done.

https://chromiumcodereview.appspot.com/9455038/diff/1/chrome/browser/chromeos...
chrome/browser/chromeos/kiosk_mode/kiosk_mode_screensaver.cc:74: 
On 2012/02/24 05:03:53, sky wrote:
> nit: only one newline here.

Done.

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

https://chromiumcodereview.appspot.com/9455038/diff/1/chrome/browser/chromeos...
chrome/browser/chromeos/kiosk_mode/kiosk_mode_screensaver.h:37: class
KioskModeScreensaver {
On 2012/02/24 05:03:53, sky wrote:
> Why bother with an extra class here? Why not rename Observer to
> KioskModeScreensaver?

Done.

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

https://chromiumcodereview.appspot.com/9455038/diff/1/chrome/browser/chromeos...
chrome/browser/chromeos/kiosk_mode/kiosk_mode_screensaver_unittest.cc:26:
KioskModeScreensaverTest() : ui_thread_(BrowserThread::UI, &message_loop_),
On 2012/02/24 05:03:53, sky wrote:
> : on next line

Done.

https://chromiumcodereview.appspot.com/9455038/diff/1/chrome/browser/chromeos...
chrome/browser/chromeos/kiosk_mode/kiosk_mode_screensaver_unittest.cc:34:
BrowserThread::PostTask(BrowserThread::UI,
On 2012/02/24 05:03:53, sky wrote:
> Document why this needs to be delayed.

It doesn't. I'm new to using test threads :)
Fixed.
Done.

https://chromiumcodereview.appspot.com/9455038/diff/1/chrome/browser/ui/views...
File chrome/browser/ui/views/extensions/extension_dialog.cc (right):

https://chromiumcodereview.appspot.com/9455038/diff/1/chrome/browser/ui/views...
chrome/browser/ui/views/extensions/extension_dialog.cc:133: return
manager->CreatePopupHost(url, browser);
On 2012/02/24 21:51:05, Yoyo Zhou wrote:
> browser is NULL here; it's clearer to make NULL be the argument.

Done.

https://chromiumcodereview.appspot.com/9455038/diff/1/chrome/browser/ui/views...
chrome/browser/ui/views/extensions/extension_dialog.cc:137: void
ExtensionDialog::InitWindowFullscreen() {
On 2012/02/24 05:03:53, sky wrote:
> Looks like you still have the browser in the one place that calls this. How
come
> you need to move it? I ask as it's best to get the rootwindow from an existing
> window rather than like this.

Done.

https://chromiumcodereview.appspot.com/9455038/diff/1/chrome/browser/ui/views...
File chrome/browser/ui/views/screensaver_extension_dialog.cc (right):

https://chromiumcodereview.appspot.com/9455038/diff/1/chrome/browser/ui/views...
chrome/browser/ui/views/screensaver_extension_dialog.cc:41: if (g_instance)
On 2012/02/24 21:51:05, Yoyo Zhou wrote:
> So once the screensaver is closed, it will never appear again (even if no one
> logs in)?

This is incorrect code, fixed.

Done.

https://chromiumcodereview.appspot.com/9455038/diff/1/chrome/browser/ui/views...
chrome/browser/ui/views/screensaver_extension_dialog.cc:59: std::string error;
On 2012/02/24 05:03:53, sky wrote:
> If this needs to run on a specific thread, DCHECK that.
> This is also error prone because you're using the same object on multiple
> threads. I would rather see a standalone function that loads the extension and
> callback with the loaded extension.

Done.

https://chromiumcodereview.appspot.com/9455038/diff/1/chrome/browser/ui/views...
chrome/browser/ui/views/screensaver_extension_dialog.cc:62:
screensaver_extension_ = extension_file_util::LoadExtension(
On 2012/02/24 21:51:05, Yoyo Zhou wrote:
> This should probably call Extension::Create, but see comment below about
> ComponentLoader.

Answered below.

https://chromiumcodereview.appspot.com/9455038/diff/1/chrome/browser/ui/views...
chrome/browser/ui/views/screensaver_extension_dialog.cc:92: GURL
screensaver_url("chrome-extension://" +
On 2012/02/24 21:51:05, Yoyo Zhou wrote:
> On 2012/02/24 05:03:53, sky wrote:
> > Surely there must be constants for this sort of thing around?
> 
> Use Extension::GetResourceURL for this.

Done.

https://chromiumcodereview.appspot.com/9455038/diff/1/chrome/browser/ui/views...
chrome/browser/ui/views/screensaver_extension_dialog.cc:92: GURL
screensaver_url("chrome-extension://" +
On 2012/02/24 05:03:53, sky wrote:
> Surely there must be constants for this sort of thing around?

Done.

https://chromiumcodereview.appspot.com/9455038/diff/1/chrome/browser/ui/views...
chrome/browser/ui/views/screensaver_extension_dialog.cc:94: "/background.html");
On 2012/02/24 21:51:05, Yoyo Zhou wrote:
> Why hardcode this? Also, there seems to be no reason it needs to be a
background
> page; it seems this is more of a packaged app.

Done.

https://chromiumcodereview.appspot.com/9455038/diff/1/chrome/browser/ui/views...
chrome/browser/ui/views/screensaver_extension_dialog.cc:96: Profile*
default_profile = ProfileManager::GetDefaultProfile();
On 2012/02/24 21:51:05, Yoyo Zhou wrote:
> Are you sure this will be the correct profile? I'm not familiar with the login
> screen profile situation.

This is the only profile available during login.

For the next iteration, we may be creating our own temp profile and adding the
extension to do that and using that instead for security reasons - but for now,
this should work fine for us.

https://chromiumcodereview.appspot.com/9455038/diff/1/chrome/browser/ui/views...
chrome/browser/ui/views/screensaver_extension_dialog.cc:97: if
(default_profile->GetExtensionService()->AddExtension(
On 2012/02/24 21:51:05, Yoyo Zhou wrote:
> If this is a component extension it should use ComponentLoader. Perhaps even
> ComponentLoader::Load.

This isn't a component extension but we need it loaded as one. If we don't load
it as a component extension, the extension won't load on the login screen (loads
everywhere else just fine).

https://chromiumcodereview.appspot.com/9455038/diff/1/chrome/browser/ui/views...
chrome/browser/ui/views/screensaver_extension_dialog.cc:114: if
(extension_dialog_)
On 2012/02/24 05:03:53, sky wrote:
> Who deletes this class?

I believe extension_dialog_ deletes itself by calling Release on itself when
closing.

https://chromiumcodereview.appspot.com/9455038/diff/1/chrome/browser/ui/views...
chrome/browser/ui/views/screensaver_extension_dialog.cc:126:
MessageLoop::current()->PostTask(FROM_HERE,
On 2012/02/24 05:03:53, sky wrote:
> Why does this need to be delayed?

If we don't delay this, the extension hasn't actually finished terminating,
landing us in the middle of a state which causes our extension to not reload
since it hasn't actually been unloaded yet.

This is a bit of a hack but currently there are no good solutions around this -
I've discussed this with yoz@ and aa@ when implementing reload on crash for the
select_file_dialog_extension.

The correct solution will be not even needing to reload the extension but that
is not working yet for views hosted extensions.

https://chromiumcodereview.appspot.com/9455038/diff/1/chrome/browser/ui/views...
File chrome/browser/ui/views/screensaver_extension_dialog.h (right):

https://chromiumcodereview.appspot.com/9455038/diff/1/chrome/browser/ui/views...
chrome/browser/ui/views/screensaver_extension_dialog.h:1: // Copyright (c) 2011
The Chromium Authors. All rights reserved.
On 2012/02/24 05:03:53, sky wrote:
> 2012

Done.

https://chromiumcodereview.appspot.com/9455038/diff/1/chrome/browser/ui/views...
chrome/browser/ui/views/screensaver_extension_dialog.h:5: #ifndef
CHROME_BROWSER_UI_VIEWS_SCREENSAVER_EXTENSION_DIALOG_H_
On 2012/02/24 05:03:53, sky wrote:
> Should this class exist in chrome/browser/chromeos some where?

Done.

https://chromiumcodereview.appspot.com/9455038/diff/1/chrome/browser/ui/views...
chrome/browser/ui/views/screensaver_extension_dialog.h:36: virtual void Show();
On 2012/02/24 05:03:53, sky wrote:
> OVERRIDE on these two.

Done.

https://chromiumcodereview.appspot.com/9455038/diff/1/chrome/browser/ui/views...
chrome/browser/ui/views/screensaver_extension_dialog.h:43: explicit
ScreensaverExtensionDialog();
On 2012/02/24 05:03:53, sky wrote:
> no explicit

Done.

[sky, 2012-02-27 15:52:42]

https://chromiumcodereview.appspot.com/9455038/diff/6002/chrome/browser/chrom...
File chrome/browser/chromeos/ui/screensaver_extension_dialog.cc (right):

https://chromiumcodereview.appspot.com/9455038/diff/6002/chrome/browser/chrom...
chrome/browser/chromeos/ui/screensaver_extension_dialog.cc:53: :
screensaver_extension_(NULL), loading_extension_(false) {
nit: when you wrap, each param on its own line.

https://chromiumcodereview.appspot.com/9455038/diff/6002/chrome/browser/chrom...
chrome/browser/chromeos/ui/screensaver_extension_dialog.cc:94: if
(loading_extension_) return;
Don't use single line ifs.

https://chromiumcodereview.appspot.com/9455038/diff/6002/chrome/browser/chrom...
chrome/browser/chromeos/ui/screensaver_extension_dialog.cc:121: if
(extension_dialog_) {
Is it possible for close to be invoked while you're loading the extension?

https://chromiumcodereview.appspot.com/9455038/diff/6002/chrome/browser/chrom...
chrome/browser/chromeos/ui/screensaver_extension_dialog.cc:140:
MessageLoop::current()->PostTask(FROM_HERE,
Add comment as to why you need to delay this.

https://chromiumcodereview.appspot.com/9455038/diff/6002/chrome/browser/ui/vi...
File chrome/browser/ui/views/extensions/extension_dialog.cc (right):

https://chromiumcodereview.appspot.com/9455038/diff/6002/chrome/browser/ui/vi...
chrome/browser/ui/views/extensions/extension_dialog.cc:144:
root_window->StackChildAtTop(window_->GetNativeWindow());
Use the views API for this sort of stuff:
window_->StackAdTop().

https://chromiumcodereview.appspot.com/9455038/diff/6002/chrome/browser/ui/vi...
chrome/browser/ui/views/extensions/extension_dialog.cc:146: int width =
root_window->GetHostSize().width();
gfx::Screen::GetMonitorWorkAreaNearestWindow

[xiyuan, 2012-02-27 18:26:30]

http://codereview.chromium.org/9455038/diff/6002/chrome/browser/chromeos/kios...
File chrome/browser/chromeos/kiosk_mode/kiosk_mode_screensaver.cc (right):

http://codereview.chromium.org/9455038/diff/6002/chrome/browser/chromeos/kios...
chrome/browser/chromeos/kiosk_mode/kiosk_mode_screensaver.cc:45:
chromeos::DBusThreadManager::Get()->GetPowerManagerClient();
When will this dtor be called? DBusThreadManager will be released when
BrowserMainLoop is gone. So if this is called after that, it might crash.

http://codereview.chromium.org/9455038/diff/6002/chrome/browser/ui/views/exte...
File chrome/browser/ui/views/extensions/extension_dialog.cc (right):

http://codereview.chromium.org/9455038/diff/6002/chrome/browser/ui/views/exte...
chrome/browser/ui/views/extensions/extension_dialog.cc:10: #include
"chrome/browser/profiles/profile_manager.h"
nit: ProfileManager seems not used. Do we need this include?

[rkc, 2012-02-27 22:24:42]

http://codereview.chromium.org/9455038/diff/6002/chrome/browser/chromeos/kios...
File chrome/browser/chromeos/kiosk_mode/kiosk_mode_screensaver.cc (right):

http://codereview.chromium.org/9455038/diff/6002/chrome/browser/chromeos/kios...
chrome/browser/chromeos/kiosk_mode/kiosk_mode_screensaver.cc:45:
chromeos::DBusThreadManager::Get()->GetPowerManagerClient();
On 2012/02/27 18:26:30, xiyuan wrote:
> When will this dtor be called? DBusThreadManager will be released when
> BrowserMainLoop is gone. So if this is called after that, it might crash.

As discussed, made changes to add a shutdown function to the screensaver and
call it before DBusThreadManager shuts down.

Also added code to shutdown in case a user logs in, since we really won't need
to be around after that anyway.

http://codereview.chromium.org/9455038/diff/6002/chrome/browser/chromeos/ui/s...
File chrome/browser/chromeos/ui/screensaver_extension_dialog.cc (right):

http://codereview.chromium.org/9455038/diff/6002/chrome/browser/chromeos/ui/s...
chrome/browser/chromeos/ui/screensaver_extension_dialog.cc:53: :
screensaver_extension_(NULL), loading_extension_(false) {
On 2012/02/27 15:52:43, sky wrote:
> nit: when you wrap, each param on its own line.

Done.

http://codereview.chromium.org/9455038/diff/6002/chrome/browser/chromeos/ui/s...
chrome/browser/chromeos/ui/screensaver_extension_dialog.cc:94: if
(loading_extension_) return;
On 2012/02/27 15:52:43, sky wrote:
> Don't use single line ifs.

Done.

http://codereview.chromium.org/9455038/diff/6002/chrome/browser/chromeos/ui/s...
chrome/browser/chromeos/ui/screensaver_extension_dialog.cc:121: if
(extension_dialog_) {
On 2012/02/27 15:52:43, sky wrote:
> Is it possible for close to be invoked while you're loading the extension?

Yep, but in that case extension_dialog will be null so this code will do
nothing.

http://codereview.chromium.org/9455038/diff/6002/chrome/browser/chromeos/ui/s...
chrome/browser/chromeos/ui/screensaver_extension_dialog.cc:140:
MessageLoop::current()->PostTask(FROM_HERE,
On 2012/02/27 15:52:43, sky wrote:
> Add comment as to why you need to delay this.

Done.

http://codereview.chromium.org/9455038/diff/6002/chrome/browser/ui/views/exte...
File chrome/browser/ui/views/extensions/extension_dialog.cc (right):

http://codereview.chromium.org/9455038/diff/6002/chrome/browser/ui/views/exte...
chrome/browser/ui/views/extensions/extension_dialog.cc:10: #include
"chrome/browser/profiles/profile_manager.h"
On 2012/02/27 18:26:30, xiyuan wrote:
> nit: ProfileManager seems not used. Do we need this include?

Done.

http://codereview.chromium.org/9455038/diff/6002/chrome/browser/ui/views/exte...
chrome/browser/ui/views/extensions/extension_dialog.cc:144:
root_window->StackChildAtTop(window_->GetNativeWindow());
On 2012/02/27 15:52:43, sky wrote:
> Use the views API for this sort of stuff:
> window_->StackAdTop().

Done.

http://codereview.chromium.org/9455038/diff/6002/chrome/browser/ui/views/exte...
chrome/browser/ui/views/extensions/extension_dialog.cc:146: int width =
root_window->GetHostSize().width();
On 2012/02/27 15:52:43, sky wrote:
> gfx::Screen::GetMonitorWorkAreaNearestWindow

Done.

[sky, 2012-02-27 23:27:27]

LGTM

[xiyuan, 2012-02-27 23:32:30]

lgtm

[Yoyo Zhou, 2012-02-27 23:45:12]

LGTM

[commit-bot: I haz the power, 2012-02-27 23:46:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rkc@chromium.org/9455038/12001

[commit-bot: I haz the power, 2012-02-27 23:46:35]

Can't apply patch for file
chrome/browser/chromeos/chrome_browser_main_chromeos.cc.
While running patch -p1 --forward --force;
patching file chrome/browser/chromeos/chrome_browser_main_chromeos.cc
Hunk #3 FAILED at 226.
1 out of 3 hunks FAILED -- saving rejects to file
chrome/browser/chromeos/chrome_browser_main_chromeos.cc.rej

[commit-bot: I haz the power, 2012-02-27 23:50:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rkc@chromium.org/9455038/13018

[commit-bot: I haz the power, 2012-02-28 05:36:13]

Change committed as 123906

[Nikita (slow), 2012-02-28 12:10:59]

https://chromiumcodereview.appspot.com/9455038/diff/13018/chrome/browser/chro...
File chrome/browser/chromeos/chrome_browser_main_chromeos.cc (right):

https://chromiumcodereview.appspot.com/9455038/diff/13018/chrome/browser/chro...
chrome/browser/chromeos/chrome_browser_main_chromeos.cc:204: if
(parsed_command_line.HasSwitch(switches::kEnableKioskMode))
Does enterprise policy support altering cmdline flags?
These flags are not mentioned in the design document.

https://chromiumcodereview.appspot.com/9455038/diff/13018/chrome/browser/chro...
File chrome/browser/chromeos/kiosk_mode/kiosk_mode_screensaver.cc (right):

https://chromiumcodereview.appspot.com/9455038/diff/13018/chrome/browser/chro...
chrome/browser/chromeos/kiosk_mode/kiosk_mode_screensaver.cc:97: LOG(WARNING) <<
"Screensaver shutdown called when uninitialized.";
When kiosk mode is not enabled, you'll be calling ShutdownKioskModeScreensaver()
on each Chrome shutdown so this warning should be removed.

[rkc, 2012-02-29 04:43:27]

https://chromiumcodereview.appspot.com/9455038/diff/13018/chrome/browser/chro...
File chrome/browser/chromeos/chrome_browser_main_chromeos.cc (right):

https://chromiumcodereview.appspot.com/9455038/diff/13018/chrome/browser/chro...
chrome/browser/chromeos/chrome_browser_main_chromeos.cc:204: if
(parsed_command_line.HasSwitch(switches::kEnableKioskMode))
On 2012/02/28 12:10:59, Nikita Kostylev wrote:
> Does enterprise policy support altering cmdline flags?
> These flags are not mentioned in the design document.

These flags are placeholders for till when the enterprise policy is ready.

https://chromiumcodereview.appspot.com/9455038/diff/13018/chrome/browser/chro...
File chrome/browser/chromeos/kiosk_mode/kiosk_mode_screensaver.cc (right):

https://chromiumcodereview.appspot.com/9455038/diff/13018/chrome/browser/chro...
chrome/browser/chromeos/kiosk_mode/kiosk_mode_screensaver.cc:97: LOG(WARNING) <<
"Screensaver shutdown called when uninitialized.";
On 2012/02/28 12:10:59, Nikita Kostylev wrote:
> When kiosk mode is not enabled, you'll be calling
ShutdownKioskModeScreensaver()
> on each Chrome shutdown so this warning should be removed.

Fixed in a follow-on CL (http://codereview.chromium.org/9538002/).