Implemented WebStateObserver::DidFinishNavigation(NavigationHandle*).

ios/web/public/web_state/web_state_observer.h

Index: ios/web/public/web_state/web_state_observer.h
diff --git a/ios/web/public/web_state/web_state_observer.h b/ios/web/public/web_state/web_state_observer.h
index 6fb99f3540d0186c3bc8c172febc1fb887351271..395265d9a780cb546580c47637262999b7a5fd44 100644
--- a/ios/web/public/web_state/web_state_observer.h
+++ b/ios/web/public/web_state/web_state_observer.h
@@ -18,6 +18,7 @@ namespace web {
 
 struct Credential;
 struct FaviconURL;
+class NavigationHandle;
 struct LoadCommittedDetails;
 class WebState;
 class TestWebState;
@@ -48,6 +49,25 @@ class WebStateObserver {
   virtual void NavigationItemCommitted(
       const LoadCommittedDetails& load_details) {}
 
+  // Called when a navigation finished in the WebState. This happens when a
+  // navigation is committed, aborted or replaced by a new one. To know if the
+  // navigation has resulted in an error page, use
+  // NavigationHandle::IsErrorPage.

[kkhorimoto, 2017/02/22 01:42:38]

IsErrorPage()

[Eugene But (OOO till 7-30), 2017/02/22 22:08:32]

Done.

+  //
+  // If this is called because the navigation committed, then the document load
+  // will still be ongoing in the WebState returned by |navigation_handle|.

[kkhorimoto, 2017/02/22 01:42:38]

This comment should be updated if we remove NavigationHandle::GetWebState().

[Eugene But (OOO till 7-30), 2017/02/22 22:08:32]

Acknowledged.

+  // Use the document loads events such as DidStopLoading
+  // and related methods to listen for continued events from this
+  // WebState.
+  //
+  // Note that this is fired by same-page navigations, such as fragment

[kkhorimoto, 2017/02/22 01:42:38]

NIT: s/Note that this is fired/This is also fired/

"Note that" has a conversational, imperative tone similarly to when we use "We"
in comments.  I think it's better to have comments that are simply declarative
statements about functionality.

[Eugene But (OOO till 7-30), 2017/02/22 22:08:32]

Done.

+  // navigations or pushState/replaceState, which will not result in a document
+  // change. To filter these out, use NavigationHandle::IsSamePage.

[kkhorimoto, 2017/02/22 01:42:38]

IsSameDocument()

[Eugene But (OOO till 7-30), 2017/02/22 22:08:32]

Done.

+  //
+  // Note that |navigation_handle| will be destroyed at the end of this call,

[kkhorimoto, 2017/02/22 01:42:38]

Same comment about "Note that"

[Eugene But (OOO till 7-30), 2017/02/22 22:08:32]

Done.

+  // so do not keep a reference to it afterward.
+  virtual void DidFinishNavigation(NavigationHandle* navigation_handle) {}

[kkhorimoto, 2017/02/22 01:42:38]

Should we use a "const NavigationHandle&" here?

[kkhorimoto, 2017/02/22 21:55:44]

Can you address the comments in this file as well?  They're mostly comment nits
(and page/document stuff), but this would be an actual implementation change
that we might want, since we're passing LoadCommittedDetails this way.

[Eugene But (OOO till 7-30), 2017/02/22 22:08:32]

Pointer is used here, so clients can hold a pointer to this object. Reference
must always be valid, so it does not work that well.

[Eugene But (OOO till 7-30), 2017/02/22 22:08:32]

Sorry, I missed those.

+
   // Called when the current page has started loading.
   virtual void DidStartLoading() {}
 
@@ -61,9 +81,11 @@ class WebStateObserver {
   virtual void InterstitialDismissed() {}
 
   // Called on URL hash change events.
+  // TODO(crbug.com/692331): Remove this method and use |DidFinishNavigation|.
   virtual void UrlHashChanged() {}
 
   // Called on history state change events.
+  // TODO(crbug.com/692331): Remove this method and use |DidFinishNavigation|.
   virtual void HistoryStateChanged() {}
 
   // Notifies the observer that the page has made some progress loading.