Consistent CopyFromSurface() API, consolidated to RWHV

content/browser/frame_host/navigation_entry_screenshot_manager.cc

 // Copyright 2013 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/frame_host/navigation_entry_screenshot_manager.h"
 
 #include "base/command_line.h"
 #include "base/macros.h"
 #include "base/task_scheduler/post_task.h"
 #include "content/browser/frame_host/navigation_controller_impl.h"
@@ skipping 95 matching lines @@
   int count = owner_->GetEntryCount();
   for (int i = 0; i < count; ++i) {
     ClearScreenshot(owner_->GetEntryAtIndex(i));
   }
   DCHECK_EQ(GetScreenshotCount(), 0);
 }
 
 void NavigationEntryScreenshotManager::TakeScreenshotImpl(
     RenderViewHost* host,
     NavigationEntryImpl* entry) {
-  DCHECK(host && host->GetWidget()->GetView());
+  DCHECK(host && host->GetWidget());
   DCHECK(entry);
-  host->GetWidget()->CopyFromBackingStore(
-      gfx::Rect(), host->GetWidget()->GetView()->GetViewBounds().size(),
-      base::Bind(&NavigationEntryScreenshotManager::OnScreenshotTaken,
-                 screenshot_factory_.GetWeakPtr(), entry->GetUniqueID()),
-      kAlpha_8_SkColorType);
+  if (auto* view = host->GetWidget()->GetView()) {

[alexmos, 2017/02/28 19:44:09]

Do you need this check?  It looks like
NavigationEntryScreenshotManager::TakeScreenshot already null-checks this before
calling TakeScreenshotImpl, so it doesn't seem like the else clause would ever
be reached.  If you want to keep the null case handling here, perhaps remove the
null check from TakeScreenshot?

[miu, 2017/03/01 01:17:02]

Done. Plus, things were simpler when I just dumped the remaining code at the end
of TakeScreenshot() (i.e., no need for a separate TakeScreenshotImpl() method).

[alexmos, 2017/03/01 01:30:05]

Sounds good, but looks like there's another test in
web_contents_view_aura_browsertest.cc that overrides TakeScreenshotImpl, so do
you need to fix that test as well?

[miu, 2017/03/01 21:57:18]

Ah, yes. Fixed. I had to add back a "notification" method for the test to
override.

+    // This screenshot is destined for the UI, so size the result to the actual
+    // on-screen size of the view (and not its device-rendering size).
+    const gfx::Size view_size_on_screen = view->GetViewBounds().size();
+    view->CopyFromSurface(
+        gfx::Rect(), view_size_on_screen,
+        base::Bind(&NavigationEntryScreenshotManager::OnScreenshotTaken,
+                   screenshot_factory_.GetWeakPtr(), entry->GetUniqueID()),
+        kAlpha_8_SkColorType);
+  } else {
+    OnScreenshotTaken(entry->GetUniqueID(), SkBitmap(),
+                      READBACK_SURFACE_UNAVAILABLE);
+  }
 }
 
 void NavigationEntryScreenshotManager::SetMinScreenshotIntervalMS(
     int interval_ms) {
   DCHECK_GE(interval_ms, 0);
   min_screenshot_interval_ms_ = interval_ms;
 }
 
 void NavigationEntryScreenshotManager::OnScreenshotTaken(
     int unique_id,
@@ skipping 110 matching lines @@
     NavigationEntryImpl* entry = owner_->GetEntryAtIndex(forward);
     if (ClearScreenshot(entry))
       --screenshot_count;
     ++forward;
   }
   CHECK_GE(screenshot_count, 0);
   CHECK_LE(screenshot_count, kMaxScreenshots);
 }
 
 }  // namespace content