Implemented WebStateObserver::DidFinishNavigation(NavigationHandle*).

ios/web/public/web_state/web_state_observer.h

 // Copyright 2014 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.
 
 #ifndef IOS_WEB_PUBLIC_WEB_STATE_WEB_STATE_OBSERVER_H_
 #define IOS_WEB_PUBLIC_WEB_STATE_WEB_STATE_OBSERVER_H_
 
 #include <stddef.h>
 
 #include <string>
 #include <vector>
 
 #include "base/macros.h"
 
 class GURL;
 
 namespace web {
 
 struct Credential;
 struct FaviconURL;
+class NavigationHandle;
 struct LoadCommittedDetails;
 class WebState;
 class TestWebState;
 class WebStateImpl;
 
 enum class PageLoadCompletionStatus : bool { SUCCESS = 0, FAILURE = 1 };
 
 // An observer API implemented by classes which are interested in various page
 // load events from WebState.
 class WebStateObserver {
@@ skipping 10 matching lines @@
   // This method is invoked when a navigation item has changed.
   virtual void NavigationItemChanged() {}
 
   // This method is invoked when a new non-pending navigation item is created.
   // This corresponds to one NavigationManager item being created
   // (in the case of new navigations) or renavigated to (for back/forward
   // navigations).
   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() {}
 
   // Called when the current page has stopped loading.
   virtual void DidStopLoading() {}
 
   // Called when the current page is loaded.
   virtual void PageLoaded(PageLoadCompletionStatus load_completion_status) {}
 
   // Called when the interstitial is dismissed by the user.
   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.
   // |progress| is a value between 0.0 (nothing loaded) to 1.0 (page fully
   // loaded).
   virtual void LoadProgressChanged(double progress) {}
 
   // Called on form submission. |user_initiated| is true if the user
   // interacted with the page.
   virtual void DocumentSubmitted(const std::string& form_name,
@@ skipping 89 matching lines @@
   void ResetWebState();
 
   WebState* web_state_;
 
   DISALLOW_COPY_AND_ASSIGN(WebStateObserver);
 };
 
 }  // namespace web
 
 #endif  // IOS_WEB_PUBLIC_WEB_STATE_WEB_STATE_OBSERVER_H_