Merge pull request #4495 from atom/frame-subscriber

Use weak pointer instead of manual bookkeeping

atom/browser/api/atom_api_web_contents.cc

@@ -1072,11 +1072,9 @@ void WebContents::BeginFrameSubscription(
     const FrameSubscriber::FrameCaptureCallback& callback) {
   const auto view = web_contents()->GetRenderWidgetHostView();
   if (view) {
-    FrameSubscriber* frame_subscriber = new FrameSubscriber(
-      isolate(), view->GetVisibleViewportSize(), callback);
-    scoped_ptr<FrameSubscriber::Subscriber> del_frame_subscriber(
-      frame_subscriber->GetSubscriber());
-    view->BeginFrameSubscription(del_frame_subscriber.Pass());
+    scoped_ptr<FrameSubscriber> frame_subscriber(new FrameSubscriber(
+        isolate(), view->GetVisibleViewportSize(), callback));
+    view->BeginFrameSubscription(frame_subscriber.Pass());
   }
 }
 

atom/browser/api/frame_subscriber.cc

@@ -13,58 +13,28 @@ namespace atom {
 
 namespace api {
 
-using Subscriber = FrameSubscriber::Subscriber;
-
 FrameSubscriber::FrameSubscriber(v8::Isolate* isolate,
                                  const gfx::Size& size,
                                  const FrameCaptureCallback& callback)
-    : isolate_(isolate), size_(size), callback_(callback), pending_frames(0) {
-  subscriber_ = new Subscriber(this);
+    : isolate_(isolate), size_(size), callback_(callback), weak_factory_(this) {
 }
 
-Subscriber::Subscriber(
-  FrameSubscriber* frame_subscriber) : frame_subscriber_(frame_subscriber) {
-}
-
-Subscriber::~Subscriber() {
-  frame_subscriber_->subscriber_ = NULL;
-  frame_subscriber_->RequestDestruct();
-}
-
-bool Subscriber::ShouldCaptureFrame(
+bool FrameSubscriber::ShouldCaptureFrame(
     const gfx::Rect& damage_rect,
     base::TimeTicks present_time,
     scoped_refptr<media::VideoFrame>* storage,
     DeliverFrameCallback* callback) {
   *storage = media::VideoFrame::CreateFrame(
       media::PIXEL_FORMAT_YV12,
-      frame_subscriber_->size_, gfx::Rect(frame_subscriber_->size_),
-      frame_subscriber_->size_, base::TimeDelta());
+      size_, gfx::Rect(size_), size_, base::TimeDelta());
   *callback = base::Bind(&FrameSubscriber::OnFrameDelivered,
-                         base::Unretained(frame_subscriber_), *storage);
-  frame_subscriber_->pending_frames++;
+                         weak_factory_.GetWeakPtr(), *storage);
   return true;
 }
 
-Subscriber* FrameSubscriber::GetSubscriber() {
-  return subscriber_;
-}
-
-bool FrameSubscriber::RequestDestruct() {
-  bool deletable = (subscriber_ == NULL && pending_frames == 0);
-  // Destruct FrameSubscriber if we're not waiting for frames and the
-  // subscription has ended
-  if (deletable)
-    delete this;
-
-  return deletable;
-}
-
 void FrameSubscriber::OnFrameDelivered(
     scoped_refptr<media::VideoFrame> frame, base::TimeTicks, bool result) {
-  pending_frames--;
-
-  if (RequestDestruct() || subscriber_ == NULL || !result)
+  if (!result)
     return;
 
   v8::Locker locker(isolate_);

atom/browser/api/frame_subscriber.h

@@ -6,6 +6,7 @@
 #define ATOM_BROWSER_API_FRAME_SUBSCRIBER_H_
 
 #include "base/callback.h"
+#include "base/memory/weak_ptr.h"
 #include "content/public/browser/render_widget_host_view_frame_subscriber.h"
 #include "ui/gfx/geometry/size.h"
 #include "v8/include/v8.h"
@@ -14,43 +15,28 @@ namespace atom {
 
 namespace api {
 
-class FrameSubscriber {
+class FrameSubscriber : public content::RenderWidgetHostViewFrameSubscriber {
  public:
   using FrameCaptureCallback = base::Callback<void(v8::Local<v8::Value>)>;
 
-  // Inner class that is the actual subscriber sent to chromium
-  class Subscriber :
-    public content::RenderWidgetHostViewFrameSubscriber {
-   public:
-    explicit Subscriber(FrameSubscriber* frame_subscriber);
-
-    bool ShouldCaptureFrame(const gfx::Rect& damage_rect,
-                            base::TimeTicks present_time,
-                            scoped_refptr<media::VideoFrame>* storage,
-                            DeliverFrameCallback* callback) override;
-
-    ~Subscriber();
-   private:
-    FrameSubscriber* frame_subscriber_;
-  };
-
   FrameSubscriber(v8::Isolate* isolate,
                   const gfx::Size& size,
                   const FrameCaptureCallback& callback);
 
-  Subscriber* GetSubscriber();
+  bool ShouldCaptureFrame(const gfx::Rect& damage_rect,
+                          base::TimeTicks present_time,
+                          scoped_refptr<media::VideoFrame>* storage,
+                          DeliverFrameCallback* callback) override;
 
  private:
   void OnFrameDelivered(
       scoped_refptr<media::VideoFrame> frame, base::TimeTicks, bool);
 
-  bool RequestDestruct();
-
   v8::Isolate* isolate_;
   gfx::Size size_;
   FrameCaptureCallback callback_;
-  Subscriber* subscriber_;
-  int pending_frames;
+
+  base::WeakPtrFactory<FrameSubscriber> weak_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(FrameSubscriber);
 };

spec/api-auto-updater-spec.js

@@ -2,6 +2,10 @@ const assert = require('assert');
 const autoUpdater = require('electron').remote.autoUpdater;
 const ipcRenderer = require('electron').ipcRenderer;
 
+// Skip autoUpdater tests in MAS build.
+if (process.mas)
+  return;
+
 describe('autoUpdater module', function() {
   describe('checkForUpdates', function() {
     it('emits an error on Windows when called the feed URL is not set', function (done) {

spec/api-browser-window-spec.js

@@ -478,10 +478,16 @@ describe('browser-window module', function() {
     });
   });
 
-  xdescribe('beginFrameSubscription method', function() {
+  describe('beginFrameSubscription method', function() {
     it('subscribes frame updates', function(done) {
+      let called = false;
       w.loadURL("file://" + fixtures + "/api/blank.html");
       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();