[Signin Error Dialog] (1/3) Adds 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

Committed: https://crrev.com/22d42439d0a2c92efac24822dea1d995ad35985e
Cr-Commit-Position: refs/heads/master@{#416633}

[anthonyvd, 2016-08-30 20:00:38]

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.

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

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

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

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

[anthonyvd, 2016-08-30 20:07:42]

anthonyvd@chromium.org changed reviewers:
+ dbeam@chromium.org, tommycli@chromium.org

[anthonyvd, 2016-08-30 20:07:43]

Hello everyone,

This CL is a re-upload of https://codereview.chromium.org/2274013002/. Jane's
internship is already over so I'll be landing the Signin Error Dialog CLs on her
behalf.

This is exactly the same patch as the last one in that CL, except it addresses
dbeam@'s comments.

dbeam@, you commented:

> can you use preprocess="true" instead of flattenhtml="true" here?

The toolchain throws an error about preprocess and it doesn't appear to be used
elsewhere in that file. Am I missing something?

tommycli@: This shouldn't change anything on your side but feel free to take a
look :)

Thanks!

[tommycli, 2016-08-30 20:10:09]

still lgtm from me, other than Dan's comments.

[Dan Beam, 2016-08-31 20:20:07]

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

https://codereview.chromium.org/2298503002/diff/1/chrome/browser/browser_reso...
chrome/browser/browser_resources.grd:382: <include
name="IDR_SIGNIN_SHARED_CSS_HTML" file="resources\signin\signin_shared_css.html"
flattenhtml="true" allowexternalscript="true" type="BINDATA" />
can you just change from type="BINDATA" to type="chrome_html"?

[anthonyvd, 2016-09-01 17:13:16]

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

https://codereview.chromium.org/2298503002/diff/1/chrome/browser/browser_reso...
chrome/browser/browser_resources.grd:382: <include
name="IDR_SIGNIN_SHARED_CSS_HTML" file="resources\signin\signin_shared_css.html"
flattenhtml="true" allowexternalscript="true" type="BINDATA" />
On 2016/08/31 at 20:20:07, Dan Beam wrote:
> can you just change from type="BINDATA" to type="chrome_html"?

Done. Just wondering: what's the difference?

[Dan Beam, 2016-09-01 17:30:23]

so the reason why flattenhtml="true" is bad is that it magically inlines SOME
resources SOME of the time.  magically.  like not the good magic; more Voldemort
stuff.

it goes through the source of the file and does regex-based parsing to try to
find <img src="..."> or <script src="..."> or <link rel="stylesheet"
href="...">, or url() in CSS.

when/IF it finds these patterns, it inlines the content, potentially bloating
the on-disk size of Chrome.  in some cases (binary resources), it base 64
encodes stuff as a data URI or something.  this may be OK, but I suspect it may
pause the HTML parser at inopportune times.

tl;dr - using flattenhtml="true" is more-or-less discouraged in new code.  there
are better ways to handle resource dependency management (<link rel="import">)
or resource flattening (vulcanize).

if you really just need <if> for compile-time forking, just use
preprocess="true".

maybe I'll write a presubmit to explain this in the near future.

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

https://codereview.chromium.org/2298503002/diff/1/chrome/browser/browser_reso...
chrome/browser/browser_resources.grd:382: <include
name="IDR_SIGNIN_SHARED_CSS_HTML" file="resources\signin\signin_shared_css.html"
flattenhtml="true" allowexternalscript="true" type="BINDATA" />
On 2016/09/01 17:13:16, anthonyvd wrote:
> On 2016/08/31 at 20:20:07, Dan Beam wrote:
> > can you just change from type="BINDATA" to type="chrome_html"?
> 
> Done. Just wondering: what's the difference?

one mysteriously lets you use preprocess="true", of course! :P

[anthonyvd, 2016-09-01 17:35:24]

On 2016/09/01 at 17:30:23, dbeam wrote:
> so the reason why flattenhtml="true" is bad is that it magically inlines SOME
resources SOME of the time.  magically.  like not the good magic; more Voldemort
stuff.
> 
> it goes through the source of the file and does regex-based parsing to try to
find <img src="..."> or <script src="..."> or <link rel="stylesheet"
href="...">, or url() in CSS.
> 
> when/IF it finds these patterns, it inlines the content, potentially bloating
the on-disk size of Chrome.  in some cases (binary resources), it base 64
encodes stuff as a data URI or something.  this may be OK, but I suspect it may
pause the HTML parser at inopportune times.
> 
> tl;dr - using flattenhtml="true" is more-or-less discouraged in new code. 
there are better ways to handle resource dependency management (<link
rel="import">) or resource flattening (vulcanize).
> 
> if you really just need <if> for compile-time forking, just use
preprocess="true".
> 
> maybe I'll write a presubmit to explain this in the near future.
> 
>
https://codereview.chromium.org/2298503002/diff/1/chrome/browser/browser_reso...
> File chrome/browser/browser_resources.grd (right):
> 
>
https://codereview.chromium.org/2298503002/diff/1/chrome/browser/browser_reso...
> chrome/browser/browser_resources.grd:382: <include
name="IDR_SIGNIN_SHARED_CSS_HTML" file="resources\signin\signin_shared_css.html"
flattenhtml="true" allowexternalscript="true" type="BINDATA" />
> On 2016/09/01 17:13:16, anthonyvd wrote:
> > On 2016/08/31 at 20:20:07, Dan Beam wrote:
> > > can you just change from type="BINDATA" to type="chrome_html"?
> > 
> > Done. Just wondering: what's the difference?
> 
> one mysteriously lets you use preprocess="true", of course! :P

Thanks for the explanation! I'm still getting an error
(grit.exception.UnexpectedAttribute: The attribute was not expected: preprocess)
when changing flattenhtml to preprocess in that file, even with
type="chrome_html". Any other setup required?

[Dan Beam, 2016-09-01 17:40:27]

On 2016/09/01 17:35:24, anthonyvd wrote:
> On 2016/09/01 at 17:30:23, dbeam wrote:
> > so the reason why flattenhtml="true" is bad is that it magically inlines
SOME
> resources SOME of the time.  magically.  like not the good magic; more
Voldemort
> stuff.
> > 
> > it goes through the source of the file and does regex-based parsing to try
to
> find <img src="..."> or <script src="..."> or <link rel="stylesheet"
> href="...">, or url() in CSS.
> > 
> > when/IF it finds these patterns, it inlines the content, potentially
bloating
> the on-disk size of Chrome.  in some cases (binary resources), it base 64
> encodes stuff as a data URI or something.  this may be OK, but I suspect it
may
> pause the HTML parser at inopportune times.
> > 
> > tl;dr - using flattenhtml="true" is more-or-less discouraged in new code. 
> there are better ways to handle resource dependency management (<link
> rel="import">) or resource flattening (vulcanize).
> > 
> > if you really just need <if> for compile-time forking, just use
> preprocess="true".
> > 
> > maybe I'll write a presubmit to explain this in the near future.
> > 
> >
>
https://codereview.chromium.org/2298503002/diff/1/chrome/browser/browser_reso...
> > File chrome/browser/browser_resources.grd (right):
> > 
> >
>
https://codereview.chromium.org/2298503002/diff/1/chrome/browser/browser_reso...
> > chrome/browser/browser_resources.grd:382: <include
> name="IDR_SIGNIN_SHARED_CSS_HTML"
file="resources\signin\signin_shared_css.html"
> flattenhtml="true" allowexternalscript="true" type="BINDATA" />
> > On 2016/09/01 17:13:16, anthonyvd wrote:
> > > On 2016/08/31 at 20:20:07, Dan Beam wrote:
> > > > can you just change from type="BINDATA" to type="chrome_html"?
> > > 
> > > Done. Just wondering: what's the difference?
> > 
> > one mysteriously lets you use preprocess="true", of course! :P
> 
> Thanks for the explanation! I'm still getting an error
> (grit.exception.UnexpectedAttribute: The attribute was not expected:
preprocess)
> when changing flattenhtml to preprocess in that file, even with
> type="chrome_html". Any other setup required?

can you use a <structure> instead of an <include>?  I really don't know the
difference, but that's what I assume matters :(

https://cs.chromium.org/chromium/src/ui/webui/resources/webui_resources.grd?q...

[anthonyvd, 2016-09-01 17:57:04]

On 2016/09/01 at 17:40:27, dbeam wrote:
> On 2016/09/01 17:35:24, anthonyvd wrote:
> > On 2016/09/01 at 17:30:23, dbeam wrote:
> > > so the reason why flattenhtml="true" is bad is that it magically inlines
SOME
> > resources SOME of the time.  magically.  like not the good magic; more
Voldemort
> > stuff.
> > > 
> > > it goes through the source of the file and does regex-based parsing to try
to
> > find <img src="..."> or <script src="..."> or <link rel="stylesheet"
> > href="...">, or url() in CSS.
> > > 
> > > when/IF it finds these patterns, it inlines the content, potentially
bloating
> > the on-disk size of Chrome.  in some cases (binary resources), it base 64
> > encodes stuff as a data URI or something.  this may be OK, but I suspect it
may
> > pause the HTML parser at inopportune times.
> > > 
> > > tl;dr - using flattenhtml="true" is more-or-less discouraged in new code. 
> > there are better ways to handle resource dependency management (<link
> > rel="import">) or resource flattening (vulcanize).
> > > 
> > > if you really just need <if> for compile-time forking, just use
> > preprocess="true".
> > > 
> > > maybe I'll write a presubmit to explain this in the near future.
> > > 
> > >
> >
https://codereview.chromium.org/2298503002/diff/1/chrome/browser/browser_reso...
> > > File chrome/browser/browser_resources.grd (right):
> > > 
> > >
> >
https://codereview.chromium.org/2298503002/diff/1/chrome/browser/browser_reso...
> > > chrome/browser/browser_resources.grd:382: <include
> > name="IDR_SIGNIN_SHARED_CSS_HTML"
file="resources\signin\signin_shared_css.html"
> > flattenhtml="true" allowexternalscript="true" type="BINDATA" />
> > > On 2016/09/01 17:13:16, anthonyvd wrote:
> > > > On 2016/08/31 at 20:20:07, Dan Beam wrote:
> > > > > can you just change from type="BINDATA" to type="chrome_html"?
> > > > 
> > > > Done. Just wondering: what's the difference?
> > > 
> > > one mysteriously lets you use preprocess="true", of course! :P
> > 
> > Thanks for the explanation! I'm still getting an error
> > (grit.exception.UnexpectedAttribute: The attribute was not expected:
preprocess)
> > when changing flattenhtml to preprocess in that file, even with
> > type="chrome_html". Any other setup required?
> 
> can you use a <structure> instead of an <include>?  I really don't know the
difference, but that's what I assume matters :(
> 
>
https://cs.chromium.org/chromium/src/ui/webui/resources/webui_resources.grd?q...

Looks like that's it. Done!

[anthonyvd, 2016-09-02 19:22:15]

On 2016/09/01 at 17:57:04, anthonyvd wrote:
> On 2016/09/01 at 17:40:27, dbeam wrote:
> > On 2016/09/01 17:35:24, anthonyvd wrote:
> > > On 2016/09/01 at 17:30:23, dbeam wrote:
> > > > so the reason why flattenhtml="true" is bad is that it magically inlines
SOME
> > > resources SOME of the time.  magically.  like not the good magic; more
Voldemort
> > > stuff.
> > > > 
> > > > it goes through the source of the file and does regex-based parsing to
try to
> > > find <img src="..."> or <script src="..."> or <link rel="stylesheet"
> > > href="...">, or url() in CSS.
> > > > 
> > > > when/IF it finds these patterns, it inlines the content, potentially
bloating
> > > the on-disk size of Chrome.  in some cases (binary resources), it base 64
> > > encodes stuff as a data URI or something.  this may be OK, but I suspect
it may
> > > pause the HTML parser at inopportune times.
> > > > 
> > > > tl;dr - using flattenhtml="true" is more-or-less discouraged in new
code. 
> > > there are better ways to handle resource dependency management (<link
> > > rel="import">) or resource flattening (vulcanize).
> > > > 
> > > > if you really just need <if> for compile-time forking, just use
> > > preprocess="true".
> > > > 
> > > > maybe I'll write a presubmit to explain this in the near future.
> > > > 
> > > >
> > >
https://codereview.chromium.org/2298503002/diff/1/chrome/browser/browser_reso...
> > > > File chrome/browser/browser_resources.grd (right):
> > > > 
> > > >
> > >
https://codereview.chromium.org/2298503002/diff/1/chrome/browser/browser_reso...
> > > > chrome/browser/browser_resources.grd:382: <include
> > > name="IDR_SIGNIN_SHARED_CSS_HTML"
file="resources\signin\signin_shared_css.html"
> > > flattenhtml="true" allowexternalscript="true" type="BINDATA" />
> > > > On 2016/09/01 17:13:16, anthonyvd wrote:
> > > > > On 2016/08/31 at 20:20:07, Dan Beam wrote:
> > > > > > can you just change from type="BINDATA" to type="chrome_html"?
> > > > > 
> > > > > Done. Just wondering: what's the difference?
> > > > 
> > > > one mysteriously lets you use preprocess="true", of course! :P
> > > 
> > > Thanks for the explanation! I'm still getting an error
> > > (grit.exception.UnexpectedAttribute: The attribute was not expected:
preprocess)
> > > when changing flattenhtml to preprocess in that file, even with
> > > type="chrome_html". Any other setup required?
> > 
> > can you use a <structure> instead of an <include>?  I really don't know the
difference, but that's what I assume matters :(
> > 
> >
https://cs.chromium.org/chromium/src/ui/webui/resources/webui_resources.grd?q...
> 
> Looks like that's it. Done!

Friendly ping, any more feedback? :)

[Dan Beam, 2016-09-02 23:21:55]

lgtm but do you have any tests?

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

https://codereview.chromium.org/2298503002/diff/40001/chrome/browser/resource...
chrome/browser/resources/signin/signin_error/signin_error.html:35: <paper-button
class="primary-action" id="switchButton">
it'd be nice to use on-tap instead of addEventListener() from the js, but this
would probably have to be a real Polymer() element

https://codereview.chromium.org/2298503002/diff/40001/chrome/browser/resource...
chrome/browser/resources/signin/signin_error/signin_error.html:48: <script
src="chrome://resources/js/util.js"></script>
these should also be imports instead of scripts

https://codereview.chromium.org/2298503002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/signin/signin_shared_css.html (right):

https://codereview.chromium.org/2298503002/diff/40001/chrome/browser/resource...
chrome/browser/resources/signin/signin_shared_css.html:25:
--paper-button-flat-keyboard-focus: {
nit: move to top

[anthonyvd, 2016-09-06 14:36:14]

Thanks for the review!

The test situation is 2-fold. Most of this CL is a refactor of the Sync
Confirmation Dialog code to a set of common files. That functionality is already
covered by unit tests. As for the new code, it'll get unit tests in the next 2
CLs, with add the web ui handlers and the rest of the functional code. This was
initially a huge CL that was split in 3 to make reviews easier.

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

https://codereview.chromium.org/2298503002/diff/40001/chrome/browser/resource...
chrome/browser/resources/signin/signin_error/signin_error.html:48: <script
src="chrome://resources/js/util.js"></script>
On 2016/09/02 at 23:21:55, Dan Beam wrote:
> these should also be imports instead of scripts

Done.

https://codereview.chromium.org/2298503002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/signin/signin_shared_css.html (right):

https://codereview.chromium.org/2298503002/diff/40001/chrome/browser/resource...
chrome/browser/resources/signin/signin_shared_css.html:25:
--paper-button-flat-keyboard-focus: {
On 2016/09/02 at 23:21:55, Dan Beam wrote:
> nit: move to top

Done.

[anthonyvd, 2016-09-06 14:36:29]

The CQ bit was checked by anthonyvd@chromium.org

[anthonyvd, 2016-09-06 14:36:30]

The patchset sent to the CQ was uploaded after l-g-t-m from
tommycli@chromium.org, dbeam@chromium.org
Link to the patchset: https://codereview.chromium.org/2298503002/#ps60001
(title: "Address feedback.")

[commit-bot: I haz the power, 2016-09-06 14:36:43]

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-09-06 15:55:35]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-09-06 15:59: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.

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

Committed: https://crrev.com/22d42439d0a2c92efac24822dea1d995ad35985e
Cr-Commit-Position: refs/heads/master@{#416633}
==========

[commit-bot: I haz the power, 2016-09-06 15:59:13]

Patchset 4 (id:??) landed as
https://crrev.com/22d42439d0a2c92efac24822dea1d995ad35985e
Cr-Commit-Position: refs/heads/master@{#416633}