Trigger DidChangeVisisbleSSLState() after OverrideEntry()

content/browser/frame_host/interstitial_page_impl.cc

Index: content/browser/frame_host/interstitial_page_impl.cc
diff --git a/content/browser/frame_host/interstitial_page_impl.cc b/content/browser/frame_host/interstitial_page_impl.cc
index 88d720fcf8b866593c68f598d8be0acba3c8b11a..556190b7b8bcb08374ca8c6c672b6156bf9a496a 100644
--- a/content/browser/frame_host/interstitial_page_impl.cc
+++ b/content/browser/frame_host/interstitial_page_impl.cc
@@ -243,6 +243,8 @@ void InterstitialPageImpl::Show() {
     delegate_->OverrideEntry(entry);
 
     controller_->SetTransientEntry(entry);
+
+    static_cast<WebContentsImpl*>(web_contents_)->DidChangeVisibleSSLState();

[estark, 2015/06/25 00:35:47]

To avoid unnecessary repainting, we could return an argument from
OverrideEntry() indicating whether the visible SSL state has been changed, and
only call DidChangeVisibleSSLState() when the argument is true. Not sure if
that's necessary (i.e. how much we care about the extra repaints).

[nasko, 2015/06/25 12:59:32]

In general, we do care about repaints, as we are trying to have a smooth UX on
all platforms, regardless of how powerful they are. However, looking at the code
in the current delegate, it is totally unconditional, so this will always have
to be called. I'd leave it up to you to decide whether to change it to return a
boolean or not.

[estark, 2015/06/25 14:54:45]

Hmm, I think I'll leave it as is. I was thinking that we need not fire the event
on interstitials that use delegates other than SSLBlockingPage (e.g. malware
interstitial, which I imagine uses the default OverrideEntry() implementation
that does nothing). But after thinking about it more, I think we do always want
to fire the event. The malware interstitial (e.g. https://ianfette.org) shows up
as the neutral http:// page icon, and so we want observers to get notified of
that neutral icon. So I guess what I'm saying is that, if we didn't fire
VisibleSSLStateChanged() unconditionally here, we'd have a bug where devtools
security panel shows the lock icon of the previous page when you're on the
malware interstitial.

FWIW, it looks like the normal navigation path also repaints the toolbar twice
-- once by calling SSLManager::DidCommitProvisionalLoad() which calls
DiChangeVisibleSSLState(), and once by calling NotifyNavigationStateChanged() --
so we're not making interstitial repaints any worse than normal navigations.

[nasko, 2015/06/26 08:53:04]

Thanks for digging out the details and for the thorough explanation!

   }
 
   DCHECK(!render_view_host_);