Trigger DidChangeVisisbleSSLState() after OverrideEntry()

content/browser/frame_host/interstitial_page_impl.cc

 // Copyright 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "content/browser/frame_host/interstitial_page_impl.h"
 
 #include <vector>
 
 #include "base/bind.h"
 #include "base/compiler_specific.h"
@@ skipping 225 matching lines @@
   if (new_navigation_) {
     NavigationEntryImpl* entry = new NavigationEntryImpl;
     entry->SetURL(url_);
     entry->SetVirtualURL(url_);
     entry->set_page_type(PAGE_TYPE_INTERSTITIAL);
 
     // Give delegates a chance to set some states on the navigation entry.
     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_);
   render_view_host_ = CreateRenderViewHost();
   CreateWebContentsView();
 
   std::string data_url = "data:text/html;charset=utf-8," +
                          net::EscapePath(delegate_->GetHTMLContents());
   frame_tree_.root()->current_frame_host()->NavigateToURL(GURL(data_url));
 
@@ skipping 639 matching lines @@
 void InterstitialPageImpl::UnderlyingContentObserver::NavigationEntryCommitted(
     const LoadCommittedDetails& load_details) {
   interstitial_->OnNavigatingAwayOrTabClosing();
 }
 
 void InterstitialPageImpl::UnderlyingContentObserver::WebContentsDestroyed() {
   interstitial_->OnNavigatingAwayOrTabClosing();
 }
 
 }  // namespace content