[Signin Error Dialog] (2/3) Added handlers and UI constructors

[Signin Error Dialog] (2/3) Added handlers and UI constructors

Signin Error Dialog:
Part of the desktop user menu revamp project is to migrate signin error
surfacing from user menu's tutorial card to a tab modal dialog, and thus
completing the flow of signin modal -> sync confirmation modal dialog or signin
error modal dialog.

Particularly, this CL adds the code for appropriately constructing and
handling the dialog.
DO NOT LAND until after the first CL lands (https://codereview.chromium.org/2274013002/).

Design doc:
https://docs.google.com/document/d/17yHrKd_EU6WoIAgljnZmNCSJLRlbM3O2Pxr7aPEnqik/edit?ts=57445a70#heading=h.9vm5owqqt3w1

BUG=630523
BUG=615893

[Jane, 2016-08-24 13:35:36]

Description was changed from

==========
(2/3) Added a tab modal dialog for surfacing signin errors

Part of the desktop user menu revamp project is to migrate signin error
surfacing from user menu's tutorial card to a tab modal dialog, and thus
completing the flow of signin modal -> sync confirmation modal dialog or signin
error modal dialog.

Particularly, this CL adds the code for appropriately constructing and
handling the dialog.
DO NOT LAND until after the first CL lands.

Design doc:
https://docs.google.com/document/d/17yHrKd_EU6WoIAgljnZmNCSJLRlbM3O2Pxr7aPEnq...

BUG=630523
BUG=615893
==========

to

==========
[Signin Error Dialog] (2/3) Added handlers and UI constructors

Signin Error Dialog:
Part of the desktop user menu revamp project is to migrate signin error
surfacing from user menu's tutorial card to a tab modal dialog, and thus
completing the flow of signin modal -> sync confirmation modal dialog or signin
error modal dialog.

Particularly, this CL adds the code for appropriately constructing and
handling the dialog.
DO NOT LAND until after the first CL lands
(https://codereview.chromium.org/2274013002/).

Design doc:
https://docs.google.com/document/d/17yHrKd_EU6WoIAgljnZmNCSJLRlbM3O2Pxr7aPEnq...

BUG=630523
BUG=615893
==========

[Jane, 2016-08-24 13:43:29]

janeliulwq@google.com changed reviewers:
+ tommycli@chromium.org

[Jane, 2016-08-24 13:43:30]

Split from the big CL. Sorry this one is still pretty big, but at least fewer
files. Thanks!

[tommycli, 2016-08-24 17:36:42]

some more comments thx

https://codereview.chromium.org/2275883003/diff/1/chrome/browser/ui/webui/sig...
File chrome/browser/ui/webui/signin/get_desktop_browser.h (right):

https://codereview.chromium.org/2275883003/diff/1/chrome/browser/ui/webui/sig...
chrome/browser/ui/webui/signin/get_desktop_browser.h:8: namespace signin {
Can this file be consolidated with get_auth_frame into something just like
signin_utils ? Does that make sense?

https://codereview.chromium.org/2275883003/diff/1/chrome/browser/ui/webui/sig...
File chrome/browser/ui/webui/signin/signin_error_handler.cc (right):

https://codereview.chromium.org/2275883003/diff/1/chrome/browser/ui/webui/sig...
chrome/browser/ui/webui/signin/signin_error_handler.cc:62: void
SigninErrorHandler::HandleInitializedWithSize(
Is this strictly speaking necessary? Could the dialog have a fixed height?

https://codereview.chromium.org/2275883003/diff/1/chrome/browser/ui/webui/sig...
chrome/browser/ui/webui/signin/signin_error_handler.cc:81:
web_ui()->CallJavascriptFunctionUnsafe("signin.error.clearFocus");
Yes, it seems odd to me that we're clearing the focus, and then also adding an
Enter handler to the document level to pretend like we're focused on a button.

[Jane, 2016-08-25 19:19:35]

Thanks!

https://codereview.chromium.org/2275883003/diff/1/chrome/browser/ui/webui/sig...
File chrome/browser/ui/webui/signin/get_desktop_browser.h (right):

https://codereview.chromium.org/2275883003/diff/1/chrome/browser/ui/webui/sig...
chrome/browser/ui/webui/signin/get_desktop_browser.h:8: namespace signin {
On 2016/08/24 17:36:41, tommycli wrote:
> Can this file be consolidated with get_auth_frame into something just like
> signin_utils ? Does that make sense?

Done. Code-wise this makes a lot of sense, I just had to edit a bunch of
#includes.

https://codereview.chromium.org/2275883003/diff/1/chrome/browser/ui/webui/sig...
File chrome/browser/ui/webui/signin/signin_error_handler.cc (right):

https://codereview.chromium.org/2275883003/diff/1/chrome/browser/ui/webui/sig...
chrome/browser/ui/webui/signin/signin_error_handler.cc:62: void
SigninErrorHandler::HandleInitializedWithSize(
On 2016/08/24 17:36:41, tommycli wrote:
> Is this strictly speaking necessary? Could the dialog have a fixed height?

I'm not exactly sure why, but the dialog doesn't show up for me at all when I
don't set the height here. What do you think?

https://codereview.chromium.org/2275883003/diff/1/chrome/browser/ui/webui/sig...
chrome/browser/ui/webui/signin/signin_error_handler.cc:81:
web_ui()->CallJavascriptFunctionUnsafe("signin.error.clearFocus");
On 2016/08/24 17:36:41, tommycli wrote:
> Yes, it seems odd to me that we're clearing the focus, and then also adding an
> Enter handler to the document level to pretend like we're focused on a button.

We discussed this on the upstream CL.

[tommycli, 2016-08-25 19:40:04]

https://codereview.chromium.org/2275883003/diff/1/chrome/browser/ui/webui/sig...
File chrome/browser/ui/webui/signin/signin_error_handler.cc (right):

https://codereview.chromium.org/2275883003/diff/1/chrome/browser/ui/webui/sig...
chrome/browser/ui/webui/signin/signin_error_handler.cc:62: void
SigninErrorHandler::HandleInitializedWithSize(
On 2016/08/25 19:19:35, Jane wrote:
> On 2016/08/24 17:36:41, tommycli wrote:
> > Is this strictly speaking necessary? Could the dialog have a fixed height?
> 
> I'm not exactly sure why, but the dialog doesn't show up for me at all when I
> don't set the height here. What do you think?

It seems that the height here is responding to the height of the rendered HTML
content.

But is that predictable? If it is - can we instead just use a fixed height? I am
asking here because this code is duplicated between this dialog and the Sync
Confirmation dialog, and it'd be good if there was not much duplicated code.

Maybe ask anthony?

[Jane, 2016-08-25 21:23:54]

https://codereview.chromium.org/2275883003/diff/1/chrome/browser/ui/webui/sig...
File chrome/browser/ui/webui/signin/signin_error_handler.cc (right):

https://codereview.chromium.org/2275883003/diff/1/chrome/browser/ui/webui/sig...
chrome/browser/ui/webui/signin/signin_error_handler.cc:62: void
SigninErrorHandler::HandleInitializedWithSize(
On 2016/08/25 19:40:03, tommycli wrote:
> On 2016/08/25 19:19:35, Jane wrote:
> > On 2016/08/24 17:36:41, tommycli wrote:
> > > Is this strictly speaking necessary? Could the dialog have a fixed height?
> > 
> > I'm not exactly sure why, but the dialog doesn't show up for me at all when
I
> > don't set the height here. What do you think?
> 
> It seems that the height here is responding to the height of the rendered HTML
> content.
> 
> But is that predictable? If it is - can we instead just use a fixed height? I
am
> asking here because this code is duplicated between this dialog and the Sync
> Confirmation dialog, and it'd be good if there was not much duplicated code.
> 
> Maybe ask anthony?

Talked to anthony, and the rationale is that the height might differ depending
on the locale (-> the height of the texts), so the fixed height is a temporary
placeholder value, whereas the height over here should be the more accurate one.
Should I refactor this into signin_utils?

[tommycli, 2016-08-25 21:25:35]

https://codereview.chromium.org/2275883003/diff/1/chrome/browser/ui/webui/sig...
File chrome/browser/ui/webui/signin/signin_error_handler.cc (right):

https://codereview.chromium.org/2275883003/diff/1/chrome/browser/ui/webui/sig...
chrome/browser/ui/webui/signin/signin_error_handler.cc:62: void
SigninErrorHandler::HandleInitializedWithSize(
On 2016/08/25 21:23:53, Jane wrote:
> On 2016/08/25 19:40:03, tommycli wrote:
> > On 2016/08/25 19:19:35, Jane wrote:
> > > On 2016/08/24 17:36:41, tommycli wrote:
> > > > Is this strictly speaking necessary? Could the dialog have a fixed
height?
> > > 
> > > I'm not exactly sure why, but the dialog doesn't show up for me at all
when
> I
> > > don't set the height here. What do you think?
> > 
> > It seems that the height here is responding to the height of the rendered
HTML
> > content.
> > 
> > But is that predictable? If it is - can we instead just use a fixed height?
I
> am
> > asking here because this code is duplicated between this dialog and the Sync
> > Confirmation dialog, and it'd be good if there was not much duplicated code.
> > 
> > Maybe ask anthony?
> 
> Talked to anthony, and the rationale is that the height might differ depending
> on the locale (-> the height of the texts), so the fixed height is a temporary
> placeholder value, whereas the height over here should be the more accurate
one.
> Should I refactor this into signin_utils?

That's a good reason. If there's some way to refactor the duplicated code into
signin_utils, that would be good, since less SLOC is better. thanks for
investigating!

[Jane, 2016-08-26 14:18:05]

Thanks!

https://codereview.chromium.org/2275883003/diff/1/chrome/browser/ui/webui/sig...
File chrome/browser/ui/webui/signin/signin_error_handler.cc (right):

https://codereview.chromium.org/2275883003/diff/1/chrome/browser/ui/webui/sig...
chrome/browser/ui/webui/signin/signin_error_handler.cc:62: void
SigninErrorHandler::HandleInitializedWithSize(
On 2016/08/25 21:25:35, tommycli wrote:
> On 2016/08/25 21:23:53, Jane wrote:
> > On 2016/08/25 19:40:03, tommycli wrote:
> > > On 2016/08/25 19:19:35, Jane wrote:
> > > > On 2016/08/24 17:36:41, tommycli wrote:
> > > > > Is this strictly speaking necessary? Could the dialog have a fixed
> height?
> > > > 
> > > > I'm not exactly sure why, but the dialog doesn't show up for me at all
> when
> > I
> > > > don't set the height here. What do you think?
> > > 
> > > It seems that the height here is responding to the height of the rendered
> HTML
> > > content.
> > > 
> > > But is that predictable? If it is - can we instead just use a fixed
height?
> I
> > am
> > > asking here because this code is duplicated between this dialog and the
Sync
> > > Confirmation dialog, and it'd be good if there was not much duplicated
code.
> > > 
> > > Maybe ask anthony?
> > 
> > Talked to anthony, and the rationale is that the height might differ
depending
> > on the locale (-> the height of the texts), so the fixed height is a
temporary
> > placeholder value, whereas the height over here should be the more accurate
> one.
> > Should I refactor this into signin_utils?
> 
> That's a good reason. If there's some way to refactor the duplicated code into
> signin_utils, that would be good, since less SLOC is better. thanks for
> investigating!

Done. Refactored the height initialization part into signin_utils.

[tommycli, 2016-08-26 16:12:11]

lgtm sans nit

https://codereview.chromium.org/2275883003/diff/40001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/signin/signin_utils.h (right):

https://codereview.chromium.org/2275883003/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/signin_utils.h:37: void
SetInitializedModalHeight(const base::ListValue* args,
nit: normally we always put the WebUI* in front of the ListValue* args.

[Jane, 2016-08-26 16:58:08]

janeliulwq@google.com changed reviewers:
+ anthonyvd@chromium.org

[Jane, 2016-08-26 16:58:09]

Hey Anthony, could you take a look at profile_metrics.h please, thanks!

[anthonyvd, 2016-08-26 17:43:11]

profile_metrics.h lgtm

[Jane, 2016-08-26 17:48:36]

janeliulwq@google.com changed reviewers:
+ achuith@chromium.org, sky@chromium.org

[Jane, 2016-08-26 17:48:37]

Hi achuith@ and sky@, I changed the file name of get_auth_frame.h ->
signin_utils.h, and updated the #include in your files. PTAL, thanks!
achuith@ - two files under c/b/chromeos/login/
sky@ - signin_view_controller_delegate.cc

[sky, 2016-08-26 19:32:13]

chrome/browser/ui/signin_view_controller_delegate.cc LGTM

[Jane, 2016-08-26 19:32:40]

The CQ bit was checked by janeliulwq@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-08-26 19:33:36]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-26 21:10:10]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-26 21:10:11]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[achuithb, 2016-08-26 21:57:55]

c/b/chromeos/login lgtm

[achuithb, 2016-08-26 21:58:00]

c/b/chromeos/login lgtm