Don't capture screenshots for in-page navigations.

content/browser/frame_host/navigator_impl.cc

Index: content/browser/frame_host/navigator_impl.cc
diff --git a/content/browser/frame_host/navigator_impl.cc b/content/browser/frame_host/navigator_impl.cc
index 6aca3d54347570876a7d072b6c1a53e381ab1740..2890e8d2b1ef2de55b7098cf7e0117ac53f47526 100644
--- a/content/browser/frame_host/navigator_impl.cc
+++ b/content/browser/frame_host/navigator_impl.cc
@@ -441,22 +441,23 @@ void NavigatorImpl::DidNavigate(
       // change WebContents::GetRenderViewHost to return the new host, instead
       // of the one that may have just been swapped out.
       if (delegate_->CanOverscrollContent()) {
-        bool page_id_changed;
-        bool url_changed;
+        bool same_page_navigation = false;

[sky, 2014/07/25 22:08:05]

Is this the same as params.was_within_same_page?

[mfomitchev, 2014/07/25 22:29:15]

Please see my reply to creis@ below.

         NavigationEntry* current_entry = controller_->GetLastCommittedEntry();
         if (current_entry) {
-          page_id_changed = params.page_id > 0 &&
-              params.page_id != current_entry->GetPageID();
-          url_changed = params.url != current_entry->GetURL();
-        } else {
-          page_id_changed = params.page_id > 0;
-          url_changed = params.url != GURL::EmptyGURL();
+          url::Replacements<char> clear_ref;
+          clear_ref.ClearRef();
+          const GURL new_url = params.url;
+          const GURL& cur_url = current_entry->GetURL();
+          if (new_url.ReplaceComponents(clear_ref) ==

[Charlie Reis, 2014/07/25 22:07:40]

We probably shouldn't add yet another way to detect in-page navigations.  For
example, this would miss pushState navigations.

In this case, params.was_within_same_page is probably the right thing to use.  I
seem to remember that it doesn't handle pushState either, but there was some
discussion of fixing it and then it would be fixed for this use case as well.

[mfomitchev, 2014/07/25 22:29:15]

Ok, so the pushState test does fail if I use params.was_within_same_page. If I
leave the raw URL comparison, it passes. The URL in that case doesn't change at
all:

new_url=http://127.0.0.1:45820/files/overscroll_navigation.html, 
cur_url=http://127.0.0.1:45820/files/overscroll_navigation.html

Here's push_state() function:
function use_push_state() {
  window.history.pushState({}, 'foo2');
}

Does this make sense?
If using URL comparison covers pushState, while was_within_same_page doesn't
would we want to keep the current way, or still change it?

[Charlie Reis, 2014/07/25 22:45:28]

That's kind of a no-op example for pushState, though, since it doesn't change
the URL.  If you try it with the following I would expect your test to fail:
window.history.pushState({}, 'foo2', 'newpath');
I'd like to have fewer ways to compute whether a navigation is in-page or not. 
It's already too complicated inside NavigationController, where we have complex
logic around {Is,Are}URLsInPageNavigation, which ends up in
LoadCommittedDetails::is_in_page below.  (This came up in
http://crbug.com/138324.)

I suggest using was_within_same_page or calling IsURLInPageNavigation, unless
japhet@ has a better idea.

[mfomitchev, 2014/07/25 23:09:58]

Yep, it fails. Changing it to use params.was_within_same_page. Thanks! Is there
a bug for pushState/was_within_same_page which I can quote in the browsertest?

It appears was_within_same_page is also set to false when I reload the page. So
that's another test I am going to disable. Would that be fixed as well?

+              cur_url.ReplaceComponents(clear_ref)) {
+            same_page_navigation = true;
+          }
         }
-
-        // We only want to take the screenshot if the are navigating to a
-        // different history entry than the current one. So if neither the
-        // page id nor the url changed - don't take the screenshot.
-        if (page_id_changed || url_changed)
+        // Don't take screenshots if we are staying on the same page. We want
+        // in-page navigations to be super fast, and taking a screenshot
+        // currently blocks GPU for a longer time than we are willing to
+        // tolerate in this use case.
+        if (!same_page_navigation)
           controller_->TakeScreenshot();
       }