Expand SecurityStyleChanged interfaces to include explanations

Expand SecurityStyleChanged interfaces to include explanations

As the next step in building the devtools security panel, we need to
plumb through human-readable strings explaining why the lock icon is red
(broken) or yellow (warning). This CL expands the WebContentsObserver
and WebContentsDelegate methods that ferry lock icon information to
devtools to include these human-readable strings.

BUG=484409
Committed: https://crrev.com/5ea80e54daf27b155d9ba2c6e7f9a3f7b4d5e803
Cr-Commit-Position: refs/heads/master@{#335306}

[estark, 2015-06-12 23:00:14]

estark@chromium.org changed reviewers:
+ creis@chromium.org, pfeldman@chromium.org, pkasting@chromium.org

[estark, 2015-06-12 23:00:15]

This is the next step in our devtools security panel plumbing Master Plan.* The
eventual goal here is to construct human-readable strings in //chrome (since
they describe chrome-specific security policy such as SHA1 deprecation) that
explain the lock icon, and send those strings to devtools to display in the
security panel. This CL just expands the WebContentsObserver and
WebContentsDelegate methods to carry these human-readable explanations, but
doesn't actually populate them yet.

creis: could you please review //content/browser/web_contents and
//content/public?
pkasting: could you please review //chrome/browser/ui?
pfeldman: could you please review //content/browser/devtools?
lgarron: FYI

(Note: This CL is based on top of crrev.com/1169213006, which I hope will be
landing soon; it's just waiting on a chrome/chrome.gyp OWNER)

* Design doc:
https://docs.google.com/document/d/1kMU5dPi2PwQ7skbgwlfQsenYdTwOi2ya2WYla2uy0...

[lgarron, 2015-06-12 23:59:59]

lgarron@chromium.org changed reviewers:
+ lgarron@chromium.org

[lgarron, 2015-06-13 00:00:00]

I like the use of two vectors for the two security levels. However, I would
rename broken_explanations to insecure_explanations, in order to match the
protocol.json names [1].

https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

[lgarron, 2015-06-13 00:00:21]

lgarron@chromium.org changed reviewers:
- lgarron@chromium.org

[estark, 2015-06-14 22:59:52]

On 2015/06/13 00:00:00, lgarron wrote:
> I like the use of two vectors for the two security levels. However, I would
> rename broken_explanations to insecure_explanations, in order to match the
> protocol.json names [1].
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

I don't have a super strong opinion, but 'broken' matches with //content's
SecurityStyle (SECURITY_STYLE_AUTHENTICATION_BROKEN), and I think it might make
more sense to align with SecurityStyle than with the devtools protocol, since
these methods are supposed to be generic and usable by any embedder, not
specific to devtools.

[Peter Kasting, 2015-06-15 20:23:43]

* It's not made clear anywhere why the added params are vectors of strings,
rather than strings.  In particular, I don't see how a naive reader (i.e. me)
would know how many strings the vector should have or what each would correspond
to.

* It seems like this change neither populates nor uses the new params.  It would
be easier to review this (and would clear up e.g. the confusion above) if it did
at least one of these things, if not both.  I'm not a big fan of reviewing API
changes where I can't see how the API will practically be used.

Otherwise the (trivial) mechanics of the change seem OK.

[lgarron, 2015-06-15 20:44:54]

On 2015/06/15 at 20:23:43, pkasting wrote:
> * It's not made clear anywhere why the added params are vectors of strings,
rather than strings.  In particular, I don't see how a naive reader (i.e. me)
would know how many strings the vector should have or what each would correspond
to.
> 
> * It seems like this change neither populates nor uses the new params.  It
would be easier to review this (and would clear up e.g. the confusion above) if
it did at least one of these things, if not both.  I'm not a big fan of
reviewing API changes where I can't see how the API will practically be used.
> 
> Otherwise the (trivial) mechanics of the change seem OK.

I know this is not a standalone answer for the CL, but here are some examples:
https://docs.google.com/document/d/1KJVID_M9PMLmWcW9TmPYPCVmFey9kqyoxDPhU7qMI...

[estark, 2015-06-16 02:09:36]

On 2015/06/15 20:23:43, Peter Kasting wrote:
> * It's not made clear anywhere why the added params are vectors of strings,
> rather than strings.  In particular, I don't see how a naive reader (i.e. me)
> would know how many strings the vector should have or what each would
correspond
> to.
> 
> * It seems like this change neither populates nor uses the new params.  It
would
> be easier to review this (and would clear up e.g. the confusion above) if it
did
> at least one of these things, if not both.  I'm not a big fan of reviewing API
> changes where I can't see how the API will practically be used.
> 
> Otherwise the (trivial) mechanics of the change seem OK.

Hmm, I was trying to keep the CL small but I guess it was a little too small to
be useful. Please take a look at the latest patchset (#5). Here is an outline of
the changes I've made:

* Added code to populate the explanations in
//chrome/browser/ssl/connection_security.cc and //chrome/browser/ui/browser.cc.
The former provides enums describing why a particular SecurityStyle was chosen
for a page, and the latter converts those enums into strings.

* Introduced a SecurityStyleExplanation struct (containing the two vectors), and
changed the API to pass around a SecurityStyleExplanation instead of the two
vectors. The motivation for this change was a conversation today about what the
devtools security panel frontend will look like (new frontend mocks show a
summary and description for each security issue instead of just a description).
I think that passing around a SecurityStyleExplanation will give us more
flexibility than passing around two vectors: if we later decide that we want to
add e.g. |bonus_point_explanations| explaining what a site is doing right in its
SSL configuration, we can change the SecurityStyleExplanation struct and the
code that populates and consumes it, instead of having to change all the APIs in
between them.

* Added comments in SecurityStyleExplanation to hopefully make it more clear
what the vectors should contain.

[Peter Kasting, 2015-06-16 06:29:11]

This makes a lot more sense than the first change.

https://codereview.chromium.org/1181293003/diff/70001/chrome/app/generated_re...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/1181293003/diff/70001/chrome/app/generated_re...
chrome/app/generated_resources.grd:15645: +        The certificate for this site
expires in 2016, and the certificate chain contains a certificate signed using
SHA-1.
Seems like from a user's perspective these first two could be combined into a
single "2016 or later" warning.

https://codereview.chromium.org/1181293003/diff/70001/chrome/app/generated_re...
chrome/app/generated_resources.grd:15663: +        There are issues with the
site's certificate chain (<ph
name="CERT_ERROR_DESCRIPTION">$1<ex>NET::ERR_CERT_AUTHORITY_INVALID</ex></ph>).
Nit: Wouldn't it be "net::"?

https://codereview.chromium.org/1181293003/diff/70001/chrome/browser/ssl/conn...
File chrome/browser/ssl/connection_security.cc (right):

https://codereview.chromium.org/1181293003/diff/70001/chrome/browser/ssl/conn...
chrome/browser/ssl/connection_security.cc:77: if
(content::CertStore::GetInstance()->RetrieveCert(ssl.cert_id, &cert))
Nit: Shorter:

  return content::CertStore::GetInstance()->RetrieveCert(ssl.cert_id, &cert) ?
      cert : nullptr;

https://codereview.chromium.org/1181293003/diff/70001/chrome/browser/ssl/conn...
chrome/browser/ssl/connection_security.cc:91: static const int64_t kJanuary2016
= INT64_C(13096080000000000);
Nit: I'd move this second constant and its comment below the first conditional.

https://codereview.chromium.org/1181293003/diff/70001/chrome/browser/ssl/conn...
chrome/browser/ssl/connection_security.cc:92: if (cert->valid_expiry() >=
base::Time::FromInternalValue(kJanuary2017)) {
Nit: No {} (2 places)

https://codereview.chromium.org/1181293003/diff/70001/chrome/browser/ssl/conn...
chrome/browser/ssl/connection_security.cc:107: if (ssl.content_status &
content::SSLStatus::RAN_INSECURE_CONTENT)
Since these are bitfield values and thus both could be set, shouldn't you check
RAN before DISPLAYED?

https://codereview.chromium.org/1181293003/diff/70001/chrome/browser/ssl/conn...
chrome/browser/ssl/connection_security.cc:153: DCHECK(mixed_content_status !=
RAN_MIXED_CONTENT);
Nit: DCHECK_NE

https://codereview.chromium.org/1181293003/diff/70001/chrome/browser/ssl/conn...
chrome/browser/ssl/connection_security.cc:161: else if (sha1_status ==
DEPRECATED_SHA1_WARNING)
Nit: No else after return

https://codereview.chromium.org/1181293003/diff/70001/chrome/browser/ssl/conn...
chrome/browser/ssl/connection_security.cc:186: if (!web_contents) {
Nit: Shorter:

  content::NavigationEntry* entry =
      web_contents ? web_contents->GetController().GetVisibleEntry() : nullptr;
  if (!entry) {
    ...

https://codereview.chromium.org/1181293003/diff/70001/chrome/browser/ssl/conn...
chrome/browser/ssl/connection_security.cc:198: const content::SSLStatus& ssl =
entry->GetSSL();
Nit: Declare this just above the first use below rather than here.

https://codereview.chromium.org/1181293003/diff/70001/chrome/browser/ssl/conn...
File chrome/browser/ssl/connection_security.h (right):

https://codereview.chromium.org/1181293003/diff/70001/chrome/browser/ssl/conn...
chrome/browser/ssl/connection_security.h:65: DEPRECATED_SHA1_WARNING
Nit: Trailing comma, for consistency with the existing enum in this file (and, I
think, the majority of Chrome code) (multiple places)

https://codereview.chromium.org/1181293003/diff/70001/chrome/browser/ssl/conn...
chrome/browser/ssl/connection_security.h:79: // |SecurityStyle| and the
information that was used to decide which
Nit: No || on type names (just variable names) (2 places)

https://codereview.chromium.org/1181293003/diff/70001/chrome/browser/ui/brows...
File chrome/browser/ui/browser.cc (right):

https://codereview.chromium.org/1181293003/diff/70001/chrome/browser/ui/brows...
chrome/browser/ui/browser.cc:1355: if
(net::IsCertStatusMinorError(security_info.cert_status)) {
Nit: No {}

https://codereview.chromium.org/1181293003/diff/70001/chrome/browser/ui/brows...
File chrome/browser/ui/browser_browsertest.cc (right):

https://codereview.chromium.org/1181293003/diff/70001/chrome/browser/ui/brows...
chrome/browser/ui/browser_browsertest.cc:365: const
content::SecurityStyleExplanation& latest_security_style_explanation()
Nit: Prefer to break before the function name rather than before "const"

https://codereview.chromium.org/1181293003/diff/70001/content/browser/devtool...
File content/browser/devtools/protocol/security_handler.h (right):

https://codereview.chromium.org/1181293003/diff/70001/content/browser/devtool...
content/browser/devtools/protocol/security_handler.h:9: #include <vector>
These #includes aren't necessary

https://codereview.chromium.org/1181293003/diff/70001/content/public/common/s...
File content/public/common/security_style.cc (right):

https://codereview.chromium.org/1181293003/diff/70001/content/public/common/s...
content/public/common/security_style.cc:16:
SecurityStyleProperty::~SecurityStyleProperty() {
Nit: Blank line between distinct function implementations (2 places)

https://codereview.chromium.org/1181293003/diff/70001/content/public/common/s...
File content/public/common/security_style.h (right):

https://codereview.chromium.org/1181293003/diff/70001/content/public/common/s...
content/public/common/security_style.h:49: // contains errors
(NET::CERT_DATE_INVALID)".
Nit: "net::"?

https://codereview.chromium.org/1181293003/diff/70001/content/public/common/s...
content/public/common/security_style.h:50: struct SecurityStyleProperty {
Nit: I might name this "SecurityStyleExplanation" and the struct below
"SecurityStyleExplanations".  Basically, this doesn't feel like a "property" of
the security style, it feels like an explanation for it, so the name should
reflect that; then the name of the other struct should make sense given this
name.

A different direction would be to name this something like "SecurityProblem" to
indicate that it's highlighting a particular issue.  That would be a little more
in keeping with your comment below, you could change "single security property
of a page" to "single security problem with a page".

[estark, 2015-06-16 15:32:35]

https://codereview.chromium.org/1181293003/diff/70001/chrome/app/generated_re...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/1181293003/diff/70001/chrome/app/generated_re...
chrome/app/generated_resources.grd:15645: +        The certificate for this site
expires in 2016, and the certificate chain contains a certificate signed using
SHA-1.
On 2015/06/16 06:29:10, Peter Kasting wrote:
> Seems like from a user's perspective these first two could be combined into a
> single "2016 or later" warning.

2017-or-later and 2016 get different UI treatments; the former is a broken red
lock and the latter is a yellow badge (warning/dubious lock). And in terms of
the devtools security panel, the former becomes a "broken explanation"
(explanation for why we are showing a red lock), while the latter becomes a
"warning" explanation (explanation for why we are showing a yellow lock).

https://codereview.chromium.org/1181293003/diff/70001/chrome/app/generated_re...
chrome/app/generated_resources.grd:15663: +        There are issues with the
site's certificate chain (<ph
name="CERT_ERROR_DESCRIPTION">$1<ex>NET::ERR_CERT_AUTHORITY_INVALID</ex></ph>).
On 2015/06/16 06:29:10, Peter Kasting wrote:
> Nit: Wouldn't it be "net::"?

Done.

(I thought these were all net::ErrorToString() outputs were all caps because I
saw them printed in all caps on SSL interstitials, but turns out it's just an
all-caps font.)

https://codereview.chromium.org/1181293003/diff/70001/chrome/browser/ssl/conn...
File chrome/browser/ssl/connection_security.cc (right):

https://codereview.chromium.org/1181293003/diff/70001/chrome/browser/ssl/conn...
chrome/browser/ssl/connection_security.cc:77: if
(content::CertStore::GetInstance()->RetrieveCert(ssl.cert_id, &cert))
On 2015/06/16 06:29:10, Peter Kasting wrote:
> Nit: Shorter:
> 
>   return content::CertStore::GetInstance()->RetrieveCert(ssl.cert_id, &cert) ?
>       cert : nullptr;

Done.

https://codereview.chromium.org/1181293003/diff/70001/chrome/browser/ssl/conn...
chrome/browser/ssl/connection_security.cc:91: static const int64_t kJanuary2016
= INT64_C(13096080000000000);
On 2015/06/16 06:29:11, Peter Kasting wrote:
> Nit: I'd move this second constant and its comment below the first
conditional.

Done.

https://codereview.chromium.org/1181293003/diff/70001/chrome/browser/ssl/conn...
chrome/browser/ssl/connection_security.cc:92: if (cert->valid_expiry() >=
base::Time::FromInternalValue(kJanuary2017)) {
On 2015/06/16 06:29:10, Peter Kasting wrote:
> Nit: No {} (2 places)

Done.

https://codereview.chromium.org/1181293003/diff/70001/chrome/browser/ssl/conn...
chrome/browser/ssl/connection_security.cc:107: if (ssl.content_status &
content::SSLStatus::RAN_INSECURE_CONTENT)
On 2015/06/16 06:29:11, Peter Kasting wrote:
> Since these are bitfield values and thus both could be set, shouldn't you
check
> RAN before DISPLAYED?

Done.

https://codereview.chromium.org/1181293003/diff/70001/chrome/browser/ssl/conn...
chrome/browser/ssl/connection_security.cc:153: DCHECK(mixed_content_status !=
RAN_MIXED_CONTENT);
On 2015/06/16 06:29:11, Peter Kasting wrote:
> Nit: DCHECK_NE

Done.

https://codereview.chromium.org/1181293003/diff/70001/chrome/browser/ssl/conn...
chrome/browser/ssl/connection_security.cc:161: else if (sha1_status ==
DEPRECATED_SHA1_WARNING)
On 2015/06/16 06:29:10, Peter Kasting wrote:
> Nit: No else after return

Done.

https://codereview.chromium.org/1181293003/diff/70001/chrome/browser/ssl/conn...
chrome/browser/ssl/connection_security.cc:186: if (!web_contents) {
On 2015/06/16 06:29:11, Peter Kasting wrote:
> Nit: Shorter:
> 
>   content::NavigationEntry* entry =
>       web_contents ? web_contents->GetController().GetVisibleEntry() :
nullptr;
>   if (!entry) {
>     ...

Done.

https://codereview.chromium.org/1181293003/diff/70001/chrome/browser/ssl/conn...
chrome/browser/ssl/connection_security.cc:198: const content::SSLStatus& ssl =
entry->GetSSL();
On 2015/06/16 06:29:10, Peter Kasting wrote:
> Nit: Declare this just above the first use below rather than here.

Done.

https://codereview.chromium.org/1181293003/diff/70001/chrome/browser/ssl/conn...
File chrome/browser/ssl/connection_security.h (right):

https://codereview.chromium.org/1181293003/diff/70001/chrome/browser/ssl/conn...
chrome/browser/ssl/connection_security.h:65: DEPRECATED_SHA1_WARNING
On 2015/06/16 06:29:11, Peter Kasting wrote:
> Nit: Trailing comma, for consistency with the existing enum in this file (and,
I
> think, the majority of Chrome code) (multiple places)

Done.

https://codereview.chromium.org/1181293003/diff/70001/chrome/browser/ssl/conn...
chrome/browser/ssl/connection_security.h:79: // |SecurityStyle| and the
information that was used to decide which
On 2015/06/16 06:29:11, Peter Kasting wrote:
> Nit: No || on type names (just variable names) (2 places)

Done.

https://codereview.chromium.org/1181293003/diff/70001/chrome/browser/ui/brows...
File chrome/browser/ui/browser.cc (right):

https://codereview.chromium.org/1181293003/diff/70001/chrome/browser/ui/brows...
chrome/browser/ui/browser.cc:1355: if
(net::IsCertStatusMinorError(security_info.cert_status)) {
On 2015/06/16 06:29:11, Peter Kasting wrote:
> Nit: No {}

Done.

https://codereview.chromium.org/1181293003/diff/70001/chrome/browser/ui/brows...
File chrome/browser/ui/browser_browsertest.cc (right):

https://codereview.chromium.org/1181293003/diff/70001/chrome/browser/ui/brows...
chrome/browser/ui/browser_browsertest.cc:365: const
content::SecurityStyleExplanation& latest_security_style_explanation()
On 2015/06/16 06:29:11, Peter Kasting wrote:
> Nit: Prefer to break before the function name rather than before "const"

Done.

https://codereview.chromium.org/1181293003/diff/70001/content/browser/devtool...
File content/browser/devtools/protocol/security_handler.h (right):

https://codereview.chromium.org/1181293003/diff/70001/content/browser/devtool...
content/browser/devtools/protocol/security_handler.h:9: #include <vector>
On 2015/06/16 06:29:11, Peter Kasting wrote:
> These #includes aren't necessary

Done.

https://codereview.chromium.org/1181293003/diff/70001/content/public/common/s...
File content/public/common/security_style.cc (right):

https://codereview.chromium.org/1181293003/diff/70001/content/public/common/s...
content/public/common/security_style.cc:16:
SecurityStyleProperty::~SecurityStyleProperty() {
On 2015/06/16 06:29:11, Peter Kasting wrote:
> Nit: Blank line between distinct function implementations (2 places)

Done.

https://codereview.chromium.org/1181293003/diff/70001/content/public/common/s...
File content/public/common/security_style.h (right):

https://codereview.chromium.org/1181293003/diff/70001/content/public/common/s...
content/public/common/security_style.h:49: // contains errors
(NET::CERT_DATE_INVALID)".
On 2015/06/16 06:29:11, Peter Kasting wrote:
> Nit: "net::"?

Done.

https://codereview.chromium.org/1181293003/diff/70001/content/public/common/s...
content/public/common/security_style.h:50: struct SecurityStyleProperty {
On 2015/06/16 06:29:11, Peter Kasting wrote:
> Nit: I might name this "SecurityStyleExplanation" and the struct below
> "SecurityStyleExplanations".  Basically, this doesn't feel like a "property"
of
> the security style, it feels like an explanation for it, so the name should
> reflect that; then the name of the other struct should make sense given this
> name.
> 
> A different direction would be to name this something like "SecurityProblem"
to
> indicate that it's highlighting a particular issue.  That would be a little
more
> in keeping with your comment below, you could change "single security property
> of a page" to "single security problem with a page".

Done. I went with SecurityStyleExplanation instead of SecurityProblem, since I
expect we might adapt this in future to include positive characteristics (for
example, certificate has public audit records for CT).

[Peter Kasting, 2015-06-16 22:11:31]

LGTM

https://codereview.chromium.org/1181293003/diff/90001/content/public/common/s...
File content/public/common/security_style.h (right):

https://codereview.chromium.org/1181293003/diff/90001/content/public/common/s...
content/public/common/security_style.h:53: const std::string&
description_input);
Nit: Name these |summary| and |description|.  You can still write the
initializer list this way:

  SecurityStyleExplanation(const std::string& summary,
                           const std::string& description)
      : summary(summary), description(description) {
  }

...and the compiler will get it right.

https://codereview.chromium.org/1181293003/diff/90001/content/public/common/s...
content/public/common/security_style.h:60: // A SecurityStyleExplanations
contains human-readable explanations for
Nit: Remove initial "A" (it just makes the sentence less grammatically clear)

https://codereview.chromium.org/1181293003/diff/90001/content/public/common/s...
content/public/common/security_style.h:66: // has multiple issues.
Nit: This last sentence is now unclear because "An explanation" no longer
clearly refers to this class.  How about:

A single site may have multiple different explanations of both "warning" and
"broken" severity levels.

[estark, 2015-06-16 23:42:19]

estark@chromium.org changed reviewers:
+ rsleevi@chromium.org

[estark, 2015-06-16 23:42:20]

Thanks pkasting.

creis: could you please look at the rest of //content?
pfeldman: could you please take a look at //content/browser/devtools?
rsleevi: could you please look at //chrome/browser/ssl?

Executive summary:
* connection_security (in //c/b/ssl) computes a SecurityStyle for the page and a
set of properties (enums) describing why that SecurityStyle was chosen.
* Browser (in //c/b/ui) translates these properties into strings, which it puts
into a SecurityStyleExplanations struct which gets ferried through //content to
devtools.
* The devtools security handler is not yet doing anything with the explanations;
that will be in a follow-up CL after the necessary Blink changes are landed.

https://codereview.chromium.org/1181293003/diff/90001/content/public/common/s...
File content/public/common/security_style.h (right):

https://codereview.chromium.org/1181293003/diff/90001/content/public/common/s...
content/public/common/security_style.h:53: const std::string&
description_input);
On 2015/06/16 22:11:31, Peter Kasting wrote:
> Nit: Name these |summary| and |description|.  You can still write the
> initializer list this way:
> 
>   SecurityStyleExplanation(const std::string& summary,
>                            const std::string& description)
>       : summary(summary), description(description) {
>   }
> 
> ...and the compiler will get it right.

Done.

https://codereview.chromium.org/1181293003/diff/90001/content/public/common/s...
content/public/common/security_style.h:60: // A SecurityStyleExplanations
contains human-readable explanations for
On 2015/06/16 22:11:31, Peter Kasting wrote:
> Nit: Remove initial "A" (it just makes the sentence less grammatically clear)

Done.

https://codereview.chromium.org/1181293003/diff/90001/content/public/common/s...
content/public/common/security_style.h:66: // has multiple issues.
On 2015/06/16 22:11:31, Peter Kasting wrote:
> Nit: This last sentence is now unclear because "An explanation" no longer
> clearly refers to this class.  How about:
> 
> A single site may have multiple different explanations of both "warning" and
> "broken" severity levels.

Done.

[Ryan Sleevi, 2015-06-16 23:58:54]

c/b/ssl LGTM % nit

https://codereview.chromium.org/1181293003/diff/110001/chrome/browser/ssl/con...
File chrome/browser/ssl/connection_security.cc (right):

https://codereview.chromium.org/1181293003/diff/110001/chrome/browser/ssl/con...
chrome/browser/ssl/connection_security.cc:85: if (cert && (ssl.cert_status &
net::CERT_STATUS_SHA1_SIGNATURE_PRESENT)) {
Suggestion: Optimize for the early return, to reduce a level of indentation

if (!cert || !(ssl.cert_status & net::CERT_STATUS_SHA1_SIGNATURE_PRESENT))
  return connection_security::NO_DEPRECATED_SHA1;
static const int64_t ...

... stuff

[estark, 2015-06-17 00:02:42]

Patchset #8 (id:130001) has been deleted

[estark, 2015-06-17 04:38:30]

https://codereview.chromium.org/1181293003/diff/110001/chrome/browser/ssl/con...
File chrome/browser/ssl/connection_security.cc (right):

https://codereview.chromium.org/1181293003/diff/110001/chrome/browser/ssl/con...
chrome/browser/ssl/connection_security.cc:85: if (cert && (ssl.cert_status &
net::CERT_STATUS_SHA1_SIGNATURE_PRESENT)) {
On 2015/06/16 23:58:54, Ryan Sleevi wrote:
> Suggestion: Optimize for the early return, to reduce a level of indentation
> 
> if (!cert || !(ssl.cert_status & net::CERT_STATUS_SHA1_SIGNATURE_PRESENT))
>   return connection_security::NO_DEPRECATED_SHA1;
> static const int64_t ...
> 
> ... stuff

Done.

[Charlie Reis, 2015-06-17 18:37:39]

creis@chromium.org changed reviewers:
+ jam@chromium.org

[Charlie Reis, 2015-06-17 18:37:40]

[+jam]

I'd like to do a quick sanity check on whether we need the new structs in the
content/public API.  On one hand, we have things like DevToolsManagerDelegate
for getting info from the embedder without threading it through most of content.
 On the other hand, we'd probably want it in a structured form inside DevTools,
so a type for it may be worthwhile.

How is it going to be consumed?  Will it need to be converted again to be passed
into Blink?  In that case, maybe we should use a public Blink type instead of a
content type.

(We generally don't want to add things to content/public until they're used both
inside and outside content, so it would be useful to know how it will be used.)

https://codereview.chromium.org/1181293003/diff/150001/content/public/browser...
File content/public/browser/web_contents_observer.h (right):

https://codereview.chromium.org/1181293003/diff/150001/content/public/browser...
content/public/browser/web_contents_observer.h:181: // changes. |security_style|
is the new
nit: Looks like "changes" fits on the previous line, as before.
Similar issue with next line.

https://codereview.chromium.org/1181293003/diff/150001/content/public/common/...
File content/public/common/security_style.h (right):

https://codereview.chromium.org/1181293003/diff/150001/content/public/common/...
content/public/common/security_style.h:50: struct SecurityStyleExplanation {
From the content API guidelines:
"each interface/struct/enum should be in a separate file"

http://www.chromium.org/developers/content-module/content-api

It's admittedly unfortunate to need separate files for both
SecurityStyleExplanation and SecurityStyleExplanations, but I want to get John's
opinion on the approach anyway.

[estark, 2015-06-17 18:50:15]

On 2015/06/17 18:37:40, Charlie Reis wrote:
> [+jam]
> 
> I'd like to do a quick sanity check on whether we need the new structs in the
> content/public API.  On one hand, we have things like DevToolsManagerDelegate
> for getting info from the embedder without threading it through most of
content.
>  On the other hand, we'd probably want it in a structured form inside
DevTools,
> so a type for it may be worthwhile.
> 
> How is it going to be consumed?  Will it need to be converted again to be
passed
> into Blink?  In that case, maybe we should use a public Blink type instead of
a
> content type.

The plan is to add a parameter for it on the SecurityStyleChanged event in
devtools's protocol.json (the remote debugging protocol). SecurityHandler (in
//content/browser/devtools) observes the
WebContentsObserver::SecurityStyleChanged event and will include the
SecurityStyleExplanations as a parameter in the message that it sends. (Which
will be received by devtools javascript code in blink). So the plan is to
basically consume it the same way SecurityStyle is currently consumed: //chrome
sets the explanation and passes it through //content to the SecurityHandler (in
//content), which sends it over the devtools remote debugging protocol.

> 
> (We generally don't want to add things to content/public until they're used
both
> inside and outside content, so it would be useful to know how it will be
used.)
> 
>
https://codereview.chromium.org/1181293003/diff/150001/content/public/browser...
> File content/public/browser/web_contents_observer.h (right):
> 
>
https://codereview.chromium.org/1181293003/diff/150001/content/public/browser...
> content/public/browser/web_contents_observer.h:181: // changes.
|security_style|
> is the new
> nit: Looks like "changes" fits on the previous line, as before.
> Similar issue with next line.
> 
>
https://codereview.chromium.org/1181293003/diff/150001/content/public/common/...
> File content/public/common/security_style.h (right):
> 
>
https://codereview.chromium.org/1181293003/diff/150001/content/public/common/...
> content/public/common/security_style.h:50: struct SecurityStyleExplanation {
> From the content API guidelines:
> "each interface/struct/enum should be in a separate file"
> 
> http://www.chromium.org/developers/content-module/content-api
> 
> It's admittedly unfortunate to need separate files for both
> SecurityStyleExplanation and SecurityStyleExplanations, but I want to get
John's
> opinion on the approach anyway.

[estark, 2015-06-17 19:00:39]

On 2015/06/17 18:50:15, estark wrote:
> On 2015/06/17 18:37:40, Charlie Reis wrote:
> > [+jam]
> > 
> > I'd like to do a quick sanity check on whether we need the new structs in
the
> > content/public API.  On one hand, we have things like
DevToolsManagerDelegate
> > for getting info from the embedder without threading it through most of
> content.
> >  On the other hand, we'd probably want it in a structured form inside
> DevTools,
> > so a type for it may be worthwhile.
> > 
> > How is it going to be consumed?  Will it need to be converted again to be
> passed
> > into Blink?  In that case, maybe we should use a public Blink type instead
of
> a
> > content type.
> 
> The plan is to add a parameter for it on the SecurityStyleChanged event in
> devtools's protocol.json (the remote debugging protocol). SecurityHandler (in
> //content/browser/devtools) observes the
> WebContentsObserver::SecurityStyleChanged event and will include the
> SecurityStyleExplanations as a parameter in the message that it sends. (Which
> will be received by devtools javascript code in blink). So the plan is to
> basically consume it the same way SecurityStyle is currently consumed:
//chrome
> sets the explanation and passes it through //content to the SecurityHandler
(in
> //content), which sends it over the devtools remote debugging protocol.

https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/de...
is where we'd like to pass the explanation to devtools, in case I wasn't clear.

> 
> > 
> > (We generally don't want to add things to content/public until they're used
> both
> > inside and outside content, so it would be useful to know how it will be
> used.)
> > 
> >
>
https://codereview.chromium.org/1181293003/diff/150001/content/public/browser...
> > File content/public/browser/web_contents_observer.h (right):
> > 
> >
>
https://codereview.chromium.org/1181293003/diff/150001/content/public/browser...
> > content/public/browser/web_contents_observer.h:181: // changes.
> |security_style|
> > is the new
> > nit: Looks like "changes" fits on the previous line, as before.
> > Similar issue with next line.
> > 
> >
>
https://codereview.chromium.org/1181293003/diff/150001/content/public/common/...
> > File content/public/common/security_style.h (right):
> > 
> >
>
https://codereview.chromium.org/1181293003/diff/150001/content/public/common/...
> > content/public/common/security_style.h:50: struct SecurityStyleExplanation {
> > From the content API guidelines:
> > "each interface/struct/enum should be in a separate file"
> > 
> > http://www.chromium.org/developers/content-module/content-api
> > 
> > It's admittedly unfortunate to need separate files for both
> > SecurityStyleExplanation and SecurityStyleExplanations, but I want to get
> John's
> > opinion on the approach anyway.

[Charlie Reis, 2015-06-17 19:38:15]

On 2015/06/17 19:00:39, estark wrote:
> On 2015/06/17 18:50:15, estark wrote:
> > On 2015/06/17 18:37:40, Charlie Reis wrote:
> > > [+jam]
> > > 
> > > I'd like to do a quick sanity check on whether we need the new structs in
> the
> > > content/public API.  On one hand, we have things like
> DevToolsManagerDelegate
> > > for getting info from the embedder without threading it through most of
> > content.
> > >  On the other hand, we'd probably want it in a structured form inside
> > DevTools,
> > > so a type for it may be worthwhile.
> > > 
> > > How is it going to be consumed?  Will it need to be converted again to be
> > passed
> > > into Blink?  In that case, maybe we should use a public Blink type instead
> of
> > a
> > > content type.
> > 
> > The plan is to add a parameter for it on the SecurityStyleChanged event in
> > devtools's protocol.json (the remote debugging protocol). SecurityHandler
(in
> > //content/browser/devtools) observes the
> > WebContentsObserver::SecurityStyleChanged event and will include the
> > SecurityStyleExplanations as a parameter in the message that it sends.
(Which
> > will be received by devtools javascript code in blink). So the plan is to
> > basically consume it the same way SecurityStyle is currently consumed:
> //chrome
> > sets the explanation and passes it through //content to the SecurityHandler
> (in
> > //content), which sends it over the devtools remote debugging protocol.
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/de...
> is where we'd like to pass the explanation to devtools, in case I wasn't
clear.
> 

I see.  So yes, it will be processed within content and then sent out via JSON. 
Given the current structure of the DevTools code, this seems reasonable to me.

Should be ok from my perspective once the other comments are addressed, though
it would be nice for John to confirm as well.

[estark, 2015-06-17 20:35:58]

Cool, thanks creis.

https://codereview.chromium.org/1181293003/diff/150001/content/public/browser...
File content/public/browser/web_contents_observer.h (right):

https://codereview.chromium.org/1181293003/diff/150001/content/public/browser...
content/public/browser/web_contents_observer.h:181: // changes. |security_style|
is the new
On 2015/06/17 18:37:40, Charlie Reis wrote:
> nit: Looks like "changes" fits on the previous line, as before.
> Similar issue with next line.

Done.

https://codereview.chromium.org/1181293003/diff/150001/content/public/common/...
File content/public/common/security_style.h (right):

https://codereview.chromium.org/1181293003/diff/150001/content/public/common/...
content/public/common/security_style.h:50: struct SecurityStyleExplanation {
On 2015/06/17 18:37:40, Charlie Reis wrote:
> From the content API guidelines:
> "each interface/struct/enum should be in a separate file"
> 
> http://www.chromium.org/developers/content-module/content-api
> 
> It's admittedly unfortunate to need separate files for both
> SecurityStyleExplanation and SecurityStyleExplanations, but I want to get
John's
> opinion on the approach anyway.

Done.

[jam, 2015-06-17 20:45:10]

so I just took a closer look at the GetSecurityStyle method which was added in
https://codereview.chromium.org/1162663004/

It's not clear to me why this is something that the embedder provides. i.e.
content seems to have all the information to set this itself without asking
chrome layer. The one exception is the chromeos bits in
connection_security_helper.cc, but it seems we can have an embedder callback
just for that  (i.e. WebContentsDelegate::UsedPolicyCertificates or something
similar). but the rest of the logic is stuff we want every content embedder to
have for free and not to have to do this themselves

[jam, 2015-06-17 20:46:00]

On 2015/06/17 20:45:10, jam wrote:
> so I just took a closer look at the GetSecurityStyle method which was added in
> https://codereview.chromium.org/1162663004/
> 
> It's not clear to me why this is something that the embedder provides. i.e.
> content seems to have all the information to set this itself without asking
> chrome layer. The one exception is the chromeos bits in
> connection_security_helper.cc, but it seems we can have an embedder callback
> just for that  (i.e. WebContentsDelegate::UsedPolicyCertificates or something
> similar). but the rest of the logic is stuff we want every content embedder to
> have for free and not to have to do this themselves

and if this is done, then these new structs wouldn't need to be exposed in
content/public, right?

[estark, 2015-06-17 20:46:41]

On 2015/06/17 20:45:10, jam wrote:
> so I just took a closer look at the GetSecurityStyle method which was added in
> https://codereview.chromium.org/1162663004/
> 
> It's not clear to me why this is something that the embedder provides. i.e.
> content seems to have all the information to set this itself without asking
> chrome layer. The one exception is the chromeos bits in
> connection_security_helper.cc, but it seems we can have an embedder callback
> just for that  (i.e. WebContentsDelegate::UsedPolicyCertificates or something
> similar). but the rest of the logic is stuff we want every content embedder to
> have for free and not to have to do this themselves

The SHA1 deprecation policy is all Chrome-specific, in that other embedders
shouldn't have to deprecate SHA1 at the same pace or in the same way that Chrome
does.

[estark, 2015-06-17 20:50:31]

On 2015/06/17 20:46:41, estark wrote:
> On 2015/06/17 20:45:10, jam wrote:
> > so I just took a closer look at the GetSecurityStyle method which was added
in
> > https://codereview.chromium.org/1162663004/
> > 
> > It's not clear to me why this is something that the embedder provides. i.e.
> > content seems to have all the information to set this itself without asking
> > chrome layer. The one exception is the chromeos bits in
> > connection_security_helper.cc, but it seems we can have an embedder callback
> > just for that  (i.e. WebContentsDelegate::UsedPolicyCertificates or
something
> > similar). but the rest of the logic is stuff we want every content embedder
to
> > have for free and not to have to do this themselves
> 
> The SHA1 deprecation policy is all Chrome-specific, in that other embedders
> shouldn't have to deprecate SHA1 at the same pace or in the same way that
Chrome
> does.

Also the GetSecurityLevelForNonSecureFieldTrial() logic, which allows users to
opt-in to show http:// sites as non-secure or dubious.

[jam, 2015-06-17 21:03:46]

On 2015/06/17 20:46:41, estark wrote:
> On 2015/06/17 20:45:10, jam wrote:
> > so I just took a closer look at the GetSecurityStyle method which was added
in
> > https://codereview.chromium.org/1162663004/
> > 
> > It's not clear to me why this is something that the embedder provides. i.e.
> > content seems to have all the information to set this itself without asking
> > chrome layer. The one exception is the chromeos bits in
> > connection_security_helper.cc, but it seems we can have an embedder callback
> > just for that  (i.e. WebContentsDelegate::UsedPolicyCertificates or
something
> > similar). but the rest of the logic is stuff we want every content embedder
to
> > have for free and not to have to do this themselves
> 
> The SHA1 deprecation policy is all Chrome-specific, in that other embedders
> shouldn't have to deprecate SHA1 at the same pace or in the same way that
Chrome
> does.

It's not clear to me why that is so?
It seems that if we decide we want to deprecate SHA1 to improve security, we
should assume that this improved security would benefit all content embedders.
Have other embedders explicitly reached out to us and said they don't want to do
this? Even if that did happen, we could add one specific embedder callback for
that.

[jam, 2015-06-17 21:05:39]

On 2015/06/17 20:50:31, estark wrote:
> On 2015/06/17 20:46:41, estark wrote:
> > On 2015/06/17 20:45:10, jam wrote:
> > > so I just took a closer look at the GetSecurityStyle method which was
added
> in
> > > https://codereview.chromium.org/1162663004/
> > > 
> > > It's not clear to me why this is something that the embedder provides.
i.e.
> > > content seems to have all the information to set this itself without
asking
> > > chrome layer. The one exception is the chromeos bits in
> > > connection_security_helper.cc, but it seems we can have an embedder
callback
> > > just for that  (i.e. WebContentsDelegate::UsedPolicyCertificates or
> something
> > > similar). but the rest of the logic is stuff we want every content
embedder
> to
> > > have for free and not to have to do this themselves
> > 
> > The SHA1 deprecation policy is all Chrome-specific, in that other embedders
> > shouldn't have to deprecate SHA1 at the same pace or in the same way that
> Chrome
> > does.
> 
> Also the GetSecurityLevelForNonSecureFieldTrial() logic, which allows users to
> opt-in to show http:// sites as non-secure or dubious.

Why can't this be in content? we have other field trials there.

[estark, 2015-06-17 21:08:28]

On 2015/06/17 21:03:46, jam wrote:
> On 2015/06/17 20:46:41, estark wrote:
> > On 2015/06/17 20:45:10, jam wrote:
> > > so I just took a closer look at the GetSecurityStyle method which was
added
> in
> > > https://codereview.chromium.org/1162663004/
> > > 
> > > It's not clear to me why this is something that the embedder provides.
i.e.
> > > content seems to have all the information to set this itself without
asking
> > > chrome layer. The one exception is the chromeos bits in
> > > connection_security_helper.cc, but it seems we can have an embedder
callback
> > > just for that  (i.e. WebContentsDelegate::UsedPolicyCertificates or
> something
> > > similar). but the rest of the logic is stuff we want every content
embedder
> to
> > > have for free and not to have to do this themselves
> > 
> > The SHA1 deprecation policy is all Chrome-specific, in that other embedders
> > shouldn't have to deprecate SHA1 at the same pace or in the same way that
> Chrome
> > does.
> 
> It's not clear to me why that is so?
> It seems that if we decide we want to deprecate SHA1 to improve security, we
> should assume that this improved security would benefit all content embedders.
> Have other embedders explicitly reached out to us and said they don't want to
do
> this? Even if that did happen, we could add one specific embedder callback for
> that.

rsleevi: I know you have strong opinions on this, do you have any thoughts?

I do remember talking about this at one point; one problem is that we'd end up
with a good number of embedder callbacks for embedder-specific things
(UsedPolicyCerts(), UsedDeprecatedSHA1(), UsesMarkNonsecureAsFieldTrial()), and
many of those callbacks probably don't even make sense for non-Chrome embedders.
The concept of using policy certs is a ChromeOS-specific idea, IIUC, and
definitely the mark-nonsecure-as field trial doesn't make sense for other
embedders.

[Ryan Sleevi, 2015-06-17 21:15:45]

On 2015/06/17 21:08:28, estark wrote:
> I do remember talking about this at one point; one problem is that we'd end up
> with a good number of embedder callbacks for embedder-specific things
> (UsedPolicyCerts(), UsedDeprecatedSHA1(), UsesMarkNonsecureAsFieldTrial()),
and
> many of those callbacks probably don't even make sense for non-Chrome
embedders.
> The concept of using policy certs is a ChromeOS-specific idea, IIUC, and
> definitely the mark-nonsecure-as field trial doesn't make sense for other
> embedders.

Chrome-only concepts:
- Certificate Transparency (requires a regular update cycle to keep the
ecosystem healthy, and policies applicable to Chrome)
- Policy certificates ( ChromeOS-only concept)
- SHA-1 deprecation (Until 2017, when it's fully turned off, then an embedder
who doesn't have a rapid update cycle is not only at risk of breaking, but their
breakage may be used as an excuse to delay the deprecation further)

It seems very wrong for us to impose UI policy (and that's what this is, moreso
than security policy) on embedders. Certainly, it doesn't seem appropriate to
extend ChromeOS ideas into content.

[jam, 2015-06-17 21:16:05]

On 2015/06/17 21:08:28, estark wrote:
> On 2015/06/17 21:03:46, jam wrote:
> > On 2015/06/17 20:46:41, estark wrote:
> > > On 2015/06/17 20:45:10, jam wrote:
> > > > so I just took a closer look at the GetSecurityStyle method which was
> added
> > in
> > > > https://codereview.chromium.org/1162663004/
> > > > 
> > > > It's not clear to me why this is something that the embedder provides.
> i.e.
> > > > content seems to have all the information to set this itself without
> asking
> > > > chrome layer. The one exception is the chromeos bits in
> > > > connection_security_helper.cc, but it seems we can have an embedder
> callback
> > > > just for that  (i.e. WebContentsDelegate::UsedPolicyCertificates or
> > something
> > > > similar). but the rest of the logic is stuff we want every content
> embedder
> > to
> > > > have for free and not to have to do this themselves
> > > 
> > > The SHA1 deprecation policy is all Chrome-specific, in that other
embedders
> > > shouldn't have to deprecate SHA1 at the same pace or in the same way that
> > Chrome
> > > does.
> > 
> > It's not clear to me why that is so?
> > It seems that if we decide we want to deprecate SHA1 to improve security, we
> > should assume that this improved security would benefit all content
embedders.
> > Have other embedders explicitly reached out to us and said they don't want
to
> do
> > this? Even if that did happen, we could add one specific embedder callback
for
> > that.
> 
> rsleevi: I know you have strong opinions on this, do you have any thoughts?
> 
> I do remember talking about this at one point; one problem is that we'd end up
> with a good number of embedder callbacks for embedder-specific things
> (UsedPolicyCerts(), UsedDeprecatedSHA1(), UsesMarkNonsecureAsFieldTrial()), 

Right now, the only thing that is required is UsedPolicyCerts, since that uses
policy code in src/chrome

The others can all be in content and we don't need embedder callbacks for them.

> and
> many of those callbacks probably don't even make sense for non-Chrome
embedders.
> The concept of using policy certs is a ChromeOS-specific idea, IIUC, and
> definitely the mark-nonsecure-as field trial doesn't make sense for other
> embedders.

Other embedders don't use field trials anyways...

[jam, 2015-06-17 21:36:50]

On 2015/06/17 21:15:45, Ryan Sleevi wrote:
> On 2015/06/17 21:08:28, estark wrote:
> > I do remember talking about this at one point; one problem is that we'd end
up
> > with a good number of embedder callbacks for embedder-specific things
> > (UsedPolicyCerts(), UsedDeprecatedSHA1(), UsesMarkNonsecureAsFieldTrial()),
> and
> > many of those callbacks probably don't even make sense for non-Chrome
> embedders.
> > The concept of using policy certs is a ChromeOS-specific idea, IIUC, and
> > definitely the mark-nonsecure-as field trial doesn't make sense for other
> > embedders.
> 
> Chrome-only concepts:
> - Certificate Transparency (requires a regular update cycle to keep the
> ecosystem healthy, and policies applicable to Chrome)

Why is this a chrome-only concept. It's a in a module that sits below content,
so any content embedder can set the whitelist.

> - Policy certificates ( ChromeOS-only concept)

(see below, i agree and this would have to be in a focused callback)

> - SHA-1 deprecation (Until 2017, when it's fully turned off, then an embedder
> who doesn't have a rapid update cycle is not only at risk of breaking, but
their
> breakage may be used as an excuse to delay the deprecation further)

In my reading of the code, this doesn't change how content loads the data, just
what it sends devtools. So unless I'm misreading the code, I'm not sure how this
would break a client.

> 
> It seems very wrong for us to impose UI policy (and that's what this is,
moreso
> than security policy) on embedders.

How is this UI policy? I don't see this changing chrome UI; the only thing I see
is that it's sent to devtools.


> Certainly, it doesn't seem appropriate to
> extend ChromeOS ideas into content.

I suggested an embedder callback just for the cros case (that is the only
possible solution if this code is in content)

[Ryan Sleevi, 2015-06-17 21:51:34]

On 2015/06/17 21:36:50, jam wrote:
> Why is this a chrome-only concept. It's a in a module that sits below content,
> so any content embedder can set the whitelist.

The same reason that HPKP (implemented in //net) is a Chrome-only concept (and
explicitly not allowed on mobile). It requires a regular update cycle, and can
cause significant harm if not.

> In my reading of the code, this doesn't change how content loads the data,
just
> what it sends devtools. So unless I'm misreading the code, I'm not sure how
this
> would break a client.

It sends to DevTools the result of UI policy, which certainly varies by
embedder. We've taken a stronger UI policy than Firefox (console.log) or
Microsoft (no warning), and that UI behaviour would be entirely inappropriate
for some uses (Chromium Embedded, Steam, etc)

There's zero reason for //content to know about //chrome's UI policies and
treatments, and the SHA-1 policy intentionally doesn't affect loading in //net
until 2016.

> How is this UI policy? I don't see this changing chrome UI; the only thing I
see
> is that it's sent to devtools.

The entirity of the "SHA-1 issue" is implemented in //chrome, and solely used to
affect UI.

To place this in //content sets a dangerous precedent that I would strongly
prefer not to set, which is that any possible UI policies that //chrome takes
should be expressed in //content. Fundamentally, the question of "How do you
present the omnibox and security indicators" is entirely left to embedders, and
the purpose of this CL is to provide an embedder-agnostic way for an embedder to
explain those policies and how the decisions were made.

> I suggested an embedder callback just for the cros case (that is the only
> possible solution if this code is in content)

I'm still having trouble seeing the value here in such a callback.


Since there's definitely a communication gap, with you asking "Why didn't you do
X" (and these previous replies being answers to that), could you help us
understand "Why should we do X?" What benefits does it provide versus the
complexity of exposing Omnibox UI policies through //content and requiring that
any embedders that wish to use alternative policies to customize //content?

[estark, 2015-06-17 21:51:58]

On 2015/06/17 21:36:50, jam wrote:
> On 2015/06/17 21:15:45, Ryan Sleevi wrote:
> > On 2015/06/17 21:08:28, estark wrote:
> > > I do remember talking about this at one point; one problem is that we'd
end
> up
> > > with a good number of embedder callbacks for embedder-specific things
> > > (UsedPolicyCerts(), UsedDeprecatedSHA1(),
UsesMarkNonsecureAsFieldTrial()),
> > and
> > > many of those callbacks probably don't even make sense for non-Chrome
> > embedders.
> > > The concept of using policy certs is a ChromeOS-specific idea, IIUC, and
> > > definitely the mark-nonsecure-as field trial doesn't make sense for other
> > > embedders.
> > 
> > Chrome-only concepts:
> > - Certificate Transparency (requires a regular update cycle to keep the
> > ecosystem healthy, and policies applicable to Chrome)
> 
> Why is this a chrome-only concept. It's a in a module that sits below content,
> so any content embedder can set the whitelist.
> 
> > - Policy certificates ( ChromeOS-only concept)
> 
> (see below, i agree and this would have to be in a focused callback)
> 
> > - SHA-1 deprecation (Until 2017, when it's fully turned off, then an
embedder
> > who doesn't have a rapid update cycle is not only at risk of breaking, but
> their
> > breakage may be used as an excuse to delay the deprecation further)
> 
> In my reading of the code, this doesn't change how content loads the data,
just
> what it sends devtools. So unless I'm misreading the code, I'm not sure how
this
> would break a client.

GetSecurityLevelForWebContents() is used to pick the lock icon in the omnibox --
is that what you mean?

> 
> > 
> > It seems very wrong for us to impose UI policy (and that's what this is,
> moreso
> > than security policy) on embedders.
> 
> How is this UI policy? I don't see this changing chrome UI; the only thing I
see
> is that it's sent to devtools.

See above; GetSecurityLevelForWebContents() is used to pick the lock icon.

> 
> 
> > Certainly, it doesn't seem appropriate to
> > extend ChromeOS ideas into content.
> 
> I suggested an embedder callback just for the cros case (that is the only
> possible solution if this code is in content)

I think the objection is that the idea of "policy certs" is itself a CrOS idea.
So having a UsedPolicyCerts() callback is introducing CrOS knowledge into
//content just by virtue of the callback name.

[jam, 2015-06-18 00:36:58]

On 2015/06/17 21:51:34, Ryan Sleevi wrote:
> On 2015/06/17 21:36:50, jam wrote:
> > Why is this a chrome-only concept. It's a in a module that sits below
content,
> > so any content embedder can set the whitelist.
> 
> The same reason that HPKP (implemented in //net) is a Chrome-only concept (and
> explicitly not allowed on mobile). It requires a regular update cycle, and can
> cause significant harm if not.
> 
> > In my reading of the code, this doesn't change how content loads the data,
> just
> > what it sends devtools. So unless I'm misreading the code, I'm not sure how
> this
> > would break a client.
> 
> It sends to DevTools the result of UI policy, which certainly varies by
> embedder. We've taken a stronger UI policy than Firefox (console.log) or
> Microsoft (no warning), and that UI behaviour would be entirely inappropriate
> for some uses (Chromium Embedded, Steam, etc)
> 
> There's zero reason for //content to know about //chrome's UI policies and
> treatments, and the SHA-1 policy intentionally doesn't affect loading in //net
> until 2016.

ah, ok I didn't notice initially that the method that is called in chrome to get
this result for WCD is the one called by chrome ui. i understand the pushback
now and i agree.

> 
> > How is this UI policy? I don't see this changing chrome UI; the only thing I
> see
> > is that it's sent to devtools.
> 
> The entirity of the "SHA-1 issue" is implemented in //chrome, and solely used
to
> affect UI.
> 
> To place this in //content sets a dangerous precedent that I would strongly
> prefer not to set, which is that any possible UI policies that //chrome takes
> should be expressed in //content. Fundamentally, the question of "How do you
> present the omnibox and security indicators" is entirely left to embedders,
and
> the purpose of this CL is to provide an embedder-agnostic way for an embedder
to
> explain those policies and how the decisions were made.
> 
> > I suggested an embedder callback just for the cros case (that is the only
> > possible solution if this code is in content)
> 
> I'm still having trouble seeing the value here in such a callback.
> 
> 
> Since there's definitely a communication gap, with you asking "Why didn't you
do
> X" (and these previous replies being answers to that), could you help us
> understand "Why should we do X?" What benefits does it provide versus the
> complexity of exposing Omnibox UI policies through //content and requiring
that
> any embedders that wish to use alternative policies to customize //content?

mostly it was about reducing content api's growth.

instead of content asking chrome for ui result so that it can send it to
devtools, can chrome send it directly instead?

[estark, 2015-06-18 00:55:41]

On 2015/06/18 00:36:58, jam wrote:
> On 2015/06/17 21:51:34, Ryan Sleevi wrote:
> > On 2015/06/17 21:36:50, jam wrote:
> > > Why is this a chrome-only concept. It's a in a module that sits below
> content,
> > > so any content embedder can set the whitelist.
> > 
> > The same reason that HPKP (implemented in //net) is a Chrome-only concept
(and
> > explicitly not allowed on mobile). It requires a regular update cycle, and
can
> > cause significant harm if not.
> > 
> > > In my reading of the code, this doesn't change how content loads the data,
> > just
> > > what it sends devtools. So unless I'm misreading the code, I'm not sure
how
> > this
> > > would break a client.
> > 
> > It sends to DevTools the result of UI policy, which certainly varies by
> > embedder. We've taken a stronger UI policy than Firefox (console.log) or
> > Microsoft (no warning), and that UI behaviour would be entirely
inappropriate
> > for some uses (Chromium Embedded, Steam, etc)
> > 
> > There's zero reason for //content to know about //chrome's UI policies and
> > treatments, and the SHA-1 policy intentionally doesn't affect loading in
//net
> > until 2016.
> 
> ah, ok I didn't notice initially that the method that is called in chrome to
get
> this result for WCD is the one called by chrome ui. i understand the pushback
> now and i agree.
> 
> > 
> > > How is this UI policy? I don't see this changing chrome UI; the only thing
I
> > see
> > > is that it's sent to devtools.
> > 
> > The entirity of the "SHA-1 issue" is implemented in //chrome, and solely
used
> to
> > affect UI.
> > 
> > To place this in //content sets a dangerous precedent that I would strongly
> > prefer not to set, which is that any possible UI policies that //chrome
takes
> > should be expressed in //content. Fundamentally, the question of "How do you
> > present the omnibox and security indicators" is entirely left to embedders,
> and
> > the purpose of this CL is to provide an embedder-agnostic way for an
embedder
> to
> > explain those policies and how the decisions were made.
> > 
> > > I suggested an embedder callback just for the cros case (that is the only
> > > possible solution if this code is in content)
> > 
> > I'm still having trouble seeing the value here in such a callback.
> > 
> > 
> > Since there's definitely a communication gap, with you asking "Why didn't
you
> do
> > X" (and these previous replies being answers to that), could you help us
> > understand "Why should we do X?" What benefits does it provide versus the
> > complexity of exposing Omnibox UI policies through //content and requiring
> that
> > any embedders that wish to use alternative policies to customize //content?
> 
> mostly it was about reducing content api's growth.
> 
> instead of content asking chrome for ui result so that it can send it to
> devtools, can chrome send it directly instead?

We discussed this approach quite a bit with Pavel. My understanding is that
there does exist a way to intercept commands sent from devtools to the browser
in //chrome such that they don't go through //content
(https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/dev...),
but the drawbacks are:

1. It's kind of hacky and discouraged unless really necessary.
2. The devtools infrastructure currently supports handling commands from
devtools to the browser in //chrome, but not sending events from //chrome to
devtools, which is what we want to do (so we'd have to wait for the devtools
team to figure out how to do that and build it).
3. I think it actually makes sense for the event to be sent from //content: it
would be nice for any embedder to be able to get the security panel
functionality by just implementing WCD::GetSecurityStyle instead of having to
re-implement all the boilerplate of a devtools protocol handler itself.

So we wanted to avoid the //chrome-direct-to-devtools approach until we actually
had to send Chrome-specific information.

[lgarron, 2015-06-18 01:28:46]

https://codereview.chromium.org/1181293003/diff/210001/chrome/app/generated_r...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/1181293003/diff/210001/chrome/app/generated_r...
chrome/app/generated_resources.grd:15667: +        Mixed Content
estark@: Would you mind changing this to "Mixed Scripts" (or "Active Mixed
Content")?

[Ryan Sleevi, 2015-06-18 01:34:04]

https://codereview.chromium.org/1181293003/diff/210001/chrome/app/generated_r...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/1181293003/diff/210001/chrome/app/generated_r...
chrome/app/generated_resources.grd:15667: +        Mixed Content
On 2015/06/18 01:28:45, lgarron wrote:
> estark@: Would you mind changing this to "Mixed Scripts" (or "Active Mixed
> Content")?

-1 scripts (can apply to other bits, such as ws:// connections)
+1 to Active Mixed Content

and with the bonus nitting that these have yet another (not _yet_ well
publicized) name of "Blockable Content" (
https://w3c.github.io/webappsec/specs/mixedcontent/#category-blockable )

[lgarron, 2015-06-18 01:47:24]

lgarron@chromium.org changed reviewers:
+ lgarron@chromium.org

[lgarron, 2015-06-18 01:47:25]

https://codereview.chromium.org/1181293003/diff/210001/chrome/app/generated_r...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/1181293003/diff/210001/chrome/app/generated_r...
chrome/app/generated_resources.grd:15667: +        Mixed Content
On 2015/06/18 at 01:34:03, Ryan Sleevi wrote:
> On 2015/06/18 01:28:45, lgarron wrote:
> > estark@: Would you mind changing this to "Mixed Scripts" (or "Active Mixed
> > Content")?
> 
> -1 scripts (can apply to other bits, such as ws:// connections)
> +1 to Active Mixed Content
> 
> and with the bonus nitting that these have yet another (not _yet_ well
publicized) name of "Blockable Content" (
https://w3c.github.io/webappsec/specs/mixedcontent/#category-blockable )

We only talk about "scripts" to end users [1]. I'm fine with "Active Mixed
Content", although then we should probably update
IDS_ACTIVE_MIXED_CONTENT_DESCRIPTION to mention other common active mixed
content (e.g. iframes).

[1] https://support.google.com/chrome/answer/1342714

[jam, 2015-06-18 14:51:12]

On 2015/06/18 00:55:41, estark wrote:
> On 2015/06/18 00:36:58, jam wrote:
> > On 2015/06/17 21:51:34, Ryan Sleevi wrote:
> > > On 2015/06/17 21:36:50, jam wrote:
> > > > Why is this a chrome-only concept. It's a in a module that sits below
> > content,
> > > > so any content embedder can set the whitelist.
> > > 
> > > The same reason that HPKP (implemented in //net) is a Chrome-only concept
> (and
> > > explicitly not allowed on mobile). It requires a regular update cycle, and
> can
> > > cause significant harm if not.
> > > 
> > > > In my reading of the code, this doesn't change how content loads the
data,
> > > just
> > > > what it sends devtools. So unless I'm misreading the code, I'm not sure
> how
> > > this
> > > > would break a client.
> > > 
> > > It sends to DevTools the result of UI policy, which certainly varies by
> > > embedder. We've taken a stronger UI policy than Firefox (console.log) or
> > > Microsoft (no warning), and that UI behaviour would be entirely
> inappropriate
> > > for some uses (Chromium Embedded, Steam, etc)
> > > 
> > > There's zero reason for //content to know about //chrome's UI policies and
> > > treatments, and the SHA-1 policy intentionally doesn't affect loading in
> //net
> > > until 2016.
> > 
> > ah, ok I didn't notice initially that the method that is called in chrome to
> get
> > this result for WCD is the one called by chrome ui. i understand the
pushback
> > now and i agree.
> > 
> > > 
> > > > How is this UI policy? I don't see this changing chrome UI; the only
thing
> I
> > > see
> > > > is that it's sent to devtools.
> > > 
> > > The entirity of the "SHA-1 issue" is implemented in //chrome, and solely
> used
> > to
> > > affect UI.
> > > 
> > > To place this in //content sets a dangerous precedent that I would
strongly
> > > prefer not to set, which is that any possible UI policies that //chrome
> takes
> > > should be expressed in //content. Fundamentally, the question of "How do
you
> > > present the omnibox and security indicators" is entirely left to
embedders,
> > and
> > > the purpose of this CL is to provide an embedder-agnostic way for an
> embedder
> > to
> > > explain those policies and how the decisions were made.
> > > 
> > > > I suggested an embedder callback just for the cros case (that is the
only
> > > > possible solution if this code is in content)
> > > 
> > > I'm still having trouble seeing the value here in such a callback.
> > > 
> > > 
> > > Since there's definitely a communication gap, with you asking "Why didn't
> you
> > do
> > > X" (and these previous replies being answers to that), could you help us
> > > understand "Why should we do X?" What benefits does it provide versus the
> > > complexity of exposing Omnibox UI policies through //content and requiring
> > that
> > > any embedders that wish to use alternative policies to customize
//content?
> > 
> > mostly it was about reducing content api's growth.
> > 
> > instead of content asking chrome for ui result so that it can send it to
> > devtools, can chrome send it directly instead?
> 
> We discussed this approach quite a bit with Pavel. My understanding is that
> there does exist a way to intercept commands sent from devtools to the browser
> in //chrome such that they don't go through //content
>
(https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/dev...),
> but the drawbacks are:
> 
> 1. It's kind of hacky and discouraged unless really necessary.
> 2. The devtools infrastructure currently supports handling commands from
> devtools to the browser in //chrome, but not sending events from //chrome to
> devtools, which is what we want to do (so we'd have to wait for the devtools
> team to figure out how to do that and build it).

Shouldn't this be easy, i.e. a method that takes a string?

> 3. I think it actually makes sense for the event to be sent from //content: it
> would be nice for any embedder to be able to get the security panel
> functionality by just implementing WCD::GetSecurityStyle instead of having to
> re-implement all the boilerplate of a devtools protocol handler itself.

That depends on what the API of 2 is right? i.e. if there's a method in
content/public that sends a string to devtools, then there shouldn't be much
work to implement on the embedder side.

> 
> So we wanted to avoid the //chrome-direct-to-devtools approach until we
actually
> had to send Chrome-specific information.

[estark, 2015-06-18 14:56:28]

On 2015/06/18 14:51:12, jam wrote:
> On 2015/06/18 00:55:41, estark wrote:
> > On 2015/06/18 00:36:58, jam wrote:
> > > On 2015/06/17 21:51:34, Ryan Sleevi wrote:
> > > > On 2015/06/17 21:36:50, jam wrote:
> > > > > Why is this a chrome-only concept. It's a in a module that sits below
> > > content,
> > > > > so any content embedder can set the whitelist.
> > > > 
> > > > The same reason that HPKP (implemented in //net) is a Chrome-only
concept
> > (and
> > > > explicitly not allowed on mobile). It requires a regular update cycle,
and
> > can
> > > > cause significant harm if not.
> > > > 
> > > > > In my reading of the code, this doesn't change how content loads the
> data,
> > > > just
> > > > > what it sends devtools. So unless I'm misreading the code, I'm not
sure
> > how
> > > > this
> > > > > would break a client.
> > > > 
> > > > It sends to DevTools the result of UI policy, which certainly varies by
> > > > embedder. We've taken a stronger UI policy than Firefox (console.log) or
> > > > Microsoft (no warning), and that UI behaviour would be entirely
> > inappropriate
> > > > for some uses (Chromium Embedded, Steam, etc)
> > > > 
> > > > There's zero reason for //content to know about //chrome's UI policies
and
> > > > treatments, and the SHA-1 policy intentionally doesn't affect loading in
> > //net
> > > > until 2016.
> > > 
> > > ah, ok I didn't notice initially that the method that is called in chrome
to
> > get
> > > this result for WCD is the one called by chrome ui. i understand the
> pushback
> > > now and i agree.
> > > 
> > > > 
> > > > > How is this UI policy? I don't see this changing chrome UI; the only
> thing
> > I
> > > > see
> > > > > is that it's sent to devtools.
> > > > 
> > > > The entirity of the "SHA-1 issue" is implemented in //chrome, and solely
> > used
> > > to
> > > > affect UI.
> > > > 
> > > > To place this in //content sets a dangerous precedent that I would
> strongly
> > > > prefer not to set, which is that any possible UI policies that //chrome
> > takes
> > > > should be expressed in //content. Fundamentally, the question of "How do
> you
> > > > present the omnibox and security indicators" is entirely left to
> embedders,
> > > and
> > > > the purpose of this CL is to provide an embedder-agnostic way for an
> > embedder
> > > to
> > > > explain those policies and how the decisions were made.
> > > > 
> > > > > I suggested an embedder callback just for the cros case (that is the
> only
> > > > > possible solution if this code is in content)
> > > > 
> > > > I'm still having trouble seeing the value here in such a callback.
> > > > 
> > > > 
> > > > Since there's definitely a communication gap, with you asking "Why
didn't
> > you
> > > do
> > > > X" (and these previous replies being answers to that), could you help us
> > > > understand "Why should we do X?" What benefits does it provide versus
the
> > > > complexity of exposing Omnibox UI policies through //content and
requiring
> > > that
> > > > any embedders that wish to use alternative policies to customize
> //content?
> > > 
> > > mostly it was about reducing content api's growth.
> > > 
> > > instead of content asking chrome for ui result so that it can send it to
> > > devtools, can chrome send it directly instead?
> > 
> > We discussed this approach quite a bit with Pavel. My understanding is that
> > there does exist a way to intercept commands sent from devtools to the
browser
> > in //chrome such that they don't go through //content
> >
>
(https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/dev...),
> > but the drawbacks are:
> > 
> > 1. It's kind of hacky and discouraged unless really necessary.
> > 2. The devtools infrastructure currently supports handling commands from
> > devtools to the browser in //chrome, but not sending events from //chrome to
> > devtools, which is what we want to do (so we'd have to wait for the devtools
> > team to figure out how to do that and build it).
> 
> Shouldn't this be easy, i.e. a method that takes a string?

I don't understand, sorry -- can you be more specific? Or is there an example
you can point to? (I guess I don't understand what "the string" is that you're
referring to, or what the method would be called and what exactly it would do.)

> 
> > 3. I think it actually makes sense for the event to be sent from //content:
it
> > would be nice for any embedder to be able to get the security panel
> > functionality by just implementing WCD::GetSecurityStyle instead of having
to
> > re-implement all the boilerplate of a devtools protocol handler itself.
> 
> That depends on what the API of 2 is right? i.e. if there's a method in
> content/public that sends a string to devtools, then there shouldn't be much
> work to implement on the embedder side.
> 
> > 
> > So we wanted to avoid the //chrome-direct-to-devtools approach until we
> actually
> > had to send Chrome-specific information.

[jam, 2015-06-18 15:19:41]

On 2015/06/18 14:56:28, estark wrote:
> On 2015/06/18 14:51:12, jam wrote:
> > On 2015/06/18 00:55:41, estark wrote:
> > > On 2015/06/18 00:36:58, jam wrote:
> > > > On 2015/06/17 21:51:34, Ryan Sleevi wrote:
> > > > > On 2015/06/17 21:36:50, jam wrote:
> > > > > > Why is this a chrome-only concept. It's a in a module that sits
below
> > > > content,
> > > > > > so any content embedder can set the whitelist.
> > > > > 
> > > > > The same reason that HPKP (implemented in //net) is a Chrome-only
> concept
> > > (and
> > > > > explicitly not allowed on mobile). It requires a regular update cycle,
> and
> > > can
> > > > > cause significant harm if not.
> > > > > 
> > > > > > In my reading of the code, this doesn't change how content loads the
> > data,
> > > > > just
> > > > > > what it sends devtools. So unless I'm misreading the code, I'm not
> sure
> > > how
> > > > > this
> > > > > > would break a client.
> > > > > 
> > > > > It sends to DevTools the result of UI policy, which certainly varies
by
> > > > > embedder. We've taken a stronger UI policy than Firefox (console.log)
or
> > > > > Microsoft (no warning), and that UI behaviour would be entirely
> > > inappropriate
> > > > > for some uses (Chromium Embedded, Steam, etc)
> > > > > 
> > > > > There's zero reason for //content to know about //chrome's UI policies
> and
> > > > > treatments, and the SHA-1 policy intentionally doesn't affect loading
in
> > > //net
> > > > > until 2016.
> > > > 
> > > > ah, ok I didn't notice initially that the method that is called in
chrome
> to
> > > get
> > > > this result for WCD is the one called by chrome ui. i understand the
> > pushback
> > > > now and i agree.
> > > > 
> > > > > 
> > > > > > How is this UI policy? I don't see this changing chrome UI; the only
> > thing
> > > I
> > > > > see
> > > > > > is that it's sent to devtools.
> > > > > 
> > > > > The entirity of the "SHA-1 issue" is implemented in //chrome, and
solely
> > > used
> > > > to
> > > > > affect UI.
> > > > > 
> > > > > To place this in //content sets a dangerous precedent that I would
> > strongly
> > > > > prefer not to set, which is that any possible UI policies that
//chrome
> > > takes
> > > > > should be expressed in //content. Fundamentally, the question of "How
do
> > you
> > > > > present the omnibox and security indicators" is entirely left to
> > embedders,
> > > > and
> > > > > the purpose of this CL is to provide an embedder-agnostic way for an
> > > embedder
> > > > to
> > > > > explain those policies and how the decisions were made.
> > > > > 
> > > > > > I suggested an embedder callback just for the cros case (that is the
> > only
> > > > > > possible solution if this code is in content)
> > > > > 
> > > > > I'm still having trouble seeing the value here in such a callback.
> > > > > 
> > > > > 
> > > > > Since there's definitely a communication gap, with you asking "Why
> didn't
> > > you
> > > > do
> > > > > X" (and these previous replies being answers to that), could you help
us
> > > > > understand "Why should we do X?" What benefits does it provide versus
> the
> > > > > complexity of exposing Omnibox UI policies through //content and
> requiring
> > > > that
> > > > > any embedders that wish to use alternative policies to customize
> > //content?
> > > > 
> > > > mostly it was about reducing content api's growth.
> > > > 
> > > > instead of content asking chrome for ui result so that it can send it to
> > > > devtools, can chrome send it directly instead?
> > > 
> > > We discussed this approach quite a bit with Pavel. My understanding is
that
> > > there does exist a way to intercept commands sent from devtools to the
> browser
> > > in //chrome such that they don't go through //content
> > >
> >
>
(https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/dev...),
> > > but the drawbacks are:
> > > 
> > > 1. It's kind of hacky and discouraged unless really necessary.
> > > 2. The devtools infrastructure currently supports handling commands from
> > > devtools to the browser in //chrome, but not sending events from //chrome
to
> > > devtools, which is what we want to do (so we'd have to wait for the
devtools
> > > team to figure out how to do that and build it).
> > 
> > Shouldn't this be easy, i.e. a method that takes a string?
> 
> I don't understand, sorry -- can you be more specific? Or is there an example
> you can point to? (I guess I don't understand what "the string" is that you're
> referring to, or what the method would be called and what exactly it would
do.)

oh, I'm just looking at SecurityHandler::SecurityStyleChanged which calls
Client::SecurityStateChanged and that eventually ends up in
DevToolsProtocolClient::SendRawMessage with a string.

> 
> > 
> > > 3. I think it actually makes sense for the event to be sent from
//content:
> it
> > > would be nice for any embedder to be able to get the security panel
> > > functionality by just implementing WCD::GetSecurityStyle instead of having
> to
> > > re-implement all the boilerplate of a devtools protocol handler itself.
> > 
> > That depends on what the API of 2 is right? i.e. if there's a method in
> > content/public that sends a string to devtools, then there shouldn't be much
> > work to implement on the embedder side.
> > 
> > > 
> > > So we wanted to avoid the //chrome-direct-to-devtools approach until we
> > actually
> > > had to send Chrome-specific information.

[estark, 2015-06-18 15:35:58]

On 2015/06/18 15:19:41, jam wrote:
> On 2015/06/18 14:56:28, estark wrote:
> > On 2015/06/18 14:51:12, jam wrote:
> > > On 2015/06/18 00:55:41, estark wrote:
> > > > On 2015/06/18 00:36:58, jam wrote:
> > > > > On 2015/06/17 21:51:34, Ryan Sleevi wrote:
> > > > > > On 2015/06/17 21:36:50, jam wrote:
> > > > > > > Why is this a chrome-only concept. It's a in a module that sits
> below
> > > > > content,
> > > > > > > so any content embedder can set the whitelist.
> > > > > > 
> > > > > > The same reason that HPKP (implemented in //net) is a Chrome-only
> > concept
> > > > (and
> > > > > > explicitly not allowed on mobile). It requires a regular update
cycle,
> > and
> > > > can
> > > > > > cause significant harm if not.
> > > > > > 
> > > > > > > In my reading of the code, this doesn't change how content loads
the
> > > data,
> > > > > > just
> > > > > > > what it sends devtools. So unless I'm misreading the code, I'm not
> > sure
> > > > how
> > > > > > this
> > > > > > > would break a client.
> > > > > > 
> > > > > > It sends to DevTools the result of UI policy, which certainly varies
> by
> > > > > > embedder. We've taken a stronger UI policy than Firefox
(console.log)
> or
> > > > > > Microsoft (no warning), and that UI behaviour would be entirely
> > > > inappropriate
> > > > > > for some uses (Chromium Embedded, Steam, etc)
> > > > > > 
> > > > > > There's zero reason for //content to know about //chrome's UI
policies
> > and
> > > > > > treatments, and the SHA-1 policy intentionally doesn't affect
loading
> in
> > > > //net
> > > > > > until 2016.
> > > > > 
> > > > > ah, ok I didn't notice initially that the method that is called in
> chrome
> > to
> > > > get
> > > > > this result for WCD is the one called by chrome ui. i understand the
> > > pushback
> > > > > now and i agree.
> > > > > 
> > > > > > 
> > > > > > > How is this UI policy? I don't see this changing chrome UI; the
only
> > > thing
> > > > I
> > > > > > see
> > > > > > > is that it's sent to devtools.
> > > > > > 
> > > > > > The entirity of the "SHA-1 issue" is implemented in //chrome, and
> solely
> > > > used
> > > > > to
> > > > > > affect UI.
> > > > > > 
> > > > > > To place this in //content sets a dangerous precedent that I would
> > > strongly
> > > > > > prefer not to set, which is that any possible UI policies that
> //chrome
> > > > takes
> > > > > > should be expressed in //content. Fundamentally, the question of
"How
> do
> > > you
> > > > > > present the omnibox and security indicators" is entirely left to
> > > embedders,
> > > > > and
> > > > > > the purpose of this CL is to provide an embedder-agnostic way for an
> > > > embedder
> > > > > to
> > > > > > explain those policies and how the decisions were made.
> > > > > > 
> > > > > > > I suggested an embedder callback just for the cros case (that is
the
> > > only
> > > > > > > possible solution if this code is in content)
> > > > > > 
> > > > > > I'm still having trouble seeing the value here in such a callback.
> > > > > > 
> > > > > > 
> > > > > > Since there's definitely a communication gap, with you asking "Why
> > didn't
> > > > you
> > > > > do
> > > > > > X" (and these previous replies being answers to that), could you
help
> us
> > > > > > understand "Why should we do X?" What benefits does it provide
versus
> > the
> > > > > > complexity of exposing Omnibox UI policies through //content and
> > requiring
> > > > > that
> > > > > > any embedders that wish to use alternative policies to customize
> > > //content?
> > > > > 
> > > > > mostly it was about reducing content api's growth.
> > > > > 
> > > > > instead of content asking chrome for ui result so that it can send it
to
> > > > > devtools, can chrome send it directly instead?
> > > > 
> > > > We discussed this approach quite a bit with Pavel. My understanding is
> that
> > > > there does exist a way to intercept commands sent from devtools to the
> > browser
> > > > in //chrome such that they don't go through //content
> > > >
> > >
> >
>
(https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/dev...),
> > > > but the drawbacks are:
> > > > 
> > > > 1. It's kind of hacky and discouraged unless really necessary.
> > > > 2. The devtools infrastructure currently supports handling commands from
> > > > devtools to the browser in //chrome, but not sending events from
//chrome
> to
> > > > devtools, which is what we want to do (so we'd have to wait for the
> devtools
> > > > team to figure out how to do that and build it).
> > > 
> > > Shouldn't this be easy, i.e. a method that takes a string?
> > 
> > I don't understand, sorry -- can you be more specific? Or is there an
example
> > you can point to? (I guess I don't understand what "the string" is that
you're
> > referring to, or what the method would be called and what exactly it would
> do.)
> 
> oh, I'm just looking at SecurityHandler::SecurityStyleChanged which calls
> Client::SecurityStateChanged and that eventually ends up in
> DevToolsProtocolClient::SendRawMessage with a string.

Ok, but I'm not sure how the embedder would actually construct that string...
AFAIU it would have to get a hold of a Client somehow, and Clients are currently
constructed by generated code, and set on the SecurityHandler, so //chrome would
have to get a hold of the SecurityHandler, which dangles off the
RenderFrameDevToolsAgentHost, and I'm not sure how to get at that. (There is
currently a method to get the RFDTAH for a given RenderFrameHost in an anonymous
namespace; I'm not sure how devtools people would feel about pulling that out
and making it a public API somehow.)

I'm also not sure how the devtools people feel about having a
SendMessageToDevTools() method in //content/public. The comment on
DevToolsProtocolClient::SendRawMessage() says "Do not use unless you must."

> 
> > 
> > > 
> > > > 3. I think it actually makes sense for the event to be sent from
> //content:
> > it
> > > > would be nice for any embedder to be able to get the security panel
> > > > functionality by just implementing WCD::GetSecurityStyle instead of
having
> > to
> > > > re-implement all the boilerplate of a devtools protocol handler itself.
> > > 
> > > That depends on what the API of 2 is right? i.e. if there's a method in
> > > content/public that sends a string to devtools, then there shouldn't be
much
> > > work to implement on the embedder side.
> > > 
> > > > 
> > > > So we wanted to avoid the //chrome-direct-to-devtools approach until we
> > > actually
> > > > had to send Chrome-specific information.

[jam, 2015-06-18 17:34:56]

On 2015/06/18 15:35:58, estark wrote:
> On 2015/06/18 15:19:41, jam wrote:
> > On 2015/06/18 14:56:28, estark wrote:
> > > On 2015/06/18 14:51:12, jam wrote:
> > > > On 2015/06/18 00:55:41, estark wrote:
> > > > > On 2015/06/18 00:36:58, jam wrote:
> > > > > > On 2015/06/17 21:51:34, Ryan Sleevi wrote:
> > > > > > > On 2015/06/17 21:36:50, jam wrote:
> > > > > > > > Why is this a chrome-only concept. It's a in a module that sits
> > below
> > > > > > content,
> > > > > > > > so any content embedder can set the whitelist.
> > > > > > > 
> > > > > > > The same reason that HPKP (implemented in //net) is a Chrome-only
> > > concept
> > > > > (and
> > > > > > > explicitly not allowed on mobile). It requires a regular update
> cycle,
> > > and
> > > > > can
> > > > > > > cause significant harm if not.
> > > > > > > 
> > > > > > > > In my reading of the code, this doesn't change how content loads
> the
> > > > data,
> > > > > > > just
> > > > > > > > what it sends devtools. So unless I'm misreading the code, I'm
not
> > > sure
> > > > > how
> > > > > > > this
> > > > > > > > would break a client.
> > > > > > > 
> > > > > > > It sends to DevTools the result of UI policy, which certainly
varies
> > by
> > > > > > > embedder. We've taken a stronger UI policy than Firefox
> (console.log)
> > or
> > > > > > > Microsoft (no warning), and that UI behaviour would be entirely
> > > > > inappropriate
> > > > > > > for some uses (Chromium Embedded, Steam, etc)
> > > > > > > 
> > > > > > > There's zero reason for //content to know about //chrome's UI
> policies
> > > and
> > > > > > > treatments, and the SHA-1 policy intentionally doesn't affect
> loading
> > in
> > > > > //net
> > > > > > > until 2016.
> > > > > > 
> > > > > > ah, ok I didn't notice initially that the method that is called in
> > chrome
> > > to
> > > > > get
> > > > > > this result for WCD is the one called by chrome ui. i understand the
> > > > pushback
> > > > > > now and i agree.
> > > > > > 
> > > > > > > 
> > > > > > > > How is this UI policy? I don't see this changing chrome UI; the
> only
> > > > thing
> > > > > I
> > > > > > > see
> > > > > > > > is that it's sent to devtools.
> > > > > > > 
> > > > > > > The entirity of the "SHA-1 issue" is implemented in //chrome, and
> > solely
> > > > > used
> > > > > > to
> > > > > > > affect UI.
> > > > > > > 
> > > > > > > To place this in //content sets a dangerous precedent that I would
> > > > strongly
> > > > > > > prefer not to set, which is that any possible UI policies that
> > //chrome
> > > > > takes
> > > > > > > should be expressed in //content. Fundamentally, the question of
> "How
> > do
> > > > you
> > > > > > > present the omnibox and security indicators" is entirely left to
> > > > embedders,
> > > > > > and
> > > > > > > the purpose of this CL is to provide an embedder-agnostic way for
an
> > > > > embedder
> > > > > > to
> > > > > > > explain those policies and how the decisions were made.
> > > > > > > 
> > > > > > > > I suggested an embedder callback just for the cros case (that is
> the
> > > > only
> > > > > > > > possible solution if this code is in content)
> > > > > > > 
> > > > > > > I'm still having trouble seeing the value here in such a callback.
> > > > > > > 
> > > > > > > 
> > > > > > > Since there's definitely a communication gap, with you asking "Why
> > > didn't
> > > > > you
> > > > > > do
> > > > > > > X" (and these previous replies being answers to that), could you
> help
> > us
> > > > > > > understand "Why should we do X?" What benefits does it provide
> versus
> > > the
> > > > > > > complexity of exposing Omnibox UI policies through //content and
> > > requiring
> > > > > > that
> > > > > > > any embedders that wish to use alternative policies to customize
> > > > //content?
> > > > > > 
> > > > > > mostly it was about reducing content api's growth.
> > > > > > 
> > > > > > instead of content asking chrome for ui result so that it can send
it
> to
> > > > > > devtools, can chrome send it directly instead?
> > > > > 
> > > > > We discussed this approach quite a bit with Pavel. My understanding is
> > that
> > > > > there does exist a way to intercept commands sent from devtools to the
> > > browser
> > > > > in //chrome such that they don't go through //content
> > > > >
> > > >
> > >
> >
>
(https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/dev...),
> > > > > but the drawbacks are:
> > > > > 
> > > > > 1. It's kind of hacky and discouraged unless really necessary.
> > > > > 2. The devtools infrastructure currently supports handling commands
from
> > > > > devtools to the browser in //chrome, but not sending events from
> //chrome
> > to
> > > > > devtools, which is what we want to do (so we'd have to wait for the
> > devtools
> > > > > team to figure out how to do that and build it).
> > > > 
> > > > Shouldn't this be easy, i.e. a method that takes a string?
> > > 
> > > I don't understand, sorry -- can you be more specific? Or is there an
> example
> > > you can point to? (I guess I don't understand what "the string" is that
> you're
> > > referring to, or what the method would be called and what exactly it would
> > do.)
> > 
> > oh, I'm just looking at SecurityHandler::SecurityStyleChanged which calls
> > Client::SecurityStateChanged and that eventually ends up in
> > DevToolsProtocolClient::SendRawMessage with a string.
> 
> Ok, but I'm not sure how the embedder would actually construct that string...
> AFAIU it would have to get a hold of a Client somehow, and Clients are
currently
> constructed by generated code, and set on the SecurityHandler, so //chrome
would
> have to get a hold of the SecurityHandler, which dangles off the
> RenderFrameDevToolsAgentHost, and I'm not sure how to get at that. (There is
> currently a method to get the RFDTAH for a given RenderFrameHost in an
anonymous
> namespace; I'm not sure how devtools people would feel about pulling that out
> and making it a public API somehow.)
> 
> I'm also not sure how the devtools people feel about having a
> SendMessageToDevTools() method in //content/public. The comment on
> DevToolsProtocolClient::SendRawMessage() says "Do not use unless you must."

I see; ok the plumbing is more complicated than I thought.

lgtm then

one nit: try to avoid the .cc files in content/public unless you have to to
avoid clang errors.

[estark, 2015-06-18 18:37:50]

On 2015/06/18 17:34:56, jam wrote:
> On 2015/06/18 15:35:58, estark wrote:
> > On 2015/06/18 15:19:41, jam wrote:
> > > On 2015/06/18 14:56:28, estark wrote:
> > > > On 2015/06/18 14:51:12, jam wrote:
> > > > > On 2015/06/18 00:55:41, estark wrote:
> > > > > > On 2015/06/18 00:36:58, jam wrote:
> > > > > > > On 2015/06/17 21:51:34, Ryan Sleevi wrote:
> > > > > > > > On 2015/06/17 21:36:50, jam wrote:
> > > > > > > > > Why is this a chrome-only concept. It's a in a module that
sits
> > > below
> > > > > > > content,
> > > > > > > > > so any content embedder can set the whitelist.
> > > > > > > > 
> > > > > > > > The same reason that HPKP (implemented in //net) is a
Chrome-only
> > > > concept
> > > > > > (and
> > > > > > > > explicitly not allowed on mobile). It requires a regular update
> > cycle,
> > > > and
> > > > > > can
> > > > > > > > cause significant harm if not.
> > > > > > > > 
> > > > > > > > > In my reading of the code, this doesn't change how content
loads
> > the
> > > > > data,
> > > > > > > > just
> > > > > > > > > what it sends devtools. So unless I'm misreading the code, I'm
> not
> > > > sure
> > > > > > how
> > > > > > > > this
> > > > > > > > > would break a client.
> > > > > > > > 
> > > > > > > > It sends to DevTools the result of UI policy, which certainly
> varies
> > > by
> > > > > > > > embedder. We've taken a stronger UI policy than Firefox
> > (console.log)
> > > or
> > > > > > > > Microsoft (no warning), and that UI behaviour would be entirely
> > > > > > inappropriate
> > > > > > > > for some uses (Chromium Embedded, Steam, etc)
> > > > > > > > 
> > > > > > > > There's zero reason for //content to know about //chrome's UI
> > policies
> > > > and
> > > > > > > > treatments, and the SHA-1 policy intentionally doesn't affect
> > loading
> > > in
> > > > > > //net
> > > > > > > > until 2016.
> > > > > > > 
> > > > > > > ah, ok I didn't notice initially that the method that is called in
> > > chrome
> > > > to
> > > > > > get
> > > > > > > this result for WCD is the one called by chrome ui. i understand
the
> > > > > pushback
> > > > > > > now and i agree.
> > > > > > > 
> > > > > > > > 
> > > > > > > > > How is this UI policy? I don't see this changing chrome UI;
the
> > only
> > > > > thing
> > > > > > I
> > > > > > > > see
> > > > > > > > > is that it's sent to devtools.
> > > > > > > > 
> > > > > > > > The entirity of the "SHA-1 issue" is implemented in //chrome,
and
> > > solely
> > > > > > used
> > > > > > > to
> > > > > > > > affect UI.
> > > > > > > > 
> > > > > > > > To place this in //content sets a dangerous precedent that I
would
> > > > > strongly
> > > > > > > > prefer not to set, which is that any possible UI policies that
> > > //chrome
> > > > > > takes
> > > > > > > > should be expressed in //content. Fundamentally, the question of
> > "How
> > > do
> > > > > you
> > > > > > > > present the omnibox and security indicators" is entirely left to
> > > > > embedders,
> > > > > > > and
> > > > > > > > the purpose of this CL is to provide an embedder-agnostic way
for
> an
> > > > > > embedder
> > > > > > > to
> > > > > > > > explain those policies and how the decisions were made.
> > > > > > > > 
> > > > > > > > > I suggested an embedder callback just for the cros case (that
is
> > the
> > > > > only
> > > > > > > > > possible solution if this code is in content)
> > > > > > > > 
> > > > > > > > I'm still having trouble seeing the value here in such a
callback.
> > > > > > > > 
> > > > > > > > 
> > > > > > > > Since there's definitely a communication gap, with you asking
"Why
> > > > didn't
> > > > > > you
> > > > > > > do
> > > > > > > > X" (and these previous replies being answers to that), could you
> > help
> > > us
> > > > > > > > understand "Why should we do X?" What benefits does it provide
> > versus
> > > > the
> > > > > > > > complexity of exposing Omnibox UI policies through //content and
> > > > requiring
> > > > > > > that
> > > > > > > > any embedders that wish to use alternative policies to customize
> > > > > //content?
> > > > > > > 
> > > > > > > mostly it was about reducing content api's growth.
> > > > > > > 
> > > > > > > instead of content asking chrome for ui result so that it can send
> it
> > to
> > > > > > > devtools, can chrome send it directly instead?
> > > > > > 
> > > > > > We discussed this approach quite a bit with Pavel. My understanding
is
> > > that
> > > > > > there does exist a way to intercept commands sent from devtools to
the
> > > > browser
> > > > > > in //chrome such that they don't go through //content
> > > > > >
> > > > >
> > > >
> > >
> >
>
(https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/dev...),
> > > > > > but the drawbacks are:
> > > > > > 
> > > > > > 1. It's kind of hacky and discouraged unless really necessary.
> > > > > > 2. The devtools infrastructure currently supports handling commands
> from
> > > > > > devtools to the browser in //chrome, but not sending events from
> > //chrome
> > > to
> > > > > > devtools, which is what we want to do (so we'd have to wait for the
> > > devtools
> > > > > > team to figure out how to do that and build it).
> > > > > 
> > > > > Shouldn't this be easy, i.e. a method that takes a string?
> > > > 
> > > > I don't understand, sorry -- can you be more specific? Or is there an
> > example
> > > > you can point to? (I guess I don't understand what "the string" is that
> > you're
> > > > referring to, or what the method would be called and what exactly it
would
> > > do.)
> > > 
> > > oh, I'm just looking at SecurityHandler::SecurityStyleChanged which calls
> > > Client::SecurityStateChanged and that eventually ends up in
> > > DevToolsProtocolClient::SendRawMessage with a string.
> > 
> > Ok, but I'm not sure how the embedder would actually construct that
string...
> > AFAIU it would have to get a hold of a Client somehow, and Clients are
> currently
> > constructed by generated code, and set on the SecurityHandler, so //chrome
> would
> > have to get a hold of the SecurityHandler, which dangles off the
> > RenderFrameDevToolsAgentHost, and I'm not sure how to get at that. (There is
> > currently a method to get the RFDTAH for a given RenderFrameHost in an
> anonymous
> > namespace; I'm not sure how devtools people would feel about pulling that
out
> > and making it a public API somehow.)
> > 
> > I'm also not sure how the devtools people feel about having a
> > SendMessageToDevTools() method in //content/public. The comment on
> > DevToolsProtocolClient::SendRawMessage() says "Do not use unless you must."
> 
> I see; ok the plumbing is more complicated than I thought.
> 
> lgtm then
> 
> one nit: try to avoid the .cc files in content/public unless you have to to
> avoid clang errors.

Great, thanks jam.

For avoiding the .cc files: are you suggesting that as a general best practice
to know about, or specifically that I should move the content/public .cc files
in this CL? I thought it's okay to have them because of this content API
guideline: "it's acceptable to put implementation files that hold
constructors/destructors of interfaces/structs which might have member
variables. For structs, this covers initializing member variables."

If security_explanation(s).cc don't belong in content/public, would I just be
moving the files as-is to content/browser? Or should I make
SecurityExplanation(s) a virtual interface with a SecurityExplanation(s)Impl in
content/browser? (That approach seems heavy-handed)

[estark, 2015-06-18 18:52:23]

creis, pfeldman: would you like to take another look before this lands?

https://codereview.chromium.org/1181293003/diff/210001/chrome/app/generated_r...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/1181293003/diff/210001/chrome/app/generated_r...
chrome/app/generated_resources.grd:15667: +        Mixed Content
On 2015/06/18 01:47:25, lgarron wrote:
> On 2015/06/18 at 01:34:03, Ryan Sleevi wrote:
> > On 2015/06/18 01:28:45, lgarron wrote:
> > > estark@: Would you mind changing this to "Mixed Scripts" (or "Active Mixed
> > > Content")?
> > 
> > -1 scripts (can apply to other bits, such as ws:// connections)
> > +1 to Active Mixed Content
> > 
> > and with the bonus nitting that these have yet another (not _yet_ well
> publicized) name of "Blockable Content" (
> https://w3c.github.io/webappsec/specs/mixedcontent/#category-blockable )
> 
> We only talk about "scripts" to end users [1]. I'm fine with "Active Mixed
> Content", although then we should probably update
> IDS_ACTIVE_MIXED_CONTENT_DESCRIPTION to mention other common active mixed
> content (e.g. iframes).
> 
> [1] https://support.google.com/chrome/answer/1342714

Changed to "Active Mixed Content" and "You have recently allowed insecure
content to run on this site" though I'm not thrilled with the latter since it
seems kind of vague/unhelpful.

Audience for these is developers, not end users, right? So we can be more
precise, if we want to -- though I'm not sure if it's a good idea to try to be
exhaustive.

(Also I think we will probably want to have ainslie@ look these over at some
point.)

[lgarron, 2015-06-18 20:22:46]

https://codereview.chromium.org/1181293003/diff/210001/chrome/app/generated_r...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/1181293003/diff/210001/chrome/app/generated_r...
chrome/app/generated_resources.grd:15667: +        Mixed Content
On 2015/06/18 at 18:52:23, estark wrote:
> On 2015/06/18 01:47:25, lgarron wrote:
> > On 2015/06/18 at 01:34:03, Ryan Sleevi wrote:
> > > On 2015/06/18 01:28:45, lgarron wrote:
> > > > estark@: Would you mind changing this to "Mixed Scripts" (or "Active
Mixed
> > > > Content")?
> > > 
> > > -1 scripts (can apply to other bits, such as ws:// connections)
> > > +1 to Active Mixed Content
> > > 
> > > and with the bonus nitting that these have yet another (not _yet_ well
> > publicized) name of "Blockable Content" (
> > https://w3c.github.io/webappsec/specs/mixedcontent/#category-blockable )
> > 
> > We only talk about "scripts" to end users [1]. I'm fine with "Active Mixed
> > Content", although then we should probably update
> > IDS_ACTIVE_MIXED_CONTENT_DESCRIPTION to mention other common active mixed
> > content (e.g. iframes).
> > 
> > [1] https://support.google.com/chrome/answer/1342714
> 
> Changed to "Active Mixed Content" and "You have recently allowed insecure
content to run on this site" though I'm not thrilled with the latter since it
seems kind of vague/unhelpful.
> 
> Audience for these is developers, not end users, right? So we can be more
precise, if we want to -- though I'm not sure if it's a good idea to try to be
exhaustive.
> 
> (Also I think we will probably want to have ainslie@ look these over at some
point.)

+1 on definitely running things by Alex before expanding a final sec of strings.

How about "You have recently allowed insecure content (e.g. scripts, iframes) to
run on this site"?

[Charlie Reis, 2015-06-18 20:39:52]

Thanks, content/ LGTM with nits given the previous discussion.

On 2015/06/18 18:37:50, estark wrote:
> On 2015/06/18 17:34:56, jam wrote:
> > I see; ok the plumbing is more complicated than I thought.
> > 
> > lgtm then
> > 
> > one nit: try to avoid the .cc files in content/public unless you have to to
> > avoid clang errors.
> 
> Great, thanks jam.
> 
> For avoiding the .cc files: are you suggesting that as a general best practice
> to know about, or specifically that I should move the content/public .cc files
> in this CL? I thought it's okay to have them because of this content API
> guideline: "it's acceptable to put implementation files that hold
> constructors/destructors of interfaces/structs which might have member
> variables. For structs, this covers initializing member variables."
> 
> If security_explanation(s).cc don't belong in content/public, would I just be
> moving the files as-is to content/browser? Or should I make
> SecurityExplanation(s) a virtual interface with a SecurityExplanation(s)Impl
in
> content/browser? (That approach seems heavy-handed)

I think he meant to inline them in the .h file if you can, unless clang
complains that they have to live in a .cc file.  No need to move the .cc files
if they have to exist.

https://codereview.chromium.org/1181293003/diff/230001/content/public/browser...
File content/public/browser/web_contents_delegate.h (right):

https://codereview.chromium.org/1181293003/diff/230001/content/public/browser...
content/public/browser/web_contents_delegate.h:41: struct
SecurityStyleExplanations;
nit: List with the structs, not the classes.

https://codereview.chromium.org/1181293003/diff/230001/content/public/browser...
File content/public/browser/web_contents_observer.h (right):

https://codereview.chromium.org/1181293003/diff/230001/content/public/browser...
content/public/browser/web_contents_observer.h:25: struct
SecurityStyleExplanations;
Ditto.

[estark, 2015-06-18 23:13:09]

On 2015/06/18 20:39:52, Charlie Reis (OOO soon) wrote:
> Thanks, content/ LGTM with nits given the previous discussion.
> 
> On 2015/06/18 18:37:50, estark wrote:
> > On 2015/06/18 17:34:56, jam wrote:
> > > I see; ok the plumbing is more complicated than I thought.
> > > 
> > > lgtm then
> > > 
> > > one nit: try to avoid the .cc files in content/public unless you have to
to
> > > avoid clang errors.
> > 
> > Great, thanks jam.
> > 
> > For avoiding the .cc files: are you suggesting that as a general best
practice
> > to know about, or specifically that I should move the content/public .cc
files
> > in this CL? I thought it's okay to have them because of this content API
> > guideline: "it's acceptable to put implementation files that hold
> > constructors/destructors of interfaces/structs which might have member
> > variables. For structs, this covers initializing member variables."
> > 
> > If security_explanation(s).cc don't belong in content/public, would I just
be
> > moving the files as-is to content/browser? Or should I make
> > SecurityExplanation(s) a virtual interface with a SecurityExplanation(s)Impl
> in
> > content/browser? (That approach seems heavy-handed)
> 
> I think he meant to inline them in the .h file if you can, unless clang
> complains that they have to live in a .cc file.  No need to move the .cc files
> if they have to exist.

Ah, ok, thanks for the clarification. I inlined SecurityStyleExplanation but
wasn't able to with SecurityStyleExplanations.

> 
>
https://codereview.chromium.org/1181293003/diff/230001/content/public/browser...
> File content/public/browser/web_contents_delegate.h (right):
> 
>
https://codereview.chromium.org/1181293003/diff/230001/content/public/browser...
> content/public/browser/web_contents_delegate.h:41: struct
> SecurityStyleExplanations;
> nit: List with the structs, not the classes.
> 
>
https://codereview.chromium.org/1181293003/diff/230001/content/public/browser...
> File content/public/browser/web_contents_observer.h (right):
> 
>
https://codereview.chromium.org/1181293003/diff/230001/content/public/browser...
> content/public/browser/web_contents_observer.h:25: struct
> SecurityStyleExplanations;
> Ditto.

[estark, 2015-06-18 23:13:26]

https://codereview.chromium.org/1181293003/diff/230001/content/public/browser...
File content/public/browser/web_contents_delegate.h (right):

https://codereview.chromium.org/1181293003/diff/230001/content/public/browser...
content/public/browser/web_contents_delegate.h:41: struct
SecurityStyleExplanations;
On 2015/06/18 20:39:51, Charlie Reis (OOO soon) wrote:
> nit: List with the structs, not the classes.

Done.

https://codereview.chromium.org/1181293003/diff/230001/content/public/browser...
File content/public/browser/web_contents_observer.h (right):

https://codereview.chromium.org/1181293003/diff/230001/content/public/browser...
content/public/browser/web_contents_observer.h:25: struct
SecurityStyleExplanations;
On 2015/06/18 20:39:52, Charlie Reis (OOO soon) wrote:
> Ditto.

Done.

[pfeldman, 2015-06-19 06:33:34]

lgtm

[estark, 2015-06-19 14:44:02]

The CQ bit was checked by estark@chromium.org

[estark, 2015-06-19 14:44:03]

The patchset sent to the CQ was uploaded after l-g-t-m from
pkasting@chromium.org, rsleevi@chromium.org, jam@chromium.org,
creis@chromium.org
Link to the patchset: https://codereview.chromium.org/1181293003/#ps290001
(title: "creis comments")

[commit-bot: I haz the power, 2015-06-19 14:44:38]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1181293003/290001

[commit-bot: I haz the power, 2015-06-19 15:14:45]

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

[commit-bot: I haz the power, 2015-06-19 15:14:47]

Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[estark, 2015-06-19 17:23:36]

The CQ bit was checked by estark@chromium.org

[estark, 2015-06-19 17:23:36]

The patchset sent to the CQ was uploaded after l-g-t-m from
pkasting@chromium.org, rsleevi@chromium.org, jam@chromium.org,
creis@chromium.org, pfeldman@chromium.org
Link to the patchset: https://codereview.chromium.org/1181293003/#ps310001
(title: "add CONTENT_EXPORT")

[commit-bot: I haz the power, 2015-06-19 17:25:10]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1181293003/310001

[commit-bot: I haz the power, 2015-06-19 18:43:22]

Committed patchset #16 (id:310001)

[commit-bot: I haz the power, 2015-06-19 18:44:08]

Patchset 16 (id:??) landed as
https://crrev.com/5ea80e54daf27b155d9ba2c6e7f9a3f7b4d5e803
Cr-Commit-Position: refs/heads/master@{#335306}

[jam, 2015-06-22 16:04:30]

On 2015/06/18 23:13:09, estark wrote:
> On 2015/06/18 20:39:52, Charlie Reis (OOO soon) wrote:
> > Thanks, content/ LGTM with nits given the previous discussion.
> > 
> > On 2015/06/18 18:37:50, estark wrote:
> > > On 2015/06/18 17:34:56, jam wrote:
> > > > I see; ok the plumbing is more complicated than I thought.
> > > > 
> > > > lgtm then
> > > > 
> > > > one nit: try to avoid the .cc files in content/public unless you have to
> to
> > > > avoid clang errors.
> > > 
> > > Great, thanks jam.
> > > 
> > > For avoiding the .cc files: are you suggesting that as a general best
> practice
> > > to know about, or specifically that I should move the content/public .cc
> files
> > > in this CL? I thought it's okay to have them because of this content API
> > > guideline: "it's acceptable to put implementation files that hold
> > > constructors/destructors of interfaces/structs which might have member
> > > variables. For structs, this covers initializing member variables."
> > > 
> > > If security_explanation(s).cc don't belong in content/public, would I just
> be
> > > moving the files as-is to content/browser? Or should I make
> > > SecurityExplanation(s) a virtual interface with a
SecurityExplanation(s)Impl
> > in
> > > content/browser? (That approach seems heavy-handed)
> > 
> > I think he meant to inline them in the .h file if you can, unless clang
> > complains that they have to live in a .cc file.  No need to move the .cc
files
> > if they have to exist.
> 
> Ah, ok, thanks for the clarification. I inlined SecurityStyleExplanation but
> wasn't able to with SecurityStyleExplanations.

yep that's what I meant, sorry wasn't clear. (was ooo)