Implement webview.captureVisibleRegion()

extensions/browser/api/capture_web_contents.h

Index: extensions/browser/api/capture_web_contents.h
diff --git a/extensions/browser/api/capture_web_contents_function.h b/extensions/browser/api/capture_web_contents.h
similarity index 57%
rename from extensions/browser/api/capture_web_contents_function.h
rename to extensions/browser/api/capture_web_contents.h
index 53f78f7992725e2cd291c38136c70d6e3e947f09..f80f62c424f5d87babce08bd35da9b588e6325d3 100644
--- a/extensions/browser/api/capture_web_contents_function.h
+++ b/extensions/browser/api/capture_web_contents.h
@@ -2,8 +2,8 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#ifndef EXTENSIONS_BROWSER_API_CAPTURE_WEB_CONTENTS_FUNCTION_H_
-#define EXTENSIONS_BROWSER_API_CAPTURE_WEB_CONTENTS_FUNCTION_H_
+#ifndef EXTENSIONS_BROWSER_API_CAPTURE_WEB_CONTENTS_H_
+#define EXTENSIONS_BROWSER_API_CAPTURE_WEB_CONTENTS_H_
 
 #include "base/macros.h"
 #include "content/public/browser/readback_types.h"
@@ -20,48 +20,39 @@ namespace extensions {
 
 // Base class for capturing visibile area of a WebContents.
 // This is used by both webview.captureVisibleRegion and tabs.captureVisibleTab.
-class CaptureWebContentsFunction : public AsyncExtensionFunction {
+class CaptureWebContents {

[Ken Rockot(use gerrit already), 2016/01/13 22:41:35]

It's unusual to have a verb phrase for a typename. Maybe this should be
something like a WebContentsCaptureDelegate - or maybe WebContentsCaptureClient
since it's not strictly a delegate interface and has some base functionality.

[wjmaclean, 2016/01/18 16:20:12]

Done.

  public:
-  CaptureWebContentsFunction() {}
+  CaptureWebContents() {}
 
  protected:
-  ~CaptureWebContentsFunction() override {}
-
-  // ExtensionFunction implementation.
-  bool HasPermission() override;
-  bool RunAsync() override;
+  virtual ~CaptureWebContents() {}
 
   virtual bool IsScreenshotEnabled() = 0;
-  virtual content::WebContents* GetWebContentsForID(int context_id) = 0;
 
   enum FailureReason {
     FAILURE_REASON_UNKNOWN,
     FAILURE_REASON_ENCODING_FAILED,
     FAILURE_REASON_VIEW_INVISIBLE
   };
+  bool CaptureAsync(content::WebContents* web_contents,
+                    api::extension_types::ImageDetails* image_detail,

[Ken Rockot(use gerrit already), 2016/01/13 22:41:35]

If you want to pass ownership of |image_detail| this should be a scoped_ptr. It
doesn't look like you need to though, so in that case maybe this should be a
const ref?

[wjmaclean, 2016/01/18 16:20:12]

There won't always be an ImageDetails object (it comes from an optional
parameter), and so a pointer is used to be able to check whether it exists or
not. Ownership remains with the caller, which seems reasonable.

What if we make it const *, since we will never modify it?

+                    const content::ReadbackRequestCallback callback);
+  bool EncodeBitmap(const SkBitmap& bitmap, std::string& base64_result);
   virtual void OnCaptureFailure(FailureReason reason) = 0;
-
- private:
-
+  virtual void OnCaptureSuccess(const SkBitmap& bitmap) = 0;
   void CopyFromBackingStoreComplete(const SkBitmap& bitmap,
                                     content::ReadbackResponse response);
-  void OnCaptureSuccess(const SkBitmap& bitmap);
-
-  // |context_id_| is the ID used to find the relevant WebContents. In the
-  // |tabs.captureVisibleTab()| api, this represents the window-id, and in the
-  // |webview.captureVisibleRegion()| api, this represents the instance-id of
-  // the guest.
-  int context_id_;
 
+ private:
   // The format (JPEG vs PNG) of the resulting image.  Set in RunAsync().
   api::extension_types::ImageFormat image_format_;
 
   // Quality setting to use when encoding jpegs.  Set in RunAsync().
   int image_quality_;
 
-  DISALLOW_COPY_AND_ASSIGN(CaptureWebContentsFunction);
+  DISALLOW_COPY_AND_ASSIGN(CaptureWebContents);
 };
 
 }  // namespace extensions
 
-#endif  // EXTENSIONS_BROWSER_API_CAPTURE_WEB_CONTENTS_FUNCTION_H_
+#endif  // EXTENSIONS_BROWSER_API_CAPTURE_WEB_CONTENTS_H_