New SSL blocking screen

This implements the new SSL blocking screen. See the bug for screenshots of what it looks like.

BUG=276540
R=dubroy@chromium.org, palmer@chromium.org, rsleevi@chromium.org
TBR=jochen@chromium.org

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

[felt, 2013-09-06 00:53:39]

Hi Chris, can you please review chrome/browser/ssl/ssl_blocking_page.cc?

[felt, 2013-09-06 00:59:18]

Hi James, would you review everything in chrome/browser/resources/ and
chrome/browser/browser_resources.grd?

Thanks,
Adrienne

[palmer, 2013-09-06 17:01:33]

LGTM, with some thoughts.

https://codereview.chromium.org/23965003/diff/5001/chrome/app/generated_resou...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/23965003/diff/5001/chrome/app/generated_resou...
chrome/app/generated_resources.grd:9170: +        To protect your privacy,
Chrome will not load the page until it can establish a secure connection to the
real <ph name="SITE">$1<ex>google.com</ex></ph>.
Nit: Sadly, the style in this file is to have one hugely long line per
paragraph. I'm not sure if you should adhere to it, but other reviewers might
ask you to.

https://codereview.chromium.org/23965003/diff/5001/chrome/app/generated_resou...
chrome/app/generated_resources.grd:9208: +        Fingerprint: <ph
name="FINGERPRINT">$1<ex>9FEE18CC4302340DDE234345345345</ex></ph>
This is going to be the hash of the SPKI in the certificate? So that people can
report it to us in case of MITM or the like? If so, I might change the label to
"Public key fingerprint" or "Key fingerprint" or something.

https://codereview.chromium.org/23965003/diff/5001/chrome/browser/ssl/ssl_blo...
File chrome/browser/ssl/ssl_blocking_page.cc (right):

https://codereview.chromium.org/23965003/diff/5001/chrome/browser/ssl/ssl_blo...
chrome/browser/ssl/ssl_blocking_page.cc:283: errorType =
string16(ASCIIToUTF16("HSTS failure"));
This string bothers me a tiny bit, because even when HSTS is not in use, pin
validation failures are also non-overridable. So "HSTS" isn't strictly correct.
Maybe "Strict authentication failure" or something? FWIW.

https://codereview.chromium.org/23965003/diff/5001/chrome/browser/ssl/ssl_blo...
chrome/browser/ssl/ssl_blocking_page.cc:309:
base::HexEncode(ssl_info_.cert->fingerprint().data,
Ahh, so answering my previous question: The fingerprint of the whole
certificate, not just the SPKI. I am not sure which is best; how do you hope
users will use the fingerprint?

[felt, 2013-09-06 17:37:46]

https://codereview.chromium.org/23965003/diff/5001/chrome/app/generated_resou...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/23965003/diff/5001/chrome/app/generated_resou...
chrome/app/generated_resources.grd:9170: +        To protect your privacy,
Chrome will not load the page until it can establish a secure connection to the
real <ph name="SITE">$1<ex>google.com</ex></ph>.
On 2013/09/06 17:01:33, Chromium Palmer wrote:
> Nit: Sadly, the style in this file is to have one hugely long line per
> paragraph. I'm not sure if you should adhere to it, but other reviewers might
> ask you to.

Alas, done.

https://codereview.chromium.org/23965003/diff/5001/chrome/browser/ssl/ssl_blo...
File chrome/browser/ssl/ssl_blocking_page.cc (right):

https://codereview.chromium.org/23965003/diff/5001/chrome/browser/ssl/ssl_blo...
chrome/browser/ssl/ssl_blocking_page.cc:283: errorType =
string16(ASCIIToUTF16("HSTS failure"));
On 2013/09/06 17:01:33, Chromium Palmer wrote:
> This string bothers me a tiny bit, because even when HSTS is not in use, pin
> validation failures are also non-overridable. So "HSTS" isn't strictly
correct.
> Maybe "Strict authentication failure" or something? FWIW.

At the present time, only HSTS can trigger this case as far as I can tell. When
I modify* cert pinning errors to go through this warning, the cases will be
split up to differentiate between the two.

*in a CL coming soon to a code review near you

https://codereview.chromium.org/23965003/diff/5001/chrome/browser/ssl/ssl_blo...
chrome/browser/ssl/ssl_blocking_page.cc:309:
base::HexEncode(ssl_info_.cert->fingerprint().data,
On 2013/09/06 17:01:33, Chromium Palmer wrote:
> Ahh, so answering my previous question: The fingerprint of the whole
> certificate, not just the SPKI. I am not sure which is best; how do you hope
> users will use the fingerprint?

Someone (either you or Sleevi) asked me to put the fingerprint so that people
could copy and paste it and share it in a help forum if they needed to. I
figured this would be the right one, but I am honestly not sure. Do you have an
opinion?

[palmer, 2013-09-06 17:59:30]

rsleevi: Any preferences as to certificate fingerprint vs. SPKI fingerprint?

[Ryan Sleevi, 2013-09-06 18:58:14]

If we're blocking because it's revoked or because of HPKP, then it would seem
the SPKI fingerprint is more valuable than the certificate fingerprint.

That said, I have no real preference either way. I think longer-term we'll want
to get to something the user can exfil (eg: via copy/paste) into a report, so I
wouldn't worry about it now.

Either way LGTM

[felt, 2013-09-06 19:42:02]

On 2013/09/06 18:58:14, Ryan Sleevi wrote:
> If we're blocking because it's revoked or because of HPKP, then it would seem
> the SPKI fingerprint is more valuable than the certificate fingerprint.
> 
> That said, I have no real preference either way. I think longer-term we'll
want
> to get to something the user can exfil (eg: via copy/paste) into a report, so
I
> wouldn't worry about it now.
> 
> Either way LGTM

This will expose the subject, issuer, and SPKI fingerprint. What else would be
needed to make a report useful? What's your wishlist?

[Ryan Sleevi, 2013-09-06 19:44:02]

On 2013/09/06 19:42:02, felt wrote:
> On 2013/09/06 18:58:14, Ryan Sleevi wrote:
> > If we're blocking because it's revoked or because of HPKP, then it would
seem
> > the SPKI fingerprint is more valuable than the certificate fingerprint.
> > 
> > That said, I have no real preference either way. I think longer-term we'll
> want
> > to get to something the user can exfil (eg: via copy/paste) into a report,
so
> I
> > wouldn't worry about it now.
> > 
> > Either way LGTM
> 
> This will expose the subject, issuer, and SPKI fingerprint. What else would be
> needed to make a report useful? What's your wishlist?

The cert/chain both presented by the server and verified, in an extractable
form.

[felt, 2013-09-06 19:44:46]

On 2013/09/06 19:44:02, Ryan Sleevi wrote:
> On 2013/09/06 19:42:02, felt wrote:
> > On 2013/09/06 18:58:14, Ryan Sleevi wrote:
> > > If we're blocking because it's revoked or because of HPKP, then it would
> seem
> > > the SPKI fingerprint is more valuable than the certificate fingerprint.
> > > 
> > > That said, I have no real preference either way. I think longer-term we'll
> > want
> > > to get to something the user can exfil (eg: via copy/paste) into a report,
> so
> > I
> > > wouldn't worry about it now.
> > > 
> > > Either way LGTM
> > 
> > This will expose the subject, issuer, and SPKI fingerprint. What else would
be
> > needed to make a report useful? What's your wishlist?
> 
> The cert/chain both presented by the server and verified, in an extractable
> form.

Cool, I'll look into adding that later.

[palmer, 2013-09-06 20:12:38]

> > The cert/chain both presented by the server and verified, in an extractable
> > form.
> 
> Cool, I'll look into adding that later.

Yeah, definitely doesn't need to be in this CL. Just PEMs of the certs in the
constructed chain. See the ChromeFraudulentCertificateReporter for factoids on
how to extract that.

[palmer, 2013-09-06 20:14:07]

> Yeah, definitely doesn't need to be in this CL. Just PEMs of the certs in the
> constructed chain. See the ChromeFraudulentCertificateReporter for factoids on
> how to extract that.

That might sound confusing; sorry. :) I did not mean that you need to get PEMs
of the certs in *this* CL, just that that is what we'll likely want in the
future CL.

[felt, 2013-09-09 14:34:12]

Made some small updates per Palmer's review.

https://codereview.chromium.org/23965003/diff/5001/chrome/app/generated_resou...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/23965003/diff/5001/chrome/app/generated_resou...
chrome/app/generated_resources.grd:9208: +        Fingerprint: <ph
name="FINGERPRINT">$1<ex>9FEE18CC4302340DDE234345345345</ex></ph>
On 2013/09/06 17:01:33, Chromium Palmer wrote:
> This is going to be the hash of the SPKI in the certificate? So that people
can
> report it to us in case of MITM or the like? If so, I might change the label
to
> "Public key fingerprint" or "Key fingerprint" or something.

Updated this.

https://codereview.chromium.org/23965003/diff/5001/chrome/browser/ssl/ssl_blo...
File chrome/browser/ssl/ssl_blocking_page.cc (right):

https://codereview.chromium.org/23965003/diff/5001/chrome/browser/ssl/ssl_blo...
chrome/browser/ssl/ssl_blocking_page.cc:283: errorType =
string16(ASCIIToUTF16("HSTS failure"));
On 2013/09/06 17:37:47, felt wrote:
> On 2013/09/06 17:01:33, Chromium Palmer wrote:
> > This string bothers me a tiny bit, because even when HSTS is not in use, pin
> > validation failures are also non-overridable. So "HSTS" isn't strictly
> correct.
> > Maybe "Strict authentication failure" or something? FWIW.
> 
> At the present time, only HSTS can trigger this case as far as I can tell.
When
> I modify* cert pinning errors to go through this warning, the cases will be
> split up to differentiate between the two.
> 
> *in a CL coming soon to a code review near you

However, I did add the strings to the grd file so you can see them if you want.

https://codereview.chromium.org/23965003/diff/5001/chrome/browser/ssl/ssl_blo...
chrome/browser/ssl/ssl_blocking_page.cc:309:
base::HexEncode(ssl_info_.cert->fingerprint().data,
On 2013/09/06 17:37:47, felt wrote:
> On 2013/09/06 17:01:33, Chromium Palmer wrote:
> > Ahh, so answering my previous question: The fingerprint of the whole
> > certificate, not just the SPKI. I am not sure which is best; how do you hope
> > users will use the fingerprint?
> 
> Someone (either you or Sleevi) asked me to put the fingerprint so that people
> could copy and paste it and share it in a help forum if they needed to. I
> figured this would be the right one, but I am honestly not sure. Do you have
an
> opinion?

Updated this to SPKI hashes.

[felt, 2013-09-11 01:12:34]

hi jhawkins-- friendly ping.

[felt, 2013-09-12 12:33:44]

Hi Patrick, can you review the chrome/browser/resources/ part of this CL?

Thanks,
Adrienne

[Patrick Dubroy, 2013-09-12 13:12:25]

It seems like there's an opportunity for a lot more shared code here.

https://codereview.chromium.org/23965003/diff/28001/chrome/browser/resources/...
File chrome/browser/resources/ssl/blocking.css (right):

https://codereview.chromium.org/23965003/diff/28001/chrome/browser/resources/...
chrome/browser/resources/ssl/blocking.css:34: line-height: 18px;
This code seems like it should be shared with other error pages as well.

https://codereview.chromium.org/23965003/diff/28001/chrome/browser/resources/...
File chrome/browser/resources/ssl/blocking.html (right):

https://codereview.chromium.org/23965003/diff/28001/chrome/browser/resources/...
chrome/browser/resources/ssl/blocking.html:13: <body id="t">
Since this looks almost identical to neterror.html, can we use the same layout
for both?

https://codereview.chromium.org/23965003/diff/28001/chrome/browser/resources/...
chrome/browser/resources/ssl/blocking.html:26: <div class="explanation-par">
Is there a reason you can't have a single element here, and just embed the <p>
tags into the resource itself?

https://codereview.chromium.org/23965003/diff/28001/chrome/browser/resources/...
File chrome/browser/resources/ssl/blocking.js (right):

https://codereview.chromium.org/23965003/diff/28001/chrome/browser/resources/...
chrome/browser/resources/ssl/blocking.js:16: // This allows errors to be
skippped by typing "proceed" into the page.
This looks like the exact same code as in roadblock.html. Why isn't it shared
between the two files?

[felt, 2013-09-16 00:05:21]

Hi Patrick,

I've refactored to share more code. There are still some parts that had to
remain separate, but if you have ideas about how to get rid of the few lines of
duplicated HTML please let me know.

Thanks,
Adrienne

https://codereview.chromium.org/23965003/diff/28001/chrome/browser/resources/...
File chrome/browser/resources/ssl/blocking.css (right):

https://codereview.chromium.org/23965003/diff/28001/chrome/browser/resources/...
chrome/browser/resources/ssl/blocking.css:34: line-height: 18px;
On 2013/09/12 13:12:26, dubroy wrote:
> This code seems like it should be shared with other error pages as well.

The other error pages don't have any text in this area, so they don't need this
part of the structure or layout.

https://codereview.chromium.org/23965003/diff/28001/chrome/browser/resources/...
File chrome/browser/resources/ssl/blocking.html (right):

https://codereview.chromium.org/23965003/diff/28001/chrome/browser/resources/...
chrome/browser/resources/ssl/blocking.html:13: <body id="t">
On 2013/09/12 13:12:26, dubroy wrote:
> Since this looks almost identical to neterror.html, can we use the same layout
> for both?

They differ several ways:

- This version needs different JavaScript, since it supports additional
functionality (like communicating back to the parent interstitial).
- Although the outermost elements are the same, it has different internals
structure because this version has substantially more text. 
- It also needs custom CSS because this version has larger margins.

I could potentially share some of the layout with hidden elements and then apply
different CSS style sheets to them, but I think it would end up fairly complex.

However, I've done my best to reuse CSS and JS as much as possible in this
refactor.

https://codereview.chromium.org/23965003/diff/28001/chrome/browser/resources/...
chrome/browser/resources/ssl/blocking.html:26: <div class="explanation-par">
On 2013/09/12 13:12:26, dubroy wrote:
> Is there a reason you can't have a single element here, and just embed the <p>
> tags into the resource itself?

Done.

https://codereview.chromium.org/23965003/diff/28001/chrome/browser/resources/...
File chrome/browser/resources/ssl/blocking.js (right):

https://codereview.chromium.org/23965003/diff/28001/chrome/browser/resources/...
chrome/browser/resources/ssl/blocking.js:16: // This allows errors to be
skippped by typing "proceed" into the page.
On 2013/09/12 13:12:26, dubroy wrote:
> This looks like the exact same code as in roadblock.html. Why isn't it shared
> between the two files?

Done.

[Patrick Dubroy, 2013-09-18 15:01:34]

https://codereview.chromium.org/23965003/diff/28001/chrome/browser/resources/...
File chrome/browser/resources/ssl/blocking.css (right):

https://codereview.chromium.org/23965003/diff/28001/chrome/browser/resources/...
chrome/browser/resources/ssl/blocking.css:34: line-height: 18px;
On 2013/09/16 00:05:21, felt wrote:
> On 2013/09/12 13:12:26, dubroy wrote:
> > This code seems like it should be shared with other error pages as well.
> 
> The other error pages don't have any text in this area, so they don't need
this
> part of the structure or layout.

Unless I'm mistaken, you shouldn't even need the rule for .ssl-help-box-inner
here, because it is already inside #help-box-inner, so it should inherit the
line-height.

I think what you probably want to do is use <p> for all of the explanatory
paragraphs on the page. Then you can have a style in neterror.css that styles
the line-height and margins of those.

https://codereview.chromium.org/23965003/diff/28001/chrome/browser/resources/...
File chrome/browser/resources/ssl/blocking.html (right):

https://codereview.chromium.org/23965003/diff/28001/chrome/browser/resources/...
chrome/browser/resources/ssl/blocking.html:13: <body id="t">
On 2013/09/16 00:05:21, felt wrote:
> On 2013/09/12 13:12:26, dubroy wrote:
> > Since this looks almost identical to neterror.html, can we use the same
layout
> > for both?
> 
> They differ several ways:
> 
> - This version needs different JavaScript, since it supports additional
> functionality (like communicating back to the parent interstitial).
> - Although the outermost elements are the same, it has different internals
> structure because this version has substantially more text. 
> - It also needs custom CSS because this version has larger margins.

Did someone on the UX team specifically say they should have different margins?
That seems odd to me. I would think they should be consistent.
 
> I could potentially share some of the layout with hidden elements and then
apply
> different CSS style sheets to them, but I think it would end up fairly
complex.
> 
> However, I've done my best to reuse CSS and JS as much as possible in this
> refactor.

https://codereview.chromium.org/23965003/diff/39001/chrome/browser/resources/...
File chrome/browser/resources/ssl/blocking.css (right):

https://codereview.chromium.org/23965003/diff/39001/chrome/browser/resources/...
chrome/browser/resources/ssl/blocking.css:6: color: #444;
This should probably be a more general rule that sets the default text style. It
shouldn't have to be done on a paragraph-by-paragraph basis.

https://codereview.chromium.org/23965003/diff/39001/chrome/browser/resources/...
chrome/browser/resources/ssl/blocking.css:39: padding: 0 20px 20px 20px;
This is exactly the same as the default padding that's at the top of the page.

https://codereview.chromium.org/23965003/diff/39001/chrome/browser/resources/...
chrome/browser/resources/ssl/blocking.css:42: padding: 0 20px;
This is also the same as the default padding.

https://codereview.chromium.org/23965003/diff/39001/chrome/browser/resources/...
chrome/browser/resources/ssl/blocking.css:52: padding: 0 20px;
Same again.

https://codereview.chromium.org/23965003/diff/39001/chrome/browser/resources/...
chrome/browser/resources/ssl/blocking.css:56: .outer-explanation-par {
Where is the class even used?

https://codereview.chromium.org/23965003/diff/39001/chrome/browser/resources/...
chrome/browser/resources/ssl/blocking.css:62: padding: 0 20px;
Won't this cause an *additional* 20px padding on the left and right, since
#help-box-inner already has 20px padding?

https://codereview.chromium.org/23965003/diff/39001/chrome/browser/resources/...
File chrome/browser/resources/ssl/blocking.html (right):

https://codereview.chromium.org/23965003/diff/39001/chrome/browser/resources/...
chrome/browser/resources/ssl/blocking.html:11: <script src="shared.js"></script>
Can you give this a slightly more descriptive name?

https://codereview.chromium.org/23965003/diff/39001/chrome/browser/resources/...
chrome/browser/resources/ssl/blocking.html:24: <div class="explanation-par">
Looks like this should be a <p>.

https://codereview.chromium.org/23965003/diff/39001/chrome/browser/resources/...
chrome/browser/resources/ssl/blocking.html:33: <h2><span
i18n-content="moreTitle"></span></h2>
I think you can get rid of the <span> here, and just put i18n-content on the h2.

https://codereview.chromium.org/23965003/diff/39001/chrome/browser/resources/...
chrome/browser/resources/ssl/blocking.html:34: <span
i18n-values=".innerHTML:moreMessage"></p>
Mismatched opening and closing tags. I think you can just use <p>.

https://codereview.chromium.org/23965003/diff/39001/chrome/browser/resources/...
chrome/browser/resources/ssl/blocking.html:39: <span
i18n-content="errorType"></span><br>
Why are these <span>, and not <p>?

https://codereview.chromium.org/23965003/diff/39001/chrome/browser/ssl/ssl_bl...
File chrome/browser/ssl/ssl_blocking_page.cc (right):

https://codereview.chromium.org/23965003/diff/39001/chrome/browser/ssl/ssl_bl...
chrome/browser/ssl/ssl_blocking_page.cc:265: LOG(INFO) <<
l10n_util::GetStringFUTF16(IDS_SSL_BLOCKING_PAGE_BODY_TEXT,
Don't think you need this.

[felt, 2013-10-18 05:53:11]

https://codereview.chromium.org/23965003/diff/28001/chrome/browser/resources/...
File chrome/browser/resources/ssl/blocking.html (right):

https://codereview.chromium.org/23965003/diff/28001/chrome/browser/resources/...
chrome/browser/resources/ssl/blocking.html:13: <body id="t">
On 2013/09/18 15:01:35, Patrick Dubroy wrote:
> On 2013/09/16 00:05:21, felt wrote:
> > On 2013/09/12 13:12:26, dubroy wrote:
> > > Since this looks almost identical to neterror.html, can we use the same
> layout
> > > for both?
> > 
> > They differ several ways:
> > 
> > - This version needs different JavaScript, since it supports additional
> > functionality (like communicating back to the parent interstitial).
> > - Although the outermost elements are the same, it has different internals
> > structure because this version has substantially more text. 
> > - It also needs custom CSS because this version has larger margins.
> 
> Did someone on the UX team specifically say they should have different
margins?
> That seems odd to me. I would think they should be consistent.

Yes. First, this warning has a larger icon (which calls for more padding around
it). Second, this warning has substantially more text (2 paragraphs in one spot
instead of 1 sentence fragment; 4-5 paragraphs below the fold instead of 0 text
or 2 unimportant sentences). The increased amount of text calls for larger
margins to make it more readable. It looks squashed without the extra padding.

>  
> > I could potentially share some of the layout with hidden elements and then
> apply
> > different CSS style sheets to them, but I think it would end up fairly
> complex.
> > 
> > However, I've done my best to reuse CSS and JS as much as possible in this
> > refactor.
>

https://codereview.chromium.org/23965003/diff/39001/chrome/browser/resources/...
File chrome/browser/resources/ssl/blocking.css (right):

https://codereview.chromium.org/23965003/diff/39001/chrome/browser/resources/...
chrome/browser/resources/ssl/blocking.css:6: color: #444;
On 2013/09/18 15:01:35, Patrick Dubroy wrote:
> This should probably be a more general rule that sets the default text style.
It
> shouldn't have to be done on a paragraph-by-paragraph basis.

This rule is needed to override the styling set in neterror.css that makes this
area of the error centered & a darker color. Since the other paragraphs in the
ssl error don't need to override that styling, this ends up only being used for
this paragraph.

https://codereview.chromium.org/23965003/diff/39001/chrome/browser/resources/...
chrome/browser/resources/ssl/blocking.css:39: padding: 0 20px 20px 20px;
On 2013/09/18 15:01:35, Patrick Dubroy wrote:
> This is exactly the same as the default padding that's at the top of the page.

That was a mistake, decreased.

https://codereview.chromium.org/23965003/diff/39001/chrome/browser/resources/...
chrome/browser/resources/ssl/blocking.css:42: padding: 0 20px;
On 2013/09/18 15:01:35, Patrick Dubroy wrote:
> This is also the same as the default padding.

That was a mistake, decreased.

https://codereview.chromium.org/23965003/diff/39001/chrome/browser/resources/...
chrome/browser/resources/ssl/blocking.css:52: padding: 0 20px;
On 2013/09/18 15:01:35, Patrick Dubroy wrote:
> Same again.

That was a mistake, decreased.

https://codereview.chromium.org/23965003/diff/39001/chrome/browser/resources/...
chrome/browser/resources/ssl/blocking.css:56: .outer-explanation-par {
On 2013/09/18 15:01:35, Patrick Dubroy wrote:
> Where is the class even used?

Good catch, removed.

https://codereview.chromium.org/23965003/diff/39001/chrome/browser/resources/...
chrome/browser/resources/ssl/blocking.css:62: padding: 0 20px;
On 2013/09/18 15:01:35, Patrick Dubroy wrote:
> Won't this cause an *additional* 20px padding on the left and right, since
> #help-box-inner already has 20px padding?

Yes, that's exactly why I added ssl-help-box-inner in addition to
help-box-inner. This error page needs more padding because it has more text.

https://codereview.chromium.org/23965003/diff/39001/chrome/browser/resources/...
File chrome/browser/resources/ssl/blocking.html (right):

https://codereview.chromium.org/23965003/diff/39001/chrome/browser/resources/...
chrome/browser/resources/ssl/blocking.html:11: <script src="shared.js"></script>
On 2013/09/18 15:01:35, Patrick Dubroy wrote:
> Can you give this a slightly more descriptive name?

Done.

https://codereview.chromium.org/23965003/diff/39001/chrome/browser/resources/...
chrome/browser/resources/ssl/blocking.html:24: <div class="explanation-par">
On 2013/09/18 15:01:35, Patrick Dubroy wrote:
> Looks like this should be a <p>.

Done.

https://codereview.chromium.org/23965003/diff/39001/chrome/browser/resources/...
chrome/browser/resources/ssl/blocking.html:33: <h2><span
i18n-content="moreTitle"></span></h2>
On 2013/09/18 15:01:35, Patrick Dubroy wrote:
> I think you can get rid of the <span> here, and just put i18n-content on the
h2.

Done.

https://codereview.chromium.org/23965003/diff/39001/chrome/browser/resources/...
chrome/browser/resources/ssl/blocking.html:34: <span
i18n-values=".innerHTML:moreMessage"></p>
On 2013/09/18 15:01:35, Patrick Dubroy wrote:
> Mismatched opening and closing tags. I think you can just use <p>.

Done.

https://codereview.chromium.org/23965003/diff/39001/chrome/browser/resources/...
chrome/browser/resources/ssl/blocking.html:39: <span
i18n-content="errorType"></span><br>
On 2013/09/18 15:01:35, Patrick Dubroy wrote:
> Why are these <span>, and not <p>?

The <p> adds too much whitespace between them. I want them to be on newlines,
but without the extra spacing. They are meant to be viewed as a single text
block with some formatting.

https://codereview.chromium.org/23965003/diff/39001/chrome/browser/ssl/ssl_bl...
File chrome/browser/ssl/ssl_blocking_page.cc (right):

https://codereview.chromium.org/23965003/diff/39001/chrome/browser/ssl/ssl_bl...
chrome/browser/ssl/ssl_blocking_page.cc:265: LOG(INFO) <<
l10n_util::GetStringFUTF16(IDS_SSL_BLOCKING_PAGE_BODY_TEXT,
On 2013/09/18 15:01:35, Patrick Dubroy wrote:
> Don't think you need this.

Done.

[Patrick Dubroy, 2013-10-21 07:48:50]

This is looking much better. Just a few more comments.

https://codereview.chromium.org/23965003/diff/39001/chrome/browser/resources/...
File chrome/browser/resources/ssl/blocking.css (right):

https://codereview.chromium.org/23965003/diff/39001/chrome/browser/resources/...
chrome/browser/resources/ssl/blocking.css:6: color: #444;
On 2013/10/18 05:53:12, felt wrote:
> On 2013/09/18 15:01:35, Patrick Dubroy wrote:
> > This should probably be a more general rule that sets the default text
style.
> It
> > shouldn't have to be done on a paragraph-by-paragraph basis.
> 
> This rule is needed to override the styling set in neterror.css that makes
this
> area of the error centered & a darker color. Since the other paragraphs in the
> ssl error don't need to override that styling, this ends up only being used
for
> this paragraph.

Almost all the text on the page is #444 and align: left. I think it would make
more sense to change the rules in neterror.css to be something like :

#content-top h1 {
  color: #666;
  text-align: center;
}

That way if we someday have an error page that has two explanation paragraphs,
we don't end up having to add more manual rules to get the right styling.

https://codereview.chromium.org/23965003/diff/39001/chrome/browser/resources/...
File chrome/browser/resources/ssl/blocking.html (right):

https://codereview.chromium.org/23965003/diff/39001/chrome/browser/resources/...
chrome/browser/resources/ssl/blocking.html:39: <span
i18n-content="errorType"></span><br>
On 2013/10/18 05:53:12, felt wrote:
> On 2013/09/18 15:01:35, Patrick Dubroy wrote:
> > Why are these <span>, and not <p>?
> 
> The <p> adds too much whitespace between them. I want them to be on newlines,
> but without the extra spacing. They are meant to be viewed as a single text
> block with some formatting.

I see.

https://codereview.chromium.org/23965003/diff/104001/chrome/browser/resources...
File chrome/browser/resources/ssl/blocking.css (right):

https://codereview.chromium.org/23965003/diff/104001/chrome/browser/resources...
chrome/browser/resources/ssl/blocking.css:11: h2 {
Any reason not to push this into neterror.css, so that other pages that we might
introduce in the future could benefit from it?

https://codereview.chromium.org/23965003/diff/104001/chrome/browser/resources...
chrome/browser/resources/ssl/blocking.css:27: @media (min-width: 641px) and
(min-height: 641px) {
Are you sure you need this media query? This is the default, and the others, if
they apply, will override the default rules. So I think you can just drop the
whole "@media ..." part.

https://codereview.chromium.org/23965003/diff/104001/chrome/browser/resources...
chrome/browser/resources/ssl/blocking.css:31: .ssl-help-box-inner {
I'd collapse this into the rule above for brevity.

https://codereview.chromium.org/23965003/diff/104001/chrome/browser/resources...
chrome/browser/resources/ssl/blocking.css:41: .ssl-help-box-inner {
Same, collapse into one.

https://codereview.chromium.org/23965003/diff/104001/chrome/browser/resources...
chrome/browser/resources/ssl/blocking.css:51: .ssl-help-box-inner {
Same, collapse into one.

https://codereview.chromium.org/23965003/diff/104001/chrome/browser/resources...
File chrome/browser/resources/ssl/blocking.html (right):

https://codereview.chromium.org/23965003/diff/104001/chrome/browser/resources...
chrome/browser/resources/ssl/blocking.html:14: <body id="t">
Uh, is this actually used anywhere? I see you copied it from neterror.html, but
it doesn't seem to be used, and normally we'd avoid one-letter variable names.

https://codereview.chromium.org/23965003/diff/104001/chrome/browser/resources...
File chrome/browser/resources/ssl/blocking.js (right):

https://codereview.chromium.org/23965003/diff/104001/chrome/browser/resources...
chrome/browser/resources/ssl/blocking.js:5: function toggleMoreBox() {
Can't this also be shared with neterror.js? And do you need to share
swapButtonOrder() as well?

https://codereview.chromium.org/23965003/diff/104001/chrome/browser/resources...
File chrome/browser/resources/ssl/roadblock.html (right):

https://codereview.chromium.org/23965003/diff/104001/chrome/browser/resources...
chrome/browser/resources/ssl/roadblock.html:81: <script
src="allsslerrors.js"></script>
This should be named all_ssl_errors.js to match our naming standards. Though I'd
suggest ssl_errors_common.js as a slightly better name.

[felt, 2013-10-21 13:08:33]

Hi Patrick, PTAL, made the changes as requested.

https://codereview.chromium.org/23965003/diff/39001/chrome/browser/resources/...
File chrome/browser/resources/ssl/blocking.css (right):

https://codereview.chromium.org/23965003/diff/39001/chrome/browser/resources/...
chrome/browser/resources/ssl/blocking.css:6: color: #444;
On 2013/10/21 07:48:50, Patrick Dubroy wrote:
> On 2013/10/18 05:53:12, felt wrote:
> > On 2013/09/18 15:01:35, Patrick Dubroy wrote:
> > > This should probably be a more general rule that sets the default text
> style.
> > It
> > > shouldn't have to be done on a paragraph-by-paragraph basis.
> > 
> > This rule is needed to override the styling set in neterror.css that makes
> this
> > area of the error centered & a darker color. Since the other paragraphs in
the
> > ssl error don't need to override that styling, this ends up only being used
> for
> > this paragraph.
> 
> Almost all the text on the page is #444 and align: left. I think it would make
> more sense to change the rules in neterror.css to be something like :
> 
> #content-top h1 {
>   color: #666;
>   text-align: center;
> }
> 
> That way if we someday have an error page that has two explanation paragraphs,
> we don't end up having to add more manual rules to get the right styling.

Done, although note I had to put the buttons in a new <div id='buttons'> to make
the buttons be centered too.

https://codereview.chromium.org/23965003/diff/104001/chrome/browser/resources...
File chrome/browser/resources/ssl/blocking.css (right):

https://codereview.chromium.org/23965003/diff/104001/chrome/browser/resources...
chrome/browser/resources/ssl/blocking.css:11: h2 {
On 2013/10/21 07:48:51, Patrick Dubroy wrote:
> Any reason not to push this into neterror.css, so that other pages that we
might
> introduce in the future could benefit from it?

Done.

https://codereview.chromium.org/23965003/diff/104001/chrome/browser/resources...
chrome/browser/resources/ssl/blocking.css:27: @media (min-width: 641px) and
(min-height: 641px) {
On 2013/10/21 07:48:51, Patrick Dubroy wrote:
> Are you sure you need this media query? This is the default, and the others,
if
> they apply, will override the default rules. So I think you can just drop the
> whole "@media ..." part.

Done.

https://codereview.chromium.org/23965003/diff/104001/chrome/browser/resources...
chrome/browser/resources/ssl/blocking.css:31: .ssl-help-box-inner {
On 2013/10/21 07:48:51, Patrick Dubroy wrote:
> I'd collapse this into the rule above for brevity.

Done.

https://codereview.chromium.org/23965003/diff/104001/chrome/browser/resources...
chrome/browser/resources/ssl/blocking.css:41: .ssl-help-box-inner {
On 2013/10/21 07:48:51, Patrick Dubroy wrote:
> Same, collapse into one.

Done.

https://codereview.chromium.org/23965003/diff/104001/chrome/browser/resources...
chrome/browser/resources/ssl/blocking.css:51: .ssl-help-box-inner {
On 2013/10/21 07:48:51, Patrick Dubroy wrote:
> Same, collapse into one.

Done.

https://codereview.chromium.org/23965003/diff/104001/chrome/browser/resources...
File chrome/browser/resources/ssl/blocking.html (right):

https://codereview.chromium.org/23965003/diff/104001/chrome/browser/resources...
chrome/browser/resources/ssl/blocking.html:14: <body id="t">
On 2013/10/21 07:48:51, Patrick Dubroy wrote:
> Uh, is this actually used anywhere? I see you copied it from neterror.html,
but
> it doesn't seem to be used, and normally we'd avoid one-letter variable names.

Done.

https://codereview.chromium.org/23965003/diff/104001/chrome/browser/resources...
File chrome/browser/resources/ssl/blocking.js (right):

https://codereview.chromium.org/23965003/diff/104001/chrome/browser/resources...
chrome/browser/resources/ssl/blocking.js:5: function toggleMoreBox() {
On 2013/10/21 07:48:51, Patrick Dubroy wrote:
> Can't this also be shared with neterror.js? 

neterror's version of toggleMoreBox relies on this:

<button jsdisplay="more" jsvalues=".moreText:more; .lessText:less;"
jscontent="more">

instead of templatedata. I was unable to get this to work in the ssl version
even when I had copied the code directly. My working hypothesis is that this is
because WebFrame::LoadHTMLString (which is used to load the neterror resources)
does some magic, but perhaps you know better.

> And do you need to share swapButtonOrder() as well?

swapButtonOrder is already shared fine, it works across both versions.

https://codereview.chromium.org/23965003/diff/104001/chrome/browser/resources...
File chrome/browser/resources/ssl/roadblock.html (right):

https://codereview.chromium.org/23965003/diff/104001/chrome/browser/resources...
chrome/browser/resources/ssl/roadblock.html:81: <script
src="allsslerrors.js"></script>
On 2013/10/21 07:48:51, Patrick Dubroy wrote:
> This should be named all_ssl_errors.js to match our naming standards. Though
I'd
> suggest ssl_errors_common.js as a slightly better name.

Done.

[Patrick Dubroy, 2013-10-21 13:32:56]

The images should not be named _100.png and _200.png, but rather can you make a
new directory called "images", with subdirs 1x and 2x, and put the images in
there?

Other than that (and the nit listed below), LGTM.

https://codereview.chromium.org/23965003/diff/274001/chrome/browser/resources...
File chrome/browser/resources/ssl/blocking.css (right):

https://codereview.chromium.org/23965003/diff/274001/chrome/browser/resources...
chrome/browser/resources/ssl/blocking.css:19: @media (max-width: 640px) {
I missed this before, but you can actually combine the two media queries into
one:

    @media (max-width: 640px), (max-height: 640px) {
      ...
    }

[felt, 2013-10-21 14:56:56]

For posterity, images landed separately:
https://codereview.chromium.org/30663006

https://codereview.chromium.org/23965003/diff/274001/chrome/browser/resources...
File chrome/browser/resources/ssl/blocking.css (right):

https://codereview.chromium.org/23965003/diff/274001/chrome/browser/resources...
chrome/browser/resources/ssl/blocking.css:19: @media (max-width: 640px) {
On 2013/10/21 13:32:56, Patrick Dubroy wrote:
> I missed this before, but you can actually combine the two media queries into
> one:
> 
>     @media (max-width: 640px), (max-height: 640px) {
>       ...
>     }

Done.

[felt, 2013-10-21 15:49:31]

jochen@ - TBRing you for the chrome/renderer/resources/ files, since Patrick
Dubroy has already reviewed the CSS and HTML files.

[felt, 2013-10-21 15:51:04]

Committed patchset #11 manually as r229818 (presubmit successful).