Remove WebContentsObserver::DidCommitNavigation

content/public/browser/web_contents_observer.h

Index: content/public/browser/web_contents_observer.h
diff --git a/content/public/browser/web_contents_observer.h b/content/public/browser/web_contents_observer.h
index 6a2b1acdd7976fa13a5855cae2faa574cabb1530..618d5919b8f6446fbc0b25d78bed4e94424f2f79 100644
--- a/content/public/browser/web_contents_observer.h
+++ b/content/public/browser/web_contents_observer.h
@@ -122,8 +122,8 @@ class CONTENT_EXPORT WebContentsObserver : public IPC::Listener,
   // Called when a navigation started in the WebContents. |navigation_handle|
   // is unique to a specific navigation. The same |navigation_handle| will be
   // provided on subsequent calls to
-  // DidRedirect/Commit/FinishNavigation/ReadyToCommitNavigation related to
-  // this navigation.
+  // DidRedirect/FinishNavigation/ReadyToCommitNavigation related to this
+  // navigation.
   //
   // Note that this is fired by navigations in any frame of the WebContents,
   // not just the main frame.
@@ -145,16 +145,17 @@ class CONTENT_EXPORT WebContentsObserver : public IPC::Listener,
   // the first point in time where a RenderFrameHost is associated with the
   // navigation. Observers that want to initialize any renderer side
   // structures/state before the RenderFrame is navigated, should use this

[Charlie Reis, 2015/09/18 17:05:21]

nit: Drop comma.

[clamy, 2015/09/18 20:37:33]

Done.

-  // method as opposed to DidCommitNavigation, which is after the fact.
+  // method as opposed to DidCommitNavigation, which is after the fact. Other

[Charlie Reis, 2015/09/18 17:05:21]

DidCommitNavigation is gone now.

[clamy, 2015/09/18 20:37:33]

Done.

+  // observers should implement DidFinishNavigation instead.

[nasko, 2015/09/18 16:42:16]

Let's use a bit stronger language, something like "If not absolutely certain
this is the correct method to use, implement DidFinishNavigation only.".
Charlie can probably help wordsmith this a bit better too.

[Charlie Reis, 2015/09/18 17:05:21]

I'd reorder the whole thing to emphasize this point.  Something like:

"Called when the navigation is ready to be committed in a renderer. Most
observers should use DidFinishNavigation instead, which happens right after the
navigation commits. This method is for observers that want to initialize
renderer-side state just before the RenderFrame commits the navigation. 

This is the first point in time where a RenderFrameHost is associated with the
navigation."

[clamy, 2015/09/18 20:37:34]

Done.

   virtual void ReadyToCommitNavigation(NavigationHandle* navigation_handle) {}
 
-  // Called when a navigation was committed.
-  virtual void DidCommitNavigation(NavigationHandle* navigation_handle) {}
-
-  // Called when a navigation stopped in the WebContents. This happens when a
-  // navigation is either aborted, replaced by a new one, or the document load
-  // finishes. Note that |navigation_handle| will be destroyed at the end of
-  // this call, so do not keep a reference to it afterward.
+  // Called when a navigation finished in the WebContents. This happens when a
+  // navigation is either committed, aborted or replaced by a new one. Note

[nasko, 2015/09/18 16:42:16]

nit: drop "either"

[clamy, 2015/09/18 20:37:33]

Done.

+  // that |navigation_handle| will be destroyed at the end of this call, so do
+  // not keep a reference to it afterward.
+  // If this called because the navigation committed, then the document load

[Charlie Reis, 2015/09/18 17:05:21]

nit: this is

[clamy, 2015/09/18 20:37:34]

Done.

+  // will still be ongoing in the RenderFrameHost returned by
+  // |navigation_handle|.

[Charlie Reis, 2015/09/18 17:05:21]

Add something like: "Use DidStopLoading and related methods to listen for
continued events from this RenderFrameHost."

[clamy, 2015/09/18 20:37:34]

Done.

   virtual void DidFinishNavigation(NavigationHandle* navigation_handle) {}
 

[nasko, 2015/09/18 16:48:44]

Let's also organize the document loading notifications in a separate part of the
file with an explicit header, so people can find them a bit more easily.

[clamy, 2015/09/18 20:37:34]

Done. I did not include the DidNavigateMainFrame and DidNavigateAnyFrame since
they seem more navigation related, and we may want to remove them in the future.

   // ---------------------------------------------------------------------------