Add quiet safe browsing interstitial for WebView

Add quiet safe browsing interstitial for WebView

BUG=716095
Review-Url: https://codereview.chromium.org/2854263003
Cr-Commit-Position: refs/heads/master@{#473150}
Committed: https://chromium.googlesource.com/chromium/src/+/f9dfdb6d9dcd8c5f987fd7114bff3ebee6b8785f

[edwardjung, 2017-05-03 02:26:19]

edwardjung@chromium.org changed reviewers:
+ felt@chromium.org

[edwardjung, 2017-05-03 02:26:20]

Here's an initial implementation of the quiet interstitial for your input.

I have the details section implemented, but I wonder if I should remove it
entirely. At the moment it's just disabled as the details button strings are not
populated. 

I'm back on Thursday.

Thanks

Edward

[felt, 2017-05-05 04:17:33]

Yeah, it would make sense to pull out the details section I think. Do that in
another CL, when the button strings are available?

I'm generally uncomfortable adding untested code. What's your timeline / CL
breakdown plan for testing this? I suppose this is technically all dead code for
now but I want to make sure there are tests in place before this gets called
from anywhere.

Also I have to admit that I am not 100% comfortable reviewing HTML/CSS; I don't
mind for small changes, but you should get a better reviewer for a lot of new
code like this. Could you ask dbeam (or someone else who works on Web UI) to
also review?

[felt, 2017-05-05 04:35:01]

https://codereview.chromium.org/2854263003/diff/20001/components/security_int...
File
components/security_interstitials/core/browser/resources/interstitial_common.css
(right):

https://codereview.chromium.org/2854263003/diff/20001/components/security_int...
components/security_interstitials/core/browser/resources/interstitial_common.css:11:
color: #646464;
I think the preferred Chrome style is rgb for colors. However I know this is a
refactor and this was previously done with the hex #. (Dan would know.)

https://codereview.chromium.org/2854263003/diff/20001/components/security_int...
File
components/security_interstitials/core/browser/resources/interstitial_webview_quiet.css
(right):

https://codereview.chromium.org/2854263003/diff/20001/components/security_int...
components/security_interstitials/core/browser/resources/interstitial_webview_quiet.css:19:
background: #EDEDED;
nit: lowercase recommended if using hex

https://codereview.chromium.org/2854263003/diff/20001/components/security_int...
components/security_interstitials/core/browser/resources/interstitial_webview_quiet.css:21:
color: rgba(0,0,0,.54);
nit: should there be spaces here? (and elsewhere when specifying rgba)

https://codereview.chromium.org/2854263003/diff/20001/components/security_int...
File
components/security_interstitials/core/browser/resources/interstitial_webview_quiet.html
(right):

https://codereview.chromium.org/2854263003/diff/20001/components/security_int...
components/security_interstitials/core/browser/resources/interstitial_webview_quiet.html:11:
// <script src="interstitial_webview_quiet.js"></script>
Why the // ?

https://codereview.chromium.org/2854263003/diff/20001/components/security_int...
File
components/security_interstitials/core/browser/resources/interstitial_webview_quiet.js
(right):

https://codereview.chromium.org/2854263003/diff/20001/components/security_int...
components/security_interstitials/core/browser/resources/interstitial_webview_quiet.js:10:
// Should match security_interstitials::SecurityInterstitialCommands
Some of this is shared, can it go in a shared JS file?

https://codereview.chromium.org/2854263003/diff/20001/components/security_int...
components/security_interstitials/core/browser/resources/interstitial_webview_quiet.js:52:
var webViewSize = 'medium' // loadTimeData.getString('size');
guessing that's left in from testing?

https://codereview.chromium.org/2854263003/diff/20001/components/security_int...
components/security_interstitials/core/browser/resources/interstitial_webview_quiet.js:57:
if (webViewSize == 'medium') {
Hmm, so I was imagining that the HTML/CSS/JS will resize itself appropriately
instead of having the caller tell it which size it is. Do you think that's
possible?

https://codereview.chromium.org/2854263003/diff/20001/components/security_int...
components/security_interstitials/core/browser/resources/interstitial_webview_quiet.js:57:
if (webViewSize == 'medium') {
nit: return early instead of having a long if-statement.

if (webViewSize != 'medium')
  return;

[edwardjung, 2017-05-05 12:32:28]

> Yeah, it would make sense to pull out the details section I think. Do that in
> another CL, when the button strings are available?

I've got the strings but was still getting some design tweaks and a final icon. 
 
> I'm generally uncomfortable adding untested code. What's your timeline / CL
> breakdown plan for testing this? I suppose this is technically all dead code
for
> now but I want to make sure there are tests in place before this gets called
> from anywhere.
> 
Once the rename CL has been checked in, I can make the appropriate changes here
to the quiet class.
I'll look into getting tests done. Testing the different sizing we can test the
sizing.

> Also I have to admit that I am not 100% comfortable reviewing HTML/CSS; I
don't
> mind for small changes, but you should get a better reviewer for a lot of new
> code like this. Could you ask dbeam (or someone else who works on Web UI) to
> also review?

Sounds good, I'll clean this up first.

[edwardjung, 2017-05-08 16:22:29]

The CQ bit was checked by edwardjung@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-08 16:22:56]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[edwardjung, 2017-05-08 16:27:33]

PTAL made additional changes. The SafeBrowsingQuietErrorUI, now requires
specifying whether it's a giant size webview.

https://codereview.chromium.org/2854263003/diff/20001/components/security_int...
File
components/security_interstitials/core/browser/resources/interstitial_webview_quiet.css
(right):

https://codereview.chromium.org/2854263003/diff/20001/components/security_int...
components/security_interstitials/core/browser/resources/interstitial_webview_quiet.css:19:
background: #EDEDED;
On 2017/05/05 04:35:01, felt wrote:
> nit: lowercase recommended if using hex

Done.

https://codereview.chromium.org/2854263003/diff/20001/components/security_int...
components/security_interstitials/core/browser/resources/interstitial_webview_quiet.css:21:
color: rgba(0,0,0,.54);
On 2017/05/05 04:35:01, felt wrote:
> nit: should there be spaces here? (and elsewhere when specifying rgba)

Done.

https://codereview.chromium.org/2854263003/diff/20001/components/security_int...
File
components/security_interstitials/core/browser/resources/interstitial_webview_quiet.html
(right):

https://codereview.chromium.org/2854263003/diff/20001/components/security_int...
components/security_interstitials/core/browser/resources/interstitial_webview_quiet.html:11:
// <script src="interstitial_webview_quiet.js"></script>
On 2017/05/05 04:35:01, felt wrote:
> Why the // ?

I'll remove the details to a new CL

https://codereview.chromium.org/2854263003/diff/20001/components/security_int...
File
components/security_interstitials/core/browser/resources/interstitial_webview_quiet.js
(right):

https://codereview.chromium.org/2854263003/diff/20001/components/security_int...
components/security_interstitials/core/browser/resources/interstitial_webview_quiet.js:10:
// Should match security_interstitials::SecurityInterstitialCommands
On 2017/05/05 04:35:01, felt wrote:
> Some of this is shared, can it go in a shared JS file?

Done.

https://codereview.chromium.org/2854263003/diff/20001/components/security_int...
components/security_interstitials/core/browser/resources/interstitial_webview_quiet.js:57:
if (webViewSize == 'medium') {
On 2017/05/05 04:35:01, felt wrote:
> Hmm, so I was imagining that the HTML/CSS/JS will resize itself appropriately
> instead of having the caller tell it which size it is. Do you think that's
> possible?

I can determine the small / medium size in the JS but for will need some
indicator when the size is giant which extends outside the size of the frame. I
don't think it's possible to determine this from the client side.

[edwardjung, 2017-05-08 16:29:23]

edwardjung@chromium.org changed reviewers:
+ dbeam@chromium.org

[edwardjung, 2017-05-08 16:29:24]

@dbeam could you take a look at the web UI parts of this CL on behalf of felt:

components/neterror/resources/neterror.html
components/security_interstitials/core/browser/resources/*

Thanks

[commit-bot: I haz the power, 2017-05-08 16:45:41]

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

[commit-bot: I haz the power, 2017-05-08 16:45:42]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[Dan Beam, 2017-05-08 17:58:04]

https://codereview.chromium.org/2854263003/diff/80001/components/security_int...
File
components/security_interstitials/core/browser/resources/interstitial_common.css
(right):

https://codereview.chromium.org/2854263003/diff/80001/components/security_int...
components/security_interstitials/core/browser/resources/interstitial_common.css:3:
found in the LICENSE file. */
nit:

/* Copyright 2017 The Chromium Authors. All rights reserved.
 * Use of this source code is governed by a BSD-style license that can be
 * found in the LICENSE file. */

https://codereview.chromium.org/2854263003/diff/80001/components/security_int...
components/security_interstitials/core/browser/resources/interstitial_common.css:10:
background-color: rgb(247,247,247);
nit: spaces between numbers

https://codereview.chromium.org/2854263003/diff/80001/components/security_int...
File
components/security_interstitials/core/browser/resources/interstitial_common.js
(right):

https://codereview.chromium.org/2854263003/diff/80001/components/security_int...
components/security_interstitials/core/browser/resources/interstitial_common.js:28:
var CMD_REPORT_PHISHING_ERROR = 12;
nit: maybe make this an @enum {number}?

https://codereview.chromium.org/2854263003/diff/80001/components/security_int...
File
components/security_interstitials/core/browser/resources/interstitial_webview_quiet.css
(right):

https://codereview.chromium.org/2854263003/diff/80001/components/security_int...
components/security_interstitials/core/browser/resources/interstitial_webview_quiet.css:7:
font-family: Roboto-Regular;
can we use text_defaults_md.css instead to set the font?

https://codereview.chromium.org/2854263003/diff/80001/components/security_int...
components/security_interstitials/core/browser/resources/interstitial_webview_quiet.css:103:
/* Views that don't fit the details text */
nit: end with .

https://codereview.chromium.org/2854263003/diff/80001/components/security_int...
File
components/security_interstitials/core/browser/resources/interstitial_webview_quiet.html
(right):

https://codereview.chromium.org/2854263003/diff/80001/components/security_int...
components/security_interstitials/core/browser/resources/interstitial_webview_quiet.html:2:
<html i18n-values="dir:textdirection;lang:language">
can we use dir="$i18n{textdirection}" lang="$i18n{language}" and $i18n{} for all
other translations?

translating with JavaScript (using i18n-values or i18n-content) is not preferred

https://codereview.chromium.org/2854263003/diff/80001/components/security_int...
components/security_interstitials/core/browser/resources/interstitial_webview_quiet.html:18:
<span i18n-content="heading"></span>
<span>$i18n{heading}</span>

https://codereview.chromium.org/2854263003/diff/80001/components/security_int...
components/security_interstitials/core/browser/resources/interstitial_webview_quiet.html:24:
<p i18n-values=".innerHTML:explanationParagraph"></p>
why does this need to be innerHTML?  it is insecure and can run scripts and lead
to security problems (which would be ironic on this page)

https://codereview.chromium.org/2854263003/diff/80001/components/security_int...
File components/security_interstitials/core/safe_browsing_quiet_error_ui.cc
(right):

https://codereview.chromium.org/2854263003/diff/80001/components/security_int...
components/security_interstitials/core/safe_browsing_quiet_error_ui.cc:38: if
(display_options_.is_proceed_anyway_disabled)
curlies

https://codereview.chromium.org/2854263003/diff/80001/components/security_int...
components/security_interstitials/core/safe_browsing_quiet_error_ui.cc:47: void
SafeBrowsingQuietErrorUI::PopulateStringsForHTML(
nit: ForHtml

https://codereview.chromium.org/2854263003/diff/80001/components/security_int...
components/security_interstitials/core/safe_browsing_quiet_error_ui.cc:57: 
bool phishing = interstitial_reason_ == SafeBrowsingErrorUI::SB_REASON_PHISHING;
load_time_data->SetBoolean("phishing", phishing);

int explanation_ids = -1;
if (phishing)
  explanation_ids = IDS_PHISHING_WEBVIEW_EXPLANATION_PARAGRAPH;
else if (interstitial_reason_ == SafeBrowsingErrorUI::SB_REASON_MALWARE)
  explanation_ids = IDS_MALWARE_WEBVIEW_EXPLANATION_PARAGRAPH;

if (explanation_ids > -1) {
  load_time_data->SetString("explanationParagraph",
      l10n_util::GetStringUTF16(explanation_ids));
}

https://codereview.chromium.org/2854263003/diff/80001/components/security_int...
File components/security_interstitials/core/safe_browsing_quiet_error_ui.h
(right):

https://codereview.chromium.org/2854263003/diff/80001/components/security_int...
components/security_interstitials/core/safe_browsing_quiet_error_ui.h:48:
ControllerClient* controller_;
please document this relationship (who owns controller_?)

https://codereview.chromium.org/2854263003/diff/80001/components/security_int...
File components/security_interstitials_strings.grdp (right):

https://codereview.chromium.org/2854263003/diff/80001/components/security_int...
components/security_interstitials_strings.grdp:194: This content might try to
install dangerous software on your device that steals or deletes your
information. <ph name="BEGIN_LINK">&lt;a href="#" id="proceed-link"&gt;</ph>Show
anyway<ph name="END_LINK">&lt;/a&gt;</ph>.
does clicking this link navigate to #? does it have to?

where is this ID used?  can we remove it from the translation and use either
event delegation or dynamically assign the ID?

[edwardjung, 2017-05-08 23:21:10]

edwardjung@chromium.org changed reviewers:
+ jochen@chromium.org

[edwardjung, 2017-05-08 23:21:12]

Thanks Dan, PTAL. 

@jochen could you have a look at: 
  components/security_interstitials_strings.grdp

https://codereview.chromium.org/2854263003/diff/80001/components/security_int...
File
components/security_interstitials/core/browser/resources/interstitial_common.css
(right):

https://codereview.chromium.org/2854263003/diff/80001/components/security_int...
components/security_interstitials/core/browser/resources/interstitial_common.css:3:
found in the LICENSE file. */
On 2017/05/08 17:58:04, Dan Beam wrote:
> nit:
> 
> /* Copyright 2017 The Chromium Authors. All rights reserved.
>  * Use of this source code is governed by a BSD-style license that can be
>  * found in the LICENSE file. */

Done.

https://codereview.chromium.org/2854263003/diff/80001/components/security_int...
components/security_interstitials/core/browser/resources/interstitial_common.css:10:
background-color: rgb(247,247,247);
On 2017/05/08 17:58:04, Dan Beam wrote:
> nit: spaces between numbers

Done.

https://codereview.chromium.org/2854263003/diff/80001/components/security_int...
File
components/security_interstitials/core/browser/resources/interstitial_common.js
(right):

https://codereview.chromium.org/2854263003/diff/80001/components/security_int...
components/security_interstitials/core/browser/resources/interstitial_common.js:28:
var CMD_REPORT_PHISHING_ERROR = 12;
On 2017/05/08 17:58:04, Dan Beam wrote:
> nit: maybe make this an @enum {number}?

Done.

https://codereview.chromium.org/2854263003/diff/80001/components/security_int...
File
components/security_interstitials/core/browser/resources/interstitial_webview_quiet.css
(right):

https://codereview.chromium.org/2854263003/diff/80001/components/security_int...
components/security_interstitials/core/browser/resources/interstitial_webview_quiet.css:7:
font-family: Roboto-Regular;
On 2017/05/08 17:58:04, Dan Beam wrote:
> can we use text_defaults_md.css instead to set the font?

Done.

https://codereview.chromium.org/2854263003/diff/80001/components/security_int...
components/security_interstitials/core/browser/resources/interstitial_webview_quiet.css:103:
/* Views that don't fit the details text */
On 2017/05/08 17:58:04, Dan Beam wrote:
> nit: end with .

Done.

https://codereview.chromium.org/2854263003/diff/80001/components/security_int...
File
components/security_interstitials/core/browser/resources/interstitial_webview_quiet.html
(right):

https://codereview.chromium.org/2854263003/diff/80001/components/security_int...
components/security_interstitials/core/browser/resources/interstitial_webview_quiet.html:2:
<html i18n-values="dir:textdirection;lang:language">
On 2017/05/08 17:58:04, Dan Beam wrote:
> can we use dir="$i18n{textdirection}" lang="$i18n{language}" and $i18n{} for
all
> other translations?
> 
> translating with JavaScript (using i18n-values or i18n-content) is not
preferred

Nice suggestion. I didn't know about this. Much more readable.

https://codereview.chromium.org/2854263003/diff/80001/components/security_int...
components/security_interstitials/core/browser/resources/interstitial_webview_quiet.html:18:
<span i18n-content="heading"></span>
On 2017/05/08 17:58:04, Dan Beam wrote:
> <span>$i18n{heading}</span>

Done.

https://codereview.chromium.org/2854263003/diff/80001/components/security_int...
components/security_interstitials/core/browser/resources/interstitial_webview_quiet.html:24:
<p i18n-values=".innerHTML:explanationParagraph"></p>
On 2017/05/08 17:58:04, Dan Beam wrote:
> why does this need to be innerHTML?  it is insecure and can run scripts and
lead
> to security problems (which would be ironic on this page)

Understand that innerHTML is evil. However the strings contain anchor links
which aren't parsed with $i18n{}. I've split this into two strings.

Genuine question, is it realistically possible to inject bad scripts onto these
web Ui pages? I don't think it's possible to link to these pages directly from
external pages.

For another instance the interstitial_v2.html template it's used by four
different variants with different text some with links and formatting and others
with none. Is there a suggested solution for that? Also including the link in
with the rest of the explanation paragraph would be better for the translator to
write a cohesive paragraph as they can move the position of the link if it made
sense.

https://codereview.chromium.org/2854263003/diff/80001/components/security_int...
File components/security_interstitials/core/safe_browsing_quiet_error_ui.cc
(right):

https://codereview.chromium.org/2854263003/diff/80001/components/security_int...
components/security_interstitials/core/safe_browsing_quiet_error_ui.cc:38: if
(display_options_.is_proceed_anyway_disabled)
On 2017/05/08 17:58:04, Dan Beam wrote:
> curlies

Done.

https://codereview.chromium.org/2854263003/diff/80001/components/security_int...
components/security_interstitials/core/safe_browsing_quiet_error_ui.cc:47: void
SafeBrowsingQuietErrorUI::PopulateStringsForHTML(
On 2017/05/08 17:58:04, Dan Beam wrote:
> nit: ForHtml

Done.

https://codereview.chromium.org/2854263003/diff/80001/components/security_int...
components/security_interstitials/core/safe_browsing_quiet_error_ui.cc:57: 
On 2017/05/08 17:58:04, Dan Beam wrote:
> bool phishing = interstitial_reason_ ==
SafeBrowsingErrorUI::SB_REASON_PHISHING;
> load_time_data->SetBoolean("phishing", phishing);
> 
> int explanation_ids = -1;
> if (phishing)
>   explanation_ids = IDS_PHISHING_WEBVIEW_EXPLANATION_PARAGRAPH;
> else if (interstitial_reason_ == SafeBrowsingErrorUI::SB_REASON_MALWARE)
>   explanation_ids = IDS_MALWARE_WEBVIEW_EXPLANATION_PARAGRAPH;
> 
> if (explanation_ids > -1) {
>   load_time_data->SetString("explanationParagraph",
>       l10n_util::GetStringUTF16(explanation_ids));
> }

Done.

https://codereview.chromium.org/2854263003/diff/80001/components/security_int...
File components/security_interstitials/core/safe_browsing_quiet_error_ui.h
(right):

https://codereview.chromium.org/2854263003/diff/80001/components/security_int...
components/security_interstitials/core/safe_browsing_quiet_error_ui.h:48:
ControllerClient* controller_;
On 2017/05/08 17:58:04, Dan Beam wrote:
> please document this relationship (who owns controller_?)

Done.

https://codereview.chromium.org/2854263003/diff/80001/components/security_int...
File components/security_interstitials_strings.grdp (right):

https://codereview.chromium.org/2854263003/diff/80001/components/security_int...
components/security_interstitials_strings.grdp:194: This content might try to
install dangerous software on your device that steals or deletes your
information. <ph name="BEGIN_LINK">&lt;a href="#" id="proceed-link"&gt;</ph>Show
anyway<ph name="END_LINK">&lt;/a&gt;</ph>.
On 2017/05/08 17:58:04, Dan Beam wrote:
> does clicking this link navigate to #? does it have to?
> 
> where is this ID used?  can we remove it from the translation and use either
> event delegation or dynamically assign the ID?

It's used by JS in a follow up CL. I'll switch the way I code it.

[jochen (gone - plz use gerrit), 2017-05-09 16:22:25]

can you add a per-file owners rule to the grdp file with
/components/security_interstitials/OWNERS as owners?

[edwardjung, 2017-05-09 17:28:04]

On 2017/05/09 16:22:25, jochen wrote:
> can you add a per-file owners rule to the grdp file with
> /components/security_interstitials/OWNERS as owners?

Apologies, there's already a per-file line in the OWNERS for that file. Not sure
why you showed up in the presubmit then.

[edwardjung, 2017-05-09 17:28:17]

edwardjung@chromium.org changed reviewers:
- jochen@chromium.org

[edwardjung, 2017-05-09 19:29:56]

edwardjung@chromium.org changed reviewers:
+ jochen@chromium.org

[edwardjung, 2017-05-09 19:29:57]

> can you add a per-file owners rule to the grdp file with
/components/security_interstitials/OWNERS as owners?

Apologies jochen, misread the OWNERS file, added the right grdp file to OWNERS.

[jochen (gone - plz use gerrit), 2017-05-09 19:38:12]

lgtm

[edwardjung, 2017-05-10 20:13:10]

dbeam@ friendly ping!

[edwardjung, 2017-05-15 11:59:42]

The CQ bit was checked by edwardjung@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-15 11:59:57]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-15 13:00:35]

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

[commit-bot: I haz the power, 2017-05-15 13:00:36]

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_...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[edwardjung, 2017-05-15 16:16:12]

@felt, and dbeam, 

Please take another look, I've made some changes to:
+ Fix browser tests
+ added interstitial_webview_quiet.js as JS templates isn't being used.
+ As BaseSafeBrowsingErrorUi is now checked in, I've made the requisite changes.

Thanks.

[Dan Beam, 2017-05-16 03:37:30]

sorry, forgot to publish

https://codereview.chromium.org/2854263003/diff/80001/components/security_int...
File
components/security_interstitials/core/browser/resources/interstitial_webview_quiet.html
(right):

https://codereview.chromium.org/2854263003/diff/80001/components/security_int...
components/security_interstitials/core/browser/resources/interstitial_webview_quiet.html:24:
<p i18n-values=".innerHTML:explanationParagraph"></p>
On 2017/05/08 23:21:11, edwardjung wrote:
> On 2017/05/08 17:58:04, Dan Beam wrote:
> > why does this need to be innerHTML?  it is insecure and can run scripts and
> lead
> > to security problems (which would be ironic on this page)
> 
> Understand that innerHTML is evil. However the strings contain anchor links
> which aren't parsed with $i18n{}. I've split this into two strings.

see also: $i18nRaw{}

> 
> Genuine question, is it realistically possible to inject bad scripts onto
these
> web Ui pages? I don't think it's possible to link to these pages directly from
> external pages.

yes, XSS via innerHTML has been used through various places

> 
> For another instance the interstitial_v2.html template it's used by four
> different variants with different text some with links and formatting and
others
> with none. Is there a suggested solution for that? Also including the link in
> with the rest of the explanation paragraph would be better for the translator
to
> write a cohesive paragraph as they can move the position of the link if it
made
> sense.

https://codereview.chromium.org/2854263003/diff/100001/components/security_in...
File
components/security_interstitials/core/browser/resources/interstitial_common.js
(right):

https://codereview.chromium.org/2854263003/diff/100001/components/security_in...
components/security_interstitials/core/browser/resources/interstitial_common.js:8:
// Should match security_interstitials::SecurityInterstitialCommands
/** @enum {string} */

https://codereview.chromium.org/2854263003/diff/100001/components/security_in...
components/security_interstitials/core/browser/resources/interstitial_common.js:9:
var securityInterstitialCommandId = {
nit: capitalize

var SecurityInterstitialCommandId = {
    ^

https://codereview.chromium.org/2854263003/diff/100001/components/security_in...
File
components/security_interstitials/core/browser/resources/interstitial_webview_quiet.css
(right):

https://codereview.chromium.org/2854263003/diff/100001/components/security_in...
components/security_interstitials/core/browser/resources/interstitial_webview_quiet.css:3:
found in the LICENSE file. */
same nit regarding starting all lines of this comment with *

https://codereview.chromium.org/2854263003/diff/100001/components/security_in...
File
components/security_interstitials/core/browser/resources/interstitial_webview_quiet.html
(right):

https://codereview.chromium.org/2854263003/diff/100001/components/security_in...
components/security_interstitials/core/browser/resources/interstitial_webview_quiet.html:13:
<body id="body" jsvalues=".className:is_giant ? 'giant' : ''">
do you need to use jsvalues to do this?

https://codereview.chromium.org/2854263003/diff/200001/components/security_in...
File
components/security_interstitials/core/browser/resources/interstitial_common.js
(right):

https://codereview.chromium.org/2854263003/diff/200001/components/security_in...
components/security_interstitials/core/browser/resources/interstitial_common.js:28:

nit: just \n instead of \n\n

[edwardjung, 2017-05-16 13:59:09]

edwardjung@chromium.org changed reviewers:
+ jialiul@chromium.org

[edwardjung, 2017-05-16 13:59:11]

Thanks Dan.

@felt, @jialiul I've added basic unit tests for the new interstitial. Let me
know what you think.

https://codereview.chromium.org/2854263003/diff/100001/components/security_in...
File
components/security_interstitials/core/browser/resources/interstitial_common.js
(right):

https://codereview.chromium.org/2854263003/diff/100001/components/security_in...
components/security_interstitials/core/browser/resources/interstitial_common.js:8:
// Should match security_interstitials::SecurityInterstitialCommands
On 2017/05/16 03:37:30, Dan Beam wrote:
> /** @enum {string} */

Done.

https://codereview.chromium.org/2854263003/diff/100001/components/security_in...
components/security_interstitials/core/browser/resources/interstitial_common.js:9:
var securityInterstitialCommandId = {
On 2017/05/16 03:37:30, Dan Beam wrote:
> nit: capitalize
> 
> var SecurityInterstitialCommandId = {
>     ^

Done.

https://codereview.chromium.org/2854263003/diff/100001/components/security_in...
File
components/security_interstitials/core/browser/resources/interstitial_webview_quiet.css
(right):

https://codereview.chromium.org/2854263003/diff/100001/components/security_in...
components/security_interstitials/core/browser/resources/interstitial_webview_quiet.css:3:
found in the LICENSE file. */
On 2017/05/16 03:37:30, Dan Beam wrote:
> same nit regarding starting all lines of this comment with *

Done.

https://codereview.chromium.org/2854263003/diff/100001/components/security_in...
File
components/security_interstitials/core/browser/resources/interstitial_webview_quiet.html
(right):

https://codereview.chromium.org/2854263003/diff/100001/components/security_in...
components/security_interstitials/core/browser/resources/interstitial_webview_quiet.html:13:
<body id="body" jsvalues=".className:is_giant ? 'giant' : ''">
On 2017/05/16 03:37:30, Dan Beam wrote:
> do you need to use jsvalues to do this?

Removed. Switched to using JS.

https://codereview.chromium.org/2854263003/diff/200001/components/security_in...
File
components/security_interstitials/core/browser/resources/interstitial_common.js
(right):

https://codereview.chromium.org/2854263003/diff/200001/components/security_in...
components/security_interstitials/core/browser/resources/interstitial_common.js:28:

On 2017/05/16 03:37:30, Dan Beam wrote:
> nit: just \n instead of \n\n

Done.

[edwardjung, 2017-05-16 14:16:48]

The CQ bit was checked by edwardjung@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-16 14:17:24]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-16 14:20:34]

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

[commit-bot: I haz the power, 2017-05-16 14:20:35]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[edwardjung, 2017-05-16 15:20:39]

The CQ bit was checked by edwardjung@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-16 15:21:15]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-16 15:35:56]

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

[commit-bot: I haz the power, 2017-05-16 15:35:57]

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

[Jialiu Lin, 2017-05-16 17:25:42]

Thanks for adding these tests edwardjung!  c/b/safe_browsing LGTM with a couple
of nits.

BTW, you probably need to do a rebase.
components/safe_browsing_db/safe_browsing_prefs.h has been moved to
components/safe_browsing/common/safe_browsing_prefs.h

https://codereview.chromium.org/2854263003/diff/240001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/safe_browsing_blocking_page_unittest.cc
(right):

https://codereview.chromium.org/2854263003/diff/240001/chrome/browser/safe_br...
chrome/browser/safe_browsing/safe_browsing_blocking_page_unittest.cc:991:
ASSERT_EQ(str, l10n_util::GetStringUTF16(IDS_MALWARE_WEBVIEW_HEADING));
nit: ASSERT_EQ -> EXPECT_EQ

ASSERT_EQ means this must be true in order to execute later instruction,
otherwise test aborts at this line.

EXPECT_EQ means execute all the checks till the end of this test.

https://codereview.chromium.org/2854263003/diff/240001/chrome/browser/safe_br...
chrome/browser/safe_browsing/safe_browsing_blocking_page_unittest.cc:994:
ASSERT_FALSE(is_giant);
nit: EXPECT_FALSE

similar nits apply to the other 2 tests.

[edwardjung, 2017-05-16 18:48:31]

The CQ bit was checked by edwardjung@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-16 18:49:23]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[edwardjung, 2017-05-16 21:05:05]

Thanks Jailiu!

https://codereview.chromium.org/2854263003/diff/240001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/safe_browsing_blocking_page_unittest.cc
(right):

https://codereview.chromium.org/2854263003/diff/240001/chrome/browser/safe_br...
chrome/browser/safe_browsing/safe_browsing_blocking_page_unittest.cc:991:
ASSERT_EQ(str, l10n_util::GetStringUTF16(IDS_MALWARE_WEBVIEW_HEADING));
On 2017/05/16 17:25:42, Jialiu Lin wrote:
> nit: ASSERT_EQ -> EXPECT_EQ
> 
> ASSERT_EQ means this must be true in order to execute later instruction,
> otherwise test aborts at this line.
> 
> EXPECT_EQ means execute all the checks till the end of this test. 

Done.

https://codereview.chromium.org/2854263003/diff/240001/chrome/browser/safe_br...
chrome/browser/safe_browsing/safe_browsing_blocking_page_unittest.cc:994:
ASSERT_FALSE(is_giant);
On 2017/05/16 17:25:42, Jialiu Lin wrote:
> nit: EXPECT_FALSE
> 
> similar nits apply to the other 2 tests. 

Done.

[commit-bot: I haz the power, 2017-05-16 23:55:26]

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

[commit-bot: I haz the power, 2017-05-16 23:55:27]

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

[felt, 2017-05-17 05:14:18]

FYI I'll look at this first thing in the morning, I know you're eager to land it

[felt, 2017-05-17 16:33:26]

looks good just a few small comments

https://codereview.chromium.org/2854263003/diff/260001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/safe_browsing_blocking_page_unittest.cc
(right):

https://codereview.chromium.org/2854263003/diff/260001/chrome/browser/safe_br...
chrome/browser/safe_browsing/safe_browsing_blocking_page_unittest.cc:150: 
meaning TODO before landing, or a TODO that'll stay in the code? if the latter,
can you please add a note explaining why it's separate and mark as
TODO(edwardjung) ?

https://codereview.chromium.org/2854263003/diff/260001/components/security_in...
File
components/security_interstitials/core/browser/resources/interstitial_common.js
(right):

https://codereview.chromium.org/2854263003/diff/260001/components/security_in...
components/security_interstitials/core/browser/resources/interstitial_common.js:5:
// This is the shared code for the new (Chrome 37) security interstitials. It is
nit: while you're at it, can you remove "(Chrome 37")? 😂 not so new anymore,
makes me feel old

https://codereview.chromium.org/2854263003/diff/260001/components/security_in...
components/security_interstitials/core/browser/resources/interstitial_common.js:10:
var SecurityInterstitialCommandId = {
this is a nice improvement!

https://codereview.chromium.org/2854263003/diff/260001/components/security_in...
File components/security_interstitials/core/safe_browsing_quiet_error_ui.cc
(right):

https://codereview.chromium.org/2854263003/diff/260001/components/security_in...
components/security_interstitials/core/safe_browsing_quiet_error_ui.cc:80:
load_time_data->SetString("explanationParagraph",
is it safe to leave explanationParagraph blank otherwise?

https://codereview.chromium.org/2854263003/diff/260001/components/security_in...
File components/security_interstitials/core/safe_browsing_quiet_error_ui.h
(right):

https://codereview.chromium.org/2854263003/diff/260001/components/security_in...
components/security_interstitials/core/safe_browsing_quiet_error_ui.h:45: void
SetGiantWebView(bool is_giant_webview);
if this is only for testing, can you please name SetGiantWebViewForTesting?

https://codereview.chromium.org/2854263003/diff/260001/components/security_in...
File components/security_interstitials_strings.grdp (right):

https://codereview.chromium.org/2854263003/diff/260001/components/security_in...
components/security_interstitials_strings.grdp:193: <message
name="IDS_MALWARE_WEBVIEW_EXPLANATION_PARAGRAPH" desc="The explanation of why
Safe Browsing has blocked the page. Allows the user to proceed using a link.">
does this allow the user to proceed using a link? seems like the link is gone

[edwardjung, 2017-05-17 22:44:07]

The CQ bit was checked by edwardjung@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-17 22:45:00]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[edwardjung, 2017-05-17 22:49:33]

Thanks, I'll look to improve the tests using the visibility method you sent  
across down the line when the details section is implemented.

https://codereview.chromium.org/2854263003/diff/260001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/safe_browsing_blocking_page_unittest.cc
(right):

https://codereview.chromium.org/2854263003/diff/260001/chrome/browser/safe_br...
chrome/browser/safe_browsing/safe_browsing_blocking_page_unittest.cc:150: 
On 2017/05/17 16:33:26, felt wrote:
> meaning TODO before landing, or a TODO that'll stay in the code? if the
latter,
> can you please add a note explaining why it's separate and mark as
> TODO(edwardjung) ?

I've added my name in there. The only reason for this separation is the time
constraint.

https://codereview.chromium.org/2854263003/diff/260001/components/security_in...
File
components/security_interstitials/core/browser/resources/interstitial_common.js
(right):

https://codereview.chromium.org/2854263003/diff/260001/components/security_in...
components/security_interstitials/core/browser/resources/interstitial_common.js:5:
// This is the shared code for the new (Chrome 37) security interstitials. It is
On 2017/05/17 16:33:26, felt wrote:
> nit: while you're at it, can you remove "(Chrome 37")? 😂 not so new anymore,
> makes me feel old

:) Done

https://codereview.chromium.org/2854263003/diff/260001/components/security_in...
components/security_interstitials/core/browser/resources/interstitial_common.js:10:
var SecurityInterstitialCommandId = {
On 2017/05/17 16:33:26, felt wrote:
> this is a nice improvement!

Acknowledged.

https://codereview.chromium.org/2854263003/diff/260001/components/security_in...
File components/security_interstitials/core/safe_browsing_quiet_error_ui.cc
(right):

https://codereview.chromium.org/2854263003/diff/260001/components/security_in...
components/security_interstitials/core/safe_browsing_quiet_error_ui.cc:80:
load_time_data->SetString("explanationParagraph",
On 2017/05/17 16:33:26, felt wrote:
> is it safe to leave explanationParagraph blank otherwise?

We're only showing this interstitial for phishing / malware so the string should
always be set. Would there be other reasons that this would be called?

https://codereview.chromium.org/2854263003/diff/260001/components/security_in...
File components/security_interstitials/core/safe_browsing_quiet_error_ui.h
(right):

https://codereview.chromium.org/2854263003/diff/260001/components/security_in...
components/security_interstitials/core/safe_browsing_quiet_error_ui.h:45: void
SetGiantWebView(bool is_giant_webview);
On 2017/05/17 16:33:26, felt wrote:
> if this is only for testing, can you please name SetGiantWebViewForTesting?

Done.

https://codereview.chromium.org/2854263003/diff/260001/components/security_in...
File components/security_interstitials_strings.grdp (right):

https://codereview.chromium.org/2854263003/diff/260001/components/security_in...
components/security_interstitials_strings.grdp:193: <message
name="IDS_MALWARE_WEBVIEW_EXPLANATION_PARAGRAPH" desc="The explanation of why
Safe Browsing has blocked the page. Allows the user to proceed using a link.">
On 2017/05/17 16:33:26, felt wrote:
> does this allow the user to proceed using a link? seems like the link is gone

Good spot, this string was refactored. But actually I just got back some
feedback from localisation that it's better to embed the link in the context of
the rest of the paragraph. So I've updated the strings and template.

[commit-bot: I haz the power, 2017-05-18 05:59:12]

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

[commit-bot: I haz the power, 2017-05-18 05:59:14]

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

[felt, 2017-05-18 16:44:51]

lgtm % one comment

https://codereview.chromium.org/2854263003/diff/260001/components/security_in...
File components/security_interstitials/core/safe_browsing_quiet_error_ui.cc
(right):

https://codereview.chromium.org/2854263003/diff/260001/components/security_in...
components/security_interstitials/core/safe_browsing_quiet_error_ui.cc:80:
load_time_data->SetString("explanationParagraph",
On 2017/05/17 22:49:32, edwardjung wrote:
> On 2017/05/17 16:33:26, felt wrote:
> > is it safe to leave explanationParagraph blank otherwise?
> 
> We're only showing this interstitial for phishing / malware so the string
should
> always be set. Would there be other reasons that this would be called?

could you then do a NOTREACHED or DCHECK to make sure that constraint is
enforced?

[edwardjung, 2017-05-18 17:16:16]

The CQ bit was checked by edwardjung@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-18 17:16:53]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-18 18:04:36]

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

[commit-bot: I haz the power, 2017-05-18 18:04:37]

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_...)

[edwardjung, 2017-05-18 19:33:57]

The CQ bit was checked by edwardjung@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-18 19:34:51]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-18 20:10:55]

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

[commit-bot: I haz the power, 2017-05-18 20:10:57]

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

[edwardjung, 2017-05-19 08:03:06]

The CQ bit was checked by edwardjung@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-19 08:03:26]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[edwardjung, 2017-05-19 08:14:58]

Done, thanks.

https://codereview.chromium.org/2854263003/diff/260001/components/security_in...
File components/security_interstitials/core/safe_browsing_quiet_error_ui.cc
(right):

https://codereview.chromium.org/2854263003/diff/260001/components/security_in...
components/security_interstitials/core/safe_browsing_quiet_error_ui.cc:80:
load_time_data->SetString("explanationParagraph",
On 2017/05/18 16:44:50, felt wrote:
> On 2017/05/17 22:49:32, edwardjung wrote:
> > On 2017/05/17 16:33:26, felt wrote:
> > > is it safe to leave explanationParagraph blank otherwise?
> > 
> > We're only showing this interstitial for phishing / malware so the string
> should
> > always be set. Would there be other reasons that this would be called?
> 
> could you then do a NOTREACHED or DCHECK to make sure that constraint is
> enforced?

Done.

[commit-bot: I haz the power, 2017-05-19 08:47:48]

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

[commit-bot: I haz the power, 2017-05-19 08:47:49]

Dry run: This issue passed the CQ dry run.

[edwardjung, 2017-05-19 10:37:07]

The CQ bit was checked by edwardjung@chromium.org

[edwardjung, 2017-05-19 10:37:08]

The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org,
jialiul@chromium.org, felt@chromium.org
Link to the patchset: https://codereview.chromium.org/2854263003/#ps340001
(title: "Fix unit test")

[commit-bot: I haz the power, 2017-05-19 10:37:42]

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, 2017-05-19 10:43:31]

CQ is committing da patch.

Bot data: {"patchset_id": 340001, "attempt_start_ts": 1495190227604810,
"parent_rev": "ac869358346b76b1339422ac9766a6aeb997a51a", "commit_rev":
"f9dfdb6d9dcd8c5f987fd7114bff3ebee6b8785f"}

[commit-bot: I haz the power, 2017-05-19 10:43:49]

Description was changed from

==========
Add quiet safe browsing interstitial for WebView

BUG=716095
==========

to

==========
Add quiet safe browsing interstitial for WebView

BUG=716095

Review-Url: https://codereview.chromium.org/2854263003
Cr-Commit-Position: refs/heads/master@{#473150}
Committed:
https://chromium.googlesource.com/chromium/src/+/f9dfdb6d9dcd8c5f987fd7114bff...
==========

[commit-bot: I haz the power, 2017-05-19 10:43:51]

Committed patchset #18 (id:340001) as
https://chromium.googlesource.com/chromium/src/+/f9dfdb6d9dcd8c5f987fd7114bff...

[Dan Beam, 2017-05-19 19:14:23]

sooo jochen@ owns a lot of things.  did you actually need a review from me? 
because you already landed...

https://codereview.chromium.org/2854263003/diff/200001/components/resources/s...
File components/resources/security_interstitials_resources.grdp (right):

https://codereview.chromium.org/2854263003/diff/200001/components/resources/s...
components/resources/security_interstitials_resources.grdp:5: <include
name="IDR_SECURITY_INTERSTITIAL_QUIET_HTML"
file="../security_interstitials/core/browser/resources/interstitial_webview_quiet.html"
flattenhtml="true" type="BINDATA" />
do you need to flattenhtml="true"?  are you relying on resources being inlined?

https://codereview.chromium.org/2854263003/diff/200001/components/security_in...
File
components/security_interstitials/core/browser/resources/interstitial_webview_quiet.css
(right):

https://codereview.chromium.org/2854263003/diff/200001/components/security_in...
components/security_interstitials/core/browser/resources/interstitial_webview_quiet.css:18:
#details.hidden {
uhhh, why is .hidden doing this rather than just using the hidden attribute?

https://codereview.chromium.org/2854263003/diff/200001/components/security_in...
File
components/security_interstitials/core/browser/resources/interstitial_webview_quiet.js
(right):

https://codereview.chromium.org/2854263003/diff/200001/components/security_in...
components/security_interstitials/core/browser/resources/interstitial_webview_quiet.js:7:
document.body.className = isGiantWebView ? 'giant' : '';
nit: document.body.classList.toggle('giant',
loadTimeData.getBoolean('is_giant'));

[Dan Beam, 2017-05-19 19:15:44]

btw, have you tried your code on iOS?  I remembered recently than $i18n{} may
not work in their fork of WebUI (but not totally sure).

[edwardjung, 2017-05-19 19:20:21]

On 2017/05/19 19:15:44, Dan Beam wrote:
> btw, have you tried your code on iOS?  I remembered recently than $i18n{} may
> not work in their fork of WebUI (but not totally sure).

The quiet interstitial is currently only for Android WebView.

Good to know though, as I was planning on updating the existing interstitial
template with $i18n{}. When I do, I'll make sure to check iOS,

[edwardjung, 2017-05-19 19:38:18]

You're weren't needed for owner approval but we did need your review! Felt is a
owner of the safebrowsing resources but wanted someone with more experience of
reviewing HTML / JS / CSS to check the code. Apologies that I didn't wait for
your LGTM.

https://codereview.chromium.org/2854263003/diff/200001/components/resources/s...
File components/resources/security_interstitials_resources.grdp (right):

https://codereview.chromium.org/2854263003/diff/200001/components/resources/s...
components/resources/security_interstitials_resources.grdp:5: <include
name="IDR_SECURITY_INTERSTITIAL_QUIET_HTML"
file="../security_interstitials/core/browser/resources/interstitial_webview_quiet.html"
flattenhtml="true" type="BINDATA" />
On 2017/05/19 19:14:22, Dan Beam wrote:
> do you need to flattenhtml="true"?  are you relying on resources being
inlined?

Yes they need to be inlined, they aren't normal WebUI pages, the interstitials
can't reference external resources via chrome:// as they are blocked. This page
replaces the site the user would have seen.

https://codereview.chromium.org/2854263003/diff/200001/components/security_in...
File
components/security_interstitials/core/browser/resources/interstitial_webview_quiet.css
(right):

https://codereview.chromium.org/2854263003/diff/200001/components/security_in...
components/security_interstitials/core/browser/resources/interstitial_webview_quiet.css:18:
#details.hidden {
On 2017/05/19 19:14:22, Dan Beam wrote:
> uhhh, why is .hidden doing this rather than just using the hidden attribute?

The details fade in / out rather than just disappearing immediately hence the
use of the class versus the attribute.

https://codereview.chromium.org/2854263003/diff/200001/components/security_in...
File
components/security_interstitials/core/browser/resources/interstitial_webview_quiet.js
(right):

https://codereview.chromium.org/2854263003/diff/200001/components/security_in...
components/security_interstitials/core/browser/resources/interstitial_webview_quiet.js:7:
document.body.className = isGiantWebView ? 'giant' : '';
On 2017/05/19 19:14:22, Dan Beam wrote:
> nit: document.body.classList.toggle('giant',
> loadTimeData.getBoolean('is_giant'));

There is a second phase to this. I'll fix this in a follow up CL.