Refactor connection_security into SecurityStateModel

Refactor connection_security into SecurityStateModel

This CL refactors the connection_security namespace (a namespace of
statics) into SecurityStateModel, a class attached to a WebContents that
drives the security UI for that WebContents. The SecurityStateModel
provides high-level security information about a page or request, with
the goal of reducing code duplication across various security UI
elements.

In this first CL, I've introduced the SecurityStateModel and am using it
to drive the omnibox/lock icon, but have not yet adapted WebsiteSettings
to use a SecurityStateModel.

BUG=528034
TBR=sky@chromium.org

Committed: https://crrev.com/83a81afbef8f265815cb6c6a48690511825a1ce5
Cr-Commit-Position: refs/heads/master@{#347775}

[estark, 2015-08-31 19:24:08]

estark@chromium.org changed reviewers:
+ lgarron@chromium.org, palmer@chromium.org

[estark, 2015-08-31 19:24:09]

palmer, lgarron: could you please take a preliminary look at this unfortunately
giant CL?

A lot of it is renaming stuff and moving code around, so here are some of the
more interesting highlights:

security_state_model.h is the basic interface I'm adding to replace the
connection_security namespace. It has a method -- SecurityStateChanged() -- to
notify when to recalculate the security info for its WebContents. This helps
avoid needless recomputation, i.e. recalculating security info every time the
omnibox redraws. SecurityStateChanged() is called from
Browser::VisibleSSLStateChanged() in desktop-land and from Tab.java in
Android-land. I'm especially interested in if you think this interface is likely
to cause bugs, and if you have any ideas for alternatives.

WebsiteSettingsPopup.java: uses SecurityStateModel to get its information
instead of duplicating SHA1 code and relying on SECURITY_WARNING to mean that
passive mixed content is present (which it no longer does mean).

SecurityStateModel.java: static wrapper functions to access the
SecurityStateModel of a WebContents in Java. Might want to clean this up to more
closely mirror the SecurityStateModel interface.

browser.cc uses the SecurityStateModel to get the info to send to devtools, and
toolbar_model_impl.cc uses it to select the lock icon.

I'm working on adding security_state_model_browser_tests.cc but it's not there
yet.

Also, note that this isn't the full refactoring, and there's still quite a bit
of work to be done in follow-up CLs -- for example, my next steps are to
componentize SecurityStateModel somehow, and to modify WebsiteSettings to
operate on a SecurityStateModel::SecurityInfo so that it doesn't have to e.g.
duplicate the SHA1 calculation.

Concrete problems that this CL solves:
- removes duplicated SHA1 calculation on Android
- recalculates lock icon whenever SSL state changes, not whenever omnibox
redraws
- fixes the mixed content connection string on Android (but this is currently
not user-visible because there is no way to view connection info on Android for
the neutral style, see my comment #20 on crbug.com/513863)

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

https://codereview.chromium.org/1314843007/diff/80001/content/public/browser/...
content/public/browser/web_contents_delegate.h:95: virtual void
VisibleSSLStateChanged(WebContents* source) {}
While this diff may look sketchy, it actually brings this interface in line with
//content guidelines which says "don't add the const identifier to interfaces"
(http://www.chromium.org/developers/content-module/content-api)

[estark, 2015-08-31 23:11:29]

Added some SecurityStateModel browser tests and updated/fixed some other
pre-existing tests

[palmer, 2015-09-01 17:39:41]

(will look more later)

https://codereview.chromium.org/1314843007/diff/120001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java
(right):

https://codereview.chromium.org/1314843007/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java:270:
// Whether the security level of the page was deprecated due to passive mixed
content.
"downgraded", not "deprecated"

https://codereview.chromium.org/1314843007/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java:428:
mPassiveMixedContentPresent =
SecurityStateModel.isPassiveMixedContentPresent(mWebContents);
I suppose I'll find out when I read more, but: How is active mixed content
handled?

https://codereview.chromium.org/1314843007/diff/120001/chrome/browser/ssl/sec...
File chrome/browser/ssl/security_state_model.h (right):

https://codereview.chromium.org/1314843007/diff/120001/chrome/browser/ssl/sec...
chrome/browser/ssl/security_state_model.h:51: // HTTPS (non-EV)
Specify valid cert

https://codereview.chromium.org/1314843007/diff/120001/chrome/browser/ssl/sec...
chrome/browser/ssl/security_state_model.h:54: // HTTPS, but unable to check
certificate revocation status or with insecure
Insecure passive content

https://codereview.chromium.org/1314843007/diff/120001/chrome/browser/ssl/sec...
chrome/browser/ssl/security_state_model.h:62: // Attempted HTTPS and failed,
page not authenticated
Does this the case in which include active mixed content ran?

https://codereview.chromium.org/1314843007/diff/120001/chrome/browser/ssl/sec...
chrome/browser/ssl/security_state_model.h:70: NO_DEPRECATED_SHA1,
Reorder these from good to bad to worse? I.e. put DEPRECATED_SHA1_WARNING before
DEPRECATED_SHA1_BROKEN?

https://codereview.chromium.org/1314843007/diff/120001/chrome/browser/ssl/sec...
chrome/browser/ssl/security_state_model.h:107: int security_bits;
This could use a comment (and/or a better name).

[estark, 2015-09-02 16:27:20]

https://codereview.chromium.org/1314843007/diff/120001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java
(right):

https://codereview.chromium.org/1314843007/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java:270:
// Whether the security level of the page was deprecated due to passive mixed
content.
On 2015/09/01 17:39:40, palmer wrote:
> "downgraded", not "deprecated"

Done.

https://codereview.chromium.org/1314843007/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java:428:
mPassiveMixedContentPresent =
SecurityStateModel.isPassiveMixedContentPresent(mWebContents);
On 2015/09/01 17:39:40, palmer wrote:
> I suppose I'll find out when I read more, but: How is active mixed content
> handled?

I think I haven't yet seen anywhere that Android UI does anything special with
active mixed content (i.e. treats it differently than broken-https). Is it
possible that there is no active mixed content shield on Android, and thus the
presence of active mixed content is impossible?

https://codereview.chromium.org/1314843007/diff/120001/chrome/browser/ssl/sec...
File chrome/browser/ssl/security_state_model.h (right):

https://codereview.chromium.org/1314843007/diff/120001/chrome/browser/ssl/sec...
chrome/browser/ssl/security_state_model.h:51: // HTTPS (non-EV)
On 2015/09/01 17:39:40, palmer wrote:
> Specify valid cert

Done.

https://codereview.chromium.org/1314843007/diff/120001/chrome/browser/ssl/sec...
chrome/browser/ssl/security_state_model.h:54: // HTTPS, but unable to check
certificate revocation status or with insecure
On 2015/09/01 17:39:40, palmer wrote:
> Insecure passive content

I moved this description and made this change on the NONE value.

The only thing we actually use SECURITY_WARNING for now is SSLv3, and I thought
SSLv3 was completely disabled now (or behind a flag or something). So maybe we
should now use NONE for sslv3 and get rid of SECURITY_WARNING all together. I
can do that in a separate follow-up CL if you think it's reasonable.

https://codereview.chromium.org/1314843007/diff/120001/chrome/browser/ssl/sec...
chrome/browser/ssl/security_state_model.h:62: // Attempted HTTPS and failed,
page not authenticated
On 2015/09/01 17:39:40, palmer wrote:
> Does this the case in which include active mixed content ran?

Yes, updated the comment to say so.

https://codereview.chromium.org/1314843007/diff/120001/chrome/browser/ssl/sec...
chrome/browser/ssl/security_state_model.h:70: NO_DEPRECATED_SHA1,
On 2015/09/01 17:39:40, palmer wrote:
> Reorder these from good to bad to worse? I.e. put DEPRECATED_SHA1_WARNING
before
> DEPRECATED_SHA1_BROKEN?

Done.

https://codereview.chromium.org/1314843007/diff/120001/chrome/browser/ssl/sec...
chrome/browser/ssl/security_state_model.h:107: int security_bits;
On 2015/09/01 17:39:40, palmer wrote:
> This could use a comment (and/or a better name).

Done.

[palmer, 2015-09-02 20:21:11]

> I think I haven't yet seen anywhere that Android UI does anything special with
> active mixed content (i.e. treats it differently than broken-https). Is it
> possible that there is no active mixed content shield on Android, and thus the
> presence of active mixed content is impossible?

Oh, right, yes. I believe that is the case.

> The only thing we actually use SECURITY_WARNING for now is SSLv3, and I
thought
> SSLv3 was completely disabled now (or behind a flag or something). So maybe we
> should now use NONE for sslv3 and get rid of SECURITY_WARNING all together. I
> can do that in a separate follow-up CL if you think it's reasonable.

I think so too. Run it by agl/rsleevi/davidben to make sure it's true that SSLv3
is fully gone, but yeah.

[palmer, 2015-09-02 20:37:05]

LGTM % nits.

https://codereview.chromium.org/1314843007/diff/140001/chrome/browser/ui/brow...
File chrome/browser/ui/browser.cc (right):

https://codereview.chromium.org/1314843007/diff/140001/chrome/browser/ui/brow...
chrome/browser/ui/browser.cc:258: // Note: This is a lossy operation. Not all of
the policies
Nit: Wrap this comment to 80 characters.

https://codereview.chromium.org/1314843007/diff/140001/chrome/browser/ui/brow...
chrome/browser/ui/browser.cc:268: return content::SECURITY_STYLE_WARNING;
Is this going to result in showing Dubious as Dubious, instead of as Neutral?

https://codereview.chromium.org/1314843007/diff/140001/chrome/browser/ui/tool...
File chrome/browser/ui/toolbar/toolbar_model_impl.cc (right):

https://codereview.chromium.org/1314843007/diff/140001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/toolbar_model_impl.cc:120: // When editing, assume no
security style.
"When editing, or if there is no available model, assume no..."

[estark, 2015-09-02 21:46:43]

Thanks palmer!

https://codereview.chromium.org/1314843007/diff/140001/chrome/browser/ui/brow...
File chrome/browser/ui/browser.cc (right):

https://codereview.chromium.org/1314843007/diff/140001/chrome/browser/ui/brow...
chrome/browser/ui/browser.cc:258: // Note: This is a lossy operation. Not all of
the policies
On 2015/09/02 20:37:05, palmer wrote:
> Nit: Wrap this comment to 80 characters.

Done.

https://codereview.chromium.org/1314843007/diff/140001/chrome/browser/ui/brow...
chrome/browser/ui/browser.cc:268: return content::SECURITY_STYLE_WARNING;
On 2015/09/02 20:37:05, palmer wrote:
> Is this going to result in showing Dubious as Dubious, instead of as Neutral?

No, SECURITY_WARNING doesn't actually get assigned to anything (except SSLv3,
which as previously discussed is probably not actually possible). So dubious
states will get assigned SecurityStateModel::NONE which will get translated in
SECURITY_STYLE_UNAUTHENTICATED here.

https://codereview.chromium.org/1314843007/diff/140001/chrome/browser/ui/tool...
File chrome/browser/ui/toolbar/toolbar_model_impl.cc (right):

https://codereview.chromium.org/1314843007/diff/140001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/toolbar_model_impl.cc:120: // When editing, assume no
security style.
On 2015/09/02 20:37:05, palmer wrote:
> "When editing, or if there is no available model, assume no..."

Done.

[estark, 2015-09-02 22:05:26]

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

[estark, 2015-09-02 22:05:27]

pkasting, could you please take a look at chrome/browser/ui? Most of the changes
in there are a mechanical rename. I think the most meaty parts are
Browser::VisibleSSLStateChanged() and ToolbarModelImpl::GetSecurityLevel(). We
now update a security model to tell it to recompute the lock icon when the SSL
state of the page changes, instead of recomputing the lock icon every time the
omnibox redraws.

If you'd like more context, the (Google-only) design doc that this change is a
part of is here:
https://docs.google.com/document/d/1nQrsn8vBcDX5M0WECqlaiEs7q5flMwcDngqiK-Zaf...

[Peter Kasting, 2015-09-03 18:25:53]

LGTM

https://codereview.chromium.org/1314843007/diff/160001/chrome/browser/ui/brow...
File chrome/browser/ui/browser.cc (right):

https://codereview.chromium.org/1314843007/diff/160001/chrome/browser/ui/brow...
chrome/browser/ui/browser.cc:276: return content::SECURITY_STYLE_UNKNOWN;
Nit: If this is supposed to never be reached, because future additions to the
enum should all be given explicit handling above, then add a NOTREACHED() above
this.

https://codereview.chromium.org/1314843007/diff/160001/chrome/browser/ui/brow...
chrome/browser/ui/browser.cc:1341: return
SecurityLevelToSecurityStyle(security_info.security_level);
Nit: Because both returns from this function return this value, either compute
it in a temp above, or convert this conditional block to something like:

  if (security_info.scheme_is_cryptographic) {
    ... most of remainder of function goes here...
  }

  return SecurityLevelToSecurityStyle(security_info.security_level);

https://codereview.chromium.org/1314843007/diff/160001/chrome/browser/ui/exte...
File chrome/browser/ui/extensions/hosted_app_browser_controller.cc (right):

https://codereview.chromium.org/1314843007/diff/160001/chrome/browser/ui/exte...
chrome/browser/ui/extensions/hosted_app_browser_controller.cc:96: if (model) {
Nit: Combine these two conditionals

https://codereview.chromium.org/1314843007/diff/160001/chrome/browser/ui/exte...
chrome/browser/ui/extensions/hosted_app_browser_controller.cc:98:
SecurityStateModel::SECURITY_ERROR) {
Nit: No {} necessary

https://codereview.chromium.org/1314843007/diff/160001/chrome/browser/ui/tool...
File chrome/browser/ui/toolbar/toolbar_model_impl.cc (right):

https://codereview.chromium.org/1314843007/diff/160001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/toolbar_model_impl.cc:114: SecurityStateModel*
model(nullptr);
Nit: Chromium code generally uses = instead of () to init raw pointers.

https://codereview.chromium.org/1314843007/diff/160001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/toolbar_model_impl.cc:118: }
Nit: Simpler:

  WebContents* web_contents = delegate_->GetActiveWebContents();
  SecurityStateModel* model = web_contents ?
      SecurityStateModel::FromWebContents(web_contents) : nullptr;

https://codereview.chromium.org/1314843007/diff/160001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/toolbar_model_impl.cc:121: // security style.
Nit: Is it worth commenting about when there might be no model?  Perhaps briefly
during startup or something?  (Or are you just being paranoid?)

[estark, 2015-09-03 21:51:18]

Patchset #11 (id:200001) has been deleted

[estark, 2015-09-03 22:11:56]

estark@chromium.org changed reviewers:
+ avi@chromium.org, tedchoc@chromium.org

[estark, 2015-09-03 22:11:58]

Thanks, Peter.

tedchoc: could you please review chrome/android, chrome/browser/android, and
components/web_contents_delegate_android? Please see my first message in this
thread and the bug for some more context about the motivation for this change.  

avi: could you please review the changes in content/? Here is some context for
the two small-ish changes in content: 1.) The interstitial_page_impl.cc change
matches https://codereview.chromium.org/1207943002 and ensures that
VisibleSSLStateChanged() gets called when interstitials are shown or hidden. 2.)
I removed a const from a WebContentsDelegate API which as I understand matches
the content API guidelines.


The google-internal design doc for this CL is here:
https://docs.google.com/document/d/1nQrsn8vBcDX5M0WECqlaiEs7q5flMwcDngqiK-Zaf...

https://codereview.chromium.org/1314843007/diff/160001/chrome/browser/ui/brow...
File chrome/browser/ui/browser.cc (right):

https://codereview.chromium.org/1314843007/diff/160001/chrome/browser/ui/brow...
chrome/browser/ui/browser.cc:276: return content::SECURITY_STYLE_UNKNOWN;
On 2015/09/03 18:25:53, Peter Kasting wrote:
> Nit: If this is supposed to never be reached, because future additions to the
> enum should all be given explicit handling above, then add a NOTREACHED()
above
> this.

Done.

https://codereview.chromium.org/1314843007/diff/160001/chrome/browser/ui/brow...
chrome/browser/ui/browser.cc:1341: return
SecurityLevelToSecurityStyle(security_info.security_level);
On 2015/09/03 18:25:53, Peter Kasting wrote:
> Nit: Because both returns from this function return this value, either compute
> it in a temp above, or convert this conditional block to something like:
> 
>   if (security_info.scheme_is_cryptographic) {
>     ... most of remainder of function goes here...
>   }
> 
>   return SecurityLevelToSecurityStyle(security_info.security_level);

Done.

https://codereview.chromium.org/1314843007/diff/160001/chrome/browser/ui/exte...
File chrome/browser/ui/extensions/hosted_app_browser_controller.cc (right):

https://codereview.chromium.org/1314843007/diff/160001/chrome/browser/ui/exte...
chrome/browser/ui/extensions/hosted_app_browser_controller.cc:96: if (model) {
On 2015/09/03 18:25:53, Peter Kasting wrote:
> Nit: Combine these two conditionals

Done.

https://codereview.chromium.org/1314843007/diff/160001/chrome/browser/ui/exte...
chrome/browser/ui/extensions/hosted_app_browser_controller.cc:98:
SecurityStateModel::SECURITY_ERROR) {
On 2015/09/03 18:25:53, Peter Kasting wrote:
> Nit: No {} necessary

Done.

https://codereview.chromium.org/1314843007/diff/160001/chrome/browser/ui/tool...
File chrome/browser/ui/toolbar/toolbar_model_impl.cc (right):

https://codereview.chromium.org/1314843007/diff/160001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/toolbar_model_impl.cc:114: SecurityStateModel*
model(nullptr);
On 2015/09/03 18:25:53, Peter Kasting wrote:
> Nit: Chromium code generally uses = instead of () to init raw pointers.

Done.

https://codereview.chromium.org/1314843007/diff/160001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/toolbar_model_impl.cc:118: }
On 2015/09/03 18:25:53, Peter Kasting wrote:
> Nit: Simpler:
> 
>   WebContents* web_contents = delegate_->GetActiveWebContents();
>   SecurityStateModel* model = web_contents ?
>       SecurityStateModel::FromWebContents(web_contents) : nullptr;

Done.

https://codereview.chromium.org/1314843007/diff/160001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/toolbar_model_impl.cc:121: // security style.
On 2015/09/03 18:25:53, Peter Kasting wrote:
> Nit: Is it worth commenting about when there might be no model?  Perhaps
briefly
> during startup or something?  (Or are you just being paranoid?)

Done.

[Avi (use Gerrit), 2015-09-03 22:57:36]

This seems slightly suboptimal. Why not attach the SSM with all the other tab
helpers?

https://codereview.chromium.org/1314843007/diff/220001/chrome/browser/ssl/sec...
File chrome/browser/ssl/security_state_model.cc (right):

https://codereview.chromium.org/1314843007/diff/220001/chrome/browser/ssl/sec...
chrome/browser/ssl/security_state_model.cc:248:
DEFINE_WEB_CONTENTS_USER_DATA_KEY(SecurityStateModel);
put this line at the top

https://codereview.chromium.org/1314843007/diff/220001/chrome/browser/ui/brow...
File chrome/browser/ui/browser.cc (right):

https://codereview.chromium.org/1314843007/diff/220001/chrome/browser/ui/brow...
chrome/browser/ui/browser.cc:1508:
SecurityStateModel::CreateForWebContents(source);
Why are you creating it here? We probably want one of these things on all the
tabs of a Browser, so create it along with all the other tab helpers in
tab_helpers.cc.

https://codereview.chromium.org/1314843007/diff/220001/chrome/browser/ui/tool...
File chrome/browser/ui/toolbar/toolbar_model_impl.cc (right):

https://codereview.chromium.org/1314843007/diff/220001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/toolbar_model_impl.cc:121: // not yet been called),
assume no security style.
If you add the SSM to the tab helper set, then we should be able to assume that
we have one always, and simplify the code here.

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

https://codereview.chromium.org/1314843007/diff/220001/content/public/browser...
content/public/browser/web_contents_delegate.h:95: virtual void
VisibleSSLStateChanged(WebContents* source) {}
I'm OK with this, though if you move the SSM creation to the tab helpers, then
you probably wouldn't need this change.

[estark, 2015-09-03 23:16:40]

Thanks avi, I had somehow missed the existence of tab_helpers.cc, so I put the
SecurityStateModel creation there now.

https://codereview.chromium.org/1314843007/diff/220001/chrome/browser/ssl/sec...
File chrome/browser/ssl/security_state_model.cc (right):

https://codereview.chromium.org/1314843007/diff/220001/chrome/browser/ssl/sec...
chrome/browser/ssl/security_state_model.cc:248:
DEFINE_WEB_CONTENTS_USER_DATA_KEY(SecurityStateModel);
On 2015/09/03 22:57:36, Avi wrote:
> put this line at the top

Done.

https://codereview.chromium.org/1314843007/diff/220001/chrome/browser/ui/brow...
File chrome/browser/ui/browser.cc (right):

https://codereview.chromium.org/1314843007/diff/220001/chrome/browser/ui/brow...
chrome/browser/ui/browser.cc:1508:
SecurityStateModel::CreateForWebContents(source);
On 2015/09/03 22:57:36, Avi wrote:
> Why are you creating it here? We probably want one of these things on all the
> tabs of a Browser, so create it along with all the other tab helpers in
> tab_helpers.cc.

Done.

https://codereview.chromium.org/1314843007/diff/220001/chrome/browser/ui/tool...
File chrome/browser/ui/toolbar/toolbar_model_impl.cc (right):

https://codereview.chromium.org/1314843007/diff/220001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/toolbar_model_impl.cc:121: // not yet been called),
assume no security style.
On 2015/09/03 22:57:36, Avi wrote:
> If you add the SSM to the tab helper set, then we should be able to assume
that
> we have one always, and simplify the code here.

Done. (Though we still need to account for a null webcontents)

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

https://codereview.chromium.org/1314843007/diff/220001/content/public/browser...
content/public/browser/web_contents_delegate.h:95: virtual void
VisibleSSLStateChanged(WebContents* source) {}
On 2015/09/03 22:57:36, Avi wrote:
> I'm OK with this, though if you move the SSM creation to the tab helpers, then
> you probably wouldn't need this change.

I think I still do need it. I believe Browser::VisibleSSLStateChanged needs a
non-const WebContents in order to get a non-const SSM in order to call
SecurityStateChanged() on it.

[Avi (use Gerrit), 2015-09-03 23:21:11]

lgtm

Make it so.

https://codereview.chromium.org/1314843007/diff/220001/chrome/browser/ui/tool...
File chrome/browser/ui/toolbar/toolbar_model_impl.cc (right):

https://codereview.chromium.org/1314843007/diff/220001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/toolbar_model_impl.cc:121: // not yet been called),
assume no security style.
On 2015/09/03 23:16:40, estark wrote:
> On 2015/09/03 22:57:36, Avi wrote:
> > If you add the SSM to the tab helper set, then we should be able to assume
> that
> > we have one always, and simplify the code here.
> 
> Done. (Though we still need to account for a null webcontents)

Re a null WC, yep.

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

https://codereview.chromium.org/1314843007/diff/220001/content/public/browser...
content/public/browser/web_contents_delegate.h:95: virtual void
VisibleSSLStateChanged(WebContents* source) {}
On 2015/09/03 23:16:40, estark wrote:
> On 2015/09/03 22:57:36, Avi wrote:
> > I'm OK with this, though if you move the SSM creation to the tab helpers,
then
> > you probably wouldn't need this change.
> 
> I think I still do need it. I believe Browser::VisibleSSLStateChanged needs a
> non-const WebContents in order to get a non-const SSM in order to call
> SecurityStateChanged() on it.

Hmmm. Yep, that's right.

[Ted C, 2015-09-04 00:36:50]

https://codereview.chromium.org/1314843007/diff/240001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/ssl/SecurityStateModel.java
(right):

https://codereview.chromium.org/1314843007/diff/240001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ssl/SecurityStateModel.java:13:
/** Notify that the security state has changed for a given web contents.
I would put this on the following ling to be consistent with your other methods.

Although my comment in the following file might remove the need for this method.

https://codereview.chromium.org/1314843007/diff/240001/chrome/android/java/sr...
File chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java (right):

https://codereview.chromium.org/1314843007/diff/240001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java:463:
SecurityStateModel.securityStateChanged(getWebContents());
To avoid exposing the securityStateChanged method in java, you can probably do
the same thing you do in browser.cc in ChromeWebContentsDelegateAndroid
(chrome_web_contents_delegate_android.cc)

https://codereview.chromium.org/1314843007/diff/240001/chrome/browser/ssl/sec...
File chrome/browser/ssl/security_state_model_android.cc (right):

https://codereview.chromium.org/1314843007/diff/240001/chrome/browser/ssl/sec...
chrome/browser/ssl/security_state_model_android.cc:22:
SecurityStateModel::CreateForWebContents(web_contents);
why do we need to create them here?

TabAndroid calls TabHelpers::AttachTabHelpers, so this should be created anytime
before this is called (assuming a non-null webcontents).

[estark, 2015-09-04 16:53:09]

https://codereview.chromium.org/1314843007/diff/240001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/ssl/SecurityStateModel.java
(right):

https://codereview.chromium.org/1314843007/diff/240001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ssl/SecurityStateModel.java:13:
/** Notify that the security state has changed for a given web contents.
On 2015/09/04 00:36:50, Ted C wrote:
> I would put this on the following ling to be consistent with your other
methods.
> 
> Although my comment in the following file might remove the need for this
method.

Ack, removed this method.

https://codereview.chromium.org/1314843007/diff/240001/chrome/android/java/sr...
File chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java (right):

https://codereview.chromium.org/1314843007/diff/240001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java:463:
SecurityStateModel.securityStateChanged(getWebContents());
On 2015/09/04 00:36:50, Ted C wrote:
> To avoid exposing the securityStateChanged method in java, you can probably do
> the same thing you do in browser.cc in ChromeWebContentsDelegateAndroid
> (chrome_web_contents_delegate_android.cc)

Done.

https://codereview.chromium.org/1314843007/diff/240001/chrome/browser/ssl/sec...
File chrome/browser/ssl/security_state_model_android.cc (right):

https://codereview.chromium.org/1314843007/diff/240001/chrome/browser/ssl/sec...
chrome/browser/ssl/security_state_model_android.cc:22:
SecurityStateModel::CreateForWebContents(web_contents);
On 2015/09/04 00:36:50, Ted C wrote:
> why do we need to create them here?
> 
> TabAndroid calls TabHelpers::AttachTabHelpers, so this should be created
anytime
> before this is called (assuming a non-null webcontents).

Done.

[Ted C, 2015-09-04 17:53:45]

On 2015/09/04 16:53:09, estark wrote:
>
https://codereview.chromium.org/1314843007/diff/240001/chrome/android/java/sr...
> File
>
chrome/android/java/src/org/chromium/chrome/browser/ssl/SecurityStateModel.java
> (right):
> 
>
https://codereview.chromium.org/1314843007/diff/240001/chrome/android/java/sr...
>
chrome/android/java/src/org/chromium/chrome/browser/ssl/SecurityStateModel.java:13:
> /** Notify that the security state has changed for a given web contents.
> On 2015/09/04 00:36:50, Ted C wrote:
> > I would put this on the following ling to be consistent with your other
> methods.
> > 
> > Although my comment in the following file might remove the need for this
> method.
> 
> Ack, removed this method.
> 
>
https://codereview.chromium.org/1314843007/diff/240001/chrome/android/java/sr...
> File chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java (right):
> 
>
https://codereview.chromium.org/1314843007/diff/240001/chrome/android/java/sr...
> chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java:463:
> SecurityStateModel.securityStateChanged(getWebContents());
> On 2015/09/04 00:36:50, Ted C wrote:
> > To avoid exposing the securityStateChanged method in java, you can probably
do
> > the same thing you do in browser.cc in ChromeWebContentsDelegateAndroid
> > (chrome_web_contents_delegate_android.cc)
> 
> Done.
> 
>
https://codereview.chromium.org/1314843007/diff/240001/chrome/browser/ssl/sec...
> File chrome/browser/ssl/security_state_model_android.cc (right):
> 
>
https://codereview.chromium.org/1314843007/diff/240001/chrome/browser/ssl/sec...
> chrome/browser/ssl/security_state_model_android.cc:22:
> SecurityStateModel::CreateForWebContents(web_contents);
> On 2015/09/04 00:36:50, Ted C wrote:
> > why do we need to create them here?
> > 
> > TabAndroid calls TabHelpers::AttachTabHelpers, so this should be created
> anytime
> > before this is called (assuming a non-null webcontents).
> 
> Done.

android - lgtm

[estark, 2015-09-04 20:02:55]

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

[estark, 2015-09-04 20:02:56]

Thanks tedchoc!

sky: I'm going to TBR the small change to chrome/chrome.gyp, PTAL when you have
a chance.

[estark, 2015-09-04 20:03:24]

The CQ bit was checked by estark@chromium.org

[estark, 2015-09-04 20:03:25]

The patchset sent to the CQ was uploaded after l-g-t-m from palmer@chromium.org,
pkasting@chromium.org, avi@chromium.org
Link to the patchset: https://codereview.chromium.org/1314843007/#ps320001
(title: "rebase fixups")

[commit-bot: I haz the power, 2015-09-04 20:03:30]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1314843007/320001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1314843007/320001

[commit-bot: I haz the power, 2015-09-04 21:44:56]

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

[commit-bot: I haz the power, 2015-09-04 21:44:57]

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

[estark, 2015-09-05 00:02:32]

Patchset #17 (id:340001) has been deleted

[estark, 2015-09-05 00:06:57]

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

[estark, 2015-09-05 00:06:58]

dzhioev: can you please review
chrome/browser/chromeos/login/ui/simple_web_view_dialog.cc?

This CL creates a SecurityStateModel class that the ToolbarModel uses to compute
the security level of a WebContents. The ChromeOS login webview's webcontents
needs a SecurityStateModel attached to it in order to use a ToolbarModel.

[sky, 2015-09-08 16:18:03]

chrome.gyp LGTM

[dzhioev (left Google), 2015-09-08 18:45:47]

On 2015/09/05 00:06:58, estark wrote:
> dzhioev: can you please review
> chrome/browser/chromeos/login/ui/simple_web_view_dialog.cc?
chrome/browser/chromeos/login/ui/simple_web_view_dialog.cc LGTM

[estark, 2015-09-08 18:47:21]

The CQ bit was checked by estark@chromium.org

[estark, 2015-09-08 18:47:21]

The patchset sent to the CQ was uploaded after l-g-t-m from avi@chromium.org,
pkasting@chromium.org, tedchoc@chromium.org, palmer@chromium.org
Link to the patchset: https://codereview.chromium.org/1314843007/#ps360001
(title: "create SecurityStateModel for chromeos login webview")

[estark, 2015-09-08 18:47:39]

On 2015/09/08 18:45:47, dzhioev wrote:
> On 2015/09/05 00:06:58, estark wrote:
> > dzhioev: can you please review
> > chrome/browser/chromeos/login/ui/simple_web_view_dialog.cc?
> chrome/browser/chromeos/login/ui/simple_web_view_dialog.cc LGTM

Thank you sky and dzhioev!

[commit-bot: I haz the power, 2015-09-08 18:50:09]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1314843007/360001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1314843007/360001

[commit-bot: I haz the power, 2015-09-08 20:07:49]

Committed patchset #17 (id:360001)

[commit-bot: I haz the power, 2015-09-08 20:08:43]

Patchset 17 (id:??) landed as
https://crrev.com/83a81afbef8f265815cb6c6a48690511825a1ce5
Cr-Commit-Position: refs/heads/master@{#347775}

[estark, 2015-09-09 19:36:07]

A revert of this CL (patchset #17 id:360001) has been created in
https://codereview.chromium.org/1324293003/ by estark@chromium.org.

The reason for reverting is: We need to call SecurityStateChanged() in a few
more places to avoid a crash and some UI bugs. See bugs 529664 and 529872..

[estark, 2015-09-09 23:31:01]

Patchset #18 (id:380001) has been deleted