Using WebContents::UpdateTitleForEntry() instead of NavigationEntry::SetTitle()

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 036f3ea83dfeef00e0cfb1b00977b31ffbb91307..583428933f0d7c358402fc9a4afdb82f935959f7 100644
--- a/content/browser/frame_host/interstitial_page_impl.cc
+++ b/content/browser/frame_host/interstitial_page_impl.cc
@@ -296,9 +296,10 @@ void InterstitialPageImpl::Hide() {
   frame_tree_.root()->ResetForNewProcess();
   controller_->delegate()->DetachInterstitialPage();
   // Let's revert to the original title if necessary.
-  NavigationEntry* entry = controller_->GetVisibleEntry();
+  NavigationEntryImpl* entry = controller_->GetVisibleEntry();
   if (entry && !new_navigation_ && should_revert_web_contents_title_) {
-    entry->SetTitle(original_web_contents_title_);
+    static_cast<WebContentsImpl*>(web_contents_)->UpdateTitleForEntry(
+        entry, original_web_contents_title_);
     controller_->delegate()->NotifyNavigationStateChanged(
         INVALIDATE_TYPE_TITLE);

[Charlie Reis, 2016/06/24 22:21:17]

One way to make this cleaner would be to move
NotifyNavigationStateChanged(INVALIDATE_TYPE_TITLE) inside UpdateTitleForEntry. 
All the call sites do it immediately afterward, and it looks like they should
only do it if it returns true.

If we put that call inside UpdateTitleForEntry, it probably doesn't need a
return value at all.  Also, it seems consistent with the intent of
UpdateTitleForEntry, which is updating both the entry and the view.

[afakhry, 2016/06/27 14:29:18]

I made it public, moved the call to NotifyNavigationStateChanged() from
WebContentsImpl::UpdateTitle() to UpdateTitleForEntry(), I didn't change the
return value however, just case it's needed in the future as
UpdateTitleForEntry() maybe a no-op sometimes.

[Charlie Reis, 2016/06/28 21:13:31]

If no one is listening to the return value, we should remove it.  It's easy to
add back if it's needed later.  Otherwise it's confusing why it's unused.

   }
@@ -391,7 +392,7 @@ void InterstitialPageImpl::UpdateTitle(
 
   RenderViewHost* render_view_host = render_frame_host->GetRenderViewHost();
   DCHECK(render_view_host == render_view_host_);
-  NavigationEntry* entry = controller_->GetVisibleEntry();
+  NavigationEntryImpl* entry = controller_->GetVisibleEntry();
   if (!entry) {
     // There may be no visible entry if no URL has committed (e.g., after
     // window.open("")).  InterstitialPages with the new_navigation flag create
@@ -410,7 +411,8 @@ void InterstitialPageImpl::UpdateTitle(
   }
   // TODO(evan): make use of title_direction.
   // http://code.google.com/p/chromium/issues/detail?id=27094
-  entry->SetTitle(title);
+  static_cast<WebContentsImpl*>(web_contents_)->UpdateTitleForEntry(entry,
+                                                                    title);
   controller_->delegate()->NotifyNavigationStateChanged(INVALIDATE_TYPE_TITLE);

[Charlie Reis, 2016/06/24 22:21:17]

Same here.

[afakhry, 2016/06/27 14:29:18]

Done.

 }
 
@@ -667,8 +669,18 @@ void InterstitialPageImpl::DontProceed() {
     TakeActionOnResourceDispatcher(CANCEL);
 
   if (should_discard_pending_nav_entry_) {
+    // Revert back to the last committed title (if any) before we discard the
+    // non-committed entries.
+    NavigationEntryImpl* last_committed_entry =
+        controller_->GetLastCommittedEntry();
+    if (last_committed_entry) {
+      NavigationEntryImpl* visible_entry = controller_->GetVisibleEntry();

[Charlie Reis, 2016/06/24 22:21:17]

I think this might be wrong.  In general, we shouldn't mix values from
GetVisibleEntry and GetLastCommittedEntry, which could be two unrelated pages.

Here, I'm not 100% sure this will always return the transient entry and not the
pending entry, in which case it might put the old title on the pending entry. 
Using GetTransientEntry would be safer if that's your intent.

That said, I'm a bit skeptical about updating the transient entry as well.  Why
would we need to put the last committed page's title on the transient entry?

[afakhry, 2016/06/27 14:29:18]

We need this if we want TitleWasSet() to be emitted when "Back to saftey" is
pressed.
I used the visible entery as I found it to be corresponding to the interstitial
page itself when it's being dismissed due to "Back to safety" pressed. I also
found that the last committed entry corresponds to the page we were at before we
navigated to the malware page that resulted in the interstitial showing up.
I used the transient_entry, as suggested, instead of the visible entry and it
worked too.

The idea is that I need different entries with different titles, otherwise
calling UpdateTitleForEntry() will be a no-op here:
https://cs.chromium.org/chromium/src/content/browser/web_contents/web_content...
and here:
https://cs.chromium.org/chromium/src/content/browser/web_contents/web_content...

[Charlie Reis, 2016/06/28 00:47:09]

I feel like we're going about this the wrong way.  We shouldn't have to change
the title of the transient entry just before it's deleted to notify observers. 
Deleting the transient entry and going back to showing the last committed entry
(on the DiscardNonCommittedEntries call below) seems like it should have a way
to trigger a title update, since the visible NavigationEntry changes at that
point.

Can you look to see if there's a reasonable way to do that?  I can try to take a
look later for suggestions, but I'm out of time for the moment.

[afakhry, 2016/06/28 14:34:17]

Yes, it happens as a result of calling:

delegate_->NotifyNavigationStateChanged(INVALIDATE_TYPE_ALL);
here:
https://cs.chromium.org/chromium/src/content/browser/frame_host/navigation_co...

Which updates the title of the view, but doesn't emit TitleWasSet() events.
That's why my first suggested solution was to do emit TitleWasSet() from inside
NotifyNavigationStateChanged() when INVALIDATE_TYPE_TITLE is one of the flags as
mentioned here: https://bugs.chromium.org/p/chromium/issues/detail?id=607074#c6

I'm not sure what to do here since by the time 
delegate_->NotifyNavigationStateChanged(INVALIDATE_TYPE_ALL);
is called, the transient entry has been discarded already.

[Charlie Reis, 2016/06/28 21:13:31]

I see.

This seems to boil down to WebContentsObserver doing a poor job of exposing
title change events.  TitleWasSet was apparently only meant for cases that a
page manually changes its title.  Observers have to separately watch for
navigation commits (which can also change the title), and don't even have a way
to watch for this case, where a visible (transient or pending) entry is canceled
and the title goes back to showing the last committed entry.

Here, we're kind of adding a hack to make TitleWasSet get fired for the last
case in addition to times the title is set manually.  Worse, we're doing it by
changing the title of the transient entry to an incorrect value just before we
throw it away.

I see two ways forward:

1) Change the WebContentsObserver API to have a TitleChanged event instead, and
fire it in all the cases that the title can change.  This means we'll have an
extra event on every navigation commit, but it simplifies the cases where people
just want to know if the title changes.  I'm not sure how much existing code
would need updating, but it does seem like a nicer end state.

2) Stick with TitleWasSet for now, and make DiscardNonCommittedEntries fire it
if the title was changed as a result.  This might require exposing a new method
on NavigationControllerDelegate (implemented by WebContents) that fires
TitleWasSet, without requiring a change to a NavigationEntry like
UpdateTitleForEntry.  Also update the documentation of TitleWasSet to say it
includes this case but not title changes due to navigation.

Option 2 seems a little like we're continuing the hacks, but I suppose it's also
a necessary step if we want to do option 1 at some point.  That might be
sufficient for this CL.  What do you think?

+      static_cast<WebContentsImpl*>(web_contents_)->UpdateTitleForEntry(
+          visible_entry, last_committed_entry->GetTitle());
+    }
+
     // Since no navigation happens we have to discard the transient entry
-    // explicitely.  Note that by calling DiscardNonCommittedEntries() we also
+    // explicitly.  Note that by calling DiscardNonCommittedEntries() we also
     // discard the pending entry, which is what we want, since the navigation is
     // cancelled.
     controller_->DiscardNonCommittedEntries();