Refactor inline_login_ui

Refactor inline_login_ui

BUG=

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

[guohui, 2013-12-18 18:50:48]

Hey,

could you please take a look at the CL?

inline_login_ui.cc has become really complicated now. This CL breaks it into
multiple classes, with common logic in a base class InlineLoginHandler, and
platform specific logic in subclasses.

Thanks,

Hui

[Roger Tawa OOO till Jul 10th, 2013-12-18 20:49:35]

lgtm

Looks great, much nicer with fewer #ifdefs.

https://codereview.chromium.org/118343003/diff/40001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/signin/inline_login_handler_impl.cc (right):

https://codereview.chromium.org/118343003/diff/40001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/signin/inline_login_handler_impl.cc:157:
auth_fetcher_->StartCookieForOAuthCodeExchange("0");
Instead of using GaiaAuthFetcher::StartCookieForOAuthCodeExchange() we should
use OAuth2TokenFecther like chromeos.  I'm OK with doing this in a follow up CL
since this one is just about refactoring the inline webui.

[guohui, 2013-12-18 20:51:46]

https://codereview.chromium.org/118343003/diff/40001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/signin/inline_login_handler_impl.cc (right):

https://codereview.chromium.org/118343003/diff/40001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/signin/inline_login_handler_impl.cc:157:
auth_fetcher_->StartCookieForOAuthCodeExchange("0");
On 2013/12/18 20:49:35, Roger Tawa wrote:
> Instead of using GaiaAuthFetcher::StartCookieForOAuthCodeExchange() we should
> use OAuth2TokenFecther like chromeos.  I'm OK with doing this in a follow up
CL
> since this one is just about refactoring the inline webui.

yup, will do it in a followup CL. Thanks!

[xiyuan, 2013-12-18 21:27:59]

https://codereview.chromium.org/118343003/diff/60001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/chromeos/login/inline_login_handler_cros_impl.h
(right):

https://codereview.chromium.org/118343003/diff/60001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/chromeos/login/inline_login_handler_cros_impl.h:16:
class InlineLoginHandlerCrosImpl : public ::InlineLoginHandler {
nit: Suggest to call it InlineLoginHandlerChromeOS to be consistent. "Cros" is
not standard and it should be a post-fix rather than in the middle.

https://codereview.chromium.org/118343003/diff/60001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/chromeos/login/inline_login_handler_cros_impl.h:25:
class InlineLoginUIOAuth2Delegate;
nit: move this to immediate after private:

https://codereview.chromium.org/118343003/diff/60001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/signin/inline_login_handler.h (right):

https://codereview.chromium.org/118343003/diff/60001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/signin/inline_login_handler.h:13: explicit
InlineLoginHandler();
nit: remove explicit

https://codereview.chromium.org/118343003/diff/60001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/signin/inline_login_handler.h:37: virtual void
CompleteLogin(const base::ListValue* args) = 0;
Should these "virtuals" in public/protected section so that they can be
overridden?

https://codereview.chromium.org/118343003/diff/60001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/signin/inline_login_handler_impl.h (right):

https://codereview.chromium.org/118343003/diff/60001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/signin/inline_login_handler_impl.h:25: protected:
nit: remove since no need to have empty section.

[guohui, 2013-12-18 22:39:16]

https://codereview.chromium.org/118343003/diff/60001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/chromeos/login/inline_login_handler_cros_impl.h
(right):

https://codereview.chromium.org/118343003/diff/60001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/chromeos/login/inline_login_handler_cros_impl.h:16:
class InlineLoginHandlerCrosImpl : public ::InlineLoginHandler {
On 2013/12/18 21:27:59, xiyuan wrote:
> nit: Suggest to call it InlineLoginHandlerChromeOS to be consistent. "Cros" is
> not standard and it should be a post-fix rather than in the middle.

Done.

https://codereview.chromium.org/118343003/diff/60001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/chromeos/login/inline_login_handler_cros_impl.h:25:
class InlineLoginUIOAuth2Delegate;
On 2013/12/18 21:27:59, xiyuan wrote:
> nit: move this to immediate after private:

Done.

https://codereview.chromium.org/118343003/diff/60001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/signin/inline_login_handler.h (right):

https://codereview.chromium.org/118343003/diff/60001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/signin/inline_login_handler.h:13: explicit
InlineLoginHandler();
On 2013/12/18 21:27:59, xiyuan wrote:
> nit: remove explicit

Done.

https://codereview.chromium.org/118343003/diff/60001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/signin/inline_login_handler.h:37: virtual void
CompleteLogin(const base::ListValue* args) = 0;
On 2013/12/18 21:27:59, xiyuan wrote:
> Should these "virtuals" in public/protected section so that they can be
> overridden?

a virtual method can be overriden by subclasses, even if it is private.

https://codereview.chromium.org/118343003/diff/60001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/signin/inline_login_handler_impl.h (right):

https://codereview.chromium.org/118343003/diff/60001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/signin/inline_login_handler_impl.h:25: protected:
On 2013/12/18 21:27:59, xiyuan wrote:
> nit: remove since no need to have empty section.

Done.

[xiyuan, 2013-12-18 23:22:50]

lgtm

https://codereview.chromium.org/118343003/diff/120001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/chromeos/login/inline_login_handler_chromeos.h
(right):

https://codereview.chromium.org/118343003/diff/120001/chrome/browser/ui/webui...
chrome/browser/ui/webui/chromeos/login/inline_login_handler_chromeos.h:18:
explicit InlineLoginHandlerCrosImpl();
nit: remove explicit

https://codereview.chromium.org/118343003/diff/120001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/signin/inline_login_handler_impl.h (right):

https://codereview.chromium.org/118343003/diff/120001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/inline_login_handler_impl.h:22: explicit
InlineLoginHandlerImpl();
nit: remove explicit

[guohui, 2013-12-19 15:23:14]

https://codereview.chromium.org/118343003/diff/120001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/chromeos/login/inline_login_handler_chromeos.h
(right):

https://codereview.chromium.org/118343003/diff/120001/chrome/browser/ui/webui...
chrome/browser/ui/webui/chromeos/login/inline_login_handler_chromeos.h:18:
explicit InlineLoginHandlerCrosImpl();
On 2013/12/18 23:22:50, xiyuan wrote:
> nit: remove explicit

Done.

https://codereview.chromium.org/118343003/diff/120001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/signin/inline_login_handler_impl.h (right):

https://codereview.chromium.org/118343003/diff/120001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/inline_login_handler_impl.h:22: explicit
InlineLoginHandlerImpl();
On 2013/12/18 23:22:50, xiyuan wrote:
> nit: remove explicit

Done.

[commit-bot: I haz the power, 2013-12-19 15:24:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/guohui@chromium.org/118343003/50013

[commit-bot: I haz the power, 2013-12-19 16:24:41]

Retried try job too often on linux_chromeos_clang for step(s) compile
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2013-12-19 20:01:19]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/guohui@chromium.org/118343003/150001

[commit-bot: I haz the power, 2013-12-19 20:31:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/guohui@chromium.org/118343003/170001

[commit-bot: I haz the power, 2013-12-19 21:31:45]

Retried try job too often on linux_chromeos for step(s) app_list_unittests,
ash_unittests, aura_unittests, base_unittests, browser_tests,
cacheinvalidation_unittests, check_deps, check_deps2git, chromeos_unittests,
components_unittests, content_browsertests, content_unittests, crypto_unittests,
dbus_unittests, device_unittests, events_unittests, google_apis_unittests,
gpu_unittests, interactive_ui_tests, ipc_tests, jingle_unittests,
media_unittests, net_unittests, ppapi_unittests, printing_unittests,
sandbox_linux_unittests, sql_unittests, sync_unit_tests, unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2013-12-20 16:16:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/guohui@chromium.org/118343003/190001

[commit-bot: I haz the power, 2013-12-20 19:13:00]

Change committed as 242134