overscroll: Take a screenshot at more appropriate times.

content/browser/web_contents/navigation_controller_impl.cc

 // 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.
 
 #include "content/browser/web_contents/navigation_controller_impl.h"
 
 #include "base/bind.h"
 #include "base/command_line.h"
 #include "base/file_util.h"
 #include "base/logging.h"
@@ skipping 119 matching lines @@
   return existing_url.ReplaceComponents(replacements) ==
       new_url.ReplaceComponents(replacements);
 }
 
 // Determines whether or not we should be carrying over a user agent override
 // between two NavigationEntries.
 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 ----------------------------------------------------
 
 const size_t kMaxEntryCountForTestingNotSet = -1;
 
 // static
 size_t NavigationControllerImpl::max_entry_count_for_testing_ =
     kMaxEntryCountForTestingNotSet;
 
@@ skipping 373 matching lines @@
   }
 
   if (!entry) {
     LOG(ERROR) << "Invalid entry with unique id: " << unique_id;
     return;
   }
 
   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 {
   return entries_.size() > 1 && GetCurrentEntryIndex() > 0;
 }
 
 bool NavigationControllerImpl::CanGoForward() const {
   int index = GetCurrentEntryIndex();
   return index >= 0 && index < (static_cast<int>(entries_.size()) - 1);
 }
@@ skipping 205 matching lines @@
 void NavigationControllerImpl::DocumentLoadedInFrame() {
   is_initial_navigation_ = false;
 }
 
 bool NavigationControllerImpl::RendererDidNavigate(
     const ViewHostMsg_FrameNavigate_Params& params,
     LoadCommittedDetails* details) {
   // 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.
   if (GetLastCommittedEntry()) {
     details->previous_url = GetLastCommittedEntry()->GetURL();
     details->previous_entry_index = GetLastCommittedEntryIndex();
   } else {
     details->previous_url = GURL();
     details->previous_entry_index = -1;
   }
@@ skipping 772 matching lines @@
   }
 
   // For session history navigations only the pending_entry_index_ is set.
   if (!pending_entry_) {
     DCHECK_NE(pending_entry_index_, -1);
     pending_entry_ = entries_[pending_entry_index_].get();
   }
 
   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.
   // This works for browser-initiated navigations. We handle renderer-initiated
   // navigations to restored entries in WebContentsImpl::OnGoToEntryAtOffset.
   if (pending_entry_ && !pending_entry_->site_instance() &&
       pending_entry_->restore_type() != NavigationEntryImpl::RESTORE_NONE) {
     pending_entry_->set_site_instance(static_cast<SiteInstanceImpl*>(
         web_contents_->GetPendingSiteInstance()));
     pending_entry_->set_restore_type(NavigationEntryImpl::RESTORE_NONE);
@@ skipping 115 matching lines @@
     }
   }
 }
 
 void NavigationControllerImpl::SetGetTimestampCallbackForTest(
     const base::Callback<base::Time()>& get_timestamp_callback) {
   get_timestamp_callback_ = get_timestamp_callback;
 }
 
 }  // namespace content