overscroll: Take a screenshot at more appropriate times.

content/browser/web_contents/navigation_controller_impl.cc

Index: content/browser/web_contents/navigation_controller_impl.cc
diff --git a/content/browser/web_contents/navigation_controller_impl.cc b/content/browser/web_contents/navigation_controller_impl.cc
index bd6971646b8d428103ed5fcb13eb1b9d226ebf1e..6b0a86631d6226927a588ff45ee783054f4bc2d8 100644
--- a/content/browser/web_contents/navigation_controller_impl.cc
+++ b/content/browser/web_contents/navigation_controller_impl.cc
@@ -137,6 +137,29 @@ bool ShouldKeepOverride(const NavigationEntry* last_entry) {
   return last_entry && last_entry->GetIsOverridingUserAgent();
 }
 
+// Returns whether a screenshot should be taken because of the page transition.

[Charlie Reis, 2013/01/07 19:50:23]

What's the intention here?  You're returning no for some known cases and yes for
the rest, so it's not clear what cases you're looking for.  It's also fragile,
if additional transition types are added, so a whitelist would be better if
possible.

+bool ShouldTakeScreenshotForTransition(PageTransition transition) {
+  // Never take a screenshot if the main page didn't navigate.
+  if (!PageTransitionIsMainFrame(transition))
+    return false;
+
+  int core = transition & PAGE_TRANSITION_CORE_MASK;
+  switch (core) {
+    case PAGE_TRANSITION_TYPED:
+    case PAGE_TRANSITION_AUTO_BOOKMARK:
+    case PAGE_TRANSITION_RELOAD:
+      return false;
+  }
+
+  int qualifier = transition & PAGE_TRANSITION_QUALIFIER_MASK;
+  if (qualifier & (PAGE_TRANSITION_FORWARD_BACK |
+                   PAGE_TRANSITION_FROM_ADDRESS_BAR)) {
+    return false;
+  }
+
+  return true;
+}
+
 }  // namespace
 
 // NavigationControllerImpl ----------------------------------------------------
@@ -530,6 +553,8 @@ void NavigationControllerImpl::OnScreenshotTaken(
   std::vector<unsigned char> data;
   if (gfx::PNGCodec::EncodeBGRASkBitmap(bitmap->GetBitmap(), true, &data))
     entry->SetScreenshotPNGData(data);
+  else
+    entry->SetScreenshotPNGData(std::vector<unsigned char>());
 }
 
 bool NavigationControllerImpl::CanGoBack() const {
@@ -755,7 +780,7 @@ bool NavigationControllerImpl::RendererDidNavigate(
   // When navigating away from the current page, take a screenshot of it in the
   // current state so that it can be used during an overscroll-navigation
   // gesture.
-  if (details->is_main_frame)
+  if (ShouldTakeScreenshotForTransition(params.transition))
     TakeScreenshot();
 
   // Save the previous state before we clobber it.
@@ -1548,6 +1573,8 @@ void NavigationControllerImpl::NavigateToPendingEntry(ReloadType reload_type) {
 
   if (!web_contents_->NavigateToPendingEntry(reload_type))
     DiscardNonCommittedEntries();
+  else if (reload_type == NO_RELOAD)

[Charlie Reis, 2013/01/07 19:50:23]

We should put a return after DiscardNonCommittedEntries and skip the "else". 
The existing logic below won't run in that case anyway, since pending_entry_
will be null.

Aside from that, I don't think this is the right place to take the screenshot. 
The navigation might never commit (making it unnecessary), and a large amount of
time could pass before it does (making it possibly stale).

It seems like you want to take the screenshot just before the previous page
tries to unload.  That would be in RenderViewHost::SwapOut, which gets called by
RVHM::OnCrossSiteResponse.

Either way, can you explain why ShouldTakeScreenshot matters in the other case
but not this one?

[sadrul, 2013/01/08 00:16:29]

The issue here is that, in some cases (especially when the navigation happens
because of a user action in the UI, e.g. entering a URL in the omnibox, or
clicking a bookmark button), we want to take the screenshot before the
RenderView is swapped out. So I do that here. But as you point it, this may be
the wrong place for this.

The check in ShouldTakeScreenshot is [an attempt] to essentially enumerate the
cases where we take a screenshot before the navigation, so that we do not try to
take the screenshot again after the navigation. But I suppose this will not be
necessary anymore if we can tell in RendererDidNavigate that
web_contents_->GetRenderViewHost() did not cause the old RVH to be swapped out.
Is that possible?

+    TakeScreenshot();
 
   // If the entry is being restored and doesn't have a SiteInstance yet, fill
   // it in now that we know. This allows us to find the entry when it commits.