Merge pull request #6244 from electron/frame-subscription-cleanup

Cleanup code of beginFrameSubscription

atom/browser/api/atom_api_web_contents.cc

@@ -1181,15 +1181,14 @@ void WebContents::SendInputEvent(v8::Isolate* isolate,
       isolate, "Invalid event object")));
 }
 
-void WebContents::BeginFrameSubscription(
-  mate::Arguments* args) {
-  FrameSubscriber::FrameCaptureCallback callback;
+void WebContents::BeginFrameSubscription(mate::Arguments* args) {
   bool only_dirty = false;
+  FrameSubscriber::FrameCaptureCallback callback;
 
+  args->GetNext(&only_dirty);
   if (!args->GetNext(&callback)) {
-    args->GetNext(&only_dirty);
-    if (!args->GetNext(&callback))
-      args->ThrowTypeError("'callback' must be defined");
+    args->ThrowError();
+    return;
   }
 
   const auto view = web_contents()->GetRenderWidgetHostView();

atom/browser/api/frame_subscriber.cc

@@ -5,8 +5,8 @@
 #include "atom/browser/api/frame_subscriber.h"
 
 #include "base/bind.h"
-#include "atom/common/node_includes.h"
 #include "atom/common/native_mate_converters/gfx_converter.h"
+#include "atom/common/node_includes.h"
 #include "content/public/browser/render_widget_host.h"
 
 namespace atom {
@@ -17,8 +17,11 @@ FrameSubscriber::FrameSubscriber(v8::Isolate* isolate,
                                  content::RenderWidgetHostView* view,
                                  const FrameCaptureCallback& callback,
                                  bool only_dirty)
-    : isolate_(isolate), view_(view), callback_(callback),
-      only_dirty_(only_dirty), weak_factory_(this) {
+    : isolate_(isolate),
+      view_(view),
+      callback_(callback),
+      only_dirty_(only_dirty),
+      weak_factory_(this) {
 }
 
 bool FrameSubscriber::ShouldCaptureFrame(
@@ -48,8 +51,9 @@ bool FrameSubscriber::ShouldCaptureFrame(
 }
 
 void FrameSubscriber::OnFrameDelivered(const FrameCaptureCallback& callback,
-  const gfx::Rect& damage_rect, const SkBitmap& bitmap,
-  content::ReadbackResponse response) {
+                                       const gfx::Rect& damage_rect,
+                                       const SkBitmap& bitmap,
+                                       content::ReadbackResponse response) {
   if (response != content::ReadbackResponse::READBACK_SUCCESS)
     return;
 
@@ -67,7 +71,7 @@ void FrameSubscriber::OnFrameDelivered(const FrameCaptureCallback& callback,
     rgb_arr_size);
 
   v8::Local<v8::Value> damage =
-    mate::Converter<gfx::Rect>::ToV8(isolate_, damage_rect);
+      mate::Converter<gfx::Rect>::ToV8(isolate_, damage_rect);
 
   callback_.Run(buffer.ToLocalChecked(), damage);
 }

atom/browser/api/frame_subscriber.h

@@ -21,7 +21,7 @@ namespace api {
 class FrameSubscriber : public content::RenderWidgetHostViewFrameSubscriber {
  public:
   using FrameCaptureCallback =
-    base::Callback<void(v8::Local<v8::Value>, v8::Local<v8::Value>)>;
+      base::Callback<void(v8::Local<v8::Value>, v8::Local<v8::Value>)>;
 
   FrameSubscriber(v8::Isolate* isolate,
                   content::RenderWidgetHostView* view,
@@ -35,8 +35,9 @@ class FrameSubscriber : public content::RenderWidgetHostViewFrameSubscriber {
 
  private:
   void OnFrameDelivered(const FrameCaptureCallback& callback,
-    const gfx::Rect& damage_rect, const SkBitmap& bitmap,
-    content::ReadbackResponse response);
+                        const gfx::Rect& damage_rect,
+                        const SkBitmap& bitmap,
+                        content::ReadbackResponse response);
 
   v8::Isolate* isolate_;
   content::RenderWidgetHostView* view_;

docs/api/web-contents.md

@@ -921,7 +921,7 @@ For the `mouseWheel` event, the `event` object also have following properties:
 
 ### `webContents.beginFrameSubscription([onlyDirty ,]callback)`
 
-* `onlyDirty` Boolean
+* `onlyDirty` Boolean (optional) - Defaults to `false`
 * `callback` Function
 
 Begin subscribing for presentation events and captured frames, the `callback`

spec/api-browser-window-spec.js

@@ -44,7 +44,10 @@ describe('browser-window module', function () {
     w = new BrowserWindow({
       show: false,
       width: 400,
-      height: 400
+      height: 400,
+      webPreferences: {
+        backgroundThrottling: false
+      }
     })
   })
 
@@ -603,12 +606,20 @@ describe('browser-window module', function () {
   })
 
   describe('beginFrameSubscription method', function () {
-    it('subscribes to frame updates', function (done) {
-      this.timeout(20000)
+    // This test is too slow, only test it on CI.
+    if (!isCI) return
 
+    this.timeout(20000)
+
+    it('subscribes to frame updates', function (done) {
+      let called = false
       w.loadURL('file://' + fixtures + '/api/frame-subscriber.html')
       w.webContents.on('dom-ready', function () {
         w.webContents.beginFrameSubscription(function (data) {
+          // This callback might be called twice.
+          if (called) return
+          called = true
+
           assert.notEqual(data.length, 0)
           w.webContents.endFrameSubscription()
           done()
@@ -617,11 +628,14 @@ describe('browser-window module', function () {
     })
 
     it('subscribes to frame updates (only dirty rectangle)', function (done) {
-      this.timeout(20000)
-
+      let called = false
       w.loadURL('file://' + fixtures + '/api/frame-subscriber.html')
       w.webContents.on('dom-ready', function () {
         w.webContents.beginFrameSubscription(true, function (data) {
+          // This callback might be called twice.
+          if (called) return
+          called = true
+
           assert.notEqual(data.length, 0)
           w.webContents.endFrameSubscription()
           done()
@@ -630,10 +644,8 @@ describe('browser-window module', function () {
     })
 
     it('throws error when subscriber is not well defined', function (done) {
-      this.timeout(20000)
-
       w.loadURL('file://' + fixtures + '/api/frame-subscriber.html')
-      try{
+      try {
         w.webContents.beginFrameSubscription(true, true)
       } catch(e) {
         done()