Track "display" and "run" separately for mixed content, and make the latter d...

Track "display" and "run" separately for mixed content, and make the latter downgrade the SSL state to "authentication broken".

Make the "display" state only affect the current tab (not the entire host).

Fix an SSL browser test by supplying the appropriate SiteInstance*.

Move a test from "disabled" to "flaky" since it at least passes for me.

Make the SSLManager header and .cc files put functions in the same order, and make that order somewhat saner.

BUG=15072, 18626, 40932, 42758
TEST=Covered by browser tests

[abarth-chromium, 2010-05-13 20:08:17]

http://codereview.chromium.org/2067003/diff/1/7
File chrome/browser/ssl/ssl_host_state.h (right):

http://codereview.chromium.org/2067003/diff/1/7#newcode31
chrome/browser/ssl/ssl_host_state.h:31: void HostDisplayedInsecureContent(const
std::string& host, int pid);
I'm not sure we should be tracking "DisplayedInsecureContent" in the
SSLHostState.  It's not really a property of the host (because display errors
don't propagate to other frames with the same host).  Instead, it's a property
of the tab or maybe the navigation entry.  It's something bigger than a
navigation entry but smaller than a tab.

[Peter Kasting, 2010-05-14 18:53:32]

New snap up that makes the "display" state tab-scoped as you've requested.

In this version, we've entirely lost the code in the old
"SSLHostState::DidMarkHostAsBroken()" function that returned true for intranet
hosts.  I don't know whether that is supposed to map to anything in the new
world.

[abarth-chromium, 2010-05-14 19:40:46]

We're supposed to treat intranet hosts like did run insecure content.

[abarth-chromium, 2010-05-14 19:41:10]

I need to eat lunch, but I'll look after my after-lunch meeting.

[Peter Kasting, 2010-05-14 19:49:10]

On 2010/05/14 19:40:46, abarth wrote:
> We're supposed to treat intranet hosts like did run insecure content.

OK, changed the code to do that.

[abarth-chromium, 2010-05-14 22:29:39]

LGTM.  The changes to SSLManager were a bit hard to follow because you moved a
bunch of code around, but I think I understood it all.

http://codereview.chromium.org/2067003/diff/18002/36008
File chrome/browser/ssl/ssl_host_state.cc (right):

http://codereview.chromium.org/2067003/diff/18002/36008#newcode40
chrome/browser/ssl/ssl_host_state.cc:40: return
!!ran_insecure_content_hosts_.count(BrokenHostEntry(host, pid));
Any reason you changed this from find to count?

http://codereview.chromium.org/2067003/diff/18002/36013
File chrome/browser/ssl/ssl_policy.cc (right):

http://codereview.chromium.org/2067003/diff/18002/36013#newcode173
chrome/browser/ssl/ssl_policy.cc:173: entry->ssl().set_ran_mixed_content();
It's a bit strange that things get renamed from "insecure" to "mixed" at this
point.  I'm not sure that's worth worrying too much about.

[Peter Kasting, 2010-05-14 22:33:11]

Sorry about moving the stuff in SSLManager around -- I originally rewrote it a
bit more than this, and was annoyed that I couldn't find any functions where I
expected.  Then I had to back that out because my new version wasn't correct. 
Blah.

http://codereview.chromium.org/2067003/diff/18002/36008
File chrome/browser/ssl/ssl_host_state.cc (right):

http://codereview.chromium.org/2067003/diff/18002/36008#newcode40
chrome/browser/ssl/ssl_host_state.cc:40: return
!!ran_insecure_content_hosts_.count(BrokenHostEntry(host, pid));
On 2010/05/14 22:29:39, abarth wrote:
> Any reason you changed this from find to count?

Only that it's shorter (one line instead of two).

http://codereview.chromium.org/2067003/diff/18002/36013
File chrome/browser/ssl/ssl_policy.cc (right):

http://codereview.chromium.org/2067003/diff/18002/36013#newcode173
chrome/browser/ssl/ssl_policy.cc:173: entry->ssl().set_ran_mixed_content();
On 2010/05/14 22:29:39, abarth wrote:
> It's a bit strange that things get renamed from "insecure" to "mixed" at this
> point.  I'm not sure that's worth worrying too much about.

I'd be happy to try for consistent naming if you have a preference.  Should I
remove "mixed content" everywhere and make it all "insecure content"?

[abarth-chromium, 2010-05-14 22:35:03]

http://codereview.chromium.org/2067003/diff/18002/36013
File chrome/browser/ssl/ssl_policy.cc (right):

http://codereview.chromium.org/2067003/diff/18002/36013#newcode173
chrome/browser/ssl/ssl_policy.cc:173: entry->ssl().set_ran_mixed_content();
> I'd be happy to try for consistent naming if you have a preference.  Should I
> remove "mixed content" everywhere and make it all "insecure content"?

Sure.  I think that's the path of least resistance for a consistent name.

[Peter Kasting, 2010-05-15 00:38:57]

Landed in r47347, closing.