[ChromeOS] WebUI OOBE/Login refactoring.

[ChromeOS] WebUI OOBE/Login refactoring.

- Rename OobeMessageHandler -> BaseScreenHandler since both OobeUI and LoginUI
  will use its derived classes;
- Merge LoginUIHandler into SigninScreenHandler;
- Misc changes:
  - Header includes optimization;
  - Don't call OobeUI for TOUCH_UI build;
  - Move LoginUIHTMLSource into login_ui.cc since it's only used there;
  - Rename SigninScreenHandler callback message names to follow javascript
    identifier rule and be consistent with other part of the code;
  - Rename user dict key names to follow javascript identifier rule;
  - Add <enter> key handling for oobe signin screen;

BUG=none.
TEST=Expect no functionality change in webui oobe/login.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=93116

[Nikita (slow), 2011-07-15 06:54:29]

>> - Don't call OobeUI for TOUCH_UI build; 

Why?

[Nikita (slow), 2011-07-15 07:11:02]

http://codereview.chromium.org/7382001/diff/1/chrome/browser/chromeos/login/w...
File chrome/browser/chromeos/login/webui_login_display_host.cc (right):

http://codereview.chromium.org/7382001/diff/1/chrome/browser/chromeos/login/w...
chrome/browser/chromeos/login/webui_login_display_host.cc:83: // TODO(fsamuel):
Use the OOBE once it's touch-friendly.
We should behave the same and show OOBE WebUI on touch builds with current
implementation.
Keeping them separate means supporting 2 different paths.

All OOBE/login WebUI is behind the flag and there's a special flag
/root/.use_webui_login 

if [ -f /root/.use_webui_login ] ; then
  SKIP_OOBE="--login-screen=login"
  WEBUI_LOGIN="--webui-login"
fi

that means that OOBE will be skipped for now.

http://codereview.chromium.org/7382001/diff/1/chrome/browser/ui/webui/chromeo...
File chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc (right):

http://codereview.chromium.org/7382001/diff/1/chrome/browser/ui/webui/chromeo...
chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc:32: //
LoginUIHandlerDelegate, public: ------------------------------------------
nit: LoginUIHandler > SigninScreenHandler

http://codereview.chromium.org/7382001/diff/1/chrome/browser/ui/webui/chromeo...
chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc:39: //
LoginUIHandlerDelegate, protected: ------------------------------------------
nit: LoginUIHandler > SigninScreenHandler

http://codereview.chromium.org/7382001/diff/1/chrome/browser/ui/webui/chromeo...
File chrome/browser/ui/webui/chromeos/login/signin_screen_handler.h (right):

http://codereview.chromium.org/7382001/diff/1/chrome/browser/ui/webui/chromeo...
chrome/browser/ui/webui/chromeos/login/signin_screen_handler.h:29: // An
interface for SigninScreenHandler to call WebUILoginDisplay.
nit: It has private data members so technically it's a base class.

http://codereview.chromium.org/7382001/diff/1/chrome/browser/ui/webui/chromeo...
chrome/browser/ui/webui/chromeos/login/signin_screen_handler.h:44: void
set_webui_handler(LoginDisplayWebUIHandler* webui_handler);
nit: Setter implementation should be located in .h

[Fady Samuel, 2011-07-15 15:06:07]

http://codereview.chromium.org/7382001/diff/1/chrome/browser/chromeos/login/w...
File chrome/browser/chromeos/login/webui_login_display_host.cc (right):

http://codereview.chromium.org/7382001/diff/1/chrome/browser/chromeos/login/w...
chrome/browser/chromeos/login/webui_login_display_host.cc:83: // TODO(fsamuel):
Use the OOBE once it's touch-friendly.
On 2011/07/15 07:11:02, Nikita Kostylev wrote:
> We should behave the same and show OOBE WebUI on touch builds with current
> implementation.
> Keeping them separate means supporting 2 different paths.
> 
> All OOBE/login WebUI is behind the flag and there's a special flag
> /root/.use_webui_login 
> 
> if [ -f /root/.use_webui_login ] ; then
>   SKIP_OOBE="--login-screen=login"
>   WEBUI_LOGIN="--webui-login"
> fi
> 
> that means that OOBE will be skipped for now.

That's fair. A couple of days ago, we were getting the OOBE on TOUCH builds
which was a bit hard to use on that platform so I added this code. Feel free to
remove it, as long as there's another way to bypass the OOBE on TOUCH_UI builds.

[xiyuan, 2011-07-15 18:26:23]

Please take another look. Thanks.

http://codereview.chromium.org/7382001/diff/1/chrome/browser/chromeos/login/w...
File chrome/browser/chromeos/login/webui_login_display_host.cc (right):

http://codereview.chromium.org/7382001/diff/1/chrome/browser/chromeos/login/w...
chrome/browser/chromeos/login/webui_login_display_host.cc:83: // TODO(fsamuel):
Use the OOBE once it's touch-friendly.
On 2011/07/15 15:06:07, fsamuel wrote:
> On 2011/07/15 07:11:02, Nikita Kostylev wrote:
> > We should behave the same and show OOBE WebUI on touch builds with current
> > implementation.
> > Keeping them separate means supporting 2 different paths.
> > 
> > All OOBE/login WebUI is behind the flag and there's a special flag
> > /root/.use_webui_login 
> > 
> > if [ -f /root/.use_webui_login ] ; then
> >   SKIP_OOBE="--login-screen=login"
> >   WEBUI_LOGIN="--webui-login"
> > fi
> > 
> > that means that OOBE will be skipped for now.
> 
> That's fair. A couple of days ago, we were getting the OOBE on TOUCH builds
> which was a bit hard to use on that platform so I added this code. Feel free
to
> remove it, as long as there's another way to bypass the OOBE on TOUCH_UI
builds.

Okay. Per discussion, I'll use kOobeUURL for now. And added <enter> key handling
to make touch easier to use.

http://codereview.chromium.org/7382001/diff/1/chrome/browser/ui/webui/chromeo...
File chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc (right):

http://codereview.chromium.org/7382001/diff/1/chrome/browser/ui/webui/chromeo...
chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc:32: //
LoginUIHandlerDelegate, public: ------------------------------------------
On 2011/07/15 07:11:02, Nikita Kostylev wrote:
> nit: LoginUIHandler > SigninScreenHandler

Removed since SigninScreenHandlerDelegate is an interface now.

http://codereview.chromium.org/7382001/diff/1/chrome/browser/ui/webui/chromeo...
chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc:39: //
LoginUIHandlerDelegate, protected: ------------------------------------------
On 2011/07/15 07:11:02, Nikita Kostylev wrote:
> nit: LoginUIHandler > SigninScreenHandler

Ditto.

http://codereview.chromium.org/7382001/diff/1/chrome/browser/ui/webui/chromeo...
File chrome/browser/ui/webui/chromeos/login/signin_screen_handler.h (right):

http://codereview.chromium.org/7382001/diff/1/chrome/browser/ui/webui/chromeo...
chrome/browser/ui/webui/chromeos/login/signin_screen_handler.h:29: // An
interface for SigninScreenHandler to call WebUILoginDisplay.
On 2011/07/15 07:11:02, Nikita Kostylev wrote:
> nit: It has private data members so technically it's a base class.

You are right. Made it an interface.

http://codereview.chromium.org/7382001/diff/1/chrome/browser/ui/webui/chromeo...
chrome/browser/ui/webui/chromeos/login/signin_screen_handler.h:44: void
set_webui_handler(LoginDisplayWebUIHandler* webui_handler);
On 2011/07/15 07:11:02, Nikita Kostylev wrote:
> nit: Setter implementation should be located in .h

Renamed to SetWebUIHandler and made it pure virtual.

[Nikita (slow), 2011-07-15 18:52:06]

LGTM with nits.

http://codereview.chromium.org/7382001/diff/6002/chrome/browser/resources/chr...
File chrome/browser/resources/chromeos/login/screen_signin.js (right):

http://codereview.chromium.org/7382001/diff/6002/chrome/browser/resources/chr...
chrome/browser/resources/chromeos/login/screen_signin.js:102: if (!
$('email').value) {
nit: nuke space between ! and $

http://codereview.chromium.org/7382001/diff/6002/chrome/browser/resources/chr...
chrome/browser/resources/chromeos/login/screen_signin.js:136: if (e.target.id ==
'email' && e.target.value)
nit: might be rewritten as

if (.. 'email') {
  if (e.target.value)
    ..
} else if (... 'password') {
}

to avoid extra comparisons.

http://codereview.chromium.org/7382001/diff/6002/chrome/browser/ui/webui/chro...
File chrome/browser/ui/webui/chromeos/login/signin_screen_handler.h (right):

http://codereview.chromium.org/7382001/diff/6002/chrome/browser/ui/webui/chro...
chrome/browser/ui/webui/chromeos/login/signin_screen_handler.h:33: virtual void
SetWebUIHandler(LoginDisplayWebUIHandler* webui_handler) = 0;
nit: move after LoginAsGuest() so that methods are alpha sorted.

[Fady Samuel, 2011-07-15 20:11:34]

http://codereview.chromium.org/7382001/diff/6002/chrome/browser/resources/chr...
File chrome/browser/resources/chromeos/login/screen_signin.js (right):

http://codereview.chromium.org/7382001/diff/6002/chrome/browser/resources/chr...
chrome/browser/resources/chromeos/login/screen_signin.js:139: if (e.target.id ==
'password')
Does this mean one doesn't need a password to login? Should we have
e.target.value as well?

[xiyuan, 2011-07-15 20:38:29]

http://codereview.chromium.org/7382001/diff/6002/chrome/browser/resources/chr...
File chrome/browser/resources/chromeos/login/screen_signin.js (right):

http://codereview.chromium.org/7382001/diff/6002/chrome/browser/resources/chr...
chrome/browser/resources/chromeos/login/screen_signin.js:102: if (!
$('email').value) {
On 2011/07/15 18:52:07, Nikita Kostylev wrote:
> nit: nuke space between ! and $

Done.

http://codereview.chromium.org/7382001/diff/6002/chrome/browser/resources/chr...
chrome/browser/resources/chromeos/login/screen_signin.js:136: if (e.target.id ==
'email' && e.target.value)
On 2011/07/15 18:52:07, Nikita Kostylev wrote:
> nit: might be rewritten as
> 
> if (.. 'email') {
>   if (e.target.value)
>     ..
> } else if (... 'password') {
> }
> 
> to avoid extra comparisons.

I think javascript uses shortcut boolean operation.
e.g "false && some_func()", some_func will not be called.

http://codereview.chromium.org/7382001/diff/6002/chrome/browser/resources/chr...
chrome/browser/resources/chromeos/login/screen_signin.js:139: if (e.target.id ==
'password')
On 2011/07/15 20:11:34, fsamuel wrote:
> Does this mean one doesn't need a password to login? Should we have
> e.target.value as well?

The part to check empty password is in checkInput and called from
handleSigninClick_.

http://codereview.chromium.org/7382001/diff/6002/chrome/browser/ui/webui/chro...
File chrome/browser/ui/webui/chromeos/login/signin_screen_handler.h (right):

http://codereview.chromium.org/7382001/diff/6002/chrome/browser/ui/webui/chro...
chrome/browser/ui/webui/chromeos/login/signin_screen_handler.h:33: virtual void
SetWebUIHandler(LoginDisplayWebUIHandler* webui_handler) = 0;
On 2011/07/15 18:52:07, Nikita Kostylev wrote:
> nit: move after LoginAsGuest() so that methods are alpha sorted.

Done.

[Nikita (slow), 2011-07-18 07:50:50]

http://codereview.chromium.org/7382001/diff/6002/chrome/browser/resources/chr...
File chrome/browser/resources/chromeos/login/screen_signin.js (right):

http://codereview.chromium.org/7382001/diff/6002/chrome/browser/resources/chr...
chrome/browser/resources/chromeos/login/screen_signin.js:136: if (e.target.id ==
'email' && e.target.value)
On 2011/07/15 20:38:29, xiyuan wrote:
> On 2011/07/15 18:52:07, Nikita Kostylev wrote:
> > nit: might be rewritten as
> > 
> > if (.. 'email') {
> >   if (e.target.value)
> >     ..
> > } else if (... 'password') {
> > }
> > 
> > to avoid extra comparisons.
> 
> I think javascript uses shortcut boolean operation.
> e.g "false && some_func()", some_func will not be called.

What I've meant was that you need to compare string only once. In current code
if (e.target.id == 'password') is executed each time.

[xiyuan, 2011-07-18 18:32:09]

http://codereview.chromium.org/7382001/diff/6002/chrome/browser/resources/chr...
File chrome/browser/resources/chromeos/login/screen_signin.js (right):

http://codereview.chromium.org/7382001/diff/6002/chrome/browser/resources/chr...
chrome/browser/resources/chromeos/login/screen_signin.js:136: if (e.target.id ==
'email' && e.target.value)
On 2011/07/18 07:50:50, Nikita Kostylev wrote:
> On 2011/07/15 20:38:29, xiyuan wrote:
> > On 2011/07/15 18:52:07, Nikita Kostylev wrote:
> > > nit: might be rewritten as
> > > 
> > > if (.. 'email') {
> > >   if (e.target.value)
> > >     ..
> > > } else if (... 'password') {
> > > }
> > > 
> > > to avoid extra comparisons.
> > 
> > I think javascript uses shortcut boolean operation.
> > e.g "false && some_func()", some_func will not be called.
> 
> What I've meant was that you need to compare string only once. In current code
> if (e.target.id == 'password') is executed each time.

I see what you mean. Changed as suggested.