NavigationController: Reload methods migration

content/public/browser/navigation_controller.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_NAVIGATION_CONTROLLER_H_
 #define CONTENT_PUBLIC_BROWSER_NAVIGATION_CONTROLLER_H_
 
 #include <stdint.h>
 
 #include <map>
@@ skipping 332 matching lines @@
   virtual void GoBack() = 0;
   virtual void GoForward() = 0;
 
   // Navigates to the specified absolute index.
   virtual void GoToIndex(int index) = 0;
 
   // Navigates to the specified offset from the "current entry". Does nothing if
   // the offset is out of bounds.
   virtual void GoToOffset(int offset) = 0;
 
-  // Reloads the current entry under the specified ReloadType.
-  // TODO(toyoshim): Change all callers to use this new Reload(), and remove
-  // old Reload* methods below. One motivation of this change is Reload(bool)
-  // interface is just confusing because in some contexts, the bool could be to
-  // specify bypassing cache. http://crbug.com/670232
-  virtual void Reload(bool check_for_repost, ReloadType reload_type) = 0;
-
-  // Reloads the current entry. If |check_for_repost| is true and the current
-  // entry has POST data the user is prompted to see if they really want to
-  // reload the page. In nearly all cases pass in true.  If a transient entry
-  // is showing, initiates a new navigation to its URL.
-  virtual void Reload(bool check_for_repost) = 0;
-
-  // Like Reload(), but don't use caches (aka "shift-reload").
-  virtual void ReloadBypassingCache(bool check_for_repost) = 0;
-
-  // Reloads the current entry using the original URL used to create it.  This
-  // is used for cases where the user wants to refresh a page using a different
-  // user agent after following a redirect.
-  virtual void ReloadOriginalRequestURL(bool check_for_repost) = 0;
-
-  // Like Reload(), but disables Lo-Fi.
-  virtual void ReloadDisableLoFi(bool check_for_repost) = 0;
+  // Reloads the current entry under the specified ReloadType. If
+  // |check_for_repost| is true and the current entry has POST data the user is
+  // prompted to see if they really want to reload the page.  In nearly all
+  // cases pass in true.  If a transient entry is showing, initiates a new

[Charlie Reis, 2016/12/15 01:32:41]

Just curious-- it seems like there's a lot of these call sites that pass false
(counter to this old comment).  Do you think that's a problem, or is it ok in
those cases?

[Takashi Toyoshima, 2016/12/15 06:21:52]

I think this should be true unless content embedders can not have a UI for
prompting. I.e., testing module like a content_shell. I checked all callers and
put comments for each.

So all use cases except one caller look reasonable. So, I will left a TODO for
the one caller, and check it in a separate CL.

[Charlie Reis, 2016/12/15 18:44:03]

Thanks for checking all these!

+  // navigation to its URL.
+  virtual void Reload(ReloadType reload_type, bool check_for_repost) = 0;

[Charlie Reis, 2016/12/15 01:32:41]

jam@, what are your thoughts on overloads?  Would it be useful to have a
Reload() method which just calls Reload(content::ReloadType::NORMAL, true)? 
(That might be a nice alternative to default params, making a lot of these call
sites a bit cleaner.)

[Takashi Toyoshima, 2016/12/15 06:21:52]

As I checked, actually there are many valid callers that use false. IMO,
Reload(bool) is very confusing as an alternative. So, my suggesting alternatives
are;

Plan 1 (current)
 - Reload(ReloadType, bool)

Plan 2
 - Reload(ReloadType, bool)
 - ReloadNormally(bool) - use Normal internally

 * Could not reduce typing so much.

Plan 3
 - Reload() - use Normal and true internally
 - ReloadWith(ReloadType, bool)

 * probably there are still many callers that use the second interface. So not
so interesting.

[Charlie Reis, 2016/12/15 18:44:03]

I was suggesting Plan 3 to get John's thoughts, but I'm ok with Plan 1 as well.

 
   // Removing of entries -------------------------------------------------------
 
   // Removes the entry at the specified |index|.  If the index is the last
   // committed index or the pending entry, this does nothing and returns false.
   // Otherwise this call discards any transient or pending entries.
   virtual bool RemoveEntryAtIndex(int index) = 0;
 
   // Random --------------------------------------------------------------------
 
@@ skipping 84 matching lines @@
 
  private:
   // This interface should only be implemented inside content.
   friend class NavigationControllerImpl;
   NavigationController() {}
 };
 
 }  // namespace content
 
 #endif  // CONTENT_PUBLIC_BROWSER_NAVIGATION_CONTROLLER_H_