[cros] Member variables cleanup in SigninScreenHandler.

[cros] Member variables cleanup in SigninScreenHandler.

Member variables was renamed. It should clear up the logic.

BUG=None
TEST=focus doesn't get stolen from pod; gaia prompt is focused on "add user"


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

[altimofeev, 2012-03-06 07:44:31]

PTAL

[Nikita (slow), 2012-03-06 14:36:02]

lgtm

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

http://codereview.chromium.org/9617003/diff/1/chrome/browser/ui/webui/chromeo...
chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc:727: LOG(ERROR)
<< "HandleShowAddUser: email=" << email_ << ", first_attempt="
nit: update comment

http://codereview.chromium.org/9617003/diff/1/chrome/browser/ui/webui/chromeo...
chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc:833:
gaia_is_preloaded_ = true;
GAIA is not preloaded at this point. So this variable means that GAIA will be
preloaded or already is preloaded.

http://codereview.chromium.org/9617003/diff/1/chrome/browser/ui/webui/chromeo...
chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc:834:
LoadAuthExtension(true, true, false);
Should be LoadAuthExtension(gaia_is_preloaded_, true, false);
This way it is more clear why are you setting gaia_is_preloaded_ in this block.

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

http://codereview.chromium.org/9617003/diff/1/chrome/browser/ui/webui/chromeo...
chrome/browser/ui/webui/chromeos/login/signin_screen_handler.h:260: // Is Gaia
page preloaded (in the background)?
Looking at the cc file it should be:

// Is Gaia page preloaded in the background (or will be preloaded)?

[Daniel Erat, 2012-03-06 15:23:47]

Thanks for doing this!  I have a few questions, but LGTM when Nikita is happy.

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

http://codereview.chromium.org/9617003/diff/1/chrome/browser/ui/webui/chromeo...
chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc:559:
gaia_is_preloaded_ = false;
could you add a comment about this?  it seems a bit strange to me that the
preloaded variable goes from true to false here.

http://codereview.chromium.org/9617003/diff/1/chrome/browser/ui/webui/chromeo...
chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc:861: if
(!gaia_is_preloaded_) {
is_first_attempt_ was initialized to true, but gaia_is_preloaded_ is initialized
to false.  does this still work because it's now getting set to true in
HandleAccountPickerReady()?

[altimofeev, 2012-03-06 17:39:22]

Tried to address your comments. PTAL.

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

http://codereview.chromium.org/9617003/diff/1/chrome/browser/ui/webui/chromeo...
chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc:559:
gaia_is_preloaded_ = false;
On 2012/03/06 15:23:47, Daniel Erat wrote:
> could you add a comment about this?  it seems a bit strange to me that the
> preloaded variable goes from true to false here.

Done.

http://codereview.chromium.org/9617003/diff/1/chrome/browser/ui/webui/chromeo...
chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc:727: LOG(ERROR)
<< "HandleShowAddUser: email=" << email_ << ", first_attempt="
On 2012/03/06 14:36:02, Nikita Kostylev wrote:
> nit: update comment

Done.

http://codereview.chromium.org/9617003/diff/1/chrome/browser/ui/webui/chromeo...
chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc:833:
gaia_is_preloaded_ = true;
On 2012/03/06 14:36:02, Nikita Kostylev wrote:
> GAIA is not preloaded at this point. So this variable means that GAIA will be
> preloaded or already is preloaded.

Done.

http://codereview.chromium.org/9617003/diff/1/chrome/browser/ui/webui/chromeo...
chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc:834:
LoadAuthExtension(true, true, false);
it couldn't be substituted everywhere, so from my point of view it will only
complicate the situation

http://codereview.chromium.org/9617003/diff/1/chrome/browser/ui/webui/chromeo...
chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc:861: if
(!gaia_is_preloaded_) {
Yes, it is correct now. We should only grab the focus when silent gaia page
loading has been done.

common flow:

- login screen loading started
- AccountPickerReady
- silent load of Gaia page
- WebuiReady fired by silently loaded Gaia page
- user clicks on 'Add new user'
- Gaia page is shown (silently loaded one)
- user clicks 'Cancel', account picker is shown
- user clicks on 'Add new user'
- Cookies, etc. are cleared, Gaia page is reloaded

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

http://codereview.chromium.org/9617003/diff/1/chrome/browser/ui/webui/chromeo...
chrome/browser/ui/webui/chromeos/login/signin_screen_handler.h:260: // Is Gaia
page preloaded (in the background)?
On 2012/03/06 14:36:02, Nikita Kostylev wrote:
> Looking at the cc file it should be:
> 
> // Is Gaia page preloaded in the background (or will be preloaded)?

Done.

[Nikita (slow), 2012-03-06 22:03:00]

lgtm

[commit-bot: I haz the power, 2012-03-14 07:52:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/altimofeev@chromium.org/9617003/4002

[commit-bot: I haz the power, 2012-03-14 07:52:06]

Can't apply patch for file
chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc.
While running patch -p1 --forward --force;
patching file chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc
Hunk #1 succeeded at 256 (offset 1 line).
Hunk #5 FAILED at 845.
1 out of 5 hunks FAILED -- saving rejects to file
chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc.rej

[commit-bot: I haz the power, 2012-03-14 08:09:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/altimofeev@chromium.org/9617003/10003

[commit-bot: I haz the power, 2012-03-14 11:33:52]

Try job failure for 9617003-10003 (retry) (retry) on win_rel for step
"browser_tests".
It's a second try, previously, step "browser_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2012-03-15 13:56:19]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/altimofeev@chromium.org/9617003/10003

[commit-bot: I haz the power, 2012-03-15 15:31:58]

Change committed as 126916