[Signin Error Dialog] (1/3) Added necessary web components

[Signin Error Dialog] (1/3) Adds necessary web components

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 html and js components needed by the dialog.

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

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-24 03:12:56]

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.

This CL is the first one (out of three) to achieve this. This CL adds
the html and js components needed by the dialog.

Mocks:

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

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

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

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.

This CL is the first one (out of three) to achieve this. This CL adds
the html and js components needed by the 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
==========

[Jane, 2016-08-24 03:15:19]

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.

This CL is the first one (out of three) to achieve this. This CL adds
the html and js components needed by the 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 html and js components needed by the 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
==========

[Jane, 2016-08-24 03:58: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.

Particularly, this CL adds the html and js components needed by the 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

==========
(1/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 html and js components needed by the 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
==========

[Jane, 2016-08-24 13:32:45]

Description was changed from

==========
(1/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 html and js components needed by the 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

==========
[Signin Error Dialog] (1/3) Adds necessary web components

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 html and js components needed by the 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
==========

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

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

[Jane, 2016-08-24 13:42:37]

Split from the big CL. Thanks!

[tommycli, 2016-08-24 17:22:52]

thanks, further comments

https://codereview.chromium.org/2274013002/diff/1/chrome/browser/browser_reso...
File chrome/browser/browser_resources.grd (right):

https://codereview.chromium.org/2274013002/diff/1/chrome/browser/browser_reso...
chrome/browser/browser_resources.grd:383: <include name="IDR_SIGNIN_ERROR_HTML"
file="resources\signin_error\signin_error.html" flattenhtml="true"
allowexternalscript="true" type="BINDATA" />
Does this work if it's simply preprocess="true" instead of flattenhtml and
allowexternalscript="true"?

https://codereview.chromium.org/2274013002/diff/1/chrome/browser/resources/si...
File chrome/browser/resources/signin_error/signin_error.html (right):

https://codereview.chromium.org/2274013002/diff/1/chrome/browser/resources/si...
chrome/browser/resources/signin_error/signin_error.html:9: <style
is="custom-style">
I've noticed a variety of rules duplicated here with sync_confirmation.css. At
the least: the a style, the body style, the container style.

I know this is a yak shave, but can you investigate if how much could be
factored into a signin_shared_css.html?

You can look at settings_shared_css.html as an example. Also see
https://www.polymer-project.org/1.0/docs/devguide/styling#style-modules

https://codereview.chromium.org/2274013002/diff/1/chrome/browser/resources/si...
chrome/browser/resources/signin_error/signin_error.html:113: <script
src="chrome://resources/js/i18n_template.js"></script>
I suspect this is no longer needed.

https://codereview.chromium.org/2274013002/diff/1/chrome/browser/resources/si...
File chrome/browser/resources/signin_error/signin_error.js (right):

https://codereview.chromium.org/2274013002/diff/1/chrome/browser/resources/si...
chrome/browser/resources/signin_error/signin_error.js:29: function clearFocus()
{
Can we confirm that this is still needed? Esp. if it's still required to be
called from the C++ side after the initialize() method?

https://codereview.chromium.org/2274013002/diff/1/chrome/browser/resources/si...
chrome/browser/resources/signin_error/signin_error.js:33: function onKeyDown(e)
{
nit: would be good to make the order of these functions same as how they're
added in the initialize() method.

Also, it seems like it would be better to just set the default focus to the
desired button, rather than add a document level input handler.

[Jane, 2016-08-25 16:35:29]

Thanks!
In case it's not super clear: I moved sync_confirmation/ into
signin/sync_confirmation, and signin_error into signin/signin_error, so that the
shared_css.html can reside in signin/.

https://codereview.chromium.org/2274013002/diff/1/chrome/browser/browser_reso...
File chrome/browser/browser_resources.grd (right):

https://codereview.chromium.org/2274013002/diff/1/chrome/browser/browser_reso...
chrome/browser/browser_resources.grd:383: <include name="IDR_SIGNIN_ERROR_HTML"
file="resources\signin_error\signin_error.html" flattenhtml="true"
allowexternalscript="true" type="BINDATA" />
On 2016/08/24 17:22:52, tommycli wrote:
> Does this work if it's simply preprocess="true" instead of flattenhtml and
> allowexternalscript="true"?

Just preprocess="true"? I got "grit.exception.UnexpectedAttribute: The attribute
was not expected: preprocess".

https://codereview.chromium.org/2274013002/diff/1/chrome/browser/resources/si...
File chrome/browser/resources/signin_error/signin_error.html (right):

https://codereview.chromium.org/2274013002/diff/1/chrome/browser/resources/si...
chrome/browser/resources/signin_error/signin_error.html:9: <style
is="custom-style">
On 2016/08/24 17:22:52, tommycli wrote:
> I've noticed a variety of rules duplicated here with sync_confirmation.css. At
> the least: the a style, the body style, the container style.
> 
> I know this is a yak shave, but can you investigate if how much could be
> factored into a signin_shared_css.html?
> 
> You can look at settings_shared_css.html as an example. Also see
> https://www.polymer-project.org/1.0/docs/devguide/styling#style-modules

Done. Factored a lot of it into signin_shared_css.html.

https://codereview.chromium.org/2274013002/diff/1/chrome/browser/resources/si...
chrome/browser/resources/signin_error/signin_error.html:113: <script
src="chrome://resources/js/i18n_template.js"></script>
On 2016/08/24 17:22:52, tommycli wrote:
> I suspect this is no longer needed.

Done.

https://codereview.chromium.org/2274013002/diff/1/chrome/browser/resources/si...
File chrome/browser/resources/signin_error/signin_error.js (right):

https://codereview.chromium.org/2274013002/diff/1/chrome/browser/resources/si...
chrome/browser/resources/signin_error/signin_error.js:29: function clearFocus()
{
On 2016/08/24 17:22:52, tommycli wrote:
> Can we confirm that this is still needed? Esp. if it's still required to be
> called from the C++ side after the initialize() method?

Confirmed that on mac, the "Learn more" link would be automatically focused
every time the dialog loads up.

https://codereview.chromium.org/2274013002/diff/1/chrome/browser/resources/si...
chrome/browser/resources/signin_error/signin_error.js:33: function onKeyDown(e)
{
On 2016/08/24 17:22:52, tommycli wrote:
> nit: would be good to make the order of these functions same as how they're
> added in the initialize() method.
> 
> Also, it seems like it would be better to just set the default focus to the
> desired button, rather than add a document level input handler.

Done nit.
I talked to anthonyvd@ who did this for sync-confirmation, and his original idea
of setting default focus to the desired button was shot down by dbeam@ because
of screenreader messups, hence the workaround (see
https://codereview.chromium.org/1949613003). What do you think?

[tommycli, 2016-08-25 16:43:58]

lgtm thanks for investigating

[Jane, 2016-08-25 16:50:24]

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

[commit-bot: I haz the power, 2016-08-25 16:50:51]

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

[Jane, 2016-08-25 19:20:12]

Description was changed from

==========
[Signin Error Dialog] (1/3) Adds necessary web components

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 html and js components needed by the 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

==========
[Signin Error Dialog] (1/3) Adds necessary web components

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 html and js components needed by the dialog.

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

[commit-bot: I haz the power, 2016-08-25 20:54:33]

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

[commit-bot: I haz the power, 2016-08-25 20:54:33]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Jane, 2016-08-26 13:47:18]

The CQ bit was checked by janeliulwq@google.com

[commit-bot: I haz the power, 2016-08-26 13:47:50]

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 13:54:02]

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

[commit-bot: I haz the power, 2016-08-26 13:54:03]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Jane, 2016-08-26 13:59:37]

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

[Jane, 2016-08-26 13:59:38]

Hi dbeam@, can you take a look at browser_resources.grd please? Thanks!

[Dan Beam, 2016-08-30 00:43:08]

https://codereview.chromium.org/2274013002/diff/20001/chrome/browser/browser_...
File chrome/browser/browser_resources.grd (right):

https://codereview.chromium.org/2274013002/diff/20001/chrome/browser/browser_...
chrome/browser/browser_resources.grd:380: <include
name="IDR_SIGNIN_SHARED_CSS_HTML" file="resources\signin\signin_shared_css.html"
flattenhtml="true" allowexternalscript="true" type="BINDATA" />
can you use preprocess="true" instead of flattenhtml="true" here?

https://codereview.chromium.org/2274013002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/signin/signin_error/signin_error.html (right):

https://codereview.chromium.org/2274013002/diff/20001/chrome/browser/resource...
chrome/browser/resources/signin/signin_error/signin_error.html:50: <script
src="chrome://resources/js/cr.js"></script>
you should be using <link rel="import" href="chrome://resources/html/cr.html">
instead for all of these chrome://resources <script>s

https://codereview.chromium.org/2274013002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/signin/signin_error/signin_error.js (right):

https://codereview.chromium.org/2274013002/diff/20001/chrome/browser/resource...
chrome/browser/resources/signin/signin_error/signin_error.js:47:
$('confirmButton').style.display = '';
can you use hidden = true/false instead of .style.display = ''/'none'?