Remove WebContentsObserver::DidCommitNavigation

content/public/browser/web_contents_observer.h

 // Copyright (c) 2012 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 CONTENT_PUBLIC_BROWSER_WEB_CONTENTS_OBSERVER_H_
 #define CONTENT_PUBLIC_BROWSER_WEB_CONTENTS_OBSERVER_H_
 
 #include "base/process/kill.h"
 #include "base/process/process_handle.h"
 #include "content/common/content_export.h"
@@ skipping 104 matching lines @@
   // been replaced is in |old_host|, which is nullptr if the old RVH was shut
   // down.
   virtual void RenderViewHostChanged(RenderViewHost* old_host,
                                      RenderViewHost* new_host) {}
 
   // Navigation related events ------------------------------------------------
 
   // 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.
   //
   // Note that more than one navigation can be ongoing in the same frame at the
   // same time (including the main frame). Each will get its own
   // NavigationHandle.
   //
   // Note that there is no guarantee that DidFinishNavigation will be called
   // for any particular navigation before DidStartNavigation is called on the
   // next.
   virtual void DidStartNavigation(NavigationHandle* navigation_handle) {}
 
   // Called when a navigation encountered a server redirect.
   virtual void DidRedirectNavigation(NavigationHandle* navigation_handle) {}
 
   // PlzNavigate
   // Called when the navigation is ready to be committed in a renderer. This is
   // 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.

   // ---------------------------------------------------------------------------
 
   // This method is invoked after the WebContents decides which RenderFrameHost
   // to use for the next browser-initiated navigation, but before the navigation
   // starts.  It is not called for most renderer-initiated navigations, and it
   // does not guarantee that the navigation will commit (e.g., 204s, downloads).
   //
   // DEPRECATED.  This method is difficult to use correctly and should be
   // removed.  TODO(creis): Remove in http://crbug.com/424641.
   virtual void AboutToNavigateRenderFrame(RenderFrameHost* old_host,
@@ skipping 283 matching lines @@
   void ResetWebContents();
 
   WebContentsImpl* web_contents_;
 
   DISALLOW_COPY_AND_ASSIGN(WebContentsObserver);
 };
 
 }  // namespace content
 
 #endif  // CONTENT_PUBLIC_BROWSER_WEB_CONTENTS_OBSERVER_H_