Migrate signin error messages to the new avatar bubble

Migrate signin error messages to the new avatar bubble

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

[guohui, 2014-08-19 14:45:29]

Hey,

could you please review the CL?

rogerta@ for general review,
sky@ for owner review of ui/views/...

Thanks,

Hui

[guohui, 2014-08-19 16:14:20]

+asvitkine@ for owner review of ui/cocoa/...

[sky, 2014-08-19 17:04:02]

https://codereview.chromium.org/485073002/diff/20001/chrome/browser/ui/views/...
File chrome/browser/ui/views/profiles/profile_chooser_view.cc (right):

https://codereview.chromium.org/485073002/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/profiles/profile_chooser_view.cc:816: DCHECK(sender ==
tutorial_learn_more_link_);
Is there really a point in adding this now without clicking the link actually
doing anything?

[Alexei Svitkine (slow), 2014-08-19 19:06:22]

https://codereview.chromium.org/485073002/diff/20001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/profiles/avatar_base_controller.mm (right):

https://codereview.chromium.org/485073002/diff/20001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/profiles/avatar_base_controller.mm:154:
static_cast<ProfileChooserController*>(menuController_);
Use ObjCCast and remove the dcheck.

https://codereview.chromium.org/485073002/diff/20001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm (right):

https://codereview.chromium.org/485073002/diff/20001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:1279:
LoginUIService* login_ui_service =
Nit: camelCase

https://codereview.chromium.org/485073002/diff/20001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:1404:
if(buttonMessage) {
Nit: Space after if

https://codereview.chromium.org/485073002/diff/20001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/signin/login_ui_service.cc (right):

https://codereview.chromium.org/485073002/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/signin/login_ui_service.cc:71: Browser* browser, const
base::string16& message) {
Nit: 1 param per line if you're wrapping them.

Maybe here first param can fit on the first line and 2nd param can be aligned?

[guohui, 2014-08-19 19:51:03]

https://codereview.chromium.org/485073002/diff/20001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/profiles/avatar_base_controller.mm (right):

https://codereview.chromium.org/485073002/diff/20001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/profiles/avatar_base_controller.mm:154:
static_cast<ProfileChooserController*>(menuController_);
On 2014/08/19 19:06:22, Alexei Svitkine wrote:
> Use ObjCCast and remove the dcheck.

Done.

https://codereview.chromium.org/485073002/diff/20001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm (right):

https://codereview.chromium.org/485073002/diff/20001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:1279:
LoginUIService* login_ui_service =
On 2014/08/19 19:06:22, Alexei Svitkine wrote:
> Nit: camelCase

Done.

https://codereview.chromium.org/485073002/diff/20001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:1404:
if(buttonMessage) {
On 2014/08/19 19:06:22, Alexei Svitkine wrote:
> Nit: Space after if

Done.

https://codereview.chromium.org/485073002/diff/20001/chrome/browser/ui/views/...
File chrome/browser/ui/views/profiles/profile_chooser_view.cc (right):

https://codereview.chromium.org/485073002/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/profiles/profile_chooser_view.cc:816: DCHECK(sender ==
tutorial_learn_more_link_);
On 2014/08/19 17:04:01, sky wrote:
> Is there really a point in adding this now without clicking the link actually
> doing anything?

Done.

https://codereview.chromium.org/485073002/diff/20001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/signin/login_ui_service.cc (right):

https://codereview.chromium.org/485073002/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/signin/login_ui_service.cc:71: Browser* browser, const
base::string16& message) {
On 2014/08/19 19:06:22, Alexei Svitkine wrote:
> Nit: 1 param per line if you're wrapping them.
> 
> Maybe here first param can fit on the first line and 2nd param can be aligned?

Done.

https://codereview.chromium.org/485073002/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/signin/login_ui_service.cc:71: Browser* browser, const
base::string16& message) {
On 2014/08/19 19:06:22, Alexei Svitkine wrote:
> Nit: 1 param per line if you're wrapping them.
> 
> Maybe here first param can fit on the first line and 2nd param can be aligned?

Done.

[sky, 2014-08-19 19:57:12]

LGTM

https://codereview.chromium.org/485073002/diff/40001/chrome/browser/ui/views/...
File chrome/browser/ui/views/profiles/profile_chooser_view.cc (right):

https://codereview.chromium.org/485073002/diff/40001/chrome/browser/ui/views/...
chrome/browser/ui/views/profiles/profile_chooser_view.cc:885: tutorial_view =
CreateSigninErrorView();
Add break.

[Alexei Svitkine (slow), 2014-08-19 19:59:51]

You have a compile failure. (With ObjCCast, you don't need the *).

[guohui, 2014-08-19 20:03:30]

fixed mac compile errors.

https://codereview.chromium.org/485073002/diff/40001/chrome/browser/ui/views/...
File chrome/browser/ui/views/profiles/profile_chooser_view.cc (right):

https://codereview.chromium.org/485073002/diff/40001/chrome/browser/ui/views/...
chrome/browser/ui/views/profiles/profile_chooser_view.cc:885: tutorial_view =
CreateSigninErrorView();
On 2014/08/19 19:57:12, sky wrote:
> Add break.

Done.

[Alexei Svitkine (slow), 2014-08-19 20:09:01]

lgtm

https://codereview.chromium.org/485073002/diff/60001/chrome/browser/signin/si...
File chrome/browser/signin/signin_ui_util.cc (right):

https://codereview.chromium.org/485073002/diff/60001/chrome/browser/signin/si...
chrome/browser/signin/signin_ui_util.cc:169: const char
kSigninErrorLearnMoreUrl[] =
Nit: Make static, which causes less binary bloat with our compilers for strings.

https://codereview.chromium.org/485073002/diff/60001/chrome/browser/signin/si...
File chrome/browser/signin/signin_ui_util.h (right):

https://codereview.chromium.org/485073002/diff/60001/chrome/browser/signin/si...
chrome/browser/signin/signin_ui_util.h:41: void
ShowSigninErrorLearnMorePage(Profile* profile);
Nit: It would be good to have comments for these methods.

[guohui, 2014-08-19 20:26:31]

https://codereview.chromium.org/485073002/diff/60001/chrome/browser/signin/si...
File chrome/browser/signin/signin_ui_util.cc (right):

https://codereview.chromium.org/485073002/diff/60001/chrome/browser/signin/si...
chrome/browser/signin/signin_ui_util.cc:169: const char
kSigninErrorLearnMoreUrl[] =
On 2014/08/19 20:09:00, Alexei Svitkine wrote:
> Nit: Make static, which causes less binary bloat with our compilers for
strings.

Done.

https://codereview.chromium.org/485073002/diff/60001/chrome/browser/signin/si...
File chrome/browser/signin/signin_ui_util.h (right):

https://codereview.chromium.org/485073002/diff/60001/chrome/browser/signin/si...
chrome/browser/signin/signin_ui_util.h:41: void
ShowSigninErrorLearnMorePage(Profile* profile);
On 2014/08/19 20:09:00, Alexei Svitkine wrote:
> Nit: It would be good to have comments for these methods.

Done.

[guohui, 2014-08-19 20:26:57]

The CQ bit was checked by guohui@chromium.org

[guohui, 2014-08-19 20:27:38]

The CQ bit was unchecked by guohui@chromium.org

[Roger Tawa OOO till Jul 10th, 2014-08-19 20:40:24]

lgtm

[guohui, 2014-08-19 20:41:56]

The CQ bit was checked by guohui@chromium.org

[commit-bot: I haz the power, 2014-08-19 20:42:37]

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

[commit-bot: I haz the power, 2014-08-19 22:15:29]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[guohui, 2014-08-19 22:21:53]

The CQ bit was checked by guohui@chromium.org

[commit-bot: I haz the power, 2014-08-19 22:25:17]

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

[commit-bot: I haz the power, 2014-08-19 23:34:05]

Committed patchset #6 (100001) as 290694