Fix captureVisibleTab function.

Previously OnCaptureFailure could be called just after CaptureAsync call
 in case, when view actually was absent. That led to 2 calls of
 SendResponse. Extension functions are designed to receive 1 response
 on each call, so 2 calls is bad behavior.
Now web capture clients behavior fixed.

R=fsamuel@chromium.org, tbarzic@chromium.org

Change-Id: I2ce195aaa3698d9c38ca2fa0e3735e6f9555f873
Reviewed-on: https://chromium-review.googlesource.com/930762
Commit-Queue: Devlin <rdevlin.cronin@chromium.org>
Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#539994}

chrome/browser/extensions/api/tabs/tabs_api.cc

@@ -1715,10 +1715,9 @@ bool TabsCaptureVisibleTabFunction::HasPermission() {
   return true;
 }
 
-bool TabsCaptureVisibleTabFunction::IsScreenshotEnabled() {
+bool TabsCaptureVisibleTabFunction::IsScreenshotEnabled() const {
   PrefService* service = chrome_details_.GetProfile()->GetPrefs();
   if (service->GetBoolean(prefs::kDisableScreenshots)) {
-    error_ = keys::kScreenshotsDisabled;
     return false;
   }
   return true;
@@ -1763,10 +1762,14 @@ bool TabsCaptureVisibleTabFunction::RunAsync() {
 
   WebContents* contents = GetWebContentsForID(context_id);
 
-  return CaptureAsync(
+  const CaptureResult capture_result = CaptureAsync(
       contents, image_details.get(),
       base::BindOnce(&TabsCaptureVisibleTabFunction::CopyFromSurfaceComplete,
                      this));
+  if (capture_result == OK)
+    return true;
+  SetErrorMessage(capture_result);
+  return false;
 }
 
 void TabsCaptureVisibleTabFunction::OnCaptureSuccess(const SkBitmap& bitmap) {
@@ -1780,9 +1783,14 @@ void TabsCaptureVisibleTabFunction::OnCaptureSuccess(const SkBitmap& bitmap) {
   SendResponse(true);
 }
 
-void TabsCaptureVisibleTabFunction::OnCaptureFailure(FailureReason reason) {
+void TabsCaptureVisibleTabFunction::OnCaptureFailure(CaptureResult result) {
+  SetErrorMessage(result);
+  SendResponse(false);
+}
+
+void TabsCaptureVisibleTabFunction::SetErrorMessage(CaptureResult result) {
   const char* reason_description = "internal error";
-  switch (reason) {
+  switch (result) {
     case FAILURE_REASON_READBACK_FAILED:
       reason_description = "image readback failed";
       break;
@@ -1792,10 +1800,16 @@ void TabsCaptureVisibleTabFunction::OnCaptureFailure(FailureReason reason) {
     case FAILURE_REASON_VIEW_INVISIBLE:
       reason_description = "view is invisible";
       break;
+    case FAILURE_REASON_SCREEN_SHOTS_DISABLED:
+      error_ = keys::kScreenshotsDisabled;
+      return;
+    case OK:
+      NOTREACHED()
+          << "SetErrorMessage should not be called with a successful result";
+      return;
   }
   error_ = ErrorUtils::FormatErrorMessage("Failed to capture tab: *",
                                           reason_description);
-  SendResponse(false);
 }
 
 void TabsCaptureVisibleTabFunction::RegisterProfilePrefs(

chrome/browser/extensions/api/tabs/tabs_api.h

@@ -215,10 +215,13 @@ class TabsCaptureVisibleTabFunction
   content::WebContents* GetWebContentsForID(int window_id);
 
   // extensions::WebContentsCaptureClient:
-  bool IsScreenshotEnabled() override;
+  bool IsScreenshotEnabled() const override;
   bool ClientAllowsTransparency() override;
   void OnCaptureSuccess(const SkBitmap& bitmap) override;
-  void OnCaptureFailure(FailureReason reason) override;
+  void OnCaptureFailure(CaptureResult result) override;
+
+ private:
+  void SetErrorMessage(CaptureResult result);
 
   DECLARE_EXTENSION_FUNCTION("tabs.captureVisibleTab", TABS_CAPTUREVISIBLETAB)
 };

chrome/browser/extensions/extension_tabs_apitest.cc

@@ -204,6 +204,11 @@ IN_PROC_BROWSER_TEST_F(ExtensionApiCaptureTest, CaptureVisibleDisabled) {
                                   "test_disabled.html")) << message_;
 }
 
+IN_PROC_BROWSER_TEST_F(ExtensionApiCaptureTest, CaptureNullWindow) {
+  ASSERT_TRUE(RunExtensionTest("tabs/capture_visible_tab_null_window"))
+      << message_;
+}
+
 IN_PROC_BROWSER_TEST_F(ExtensionApiTabTest, TabsOnCreated) {
   ASSERT_TRUE(RunExtensionTest("tabs/on_created")) << message_;
 }

chrome/test/data/extensions/api_test/tabs/capture_visible_tab_null_window/background.js

+// Copyright 2018 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.
+
+// API test for chrome.tabs.captureVisibleTab(), when current window is null
+// browser_tests.exe --gtest_filter=ExtensionApiTest.CaptureNullWindow
+
+var fail = chrome.test.callbackFail;
+
+chrome.test.runTests([function captureNullWindow() {
+  // Create a new window so we don't close the only active window.
+  chrome.windows.create(function(newWindow) {
+    chrome.windows.remove(newWindow.id);
+    chrome.tabs.captureVisibleTab(
+        newWindow.id, fail('Failed to capture tab: view is invisible'));
+  });
+}]);

chrome/test/data/extensions/api_test/tabs/capture_visible_tab_null_window/manifest.json

+{
+  "name": "chrome.tabs.captureVisibleTab test: nullWindow",
+  "version": "0.1",
+  "manifest_version": 2,
+  "description": "end-to-end browser test for chrome.tabs.captureVisibleTab",
+  "permissions": [ "tabs", "<all_urls>" ],
+  "background": {
+    "scripts": [ "background.js" ]
+  }
+}

extensions/browser/api/guest_view/web_view/web_view_internal_api.cc

@@ -315,13 +315,17 @@ bool WebViewInternalCaptureVisibleRegionFunction::RunAsyncSafe(
   }
 
   is_guest_transparent_ = guest->allow_transparency();
-  return CaptureAsync(
+  const CaptureResult capture_result = CaptureAsync(
       guest->web_contents(), image_details.get(),
       base::BindOnce(
           &WebViewInternalCaptureVisibleRegionFunction::CopyFromSurfaceComplete,
           this));
+  if (capture_result == OK)
+    return true;
+  SetErrorMessage(capture_result);
+  return false;
 }
-bool WebViewInternalCaptureVisibleRegionFunction::IsScreenshotEnabled() {
+bool WebViewInternalCaptureVisibleRegionFunction::IsScreenshotEnabled() const {
   // TODO(wjmaclean): Is it ok to always return true here?
   return true;
 }
@@ -343,9 +347,15 @@ void WebViewInternalCaptureVisibleRegionFunction::OnCaptureSuccess(
 }
 
 void WebViewInternalCaptureVisibleRegionFunction::OnCaptureFailure(
-    FailureReason reason) {
+    CaptureResult result) {
+  SetErrorMessage(result);
+  SendResponse(false);
+}
+
+void WebViewInternalCaptureVisibleRegionFunction::SetErrorMessage(
+    CaptureResult result) {
   const char* reason_description = "internal error";
-  switch (reason) {
+  switch (result) {
     case FAILURE_REASON_READBACK_FAILED:
       reason_description = "image readback failed";
       break;
@@ -355,10 +365,17 @@ void WebViewInternalCaptureVisibleRegionFunction::OnCaptureFailure(
     case FAILURE_REASON_VIEW_INVISIBLE:
       reason_description = "view is invisible";
       break;
+    case FAILURE_REASON_SCREEN_SHOTS_DISABLED:
+      NOTREACHED() << "WebViewInternalCaptureVisibleRegionFunction always have "
+                      "screenshots enabled";
+      break;
+    case OK:
+      NOTREACHED()
+          << "SetErrorMessage should not be called with a successful result";
+      return;
   }
   error_ = ErrorUtils::FormatErrorMessage("Failed to capture webview: *",
                                           reason_description);
-  SendResponse(false);
 }
 
 ExtensionFunction::ResponseAction WebViewInternalNavigateFunction::Run() {

extensions/browser/api/guest_view/web_view/web_view_internal_api.h

@@ -64,10 +64,12 @@ class WebViewInternalCaptureVisibleRegionFunction
   bool RunAsyncSafe(WebViewGuest* guest) override;
 
   // extensions::WebContentsCaptureClient:
-  bool IsScreenshotEnabled() override;
+  bool IsScreenshotEnabled() const override;
   bool ClientAllowsTransparency() override;
   void OnCaptureSuccess(const SkBitmap& bitmap) override;
-  void OnCaptureFailure(FailureReason reason) override;
+  void OnCaptureFailure(CaptureResult result) override;
+
+  void SetErrorMessage(CaptureResult result);
 
   bool is_guest_transparent_;
 

extensions/browser/api/web_contents_capture_client.cc

@@ -23,15 +23,18 @@ namespace extensions {
 
 using api::extension_types::ImageDetails;
 
-bool WebContentsCaptureClient::CaptureAsync(
+WebContentsCaptureClient::CaptureResult WebContentsCaptureClient::CaptureAsync(
     WebContents* web_contents,
     const ImageDetails* image_details,
     base::OnceCallback<void(const SkBitmap&)> callback) {
-  if (!web_contents)
-    return false;
+  // TODO(crbug/419878): Account for fullscreen render widget?
+  RenderWidgetHostView* const view =
+      web_contents ? web_contents->GetRenderWidgetHostView() : nullptr;
+  if (!view)
+    return FAILURE_REASON_VIEW_INVISIBLE;
 
   if (!IsScreenshotEnabled())
-    return false;
+    return FAILURE_REASON_SCREEN_SHOTS_DISABLED;
 
   // The default format and quality setting used when encoding jpegs.
   const api::extension_types::ImageFormat kDefaultFormat =
@@ -48,16 +51,10 @@ bool WebContentsCaptureClient::CaptureAsync(
       image_quality_ = *image_details->quality;
   }
 
-  // TODO(crbug/419878): Account for fullscreen render widget?
-  RenderWidgetHostView* const view = web_contents->GetRenderWidgetHostView();
-  if (!view) {
-    OnCaptureFailure(FAILURE_REASON_VIEW_INVISIBLE);
-    return false;
-  }
   view->CopyFromSurface(gfx::Rect(),  // Copy entire surface area.
                         gfx::Size(),  // Result contains device-level detail.
                         std::move(callback));
-  return true;
+  return OK;
 }
 
 void WebContentsCaptureClient::CopyFromSurfaceComplete(const SkBitmap& bitmap) {

extensions/browser/api/web_contents_capture_client.h

@@ -26,19 +26,22 @@ class WebContentsCaptureClient {
  protected:
   virtual ~WebContentsCaptureClient() {}
 
-  virtual bool IsScreenshotEnabled() = 0;
+  virtual bool IsScreenshotEnabled() const = 0;
   virtual bool ClientAllowsTransparency() = 0;
 
-  enum FailureReason {
+  enum CaptureResult {
+    OK,
     FAILURE_REASON_READBACK_FAILED,
     FAILURE_REASON_ENCODING_FAILED,
-    FAILURE_REASON_VIEW_INVISIBLE
+    FAILURE_REASON_SCREEN_SHOTS_DISABLED,
+    FAILURE_REASON_VIEW_INVISIBLE,
   };
-  bool CaptureAsync(content::WebContents* web_contents,
-                    const api::extension_types::ImageDetails* image_detail,
-                    base::OnceCallback<void(const SkBitmap&)> callback);
+  CaptureResult CaptureAsync(
+      content::WebContents* web_contents,
+      const api::extension_types::ImageDetails* image_detail,
+      base::OnceCallback<void(const SkBitmap&)> callback);
   bool EncodeBitmap(const SkBitmap& bitmap, std::string* base64_result);
-  virtual void OnCaptureFailure(FailureReason reason) = 0;
+  virtual void OnCaptureFailure(CaptureResult result) = 0;
   virtual void OnCaptureSuccess(const SkBitmap& bitmap) = 0;
   void CopyFromSurfaceComplete(const SkBitmap& bitmap);