Show red lock icon on captive portal SSL interstitial

Show red lock icon on captive portal SSL interstitial

We were previously showing the neutral icon on captive portal
interstitials. This isn't that big a deal because the interstitial is
non-bypassable, but we should show a red/dangerous icon to be consistent
with the bad clock interstitial.

BUG=653779
Committed: https://crrev.com/3e2553a9defa7bdf50be5d18f0cb8ce4ec539ad9
Cr-Commit-Position: refs/heads/master@{#424265}

[estark, 2016-10-10 19:09:45]

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

[commit-bot: I haz the power, 2016-10-10 19:10:04]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[estark, 2016-10-10 19:11:10]

Description was changed from

==========
ht be# Enter a description of the change.
Show red lock icon on captive portal SSL interstitial

We were previously showing the neutral icon on captive portal
interstitials. This isn't that big a deal because the interstitial is
non-bypassable, but we should show a red/dangerous icon to be consistent
with the bad clock interstitial.

BUG=653779
==========

to

==========
Show red lock icon on captive portal SSL interstitial

We were previously showing the neutral icon on captive portal
interstitials. This isn't that big a deal because the interstitial is
non-bypassable, but we should show a red/dangerous icon to be consistent
with the bad clock interstitial.

BUG=653779
==========

[estark, 2016-10-10 19:11:38]

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

[estark, 2016-10-10 19:11:39]

meacer, PTAL

[meacer, 2016-10-10 19:14:43]

LGTM with a question. 

You might also want to check if the links at chrome://interstitials still work
fine after this change.

https://codereview.chromium.org/2408713003/diff/1/chrome/browser/ssl/captive_...
File chrome/browser/ssl/captive_portal_blocking_page.cc (right):

https://codereview.chromium.org/2408713003/diff/1/chrome/browser/ssl/captive_...
chrome/browser/ssl/captive_portal_blocking_page.cc:250: }
Is it possible to push this call to the superclass? Safebrowsing also uses
SecurtyInterstitial, but I think we use a strikethrough for those as well?

[commit-bot: I haz the power, 2016-10-10 19:49:40]

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

[commit-bot: I haz the power, 2016-10-10 19:49:41]

Dry run: This issue passed the CQ dry run.

[estark, 2016-10-10 21:41:39]

https://codereview.chromium.org/2408713003/diff/1/chrome/browser/ssl/captive_...
File chrome/browser/ssl/captive_portal_blocking_page.cc (right):

https://codereview.chromium.org/2408713003/diff/1/chrome/browser/ssl/captive_...
chrome/browser/ssl/captive_portal_blocking_page.cc:250: }
On 2016/10/10 19:14:43, Mustafa Emre Acer wrote:
> Is it possible to push this call to the superclass? Safebrowsing also uses
> SecurtyInterstitial, but I think we use a strikethrough for those as well?

That's a surprisingly hard question to answer. :) We do show the red/dangerous
icon on safe browsing interstitials, but it's not implemented via
OverrideEntry(). Instead, it's implemented in
SecurityStateModel/ChromeSecurityStateModelClient (see
https://cs.chromium.org/chromium/src/chrome/browser/ssl/chrome_security_state...).

However, this whole OverrideEntry() thing is kind of weird and I don't see that
it's necessary at all. Instead of calling out to
InterstitialPageDelegate::OverrideEntry() from InterstitialPageImpl::Show(), we
should be able to just directly set the SSLStatus on the transient
NavigationEntry like we would for a normal navigation entry. I think we can set
it to content::SSLStatus(GetSecurityStyleForResource(...), ssl_info.cert,
ssl_info) -- no need to hardcode it to SECURITY_STYLE_AUTHENTICATION_BROKEN for
all interstitials, and no need to call out to the embedder to set it in a
special way.

WDYT of that idea? If you're on board, I'll do it in a separate CL (to separate
the bug fix from the refactoring.)

[estark, 2016-10-10 21:42:16]

(Oh, also, I checked that chrome://interstitials looks okay as requested; they
all show the (i) icon because of the chrome:// scheme, though.)

[meacer, 2016-10-10 22:08:50]

Still Lgtm.

https://codereview.chromium.org/2408713003/diff/1/chrome/browser/ssl/captive_...
File chrome/browser/ssl/captive_portal_blocking_page.cc (right):

https://codereview.chromium.org/2408713003/diff/1/chrome/browser/ssl/captive_...
chrome/browser/ssl/captive_portal_blocking_page.cc:250: }
On 2016/10/10 21:41:38, estark wrote:
> On 2016/10/10 19:14:43, Mustafa Emre Acer wrote:
> > Is it possible to push this call to the superclass? Safebrowsing also uses
> > SecurtyInterstitial, but I think we use a strikethrough for those as well?
> 
> That's a surprisingly hard question to answer. :) We do show the red/dangerous
> icon on safe browsing interstitials, but it's not implemented via
> OverrideEntry(). Instead, it's implemented in
> SecurityStateModel/ChromeSecurityStateModelClient (see
>
https://cs.chromium.org/chromium/src/chrome/browser/ssl/chrome_security_state...).
> 
> However, this whole OverrideEntry() thing is kind of weird and I don't see
that
> it's necessary at all. Instead of calling out to
> InterstitialPageDelegate::OverrideEntry() from InterstitialPageImpl::Show(),
we
> should be able to just directly set the SSLStatus on the transient
> NavigationEntry like we would for a normal navigation entry. I think we can
set
> it to content::SSLStatus(GetSecurityStyleForResource(...), ssl_info.cert,
> ssl_info) -- no need to hardcode it to SECURITY_STYLE_AUTHENTICATION_BROKEN
for
> all interstitials, and no need to call out to the embedder to set it in a
> special way.
> 
> WDYT of that idea? If you're on board, I'll do it in a separate CL (to
separate
> the bug fix from the refactoring.)

Sounds great! Thanks for looking into this.

[estark, 2016-10-10 22:19:48]

The CQ bit was checked by estark@chromium.org

[commit-bot: I haz the power, 2016-10-10 22:20:18]

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

[commit-bot: I haz the power, 2016-10-10 22:25:56]

Description was changed from

==========
Show red lock icon on captive portal SSL interstitial

We were previously showing the neutral icon on captive portal
interstitials. This isn't that big a deal because the interstitial is
non-bypassable, but we should show a red/dangerous icon to be consistent
with the bad clock interstitial.

BUG=653779
==========

to

==========
Show red lock icon on captive portal SSL interstitial

We were previously showing the neutral icon on captive portal
interstitials. This isn't that big a deal because the interstitial is
non-bypassable, but we should show a red/dangerous icon to be consistent
with the bad clock interstitial.

BUG=653779
==========

[commit-bot: I haz the power, 2016-10-10 22:25:57]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-10-10 22:30:25]

Description was changed from

==========
Show red lock icon on captive portal SSL interstitial

We were previously showing the neutral icon on captive portal
interstitials. This isn't that big a deal because the interstitial is
non-bypassable, but we should show a red/dangerous icon to be consistent
with the bad clock interstitial.

BUG=653779
==========

to

==========
Show red lock icon on captive portal SSL interstitial

We were previously showing the neutral icon on captive portal
interstitials. This isn't that big a deal because the interstitial is
non-bypassable, but we should show a red/dangerous icon to be consistent
with the bad clock interstitial.

BUG=653779

Committed: https://crrev.com/3e2553a9defa7bdf50be5d18f0cb8ce4ec539ad9
Cr-Commit-Position: refs/heads/master@{#424265}
==========

[commit-bot: I haz the power, 2016-10-10 22:30:26]

Patchset 1 (id:??) landed as
https://crrev.com/3e2553a9defa7bdf50be5d18f0cb8ce4ec539ad9
Cr-Commit-Position: refs/heads/master@{#424265}