Place the spinner in the right corner of the controls window.

Place the spinner in the right corner of the controls window.

Also CL contains changes which unify communication between
|UserController| and corresponding controls view (UserInput interface).

BUG=chromium-os:9770
TEST=Play with login. Notice appearing spinners.

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

[Nikita (slow), 2010-12-09 15:33:21]

http://codereview.chromium.org/5709001/diff/3021/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/screen_locker.cc (right):

http://codereview.chromium.org/5709001/diff/3021/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/screen_locker.cc:650: StartThrobber();
As I understand this throbber is shown while ScreenLock acquires grab.
Does it work?

http://codereview.chromium.org/5709001/diff/3021/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/screen_locker.cc:836: StartThrobber();
Why show throbber along with error bubble?

[whywhat, 2010-12-09 16:20:18]

Mitsuru, could you take a look too since it touches screen lock as well?

http://codereview.chromium.org/5709001/diff/3021/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/existing_user_controller.cc (right):

http://codereview.chromium.org/5709001/diff/3021/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/existing_user_controller.cc:364: for
(std::vector<UserController*>::iterator iter = controllers_.begin();
Using indices here would be shorter and more readabe, imo.

http://codereview.chromium.org/5709001/diff/3021/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/existing_user_view.cc (right):

http://codereview.chromium.org/5709001/diff/3021/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/existing_user_view.cc:150: const gfx::Size&
/*throbber*/, gfx::Rect* /*bounds*/) const {
You'll have to put each parameter on a separate line to respect "two spaces
between code and comment" rule.

http://codereview.chromium.org/5709001/diff/3021/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/existing_user_view.h (right):

http://codereview.chromium.org/5709001/diff/3021/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/existing_user_view.h:32: // Textfield::Controller:
Could you use consistent "Overridden from " prefix here and below?

http://codereview.chromium.org/5709001/diff/3021/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/guest_user_view.cc (right):

http://codereview.chromium.org/5709001/diff/3021/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/guest_user_view.cc:95:
user_controller_->OnLoginOffTheRecord();
DCHECK or maybe even CHECK for button being what we expect?

http://codereview.chromium.org/5709001/diff/3021/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/guest_user_view.h (right):

http://codereview.chromium.org/5709001/diff/3021/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/guest_user_view.h:38: // Implements UserInput:
Same here about consistent comments.

http://codereview.chromium.org/5709001/diff/3021/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/guest_user_view.h:40: 
You could remove blank lines between methods.

http://codereview.chromium.org/5709001/diff/3021/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/helper.cc (right):

http://codereview.chromium.org/5709001/diff/3021/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/helper.cc:83: return;
Shouldn't this be a check? Also, why delete throbber? Do we own it since it was
passed to us?
Or maybe we don't need to return but to create a new throbber widget?

http://codereview.chromium.org/5709001/diff/3021/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/helper.cc:119: throbber_widget_->Close();
So we delete it in Start, but Close in Stop? Why the difference?

http://codereview.chromium.org/5709001/diff/3021/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/helper.h (right):

http://codereview.chromium.org/5709001/diff/3021/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/helper.h:33: // This class manages showing of the
throbber in the separete popup widget.
nit: separete -> separate, showing of -> showing, in the -> in a

http://codereview.chromium.org/5709001/diff/3021/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/helper.h:41: void StartShow(views::Widget* host,
Why not just Start?
Also, maybe pass host and throbber in ctor?

http://codereview.chromium.org/5709001/diff/3021/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/helper.h:43: int right_shift);
Mb right_shift -> right_margin?

http://codereview.chromium.org/5709001/diff/3021/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/helper.h:48: const gfx::Rect relative_bounds);
const gfx::Rect&?

http://codereview.chromium.org/5709001/diff/3021/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/helper.h:56: };
DISABLE_COPY_AND_ASSIGN?

http://codereview.chromium.org/5709001/diff/3021/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/new_user_view.cc (right):

http://codereview.chromium.org/5709001/diff/3021/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/new_user_view.cc:98:
accel_focus_pass_(views::Accelerator(app::VKEY_P, false, false, true)),
Do we need to initialize throbber positions here too?

http://codereview.chromium.org/5709001/diff/3021/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/new_user_view.cc:494: throbber_center_y_ -
throbber.height() / 2);
Move this one space to the right.
Also as I noted in ctor, if we happen to reach this point before layout is
calculated, we're in trouble.

http://codereview.chromium.org/5709001/diff/3021/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/new_user_view.h (right):

http://codereview.chromium.org/5709001/diff/3021/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/new_user_view.h:113: 
Blank lines here are not needed.

http://codereview.chromium.org/5709001/diff/3021/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/screen_lock_view.h (right):

http://codereview.chromium.org/5709001/diff/3021/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/screen_lock_view.h:50: // Returns widget under
which throbber should be shown. NULL if there is no
"under which ..." should be "which ... under". Also is it under or above?

http://codereview.chromium.org/5709001/diff/3021/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/user_controller.cc (right):

http://codereview.chromium.org/5709001/diff/3021/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/user_controller.cc:178: throbber_manager_(new
ThrobberManager),
Please, use () when creating objects using the default ctor.

http://codereview.chromium.org/5709001/diff/3021/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/user_controller.cc:217:
UseCustomBoundsForThrobber(throbber->GetPreferredSize(), &bounds)) {
Why not split the line between ( and t?

http://codereview.chromium.org/5709001/diff/3021/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/user_controller.cc:222:
login::kThrobberRightShift);
Maybe this constant should be defined here rather than in a separate file?

http://codereview.chromium.org/5709001/diff/3021/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/user_controller.h (right):

http://codereview.chromium.org/5709001/diff/3021/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/user_controller.h:96: gfx::Rect GetScreenBounds();
const?

http://codereview.chromium.org/5709001/diff/3021/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/user_input.h (right):

http://codereview.chromium.org/5709001/diff/3021/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/user_input.h:27: virtual gfx::Rect
GetControlWithErrorBounds() const = 0;
Don't get both the comment and the method name, sorry :)

http://codereview.chromium.org/5709001/diff/3021/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/user_input.h:29: // Returns whether custom bounds
for the throbber should be used. If yes than
than -> then

[oshima, 2010-12-09 21:52:10]

http://codereview.chromium.org/5709001/diff/3021/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/screen_lock_view.cc (right):

http://codereview.chromium.org/5709001/diff/3021/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/screen_lock_view.cc:67: throbber_widget_(NULL) {
looks like this is not used?

http://codereview.chromium.org/5709001/diff/3021/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/screen_lock_view.cc:167: return
password_field_->GetWidget();
or just return password_field_ ?  ... : NULL;

http://codereview.chromium.org/5709001/diff/3021/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/screen_locker.cc (right):

http://codereview.chromium.org/5709001/diff/3021/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/screen_locker.cc:836: StartThrobber();
On 2010/12/09 15:33:22, Nikita Kostylev wrote:
> Why show throbber along with error bubble?

may be this is a bug from http://codereview.chromium.org/3583013

http://codereview.chromium.org/5709001/diff/3021/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/screen_locker.h (right):

http://codereview.chromium.org/5709001/diff/3021/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/screen_locker.h:124: void StopThrobber();
Looks like it's simpler to have these method and ThrobberManager in 
ScreenLockView than here.  What do you think?

[altimofeev, 2010-12-10 16:37:40]

http://codereview.chromium.org/5709001/diff/3021/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/existing_user_controller.cc (right):

http://codereview.chromium.org/5709001/diff/3021/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/existing_user_controller.cc:364: for
(std::vector<UserController*>::iterator iter = controllers_.begin();
On 2010/12/09 16:20:18, whywhat wrote:
> Using indices here would be shorter and more readabe, imo.

Good point! Done.

http://codereview.chromium.org/5709001/diff/3021/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/existing_user_view.cc (right):

http://codereview.chromium.org/5709001/diff/3021/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/existing_user_view.cc:150: const gfx::Size&
/*throbber*/, gfx::Rect* /*bounds*/) const {
On 2010/12/09 16:20:18, whywhat wrote:
> You'll have to put each parameter on a separate line to respect "two spaces
> between code and comment" rule.

Removed the comments.

http://codereview.chromium.org/5709001/diff/3021/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/existing_user_view.h (right):

http://codereview.chromium.org/5709001/diff/3021/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/existing_user_view.h:32: // Textfield::Controller:
On 2010/12/09 16:20:18, whywhat wrote:
> Could you use consistent "Overridden from " prefix here and below?

Done.

http://codereview.chromium.org/5709001/diff/3021/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/guest_user_view.cc (right):

http://codereview.chromium.org/5709001/diff/3021/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/guest_user_view.cc:95:
user_controller_->OnLoginOffTheRecord();
On 2010/12/09 16:20:18, whywhat wrote:
> DCHECK or maybe even CHECK for button being what we expect?

DCHECK has been added.

http://codereview.chromium.org/5709001/diff/3021/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/guest_user_view.h (right):

http://codereview.chromium.org/5709001/diff/3021/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/guest_user_view.h:38: // Implements UserInput:
On 2010/12/09 16:20:18, whywhat wrote:
> Same here about consistent comments.

Done.

http://codereview.chromium.org/5709001/diff/3021/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/guest_user_view.h:40: 
On 2010/12/09 16:20:18, whywhat wrote:
> You could remove blank lines between methods.

Done.

http://codereview.chromium.org/5709001/diff/3021/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/helper.cc (right):

http://codereview.chromium.org/5709001/diff/3021/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/helper.cc:83: return;
On 2010/12/09 16:20:18, whywhat wrote:
> Shouldn't this be a check? Also, why delete throbber? Do we own it since it
was
> passed to us?
> Or maybe we don't need to return but to create a new throbber widget?

Changed the behavior. Now, previously created throbber is closed (if any) and
new one is opened.

http://codereview.chromium.org/5709001/diff/3021/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/helper.cc:119: throbber_widget_->Close();
On 2010/12/09 16:20:18, whywhat wrote:
> So we delete it in Start, but Close in Stop? Why the difference?

In Start we have been deleting |throbber| which was an unused view.

http://codereview.chromium.org/5709001/diff/3021/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/helper.h (right):

http://codereview.chromium.org/5709001/diff/3021/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/helper.h:33: // This class manages showing of the
throbber in the separete popup widget.
On 2010/12/09 16:20:18, whywhat wrote:
> nit: separete -> separate, showing of -> showing, in the -> in a

Done.

http://codereview.chromium.org/5709001/diff/3021/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/helper.h:41: void StartShow(views::Widget* host,
On 2010/12/09 16:20:18, whywhat wrote:
> Why not just Start?
> Also, maybe pass host and throbber in ctor?

Actually it creates popup and starts the throbber. So switched to
'CreateAndStart' naming. This also explains why throbber and host have to be
passed directly to this method and not in ctor (also added comment about it).

http://codereview.chromium.org/5709001/diff/3021/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/helper.h:43: int right_shift);
On 2010/12/09 16:20:18, whywhat wrote:
> Mb right_shift -> right_margin?

Done.

http://codereview.chromium.org/5709001/diff/3021/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/helper.h:48: const gfx::Rect relative_bounds);
On 2010/12/09 16:20:18, whywhat wrote:
> const gfx::Rect&?

Done.

http://codereview.chromium.org/5709001/diff/3021/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/helper.h:56: };
On 2010/12/09 16:20:18, whywhat wrote:
> DISABLE_COPY_AND_ASSIGN?

Done.

http://codereview.chromium.org/5709001/diff/3021/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/new_user_view.cc (right):

http://codereview.chromium.org/5709001/diff/3021/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/new_user_view.cc:98:
accel_focus_pass_(views::Accelerator(app::VKEY_P, false, false, true)),
On 2010/12/09 16:20:18, whywhat wrote:
> Do we need to initialize throbber positions here too?

Added impossible initial value for |throbber_center_y| to be able distinguish
when it hasn't been calculated yet.

http://codereview.chromium.org/5709001/diff/3021/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/new_user_view.cc:494: throbber_center_y_ -
throbber.height() / 2);
On 2010/12/09 16:20:18, whywhat wrote:
> Move this one space to the right.
> Also as I noted in ctor, if we happen to reach this point before layout is
> calculated, we're in trouble.

Added call to Layout() if there wasn't any.

http://codereview.chromium.org/5709001/diff/3021/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/new_user_view.h (right):

http://codereview.chromium.org/5709001/diff/3021/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/new_user_view.h:113: 
On 2010/12/09 16:20:18, whywhat wrote:
> Blank lines here are not needed.

Done.

http://codereview.chromium.org/5709001/diff/3021/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/screen_lock_view.cc (right):

http://codereview.chromium.org/5709001/diff/3021/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/screen_lock_view.cc:67: throbber_widget_(NULL) {
On 2010/12/09 21:52:10, oshima wrote:
> looks like this is not used?

Exactly. Removed.

http://codereview.chromium.org/5709001/diff/3021/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/screen_lock_view.cc:167: return
password_field_->GetWidget();
On 2010/12/09 21:52:10, oshima wrote:
> or just return password_field_ ?  ... : NULL;

Done.

http://codereview.chromium.org/5709001/diff/3021/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/screen_lock_view.h (right):

http://codereview.chromium.org/5709001/diff/3021/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/screen_lock_view.h:50: // Returns widget under
which throbber should be shown. NULL if there is no
On 2010/12/09 16:20:18, whywhat wrote:
> "under which ..." should be "which ... under". Also is it under or above?

Above. Fixed.

http://codereview.chromium.org/5709001/diff/3021/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/screen_locker.cc (right):

http://codereview.chromium.org/5709001/diff/3021/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/screen_locker.cc:836: StartThrobber();
On 2010/12/09 21:52:10, oshima wrote:
> On 2010/12/09 15:33:22, Nikita Kostylev wrote:
> > Why show throbber along with error bubble?
> 
> may be this is a bug from http://codereview.chromium.org/3583013

Oshima-san is right. I have automatically copied the previous behavior of the
'SetEnabled(false)' call.

Fixed.

http://codereview.chromium.org/5709001/diff/3021/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/screen_locker.h (right):

http://codereview.chromium.org/5709001/diff/3021/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/screen_locker.h:124: void StopThrobber();
On 2010/12/09 21:52:10, oshima wrote:
> Looks like it's simpler to have these method and ThrobberManager in 
> ScreenLockView than here.  What do you think?

I put it here because created throbber belongs to views hierarchy that differs
from the one of the ScreenLockView.

Anyway, since there is the only view that is used by ScreenLocker, I moved the
ThrobberManager to ScreenLockView, as you had suggested.

http://codereview.chromium.org/5709001/diff/3021/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/user_controller.cc (right):

http://codereview.chromium.org/5709001/diff/3021/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/user_controller.cc:178: throbber_manager_(new
ThrobberManager),
On 2010/12/09 16:20:18, whywhat wrote:
> Please, use () when creating objects using the default ctor.

Done.

http://codereview.chromium.org/5709001/diff/3021/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/user_controller.cc:217:
UseCustomBoundsForThrobber(throbber->GetPreferredSize(), &bounds)) {
On 2010/12/09 16:20:18, whywhat wrote:
> Why not split the line between ( and t?

Done.

http://codereview.chromium.org/5709001/diff/3021/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/user_controller.cc:222:
login::kThrobberRightShift);
On 2010/12/09 16:20:18, whywhat wrote:
> Maybe this constant should be defined here rather than in a separate file?

It is also used by ScreenLockView.

http://codereview.chromium.org/5709001/diff/3021/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/user_controller.h (right):

http://codereview.chromium.org/5709001/diff/3021/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/user_controller.h:96: gfx::Rect GetScreenBounds();
On 2010/12/09 16:20:18, whywhat wrote:
> const?

Done.

http://codereview.chromium.org/5709001/diff/3021/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/user_input.h (right):

http://codereview.chromium.org/5709001/diff/3021/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/user_input.h:27: virtual gfx::Rect
GetControlWithErrorBounds() const = 0;
On 2010/12/09 16:20:18, whywhat wrote:
> Don't get both the comment and the method name, sorry :)

I have slightly changed both. Is it better now?

http://codereview.chromium.org/5709001/diff/3021/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/user_input.h:29: // Returns whether custom bounds
for the throbber should be used. If yes than
On 2010/12/09 16:20:18, whywhat wrote:
> than -> then

Done.

[whywhat, 2010-12-13 07:57:35]

LGTM with a couple of nits and suggestions

http://codereview.chromium.org/5709001/diff/25001/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/existing_user_controller.cc (left):

http://codereview.chromium.org/5709001/diff/25001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/existing_user_controller.cc:218:
new_user_view->SetUsername(username);
Note: this was used to auto fill username after account creation though password
was empty (we didn't want to store it in memory to pass between screens). I
guess it's okay for now to break this and fix when we'll implement account
creation.

http://codereview.chromium.org/5709001/diff/25001/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/existing_user_controller.cc (right):

http://codereview.chromium.org/5709001/diff/25001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/existing_user_controller.cc:477:
controllers_[selected_view_index_]->GetMainInputScreenBounds();
Maybe simply call the method GetMessageBubblePosition() if that's what the
result is used for?

http://codereview.chromium.org/5709001/diff/25001/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/existing_user_view.cc (right):

http://codereview.chromium.org/5709001/diff/25001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/existing_user_view.cc:104:
user_controller_->OnLogin("", UTF16ToUTF8(password_field_->text()));
Maybe std::string() is better here for empty username? Why do we pass an empty
one, btw?

http://codereview.chromium.org/5709001/diff/25001/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/new_user_view.cc (right):

http://codereview.chromium.org/5709001/diff/25001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/new_user_view.cc:424: throbber_y +
sign_in_button_->GetPreferredSize().height() / 2;
Guess we could use y here and eliminate throbber_y altogether.
Can't we calculate these coordinates each time using sing_in_button_.GetBounds()
for x, y, width and height? Wouldn't this be better than saving these values in
class members?

http://codereview.chromium.org/5709001/diff/25001/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/user_controller.h (right):

http://codereview.chromium.org/5709001/diff/25001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/user_controller.h:96: // Returns bounds of the
main input field in screen coordinats (e.g. these
nit: coordinats -> coordinates

http://codereview.chromium.org/5709001/diff/25001/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/user_input.h (right):

http://codereview.chromium.org/5709001/diff/25001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/user_input.h:26: // Returns bounds of the main
input field in screen coordinats (e.g. these
same nit about coordinats

http://codereview.chromium.org/5709001/diff/25001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/user_input.h:32: virtual bool
UseCustomBoundsForThrobber(const gfx::Size& throbber,
Could we change this method to smth like StartThrobber(ThrobberManager*)? Thus
each UserInput would choose where to put the throbber without exposing bounds to
the caller. Caller won't have to treat the return code of this method
differently then.

[altimofeev, 2010-12-13 15:42:45]

Anton, please take another look since I have changed enough from the last
review. Sorry for that...

http://codereview.chromium.org/5709001/diff/25001/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/existing_user_controller.cc (left):

http://codereview.chromium.org/5709001/diff/25001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/existing_user_controller.cc:218:
new_user_view->SetUsername(username);
On 2010/12/13 07:57:35, whywhat wrote:
> Note: this was used to auto fill username after account creation though
password
> was empty (we didn't want to store it in memory to pass between screens). I
> guess it's okay for now to break this and fix when we'll implement account
> creation.

Agree.

http://codereview.chromium.org/5709001/diff/25001/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/existing_user_controller.cc (right):

http://codereview.chromium.org/5709001/diff/25001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/existing_user_controller.cc:477:
controllers_[selected_view_index_]->GetMainInputScreenBounds();
On 2010/12/13 07:57:35, whywhat wrote:
> Maybe simply call the method GetMessageBubblePosition() if that's what the
> result is used for?

Actually it is used to calculate the bubble position, not the position itself.

http://codereview.chromium.org/5709001/diff/25001/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/existing_user_view.cc (right):

http://codereview.chromium.org/5709001/diff/25001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/existing_user_view.cc:104:
user_controller_->OnLogin("", UTF16ToUTF8(password_field_->text()));
On 2010/12/13 07:57:35, whywhat wrote:
> Maybe std::string() is better here for empty username? Why do we pass an empty
> one, btw?

I think "" is more representative. Empty string is used since user_controller
already has the username. At the same time ExistingUserView doesn't know the
username.

http://codereview.chromium.org/5709001/diff/25001/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/new_user_view.cc (right):

http://codereview.chromium.org/5709001/diff/25001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/new_user_view.cc:424: throbber_y +
sign_in_button_->GetPreferredSize().height() / 2;
On 2010/12/13 07:57:35, whywhat wrote:
> Guess we could use y here and eliminate throbber_y altogether.
> Can't we calculate these coordinates each time using
sing_in_button_.GetBounds()
> for x, y, width and height? Wouldn't this be better than saving these values
in
> class members?

Good point! Fixed.

http://codereview.chromium.org/5709001/diff/25001/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/user_controller.h (right):

http://codereview.chromium.org/5709001/diff/25001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/user_controller.h:96: // Returns bounds of the
main input field in screen coordinats (e.g. these
On 2010/12/13 07:57:35, whywhat wrote:
> nit: coordinats -> coordinates

Done.

http://codereview.chromium.org/5709001/diff/25001/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/user_input.h (right):

http://codereview.chromium.org/5709001/diff/25001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/user_input.h:26: // Returns bounds of the main
input field in screen coordinats (e.g. these
On 2010/12/13 07:57:35, whywhat wrote:
> same nit about coordinats

Done.

http://codereview.chromium.org/5709001/diff/25001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/user_input.h:32: virtual bool
UseCustomBoundsForThrobber(const gfx::Size& throbber,
On 2010/12/13 07:57:35, whywhat wrote:
> Could we change this method to smth like StartThrobber(ThrobberManager*)? Thus
> each UserInput would choose where to put the throbber without exposing bounds
to
> the caller. Caller won't have to treat the return code of this method
> differently then.

I went further and implemented class that exposes start/stop interface.

[whywhat, 2010-12-13 16:10:55]

LGTM

http://codereview.chromium.org/5709001/diff/9003/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/existing_user_view.cc (right):

http://codereview.chromium.org/5709001/diff/9003/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/existing_user_view.cc:113: 
This blank line is not needed imho.

http://codereview.chromium.org/5709001/diff/9003/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/helper.cc (right):

http://codereview.chromium.org/5709001/diff/9003/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/helper.cc:72: : host_view_(host_view),
throbber_widget_(NULL) {
Add two spaces in front of : and put the second member on the next line.

http://codereview.chromium.org/5709001/diff/9003/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/helper.h (right):

http://codereview.chromium.org/5709001/diff/9003/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/helper.h:34: class ThrobberHost {
It'd be better if we could embed ThrobberHost into view instead of inheriting
from it since we only can inherit from one non-interface class. We'd have to add
some lines of code to each class though so I don't feel strong against this
solution now.

http://codereview.chromium.org/5709001/diff/9003/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/helper.h:61: views::Widget *throbber_widget_;
Align * with the type, please.

http://codereview.chromium.org/5709001/diff/9003/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/helper.h:119: const int kThrobberRightMargin = 10;
Comment about the constant?

http://codereview.chromium.org/5709001/diff/9003/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/new_user_view.cc (right):

http://codereview.chromium.org/5709001/diff/9003/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/new_user_view.cc:491: x +=
password_field_->width()- throbber_size.width();
space before -

http://codereview.chromium.org/5709001/diff/9003/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/new_user_view.cc:495: return
gfx::Rect(gfx::Point(x,y), throbber_size);
space between , and y

[oshima, 2010-12-13 19:17:41]

LGTM on locker bits

[altimofeev, 2010-12-14 11:15:21]

http://codereview.chromium.org/5709001/diff/9003/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/existing_user_view.cc (right):

http://codereview.chromium.org/5709001/diff/9003/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/existing_user_view.cc:113: 
On 2010/12/13 16:10:55, whywhat wrote:
> This blank line is not needed imho.

Done.

http://codereview.chromium.org/5709001/diff/9003/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/helper.cc (right):

http://codereview.chromium.org/5709001/diff/9003/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/helper.cc:72: : host_view_(host_view),
throbber_widget_(NULL) {
On 2010/12/13 16:10:55, whywhat wrote:
> Add two spaces in front of : and put the second member on the next line.

Done.

http://codereview.chromium.org/5709001/diff/9003/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/helper.h (right):

http://codereview.chromium.org/5709001/diff/9003/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/helper.h:34: class ThrobberHost {
On 2010/12/13 16:10:55, whywhat wrote:
> It'd be better if we could embed ThrobberHost into view instead of inheriting
> from it since we only can inherit from one non-interface class. We'd have to
add
> some lines of code to each class though so I don't feel strong against this
> solution now.

Subclassed from the views::View to avoid multiple inheritance.

http://codereview.chromium.org/5709001/diff/9003/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/helper.h:61: views::Widget *throbber_widget_;
On 2010/12/13 16:10:55, whywhat wrote:
> Align * with the type, please.

Done.

http://codereview.chromium.org/5709001/diff/9003/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/helper.h:119: const int kThrobberRightMargin = 10;
On 2010/12/13 16:10:55, whywhat wrote:
> Comment about the constant?

Done.

http://codereview.chromium.org/5709001/diff/9003/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/new_user_view.cc (right):

http://codereview.chromium.org/5709001/diff/9003/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/new_user_view.cc:491: x +=
password_field_->width()- throbber_size.width();
On 2010/12/13 16:10:55, whywhat wrote:
> space before -

Done.

http://codereview.chromium.org/5709001/diff/9003/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/new_user_view.cc:495: return
gfx::Rect(gfx::Point(x,y), throbber_size);
On 2010/12/13 16:10:55, whywhat wrote:
> space between , and y

Done.