Fixed title updating for back forward navigation.

ios/web/web_state/ui/crw_web_controller.mm

Index: ios/web/web_state/ui/crw_web_controller.mm
diff --git a/ios/web/web_state/ui/crw_web_controller.mm b/ios/web/web_state/ui/crw_web_controller.mm
index 5dde5d99291db2cfe51bb4cfe8c6cf642a1b5888..ee2250f9017a77fad3ad6c495daece67bb9e6d8d 100644
--- a/ios/web/web_state/ui/crw_web_controller.mm
+++ b/ios/web/web_state/ui/crw_web_controller.mm
@@ -718,6 +718,8 @@ typedef void (^ViewportStateCompletion)(const web::PageViewportState*);
                     stateObject:(NSString*)stateObject;
 // Sets _documentURL to newURL, and updates any relevant state information.
 - (void)setDocumentURL:(const GURL&)newURL;
+// Sets WKWebView's title to the last committed navigation item.
+- (void)updateNavigationItemTitle;
 // Returns YES if the current navigation item corresponds to a web page
 // loaded by a POST request.
 - (BOOL)isCurrentNavigationItemPOST;
@@ -1379,6 +1381,28 @@ const NSTimeInterval kSnapshotOverlayTransition = 0.5;
   }
 }
 
+- (void)updateNavigationItemTitle {
+  auto& navigationManager = _webStateImpl->GetNavigationManagerImpl();
+  web::NavigationItem* item = navigationManager.GetLastCommittedItem();
+  if (!item)
+    return;
+
+  base::string16 newTitle = base::SysNSStringToUTF16([_webView title]);

[kkhorimoto, 2017/02/07 23:59:44]

How about a separate variable for the NSString equal to [_webView title], since
it's used multiple places in this function?

[Eugene But (OOO till 7-30), 2017/02/08 01:50:27]

Done.

+  if (item->GetTitle() == newTitle)
+    return;
+
+  item->SetTitle(newTitle);
+  // TODO(crbug.com/546218): See if this can be removed; it's not clear that
+  // other platforms send this (tab sync triggers need to be compared against
+  // upstream).
+  navigationManager.OnNavigationItemChanged();
+
+  if ([_delegate respondsToSelector:@selector(webController:titleDidChange:)]) {
+    DCHECK([_webView title]);

[kkhorimoto, 2017/02/07 23:59:44]

Should we be DCHECKing this earlier?  Whats the consequence of passing a nil
title here?

[Eugene But (OOO till 7-30), 2017/02/08 01:50:27]

Done. I don't think anything will break, but this method should not be called
when title is nil (f.e. when web process crashes).

+    [_delegate webController:self titleDidChange:[_webView title]];
+  }
+}
+
 - (BOOL)isCurrentNavigationItemPOST {
   // |_pendingNavigationInfo| will be nil if the decidePolicy* delegate methods
   // were not called.
@@ -4754,9 +4778,10 @@ const NSTimeInterval kSnapshotOverlayTransition = 0.5;
   DCHECK_EQ(_webView, webView);
   _certVerificationErrors->Clear();
 
-  // This is the point where the document's URL has actually changed, and
-  // pending navigation information should be applied to state information.
+  // This is the point where the document's URL and title have actually changed,
+  // and pending navigation information should be applied to state information.
   [self setDocumentURL:net::GURLWithNSURL([_webView URL])];
+  [self updateNavigationItemTitle];
 
   if (!_lastRegisteredRequestURL.is_valid() &&
       _documentURL != _lastRegisteredRequestURL) {
@@ -4994,10 +5019,11 @@ const NSTimeInterval kSnapshotOverlayTransition = 0.5;
     return;
   }
 
-  if ([self.delegate
-          respondsToSelector:@selector(webController:titleDidChange:)]) {
-    DCHECK([_webView title]);
-    [self.delegate webController:self titleDidChange:[_webView title]];
+  if (web::WKNavigationState::COMMITTED <=

[kkhorimoto, 2017/02/07 23:59:44]

It's not obvious without looking up the enum definition ordering what this
means.  How about instead of <=, we check if it's == to COMMITTED, FINISHED, or
FAILED?  It's a little more verbose, but helps readability.

[Eugene But (OOO till 7-30), 2017/02/08 01:50:27]

I think checking multiple states is more error prone (one state can be missed by
accident). Added explanatory variable to improve readability.

+      [_navigationStates lastAddedNavigationState]) {
+    // Do not update the title if there is a navigation in progress because
+    // there is no way to tell if KVO change fired for new or previous page.
+    [self updateNavigationItemTitle];
   }
 }