NonValidatingReload: Monitor reload operations in NavigationControllerImpl

content/browser/frame_host/navigation_controller_impl.cc

Index: content/browser/frame_host/navigation_controller_impl.cc
diff --git a/content/browser/frame_host/navigation_controller_impl.cc b/content/browser/frame_host/navigation_controller_impl.cc
index 2b21fd9296ce3828ee0b2417a9174fc788e022e6..4b763f048a79ca1a4823dc98040f8bff90bff6ad 100644
--- a/content/browser/frame_host/navigation_controller_impl.cc
+++ b/content/browser/frame_host/navigation_controller_impl.cc
@@ -241,7 +241,9 @@ NavigationControllerImpl::NavigationControllerImpl(
       in_navigate_to_pending_entry_(false),
       pending_reload_(NO_RELOAD),
       get_timestamp_callback_(base::Bind(&base::Time::Now)),
-      screenshot_manager_(new NavigationEntryScreenshotManager(this)) {
+      screenshot_manager_(new NavigationEntryScreenshotManager(this)),
+      is_last_navigation_reload_(false),
+      last_reload_type_(NO_RELOAD) {
   DCHECK(browser_context_);
 }
 
@@ -344,6 +346,8 @@ void NavigationControllerImpl::ReloadInternal(bool check_for_repost,
   if (!entry)
     return;
 
+  entry->set_reload_type(reload_type);

[Charlie Reis, 2016/08/08 23:57:53]

What happens if the renderer initiates a reload (e.g., location.reload() in
Javascript)?  That won't go through here, will it?

Maybe there's a parameter we can check at commit time instead of storing the bit
on NavigationEntry?

[Takashi Toyoshima, 2016/08/09 07:35:26]

Right. Javascript initiated reload won't be captured here. It directly calls
FrameLoader::load() with FrameLoadTypeReload from ScheduleReload::fire().

But reloads I'm interested here are ones come from user interfaces. It's fine
that JavaScript initiated reload is ignored. Probably, I should update comments
to clarify that.

If we want to capture the JavaScript initiated reloads too, probably we can
track it in RendererDidNavigate() as one of NAVIGATION_TYPE_EXISTING_PAGE, and
entry_index and url being not changed?

Another problem I noticed here is stored reload type isn't updated on JavaScript
initiated reload. That means if I follow steps below, it mistakenly report
ReloadToRealodDuration metric.

 1. Navigate to a page
 2. Reload user action
 3. location.reload() => stored information isn't updated, and entry still has
the previous reload_type. this results in reporting ReloadToReloadDuration
metric.

To fix this issue, I should identify JavaScript initiated reloads and user
actions initiated reloads. ClientRedirectPolicy in Blink is the one I can use
for that. But it isn't available here. Shall we plumb it from FrameLoader on
CommitLoad timing?

[Charlie Reis, 2016/08/10 05:05:13]

Isn't this a problem for any renderer-initiated navigation, not just
location.reload?  I'm still concerned about the overall strategy.

[Takashi Toyoshima, 2016/08/10 06:34:15]

If we assume this reload type is valid only when the page transition type
indicates reload, it does not matter. My proposed way, using
ClientRedirectPolicy, just works becase as far as I know, all JavaScript
initiated reload-ish navigation are classified to "clientRedirect".

+
   if (g_check_for_repost && check_for_repost &&
       entry->GetHasPostData()) {
     // The user is asking to reload a page with POST data. Prompt to make sure
@@ -1945,7 +1949,36 @@ void NavigationControllerImpl::FindFramesToNavigate(
 
 void NavigationControllerImpl::NotifyNavigationEntryCommitted(
     LoadCommittedDetails* details) {
-  details->entry = GetLastCommittedEntry();
+  NavigationEntryImpl* entry = GetLastCommittedEntry();
+  CHECK(entry);
+  details->entry = entry;
+
+  ui::PageTransition transition = details->entry->GetTransitionType();
+  if (ui::PageTransitionIsMainFrame(transition)) {
+    base::Time now = base::Time::Now();
+
+    bool is_navigation_forward_back =
+        transition & ui::PAGE_TRANSITION_FORWARD_BACK;

[Takashi Toyoshima, 2016/08/09 12:06:17]

This line broke Windows build.

+    bool is_navigation_reload =
+        !is_navigation_forward_back &&
+        ui::PageTransitionCoreTypeIs(transition, ui::PAGE_TRANSITION_RELOAD);
+
+    if (is_last_navigation_reload_ && is_navigation_reload) {
+      base::TimeDelta delta = now - last_navigation_time_;
+      UMA_HISTOGRAM_MEDIUM_TIMES("Navigation.Reload.ReloadToReloadDuration",
+                                 delta);
+      // Track the case that another reload folllows RELOAD_MAIN_RESOURCE type
+      // of reload.
+      if (last_reload_type_ == NavigationController::RELOAD_MAIN_RESOURCE) {
+        UMA_HISTOGRAM_MEDIUM_TIMES(
+            "Navigation.Reload.ReloadMainResourceToReloadDuration", delta);
+      }
+    }
+
+    is_last_navigation_reload_ = is_navigation_reload;
+    last_reload_type_ = entry->reload_type();
+    last_navigation_time_ = now;
+  }
 
   // We need to notify the ssl_manager_ before the web_contents_ so the
   // location bar will have up-to-date information about the security style