From e1ad64013ecbaf1f4a3f957a37ca2fdb8ab14c02 Mon Sep 17 00:00:00 2001 From: Cheng Zhao Date: Sun, 26 Jun 2016 11:46:40 +0900 Subject: [PATCH 1/2] Cleanup code of beginFrameSubscription --- atom/browser/api/atom_api_web_contents.cc | 11 +++++------ atom/browser/api/frame_subscriber.cc | 16 ++++++++++------ atom/browser/api/frame_subscriber.h | 7 ++++--- docs/api/web-contents.md | 2 +- 4 files changed, 20 insertions(+), 16 deletions(-) diff --git a/atom/browser/api/atom_api_web_contents.cc b/atom/browser/api/atom_api_web_contents.cc index f19ac34a5d..e6959c8871 100644 --- a/atom/browser/api/atom_api_web_contents.cc +++ b/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(); diff --git a/atom/browser/api/frame_subscriber.cc b/atom/browser/api/frame_subscriber.cc index 91b6765db1..d6d2cf759e 100644 --- a/atom/browser/api/frame_subscriber.cc +++ b/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 damage = - mate::Converter::ToV8(isolate_, damage_rect); + mate::Converter::ToV8(isolate_, damage_rect); callback_.Run(buffer.ToLocalChecked(), damage); } diff --git a/atom/browser/api/frame_subscriber.h b/atom/browser/api/frame_subscriber.h index b6cff3da81..0da554ac47 100644 --- a/atom/browser/api/frame_subscriber.h +++ b/atom/browser/api/frame_subscriber.h @@ -21,7 +21,7 @@ namespace api { class FrameSubscriber : public content::RenderWidgetHostViewFrameSubscriber { public: using FrameCaptureCallback = - base::Callback, v8::Local)>; + base::Callback, v8::Local)>; 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_; diff --git a/docs/api/web-contents.md b/docs/api/web-contents.md index c25b640f91..913504cd37 100644 --- a/docs/api/web-contents.md +++ b/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` From 090c8b89bb6e98a5e6d6e1968d5b8eef0cca04d8 Mon Sep 17 00:00:00 2001 From: Cheng Zhao Date: Sun, 26 Jun 2016 11:53:58 +0900 Subject: [PATCH 2/2] spec: Make beginFrameSubscription test more reliable --- spec/api-browser-window-spec.js | 28 ++++++++++++++++++++-------- 1 file changed, 20 insertions(+), 8 deletions(-) diff --git a/spec/api-browser-window-spec.js b/spec/api-browser-window-spec.js index b5f5590558..25b852946f 100644 --- a/spec/api-browser-window-spec.js +++ b/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()