ChromeOS login webui refactoring : Move GAIA-related code to gaia_screen_handler

ChromeOS login webui refactoring : Move GAIA-related code to gaia_screen_handler

BUG=373314
R=nkostylev@chromium.org

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

[Nikita (slow), 2014-06-02 13:48:11]

Please execute cros_* trybots

[Nikita (slow), 2014-06-02 14:18:29]

https://codereview.chromium.org/304303005/diff/1/chrome/browser/ui/webui/chro...
File chrome/browser/ui/webui/chromeos/login/gaia_screen_handler.cc (right):

https://codereview.chromium.org/304303005/diff/1/chrome/browser/ui/webui/chro...
chrome/browser/ui/webui/chromeos/login/gaia_screen_handler.cc:151: // Strings
used by the fatal error dialog.
nit: by the SAML fatal error dialog

https://codereview.chromium.org/304303005/diff/1/chrome/browser/ui/webui/chro...
chrome/browser/ui/webui/chromeos/login/gaia_screen_handler.cc:311:
Delegate()->SetDisplayEmail(gaia::SanitizeEmail(email));
As discussed, let's continue checking for Delegate() existence for now. Removing
that check should be done separately i.e. making sure that lifetime of handler /
delegate are well defined.

https://codereview.chromium.org/304303005/diff/1/chrome/browser/ui/webui/chro...
chrome/browser/ui/webui/chromeos/login/gaia_screen_handler.cc:334:
signin_screen_handler_->OnShowAddUser();
Can you call ShowGaia() directly here or this is done to reset
is_account_picker_showing_first_time_ as well?

https://codereview.chromium.org/304303005/diff/1/chrome/browser/ui/webui/chro...
chrome/browser/ui/webui/chromeos/login/gaia_screen_handler.cc:379: LOG(WARNING)
<< "ShowSigninScreenIfReady() call.";
nit: ShowGaiaScreenIfReady

https://codereview.chromium.org/304303005/diff/1/chrome/browser/ui/webui/chro...
chrome/browser/ui/webui/chromeos/login/gaia_screen_handler.cc:470: // Fetching
of the extension is not started before account picker page is
nit: Move first comment sentence to SigninScreenHandler::ShouldLoadGaia().

https://codereview.chromium.org/304303005/diff/1/chrome/browser/ui/webui/chro...
chrome/browser/ui/webui/chromeos/login/gaia_screen_handler.cc:474: // Do not
load the extension for the screen locker, see crosbug.com/25018.
nit: Move screen locker comment to  SigninScreenHandler::ShouldLoadGaia()

https://codereview.chromium.org/304303005/diff/1/chrome/browser/ui/webui/chro...
chrome/browser/ui/webui/chromeos/login/gaia_screen_handler.cc:475: if
(signin_screen_handler_->ShouldLoadGaia() && !gaia_silent_load_ &&
nit: Can you please re-format this to have one condition per line.

https://codereview.chromium.org/304303005/diff/1/chrome/browser/ui/webui/chro...
File chrome/browser/ui/webui/chromeos/login/gaia_screen_handler.h (right):

https://codereview.chromium.org/304303005/diff/1/chrome/browser/ui/webui/chro...
chrome/browser/ui/webui/chromeos/login/gaia_screen_handler.h:91: void
PopulateEmail(const std::string& user_id);
nit: Add comment.

https://codereview.chromium.org/304303005/diff/1/chrome/browser/ui/webui/chro...
chrome/browser/ui/webui/chromeos/login/gaia_screen_handler.h:117: // Decides
whether an auth extension should be pre-loaded. If it should,
nit: Insert empty line before comment.

https://codereview.chromium.org/304303005/diff/1/chrome/browser/ui/webui/chro...
chrome/browser/ui/webui/chromeos/login/gaia_screen_handler.h:120: // Tells webui
to load authentication extension. |force| is used to force the
nit: Insert empty line before comment.

https://codereview.chromium.org/304303005/diff/1/chrome/browser/ui/webui/chro...
chrome/browser/ui/webui/chromeos/login/gaia_screen_handler.h:135: // Current
state of Gaia frame.
nit: Insert empty line before comment.

[Denis Kuznetsov (DE-MUC), 2014-06-02 16:47:01]

https://chromiumcodereview.appspot.com/304303005/diff/1/chrome/browser/ui/web...
File chrome/browser/ui/webui/chromeos/login/gaia_screen_handler.cc (right):

https://chromiumcodereview.appspot.com/304303005/diff/1/chrome/browser/ui/web...
chrome/browser/ui/webui/chromeos/login/gaia_screen_handler.cc:151: // Strings
used by the fatal error dialog.
On 2014/06/02 14:18:29, Nikita Kostylev wrote:
> nit: by the SAML fatal error dialog

Done.

https://chromiumcodereview.appspot.com/304303005/diff/1/chrome/browser/ui/web...
chrome/browser/ui/webui/chromeos/login/gaia_screen_handler.cc:311:
Delegate()->SetDisplayEmail(gaia::SanitizeEmail(email));
On 2014/06/02 14:18:29, Nikita Kostylev wrote:
> As discussed, let's continue checking for Delegate() existence for now.
Removing
> that check should be done separately i.e. making sure that lifetime of handler
/
> delegate are well defined.

Done.

https://chromiumcodereview.appspot.com/304303005/diff/1/chrome/browser/ui/web...
chrome/browser/ui/webui/chromeos/login/gaia_screen_handler.cc:334:
signin_screen_handler_->OnShowAddUser();
On 2014/06/02 14:18:29, Nikita Kostylev wrote:
> Can you call ShowGaia() directly here or this is done to reset
> is_account_picker_showing_first_time_ as well?

Done.

https://chromiumcodereview.appspot.com/304303005/diff/1/chrome/browser/ui/web...
chrome/browser/ui/webui/chromeos/login/gaia_screen_handler.cc:379: LOG(WARNING)
<< "ShowSigninScreenIfReady() call.";
On 2014/06/02 14:18:29, Nikita Kostylev wrote:
> nit: ShowGaiaScreenIfReady

Done.

https://chromiumcodereview.appspot.com/304303005/diff/1/chrome/browser/ui/web...
chrome/browser/ui/webui/chromeos/login/gaia_screen_handler.cc:470: // Fetching
of the extension is not started before account picker page is
On 2014/06/02 14:18:29, Nikita Kostylev wrote:
> nit: Move first comment sentence to SigninScreenHandler::ShouldLoadGaia().

Done.

https://chromiumcodereview.appspot.com/304303005/diff/1/chrome/browser/ui/web...
chrome/browser/ui/webui/chromeos/login/gaia_screen_handler.cc:474: // Do not
load the extension for the screen locker, see crosbug.com/25018.
On 2014/06/02 14:18:29, Nikita Kostylev wrote:
> nit: Move screen locker comment to  SigninScreenHandler::ShouldLoadGaia()

Done.

https://chromiumcodereview.appspot.com/304303005/diff/1/chrome/browser/ui/web...
chrome/browser/ui/webui/chromeos/login/gaia_screen_handler.cc:475: if
(signin_screen_handler_->ShouldLoadGaia() && !gaia_silent_load_ &&
On 2014/06/02 14:18:29, Nikita Kostylev wrote:
> nit: Can you please re-format this to have one condition per line.

Done.

https://chromiumcodereview.appspot.com/304303005/diff/1/chrome/browser/ui/web...
File chrome/browser/ui/webui/chromeos/login/gaia_screen_handler.h (right):

https://chromiumcodereview.appspot.com/304303005/diff/1/chrome/browser/ui/web...
chrome/browser/ui/webui/chromeos/login/gaia_screen_handler.h:91: void
PopulateEmail(const std::string& user_id);
On 2014/06/02 14:18:29, Nikita Kostylev wrote:
> nit: Add comment.

Done.

https://chromiumcodereview.appspot.com/304303005/diff/1/chrome/browser/ui/web...
chrome/browser/ui/webui/chromeos/login/gaia_screen_handler.h:117: // Decides
whether an auth extension should be pre-loaded. If it should,
On 2014/06/02 14:18:29, Nikita Kostylev wrote:
> nit: Insert empty line before comment.

Done.

https://chromiumcodereview.appspot.com/304303005/diff/1/chrome/browser/ui/web...
chrome/browser/ui/webui/chromeos/login/gaia_screen_handler.h:120: // Tells webui
to load authentication extension. |force| is used to force the
On 2014/06/02 14:18:29, Nikita Kostylev wrote:
> nit: Insert empty line before comment.

Done.

https://chromiumcodereview.appspot.com/304303005/diff/1/chrome/browser/ui/web...
chrome/browser/ui/webui/chromeos/login/gaia_screen_handler.h:135: // Current
state of Gaia frame.
On 2014/06/02 14:18:29, Nikita Kostylev wrote:
> nit: Insert empty line before comment.

Done.

[Nikita (slow), 2014-06-02 16:49:34]

lgtm

https://chromiumcodereview.appspot.com/304303005/diff/1/chrome/browser/ui/web...
File chrome/browser/ui/webui/chromeos/login/gaia_screen_handler.cc (right):

https://chromiumcodereview.appspot.com/304303005/diff/1/chrome/browser/ui/web...
chrome/browser/ui/webui/chromeos/login/gaia_screen_handler.cc:475: if
(signin_screen_handler_->ShouldLoadGaia() && !gaia_silent_load_ &&
On 2014/06/02 16:47:01, Denis Kuznetsov wrote:
> On 2014/06/02 14:18:29, Nikita Kostylev wrote:
> > nit: Can you please re-format this to have one condition per line.
> 
> Done.

I don't see changes in this condition. Please change before committing.

[ygorshenin1, 2014-06-03 17:48:04]

lgtm with nits

https://codereview.chromium.org/304303005/diff/20001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/chromeos/login/gaia_screen_handler.cc (right):

https://codereview.chromium.org/304303005/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/chromeos/login/gaia_screen_handler.cc:136:
weak_factory_.InvalidateWeakPtrs();
nit: no need to call InvalidateWeakPtrs() explicitly.

https://codereview.chromium.org/304303005/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/chromeos/login/gaia_screen_handler.cc:282: void
GaiaScreenHandler::HandleCompleteLogin(const std::string& typed_email,
nit: could you please reorder definitions according to declaration order in the
header file?

https://codereview.chromium.org/304303005/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/chromeos/login/gaia_screen_handler.cc:384: LOG(WARNING)
<< "ShowGaiaScreenIfReady() call.";
nit: debug logging?

https://codereview.chromium.org/304303005/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/chromeos/login/gaia_screen_handler.cc:473: LOG(WARNING)
<< "MaybePreloadAuthExtension() call.";
nit: debug logging?

https://codereview.chromium.org/304303005/diff/20001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/chromeos/login/gaia_screen_handler.h (right):

https://codereview.chromium.org/304303005/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/chromeos/login/gaia_screen_handler.h:99: void
OnCookiesCleared(base::Closure on_clear_callback);
nit: for consistency user const base::Closure& datatype.

https://codereview.chromium.org/304303005/diff/20001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/chromeos/login/signin_screen_handler.h (right):

https://codereview.chromium.org/304303005/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/chromeos/login/signin_screen_handler.h:441:
base::WeakPtrFactory<SigninScreenHandler> weak_factory_;
nit: btw, could you please move weak_factory_ to the end of the fields
declaration list?

[Denis Kuznetsov (DE-MUC), 2014-06-05 11:38:37]

The CQ bit was checked by antrim@chromium.org

[commit-bot: I haz the power, 2014-06-05 11:39:41]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/antrim@chromium.org/304303005/60001

[commit-bot: I haz the power, 2014-06-05 16:35:04]

Change committed as 275154