Relanding "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 8eacd9689f1a6ffff74088183a154cfebf6d980f..73b5e460aa3da3a55aa6e6621a7a0b61eba5633f 100644
--- a/ios/web/web_state/ui/crw_web_controller.mm
+++ b/ios/web/web_state/ui/crw_web_controller.mm
@@ -702,6 +702,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 |title| to the last committed navigation item. |title| can not be nil.

[kkhorimoto, 2017/02/11 01:29:02]

|title| with the bars refers to a specific variable name, meaning the input
parameter here.  Can you replace it with something more descriptive (i.e. the
current NavigationItem's title)?

[Eugene But (OOO till 7-30), 2017/02/11 01:37:34]

Done.

+- (void)setNavigationItemTitle:(NSString*)title;
 // Returns YES if the current navigation item corresponds to a web page
 // loaded by a POST request.
 - (BOOL)isCurrentNavigationItemPOST;
@@ -1368,6 +1370,28 @@ const NSTimeInterval kSnapshotOverlayTransition = 0.5;
   }
 }
 
+- (void)setNavigationItemTitle:(NSString*)title {
+  DCHECK(title);
+  auto& navigationManager = _webStateImpl->GetNavigationManagerImpl();
+  web::NavigationItem* item = navigationManager.GetLastCommittedItem();
+  if (!item)
+    return;
+
+  base::string16 newTitle = base::SysNSStringToUTF16(title);
+  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:)]) {
+    [_delegate webController:self titleDidChange:title];
+  }
+}
+
 - (BOOL)isCurrentNavigationItemPOST {
   // |_pendingNavigationInfo| will be nil if the decidePolicy* delegate methods
   // were not called.
@@ -1792,14 +1816,13 @@ const NSTimeInterval kSnapshotOverlayTransition = 0.5;
   // Perform post-load-finished updates.
   [self didFinishWithURL:currentURL loadSuccess:loadSuccess];
 
-  // Inform the embedder the title changed.
+  NSString* title = [self.nativeController title];
+  if (title)
+    [self setNavigationItemTitle:title];
+
+  // If the controller handles title change notification, route those to the
+  // delegate.
   if ([_delegate respondsToSelector:@selector(webController:titleDidChange:)]) {
-    NSString* title = [self.nativeController title];
-    // If a title is present, notify the delegate.
-    if (title)
-      [_delegate webController:self titleDidChange:title];
-    // If the controller handles title change notification, route those to the
-    // delegate.
     if ([self.nativeController respondsToSelector:@selector(setDelegate:)]) {

[kkhorimoto, 2017/02/11 01:29:02]

Is the outer condition here still necessary? It seems that we should always be
setting the native content's delegate regardless of |_delegate|'s implemented
selectors.  It makes more sense to perform the outer check in the
CRWNativeContentDelegate callbacks.

[Eugene But (OOO till 7-30), 2017/02/11 01:37:34]

I don't know why outer condition even exist, and I don't want to remove it in
this CL to minimize changes on the branch. webController:titleDidChange: is
CRWWebDleegate method, which will be removed soon, so I'd prefer to leave this
code as it is.

       [self.nativeController setDelegate:self];
     }
@@ -4774,6 +4797,10 @@ const NSTimeInterval kSnapshotOverlayTransition = 0.5;
   // Attempt to update the HTML5 history state.
   [self updateHTML5HistoryState];
 
+  // This is the point where pending entry has been committed, and navigation
+  // item title should be updated.
+  [self setNavigationItemTitle:[_webView title]];
+
   // Report cases where SSL cert is missing for a secure connection.
   if (_documentURL.SchemeIsCryptographic()) {
     scoped_refptr<net::X509Certificate> cert =
@@ -4969,10 +4996,12 @@ const NSTimeInterval kSnapshotOverlayTransition = 0.5;
     return;
   }
 
-  if ([self.delegate
-          respondsToSelector:@selector(webController:titleDidChange:)]) {
-    DCHECK([_webView title]);
-    [self.delegate webController:self titleDidChange:[_webView title]];
+  bool hasPendingNavigation = web::WKNavigationState::COMMITTED <=
+                              [_navigationStates lastAddedNavigationState];
+  if (hasPendingNavigation) {
+    // 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 setNavigationItemTitle:[_webView title]];
   }
 }