[Signin Error Dialog] (3/3) Added the triggering code

[Signin Error Dialog] (3/3) Added the triggering code

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 triggering the dialog.
DO NOT LAND until after the first two CLs land (
https://codereview.chromium.org/2274013002/,
https://codereview.chromium.org/2275883003/).

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

BUG=630523
BUG=615893
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

[Jane, 2016-08-18 22:33:52]

Description was changed from

==========
Signin error tab modal dialog

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

to

==========
Signin error tab modal dialog

BUG=630523
BUG=615893
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[Jane, 2016-08-18 22:38:50]

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

[Jane, 2016-08-18 22:38:50]

Hi Anthony, can you take a look at this CL and let me know if the general flow
is okay and if I'm missing anything? I basically created a flow parallel to your
work on sync confirmation dialog.
If the structure is okay, could you then take a first pass at this?
Sorry about the massiveness!
Thanks!

[Jane, 2016-08-19 14:03:19]

Description was changed from

==========
Signin error tab modal dialog

BUG=630523
BUG=615893
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
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.

Mocks:

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

BUG=630523
BUG=615893
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[Jane, 2016-08-22 02:46:18]

Description was changed from

==========
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.

Mocks:

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

BUG=630523
BUG=615893
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
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.

Mocks:

Screenshot:
https://drive.google.com/open?id=0B7Fvv7JszRyGamxVRVc5OUVzSVE

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

BUG=630523
BUG=615893
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[anthonyvd, 2016-08-22 14:58:46]

This looks good from my high level perspective. You should definitely start
looping in owners.

Thanks!

https://codereview.chromium.org/2255413002/diff/40001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/profiles/signin_view_controller_delegate_mac.mm
(right):

https://codereview.chromium.org/2255413002/diff/40001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/profiles/signin_view_controller_delegate_mac.mm:42:
const int kSigninErrorDialogHeight = 164;
You're sending the initializedWithSize message from JS so this is just the
temporary height before layout, right?

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

https://codereview.chromium.org/2255413002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/login_ui_service.h:83: void
SetLastLoginErrorUsername(const base::string16& username);
Since username == email here, maybe we should pick one and stick with it
everywhere (including comments). I'd prefer email since it's what we're actually
passing but either is fine as long as it's consistent.

https://codereview.chromium.org/2255413002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/login_ui_service.h:93: const base::string16&
GetLastLoginErrorUsername();
Same as above.

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

https://codereview.chromium.org/2255413002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/signin_error_handler.cc:43: false, /* reuse any
existing windows */
Is not using an existing window a conscious design decision? I'd personally set
this to true otherwise.

https://codereview.chromium.org/2255413002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/signin_error_handler.cc:79: Browser*
SigninErrorHandler::GetDesktopBrowser() {
This code seems to be written in a few places. Can we factor it out in a common
location?

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

https://codereview.chromium.org/2255413002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/signin_error_handler.h:37: virtual void
HandleSwitch(const base::ListValue* args);
Can you make this function name a bit more descriptive? Something like
HandleSwitchToExistingProfile is easier to read at call sites even if it's
longer.

[Jane, 2016-08-22 20:48:31]

Thanks Anthony! 
Also a heads up that I do need your LGTM for profile_metrics.h later.

https://codereview.chromium.org/2255413002/diff/40001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/profiles/signin_view_controller_delegate_mac.mm
(right):

https://codereview.chromium.org/2255413002/diff/40001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/profiles/signin_view_controller_delegate_mac.mm:42:
const int kSigninErrorDialogHeight = 164;
On 2016/08/22 14:58:45, anthonyvd wrote:
> You're sending the initializedWithSize message from JS so this is just the
> temporary height before layout, right?

Yes that's right!

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

https://codereview.chromium.org/2255413002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/login_ui_service.h:83: void
SetLastLoginErrorUsername(const base::string16& username);
On 2016/08/22 14:58:46, anthonyvd wrote:
> Since username == email here, maybe we should pick one and stick with it
> everywhere (including comments). I'd prefer email since it's what we're
actually
> passing but either is fine as long as it's consistent.

Done.

https://codereview.chromium.org/2255413002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/login_ui_service.h:93: const base::string16&
GetLastLoginErrorUsername();
On 2016/08/22 14:58:46, anthonyvd wrote:
> Same as above.

Done.

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

https://codereview.chromium.org/2255413002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/signin_error_handler.cc:43: false, /* reuse any
existing windows */
On 2016/08/22 14:58:46, anthonyvd wrote:
> Is not using an existing window a conscious design decision? I'd personally
set
> this to true otherwise.

This bool is always_create and setting it to false means reusing any existing
windows. Did you think the opposite? I updated the comment to hopefully clear
the confusion.

https://codereview.chromium.org/2255413002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/signin_error_handler.cc:79: Browser*
SigninErrorHandler::GetDesktopBrowser() {
On 2016/08/22 14:58:46, anthonyvd wrote:
> This code seems to be written in a few places. Can we factor it out in a
common
> location?

Done, good point! I couldn't find a good place to put this in, so I created a
new get_desktop_browser.cc. This is then used in signin_error_handler,
sync_confirmation_handler, and inline_login_handler. Let me know if this looks
good, thanks!

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

https://codereview.chromium.org/2255413002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/signin_error_handler.h:37: virtual void
HandleSwitch(const base::ListValue* args);
On 2016/08/22 14:58:46, anthonyvd wrote:
> Can you make this function name a bit more descriptive? Something like
> HandleSwitchToExistingProfile is easier to read at call sites even if it's
> longer.

Done.

[Jane, 2016-08-22 20:49:54]

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

[Jane, 2016-08-22 20:49:54]

Hi dbeam@, could you take a look at c/b/resources and c/b/ui/webui please,
thanks!

[Dan Beam, 2016-08-23 05:56:00]

dbeam@chromium.org changed reviewers:
+ tommycli@chromium.org
- dbeam@chromium.org

[Dan Beam, 2016-08-23 05:56:01]

+tommycli@ for a signin-related webui page

[tommycli, 2016-08-23 19:15:39]

some initial comments.

Can we split this CL into smaller ones of ~200 SLOC or so, for ease of review?

Thanks!

https://codereview.chromium.org/2255413002/diff/60001/chrome/browser/resource...
File chrome/browser/resources/signin_error/signin_error.css (right):

https://codereview.chromium.org/2255413002/diff/60001/chrome/browser/resource...
chrome/browser/resources/signin_error/signin_error.css:19: border-bottom: 1px
solid lightgray;
Are we sure we want to just use 'lightgray' instead of a polymer color?

https://codereview.chromium.org/2255413002/diff/60001/chrome/browser/resource...
File chrome/browser/resources/signin_error/signin_error.html (right):

https://codereview.chromium.org/2255413002/diff/60001/chrome/browser/resource...
chrome/browser/resources/signin_error/signin_error.html:9: <link
rel="stylesheet" href="signin_error.css"></link>
If that CSS file is only used in here, why not just combine it below?

Also I think external stylesheets are deprecated:
https://www.polymer-project.org/1.0/docs/devguide/styling#external-stylesheets

https://codereview.chromium.org/2255413002/diff/60001/chrome/browser/resource...
chrome/browser/resources/signin_error/signin_error.html:19: font-weight: 500;
These styles that are in common with both primary and secondary can be just put
in a paper-button selector

https://codereview.chromium.org/2255413002/diff/60001/chrome/browser/resource...
chrome/browser/resources/signin_error/signin_error.html:44: <div class="details"
i18n-values=".innerHTML:signinErrorMessage"></div>
does using $i18nRaw work?

https://codereview.chromium.org/2255413002/diff/60001/chrome/browser/resource...
File chrome/browser/resources/signin_error/signin_error.js (right):

https://codereview.chromium.org/2255413002/diff/60001/chrome/browser/resource...
chrome/browser/resources/signin_error/signin_error.js:27:
$('primaryConfirmButton').style.display = 'none';
Instead of dynamically making it hidden, can we use CSS to start it as hidden?

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

https://codereview.chromium.org/2255413002/diff/60001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/inline_login_handler_impl.cc:834:
params.handler->HandleLoginError(error_msg);
Instead of setting a global SetLastLoginErrorEmail, we should pass it along the
same lines as error_msg.

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

https://codereview.chromium.org/2255413002/diff/60001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/login_ui_service.cc:80:
browser->ShowModalSigninErrorWindow();
Knowing little about signin, this confuses me. It should show the error window
when there's a login result?

Is |message| always an error message? If so, we should rename the variable

[Jane, 2016-08-24 03:14:38]

Description was changed from

==========
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.

Mocks:

Screenshot:
https://drive.google.com/open?id=0B7Fvv7JszRyGamxVRVc5OUVzSVE

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

BUG=630523
BUG=615893
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
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 triggering the dialog.
DO NOT LAND until after the first two CLs land.

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

BUG=630523
BUG=615893
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[Jane, 2016-08-24 03:14:38]

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

[Jane, 2016-08-24 03:58:09]

Description was changed from

==========
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 triggering the dialog.
DO NOT LAND until after the first two CLs land.

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

BUG=630523
BUG=615893
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
(3/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 triggering the dialog.
DO NOT LAND until after the first two CLs land.

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

BUG=630523
BUG=615893
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

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

Description was changed from

==========
(3/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 triggering the dialog.
DO NOT LAND until after the first two CLs land.

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

BUG=630523
BUG=615893
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
[Signin Error Dialog] (3/3) Added the triggering code

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 triggering the dialog.
DO NOT LAND until after the first two CLs land (
https://codereview.chromium.org/2274013002/,
https://codereview.chromium.org/2275883003/).

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

BUG=630523
BUG=615893
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[Jane, 2016-08-24 13:40:31]

Hi Tommy, I addressed your initial comments and split this CL into three. I'll
send you the other two soon. Thanks!

https://codereview.chromium.org/2255413002/diff/60001/chrome/browser/resource...
File chrome/browser/resources/signin_error/signin_error.css (right):

https://codereview.chromium.org/2255413002/diff/60001/chrome/browser/resource...
chrome/browser/resources/signin_error/signin_error.css:19: border-bottom: 1px
solid lightgray;
On 2016/08/23 19:15:38, tommycli wrote:
> Are we sure we want to just use 'lightgray' instead of a polymer color?

Replaced with paper-grey-300. I was just copying the style from
sync-confirmation, but paper-grey-300 should be good.

https://codereview.chromium.org/2255413002/diff/60001/chrome/browser/resource...
File chrome/browser/resources/signin_error/signin_error.html (right):

https://codereview.chromium.org/2255413002/diff/60001/chrome/browser/resource...
chrome/browser/resources/signin_error/signin_error.html:9: <link
rel="stylesheet" href="signin_error.css"></link>
On 2016/08/23 19:15:39, tommycli wrote:
> If that CSS file is only used in here, why not just combine it below?
> 
> Also I think external stylesheets are deprecated:
> https://www.polymer-project.org/1.0/docs/devguide/styling#external-stylesheets

Done.

https://codereview.chromium.org/2255413002/diff/60001/chrome/browser/resource...
chrome/browser/resources/signin_error/signin_error.html:19: font-weight: 500;
On 2016/08/23 19:15:39, tommycli wrote:
> These styles that are in common with both primary and secondary can be just
put
> in a paper-button selector

Done.

https://codereview.chromium.org/2255413002/diff/60001/chrome/browser/resource...
chrome/browser/resources/signin_error/signin_error.html:44: <div class="details"
i18n-values=".innerHTML:signinErrorMessage"></div>
On 2016/08/23 19:15:39, tommycli wrote:
> does using $i18nRaw work?

Done. Yep!

https://codereview.chromium.org/2255413002/diff/60001/chrome/browser/resource...
File chrome/browser/resources/signin_error/signin_error.js (right):

https://codereview.chromium.org/2255413002/diff/60001/chrome/browser/resource...
chrome/browser/resources/signin_error/signin_error.js:27:
$('primaryConfirmButton').style.display = 'none';
On 2016/08/23 19:15:39, tommycli wrote:
> Instead of dynamically making it hidden, can we use CSS to start it as hidden?

Done.

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

https://codereview.chromium.org/2255413002/diff/60001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/inline_login_handler_impl.cc:834:
params.handler->HandleLoginError(error_msg);
On 2016/08/23 19:15:39, tommycli wrote:
> Instead of setting a global SetLastLoginErrorEmail, we should pass it along
the
> same lines as error_msg.

Done.

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

https://codereview.chromium.org/2255413002/diff/60001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/login_ui_service.cc:80:
browser->ShowModalSigninErrorWindow();
On 2016/08/23 19:15:39, tommycli wrote:
> Knowing little about signin, this confuses me. It should show the error window
> when there's a login result?
> 
> Is |message| always an error message? If so, we should rename the variable

Done. You are right, this is confusing. Somewhere buried deep it would say that
empty message means signin success, otherwise it's an error. I renamed it to be
error_message.