Add in chrome://login-container

This is the first CL for a sequence of CLs that will add in the display infrastructure that I have built for the DOM Login and touch keyboard. If have created a reference CL for what the end product of this sequence is intended to be, so that I can give context for the CL under review.This can be found at: http://codereview.chromium.org/6577003/

This CL adds in chrome://login-container to touchui==1 and chromeos==1 builds. This page is hooked into the WebUI to enable testing and development of a Browser based container for the DOM Login screens. In this CL all it does it
create a Browser with chrome://login in it. This functionality will be extended in following CLs.

Patch from Ryan Harrison <rharrison@chromium.org>;

BUG=none
TEST=manual

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

[rjkroege, 2011-02-23 20:05:30]

LGTM.

Please expand the description?

http://codereview.chromium.org/6579003/diff/1/chrome/browser/chromeos/webui/l...
File chrome/browser/chromeos/webui/login/login_container_ui.cc (right):

http://codereview.chromium.org/6579003/diff/1/chrome/browser/chromeos/webui/l...
chrome/browser/chromeos/webui/login/login_container_ui.cc:29: MessageLoop*
message_loop)
is this indented right?

http://codereview.chromium.org/6579003/diff/1/chrome/browser/resources/login_...
File chrome/browser/resources/login_container.html (right):

http://codereview.chromium.org/6579003/diff/1/chrome/browser/resources/login_...
chrome/browser/resources/login_container.html:10: <button type="button"
id="open-login-screen-button" onclick="callOpenLoginScreen();">
I'd prefer if you line-broke at onclick and indented.

http://codereview.chromium.org/6579003/diff/1/chrome/browser/webui/web_ui_fac...
File chrome/browser/webui/web_ui_factory.cc (right):

http://codereview.chromium.org/6579003/diff/1/chrome/browser/webui/web_ui_fac...
chrome/browser/webui/web_ui_factory.cc:226: #if defined(OS_CHROMEOS) &&
defined(TOUCH_UI)
this change does not alter anything? Is it necessary? Doesn't matter either.

http://codereview.chromium.org/6579003/diff/1/chrome/chrome_browser.gypi
File chrome/chrome_browser.gypi (right):

http://codereview.chromium.org/6579003/diff/1/chrome/chrome_browser.gypi#newc...
chrome/chrome_browser.gypi:732:
'browser/chromeos/webui/login/authenticator_facade_cros.cc',
didn't you strip some of these earlier?

[rharrison, 2011-02-23 20:24:50]

I also expanded the description. If there is somewhere you want me to go into
more detail I can.

http://codereview.chromium.org/6579003/diff/1/chrome/browser/chromeos/webui/l...
File chrome/browser/chromeos/webui/login/login_container_ui.cc (right):

http://codereview.chromium.org/6579003/diff/1/chrome/browser/chromeos/webui/l...
chrome/browser/chromeos/webui/login/login_container_ui.cc:29: MessageLoop*
message_loop)
On 2011/02/23 20:05:30, rjkroege wrote:
> is this indented right?
No, the argument actually does fit on a single line.

http://codereview.chromium.org/6579003/diff/1/chrome/browser/resources/login_...
File chrome/browser/resources/login_container.html (right):

http://codereview.chromium.org/6579003/diff/1/chrome/browser/resources/login_...
chrome/browser/resources/login_container.html:10: <button type="button"
id="open-login-screen-button" onclick="callOpenLoginScreen();">
On 2011/02/23 20:05:30, rjkroege wrote:
> I'd prefer if you line-broke at onclick and indented.

Done.

http://codereview.chromium.org/6579003/diff/1/chrome/browser/webui/web_ui_fac...
File chrome/browser/webui/web_ui_factory.cc (right):

http://codereview.chromium.org/6579003/diff/1/chrome/browser/webui/web_ui_fac...
chrome/browser/webui/web_ui_factory.cc:226: #if defined(OS_CHROMEOS) &&
defined(TOUCH_UI)
On 2011/02/23 20:05:30, rjkroege wrote:
> this change does not alter anything? Is it necessary
I don't think there is no specific reason for this change other then possibly
aesthetics.

http://codereview.chromium.org/6579003/diff/1/chrome/chrome_browser.gypi
File chrome/chrome_browser.gypi (right):

http://codereview.chromium.org/6579003/diff/1/chrome/chrome_browser.gypi#newc...
chrome/chrome_browser.gypi:732:
'browser/chromeos/webui/login/authenticator_facade_cros.cc',
On 2011/02/23 20:05:30, rjkroege wrote:
> didn't you strip some of these earlier?
authenticator_facade_stub* was removed, since it was used in non-cros builds.
When I integrate the DOM Login screen with the EUC from the current login
infrastructure I will remove the rest of the authenticator facade layer. I need
to have a display container for the login screen to do that integration.

[Nikita (slow), 2011-02-28 14:26:17]

I don't fully get the idea of "login-container" that navigates to
"chrome://login" URL.

Could you please provide short description of the whole design in a doc as I've
asked on the another CL.

http://codereview.chromium.org/6579003/diff/7001/chrome/browser/chromeos/webu...
File chrome/browser/chromeos/webui/login/login_container_ui.cc (right):

http://codereview.chromium.org/6579003/diff/7001/chrome/browser/chromeos/webu...
chrome/browser/chromeos/webui/login/login_container_ui.cc:28:
LoginContainerUIHTMLSource::LoginContainerUIHTMLSource(MessageLoop*
message_loop)
not fitting on a single line

http://codereview.chromium.org/6579003/diff/7001/chrome/browser/chromeos/webu...
chrome/browser/chromeos/webui/login/login_container_ui.cc:34: bool
is_off_the_record,
please align params

http://codereview.chromium.org/6579003/diff/7001/chrome/browser/chromeos/webu...
chrome/browser/chromeos/webui/login/login_container_ui.cc:77:
login_screen->AddSelectedTabWithURL(GURL("chrome://login"),
Use constant.

http://codereview.chromium.org/6579003/diff/7001/chrome/browser/webui/web_ui_...
File chrome/browser/webui/web_ui_factory.cc (right):

http://codereview.chromium.org/6579003/diff/7001/chrome/browser/webui/web_ui_...
chrome/browser/webui/web_ui_factory.cc:59: #include
"chrome/browser/chromeos/webui/login/login_container_ui.h"
nit: alpha sort headers

[oshima, 2011-02-28 18:38:07]

Have two questions.
1) I too don't fully understand the concept of container.
2) Isn't it possible to setup so that this gets compiled for OS_CHROMEOS and
enabled by switch? It will allow us to start working on WebUI login screen for
chromeos.

http://codereview.chromium.org/6579003/diff/7001/chrome/browser/chromeos/webu...
File chrome/browser/chromeos/webui/login/login_container_ui.cc (right):

http://codereview.chromium.org/6579003/diff/7001/chrome/browser/chromeos/webu...
chrome/browser/chromeos/webui/login/login_container_ui.cc:1: // Copyright (c)
2010 The Chromium Authors. All rights reserved.
2011

http://codereview.chromium.org/6579003/diff/7001/chrome/browser/chromeos/webu...
chrome/browser/chromeos/webui/login/login_container_ui.cc:24: //
LoginContainerUIHTMLSource
// LoginContainerUIHTMLSource -------------------

This is new recommendation for this type of comments

http://codereview.chromium.org/6579003/diff/7001/chrome/browser/chromeos/webu...
chrome/browser/chromeos/webui/login/login_container_ui.cc:45:
(html_bytes.get()));
fits to single line.

http://codereview.chromium.org/6579003/diff/7001/chrome/browser/chromeos/webu...
File chrome/browser/chromeos/webui/login/login_container_ui.h (right):

http://codereview.chromium.org/6579003/diff/7001/chrome/browser/chromeos/webu...
chrome/browser/chromeos/webui/login/login_container_ui.h:1: // Copyright (c)
2010 The Chromium Authors. All rights reserved.
2011

http://codereview.chromium.org/6579003/diff/7001/chrome/browser/chromeos/webu...
chrome/browser/chromeos/webui/login/login_container_ui.h:15: #include
"views/widget/widget.h"
do you need this?

http://codereview.chromium.org/6579003/diff/7001/chrome/browser/chromeos/webu...
chrome/browser/chromeos/webui/login/login_container_ui.h:48:
~LoginContainerUIHandler();
virtual  (the style guide requires virtual for destructor)

http://codereview.chromium.org/6579003/diff/7001/chrome/browser/chromeos/webu...
File chrome/browser/chromeos/webui/login/login_ui_helpers.h (right):

http://codereview.chromium.org/6579003/diff/7001/chrome/browser/chromeos/webu...
chrome/browser/chromeos/webui/login/login_ui_helpers.h:11: #include
"base/basictypes.h"
why you need this now?
I also think you need to include base/string_piece.h . The reason this works now
is probably because the helpers.cc has base/string_piece.h included before this
file somewhere.

http://codereview.chromium.org/6579003/diff/7001/chrome/browser/chromeos/webu...
File chrome/browser/chromeos/webui/login/mock_login_ui_helpers.h (right):

http://codereview.chromium.org/6579003/diff/7001/chrome/browser/chromeos/webu...
chrome/browser/chromeos/webui/login/mock_login_ui_helpers.h:52:
base::StringPiece());
looks like above two MOCK_METHOD0 can fit to single line

http://codereview.chromium.org/6579003/diff/7001/chrome/browser/resources/log...
File chrome/browser/resources/login_container.html (right):

http://codereview.chromium.org/6579003/diff/7001/chrome/browser/resources/log...
chrome/browser/resources/login_container.html:2: <html>
This is still place holder right?

[rharrison, 2011-02-28 19:23:21]

On 2011/02/28 14:26:17, Nikita Kostylev wrote:
> I don't fully get the idea of "login-container" that navigates to
> "chrome://login" URL.
> 
The purpose of this is to allow an easy way for the developer to access the
DOMBrowser stack for development and testing. When things have advanced far
enough so that we can access the DOM Login Screen via a command line flag then
this code will be removed. I prefer to have it checked in instead of as a out of
trunk patch is so that other changes that may occur like refactoring names are
aware of it and don't break it while needed.

> Could you please provide short description of the whole design in a doc as
I've
> asked on the another CL.
> 
>
http://codereview.chromium.org/6579003/diff/7001/chrome/browser/chromeos/webu...
> File chrome/browser/chromeos/webui/login/login_container_ui.cc (right):
> 
>
http://codereview.chromium.org/6579003/diff/7001/chrome/browser/chromeos/webu...
> chrome/browser/chromeos/webui/login/login_container_ui.cc:28:
> LoginContainerUIHTMLSource::LoginContainerUIHTMLSource(MessageLoop*
> message_loop)
> not fitting on a single line
> 
>
http://codereview.chromium.org/6579003/diff/7001/chrome/browser/chromeos/webu...
> chrome/browser/chromeos/webui/login/login_container_ui.cc:34: bool
> is_off_the_record,
> please align params
> 
>
http://codereview.chromium.org/6579003/diff/7001/chrome/browser/chromeos/webu...
> chrome/browser/chromeos/webui/login/login_container_ui.cc:77:
> login_screen->AddSelectedTabWithURL(GURL("chrome://login"),
> Use constant.
> 
>
http://codereview.chromium.org/6579003/diff/7001/chrome/browser/webui/web_ui_...
> File chrome/browser/webui/web_ui_factory.cc (right):
> 
>
http://codereview.chromium.org/6579003/diff/7001/chrome/browser/webui/web_ui_...
> chrome/browser/webui/web_ui_factory.cc:59: #include
> "chrome/browser/chromeos/webui/login/login_container_ui.h"
> nit: alpha sort headers

[rharrison, 2011-02-28 19:25:54]

On 2011/02/28 18:38:07, oshima wrote:
> Have two questions.
> 1) I too don't fully understand the concept of container.
I responded to this in Nikita's e-mail and in the document I wrote

> 2) Isn't it possible to setup so that this gets compiled for OS_CHROMEOS and
> enabled by switch? It will allow us to start working on WebUI login screen for
> chromeos.
Eventually this will be accessed by a command line flag, but at the moment it
isn't integrated with the login infrastructure like EUC. My plans forward are
discussed in some detail in the document that I shared today. For pure UX stuff
I think some of the work could be done as is, but I don't think much can get don
until the DOM Login is integrated with the EUC.

> 
>
http://codereview.chromium.org/6579003/diff/7001/chrome/browser/chromeos/webu...
> File chrome/browser/chromeos/webui/login/login_container_ui.cc (right):
> 
>
http://codereview.chromium.org/6579003/diff/7001/chrome/browser/chromeos/webu...
> chrome/browser/chromeos/webui/login/login_container_ui.cc:1: // Copyright (c)
> 2010 The Chromium Authors. All rights reserved.
> 2011
> 
>
http://codereview.chromium.org/6579003/diff/7001/chrome/browser/chromeos/webu...
> chrome/browser/chromeos/webui/login/login_container_ui.cc:24: //
> LoginContainerUIHTMLSource
> // LoginContainerUIHTMLSource -------------------
> 
> This is new recommendation for this type of comments
> 
>
http://codereview.chromium.org/6579003/diff/7001/chrome/browser/chromeos/webu...
> chrome/browser/chromeos/webui/login/login_container_ui.cc:45:
> (html_bytes.get()));
> fits to single line.
> 
>
http://codereview.chromium.org/6579003/diff/7001/chrome/browser/chromeos/webu...
> File chrome/browser/chromeos/webui/login/login_container_ui.h (right):
> 
>
http://codereview.chromium.org/6579003/diff/7001/chrome/browser/chromeos/webu...
> chrome/browser/chromeos/webui/login/login_container_ui.h:1: // Copyright (c)
> 2010 The Chromium Authors. All rights reserved.
> 2011
> 
>
http://codereview.chromium.org/6579003/diff/7001/chrome/browser/chromeos/webu...
> chrome/browser/chromeos/webui/login/login_container_ui.h:15: #include
> "views/widget/widget.h"
> do you need this?
> 
>
http://codereview.chromium.org/6579003/diff/7001/chrome/browser/chromeos/webu...
> chrome/browser/chromeos/webui/login/login_container_ui.h:48:
> ~LoginContainerUIHandler();
> virtual  (the style guide requires virtual for destructor)
> 
>
http://codereview.chromium.org/6579003/diff/7001/chrome/browser/chromeos/webu...
> File chrome/browser/chromeos/webui/login/login_ui_helpers.h (right):
> 
>
http://codereview.chromium.org/6579003/diff/7001/chrome/browser/chromeos/webu...
> chrome/browser/chromeos/webui/login/login_ui_helpers.h:11: #include
> "base/basictypes.h"
> why you need this now?
> I also think you need to include base/string_piece.h . The reason this works
now
> is probably because the helpers.cc has base/string_piece.h included before
this
> file somewhere.
> 
>
http://codereview.chromium.org/6579003/diff/7001/chrome/browser/chromeos/webu...
> File chrome/browser/chromeos/webui/login/mock_login_ui_helpers.h (right):
> 
>
http://codereview.chromium.org/6579003/diff/7001/chrome/browser/chromeos/webu...
> chrome/browser/chromeos/webui/login/mock_login_ui_helpers.h:52:
> base::StringPiece());
> looks like above two MOCK_METHOD0 can fit to single line
> 
>
http://codereview.chromium.org/6579003/diff/7001/chrome/browser/resources/log...
> File chrome/browser/resources/login_container.html (right):
> 
>
http://codereview.chromium.org/6579003/diff/7001/chrome/browser/resources/log...
> chrome/browser/resources/login_container.html:2: <html>
> This is still place holder right?

[rharrison, 2011-02-28 19:28:24]

Made changes as requested to the CL by Nikita and Oshima. I have also written a
design doc going into details of what I am attempting that I have sent to the
reviewers. I have also responded to outstanding comments.

http://codereview.chromium.org/6579003/diff/7001/chrome/browser/chromeos/webu...
File chrome/browser/chromeos/webui/login/login_container_ui.cc (right):

http://codereview.chromium.org/6579003/diff/7001/chrome/browser/chromeos/webu...
chrome/browser/chromeos/webui/login/login_container_ui.cc:1: // Copyright (c)
2010 The Chromium Authors. All rights reserved.
On 2011/02/28 18:38:07, oshima wrote:
> 2011

Done.

http://codereview.chromium.org/6579003/diff/7001/chrome/browser/chromeos/webu...
chrome/browser/chromeos/webui/login/login_container_ui.cc:24: //
LoginContainerUIHTMLSource
On 2011/02/28 18:38:07, oshima wrote:
> // LoginContainerUIHTMLSource -------------------
> 
> This is new recommendation for this type of comments

Done.

http://codereview.chromium.org/6579003/diff/7001/chrome/browser/chromeos/webu...
chrome/browser/chromeos/webui/login/login_container_ui.cc:28:
LoginContainerUIHTMLSource::LoginContainerUIHTMLSource(MessageLoop*
message_loop)
On 2011/02/28 14:26:17, Nikita Kostylev wrote:
> not fitting on a single line

Done.

http://codereview.chromium.org/6579003/diff/7001/chrome/browser/chromeos/webu...
chrome/browser/chromeos/webui/login/login_container_ui.cc:34: bool
is_off_the_record,
On 2011/02/28 14:26:17, Nikita Kostylev wrote:
> please align params

Done.

http://codereview.chromium.org/6579003/diff/7001/chrome/browser/chromeos/webu...
chrome/browser/chromeos/webui/login/login_container_ui.cc:45:
(html_bytes.get()));
On 2011/02/28 18:38:07, oshima wrote:
> fits to single line.

Done.

http://codereview.chromium.org/6579003/diff/7001/chrome/browser/chromeos/webu...
chrome/browser/chromeos/webui/login/login_container_ui.cc:77:
login_screen->AddSelectedTabWithURL(GURL("chrome://login"),
On 2011/02/28 14:26:17, Nikita Kostylev wrote:
> Use constant.

Done.

http://codereview.chromium.org/6579003/diff/7001/chrome/browser/chromeos/webu...
File chrome/browser/chromeos/webui/login/login_container_ui.h (right):

http://codereview.chromium.org/6579003/diff/7001/chrome/browser/chromeos/webu...
chrome/browser/chromeos/webui/login/login_container_ui.h:1: // Copyright (c)
2010 The Chromium Authors. All rights reserved.
On 2011/02/28 18:38:07, oshima wrote:
> 2011

Done.

http://codereview.chromium.org/6579003/diff/7001/chrome/browser/chromeos/webu...
chrome/browser/chromeos/webui/login/login_container_ui.h:15: #include
"views/widget/widget.h"
On 2011/02/28 18:38:07, oshima wrote:
> do you need this?
Not anymore, thanks for catching it.

http://codereview.chromium.org/6579003/diff/7001/chrome/browser/chromeos/webu...
chrome/browser/chromeos/webui/login/login_container_ui.h:48:
~LoginContainerUIHandler();
On 2011/02/28 18:38:07, oshima wrote:
> virtual  (the style guide requires virtual for destructor)

Done.

http://codereview.chromium.org/6579003/diff/7001/chrome/browser/chromeos/webu...
File chrome/browser/chromeos/webui/login/login_ui_helpers.h (right):

http://codereview.chromium.org/6579003/diff/7001/chrome/browser/chromeos/webu...
chrome/browser/chromeos/webui/login/login_ui_helpers.h:11: #include
"base/basictypes.h"
On 2011/02/28 18:38:07, oshima wrote:
> why you need this now?
> I also think you need to include base/string_piece.h . The reason this works
now
> is probably because the helpers.cc has base/string_piece.h included before
this
> file somewhere. 

Done.

http://codereview.chromium.org/6579003/diff/7001/chrome/browser/chromeos/webu...
File chrome/browser/chromeos/webui/login/mock_login_ui_helpers.h (right):

http://codereview.chromium.org/6579003/diff/7001/chrome/browser/chromeos/webu...
chrome/browser/chromeos/webui/login/mock_login_ui_helpers.h:52:
base::StringPiece());
On 2011/02/28 18:38:07, oshima wrote:
> looks like above two MOCK_METHOD0 can fit to single line

Done.

http://codereview.chromium.org/6579003/diff/7001/chrome/browser/resources/log...
File chrome/browser/resources/login_container.html (right):

http://codereview.chromium.org/6579003/diff/7001/chrome/browser/resources/log...
chrome/browser/resources/login_container.html:2: <html>
On 2011/02/28 18:38:07, oshima wrote:
> This is still place holder right?
Yes, this page and the associated LoginContainerUI only exist to help intial
development. Once we have a login screen that we can access via a command line
flag this will be removed.

http://codereview.chromium.org/6579003/diff/7001/chrome/browser/webui/web_ui_...
File chrome/browser/webui/web_ui_factory.cc (right):

http://codereview.chromium.org/6579003/diff/7001/chrome/browser/webui/web_ui_...
chrome/browser/webui/web_ui_factory.cc:59: #include
"chrome/browser/chromeos/webui/login/login_container_ui.h"
On 2011/02/28 14:26:17, Nikita Kostylev wrote:
> nit: alpha sort headers

Done.

[oshima, 2011-02-28 20:33:09]

LGTM

http://codereview.chromium.org/6579003/diff/9002/chrome/browser/chromeos/webu...
File chrome/browser/chromeos/webui/login/login_container_ui.cc (right):

http://codereview.chromium.org/6579003/diff/9002/chrome/browser/chromeos/webu...
chrome/browser/chromeos/webui/login/login_container_ui.cc:20: static const char*
kLoginURL = "chrome://login";
put this in anonymous namespace and remove static

http://codereview.chromium.org/6579003/diff/9002/chrome/browser/chromeos/webu...
File chrome/browser/chromeos/webui/login/login_ui_helpers.h (right):

http://codereview.chromium.org/6579003/diff/9002/chrome/browser/chromeos/webu...
chrome/browser/chromeos/webui/login/login_ui_helpers.h:21: };  // namespace base
remove this.

[rharrison, 2011-02-28 20:38:14]

http://codereview.chromium.org/6579003/diff/9002/chrome/browser/chromeos/webu...
File chrome/browser/chromeos/webui/login/login_container_ui.cc (right):

http://codereview.chromium.org/6579003/diff/9002/chrome/browser/chromeos/webu...
chrome/browser/chromeos/webui/login/login_container_ui.cc:20: static const char*
kLoginURL = "chrome://login";
On 2011/02/28 20:33:09, oshima wrote:
> put this in anonymous namespace and remove static

Done.

http://codereview.chromium.org/6579003/diff/9002/chrome/browser/chromeos/webu...
File chrome/browser/chromeos/webui/login/login_ui_helpers.h (right):

http://codereview.chromium.org/6579003/diff/9002/chrome/browser/chromeos/webu...
chrome/browser/chromeos/webui/login/login_ui_helpers.h:21: };  // namespace base
On 2011/02/28 20:33:09, oshima wrote:
> remove this.

Done.

[Nikita (slow), 2011-03-01 18:10:12]

LGTM

[Nikita (slow), 2011-03-02 12:13:18]

http://codereview.chromium.org/6579003/diff/11009/chrome/browser/chromeos/web...
File chrome/browser/chromeos/webui/login/login_container_ui.cc (right):

http://codereview.chromium.org/6579003/diff/11009/chrome/browser/chromeos/web...
chrome/browser/chromeos/webui/login/login_container_ui.cc:25: //
LoginContainerUIHTMLSource -------------------
nit: "-" should be filled till 80 chars limit

[rharrison, 2011-03-02 20:05:40]

Fixed the comment line length issue that Nikita noticed.

The CL has passed all of the trybots.

http://codereview.chromium.org/6579003/diff/11009/chrome/browser/chromeos/web...
File chrome/browser/chromeos/webui/login/login_container_ui.cc (right):

http://codereview.chromium.org/6579003/diff/11009/chrome/browser/chromeos/web...
chrome/browser/chromeos/webui/login/login_container_ui.cc:25: //
LoginContainerUIHTMLSource -------------------
On 2011/03/02 12:13:18, Nikita Kostylev wrote:
> nit: "-" should be filled till 80 chars limit

Done.

[commit-bot: I haz the power, 2011-03-03 17:51:03]

Failed to apply the patch.
svn: Commit failed (details follow):
svn:
'/mnt/data/b/commit-queue/workdir/chromium/remoting/host/user_authenticator_mac.h'
is scheduled for addition, but is missing

[commit-bot: I haz the power, 2011-03-04 07:58:35]

Change committed as 76894