Make cc::CompositorFrames movable [Part 1 of 2]

content/browser/devtools/protocol/page_handler.cc

 // Copyright 2014 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.
 
 #include "content/browser/devtools/protocol/page_handler.h"
 
 #include <string>
 
 #include "base/base64.h"
 #include "base/bind.h"
@@ skipping 127 matching lines @@
 
 void PageHandler::SetClient(std::unique_ptr<Client> client) {
   client_.swap(client);
 }
 
 void PageHandler::Detached() {
   Disable();
 }
 
 void PageHandler::OnSwapCompositorFrame(
-    const cc::CompositorFrameMetadata& frame_metadata) {
-  last_compositor_frame_metadata_ = frame_metadata;
+    cc::CompositorFrameMetadata frame_metadata) {
+  last_compositor_frame_metadata_ = std::move(frame_metadata);
   has_compositor_frame_metadata_ = true;
 
   if (screencast_enabled_)
     InnerSwapCompositorFrame();
   color_picker_->OnSwapCompositorFrame();
 }
 
 void PageHandler::OnSynchronousSwapCompositorFrame(
-    const cc::CompositorFrameMetadata& frame_metadata) {
-  last_compositor_frame_metadata_ = has_compositor_frame_metadata_ ?
-      next_compositor_frame_metadata_ : frame_metadata;
-  next_compositor_frame_metadata_ = frame_metadata;
+    cc::CompositorFrameMetadata frame_metadata) {
+  if (has_compositor_frame_metadata_) {
+    last_compositor_frame_metadata_ =
+        std::move(next_compositor_frame_metadata_);
+    next_compositor_frame_metadata_ = std::move(frame_metadata);
+  } else {
+    last_compositor_frame_metadata_ = std::move(frame_metadata);

[danakj, 2016/06/24 21:45:18]

nit: you could keep the next_cfm part outside the if/else

if (has)
  last = std::move(next);
else
  last = frame_metadata.Clone();
next = std::move(frame_metadata);

[Fady Samuel, 2016/06/24 22:24:37]

Done.

+    next_compositor_frame_metadata_ = last_compositor_frame_metadata_.Clone();
+  }
+
   has_compositor_frame_metadata_ = true;
 
   if (screencast_enabled_)
     InnerSwapCompositorFrame();
   color_picker_->OnSwapCompositorFrame();
 }
 
 void PageHandler::Observe(int type,
                           const NotificationSource& source,
                           const NotificationDetails& details) {
@@ skipping 246 matching lines @@
 
   if (scale <= 0)
     scale = 0.1;
 
   gfx::Size snapshot_size_dip(gfx::ToRoundedSize(
       gfx::ScaleSize(viewport_size_dip, scale)));
 
   if (snapshot_size_dip.width() > 0 && snapshot_size_dip.height() > 0) {
     gfx::Rect viewport_bounds_dip(gfx::ToRoundedSize(viewport_size_dip));
     view->CopyFromCompositingSurface(
-        viewport_bounds_dip,
-        snapshot_size_dip,
+        viewport_bounds_dip, snapshot_size_dip,
         base::Bind(&PageHandler::ScreencastFrameCaptured,
                    weak_factory_.GetWeakPtr(),
-                   last_compositor_frame_metadata_),
+                   last_compositor_frame_metadata_.Clone()),

[danakj, 2016/06/24 21:45:18]

use base::Passed( of Clone()) here

[Fady Samuel, 2016/06/24 22:24:37]

Done.

         kN32_SkColorType);
     frames_in_flight_++;
   }
 }
 
 void PageHandler::ScreencastFrameCaptured(
     const cc::CompositorFrameMetadata& metadata,

[danakj, 2016/06/24 21:45:18]

take this by value

[Fady Samuel, 2016/06/24 22:24:37]

Done.

     const SkBitmap& bitmap,
     ReadbackResponse response) {
   if (response != READBACK_SUCCESS) {
     if (capture_retry_count_) {
       --capture_retry_count_;
       base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(
           FROM_HERE, base::Bind(&PageHandler::InnerSwapCompositorFrame,
                                 weak_factory_.GetWeakPtr()),
           base::TimeDelta::FromMilliseconds(kFrameRetryDelayMs));
     }
     --frames_in_flight_;
     return;
   }
   base::PostTaskAndReplyWithResult(
-      base::WorkerPool::GetTaskRunner(true).get(),
-      FROM_HERE,
-      base::Bind(&EncodeScreencastFrame,
-                 bitmap, screencast_format_, screencast_quality_),
+      base::WorkerPool::GetTaskRunner(true).get(), FROM_HERE,
+      base::Bind(&EncodeScreencastFrame, bitmap, screencast_format_,
+                 screencast_quality_),
       base::Bind(&PageHandler::ScreencastFrameEncoded,
-                 weak_factory_.GetWeakPtr(), metadata, base::Time::Now()));
+                 weak_factory_.GetWeakPtr(), metadata.Clone(),

[danakj, 2016/06/24 21:45:18]

use base::Passed(&metadata) here,

then you avoid this copy, and the copy into the Bind in each place.

[Fady Samuel, 2016/06/24 22:24:37]

Done.

+                 base::Time::Now()));
 }
 
 void PageHandler::ScreencastFrameEncoded(
     const cc::CompositorFrameMetadata& metadata,

[danakj, 2016/06/24 21:45:18]

and take this by value

[Fady Samuel, 2016/06/24 22:24:37]

Done.

     const base::Time& timestamp,
     const std::string& data) {
   // Consider metadata empty in case it has no device scale factor.
   if (metadata.device_scale_factor == 0 || !host_ || data.empty()) {
     --frames_in_flight_;
     return;
   }
 
   RenderWidgetHostViewBase* view = static_cast<RenderWidgetHostViewBase*>(
       host_->GetView());
@@ skipping 40 matching lines @@
 
 void PageHandler::OnColorPicked(int r, int g, int b, int a) {
   scoped_refptr<dom::RGBA> color =
       dom::RGBA::Create()->set_r(r)->set_g(g)->set_b(b)->set_a(a);
   client_->ColorPicked(ColorPickedParams::Create()->set_color(color));
 }
 
 }  // namespace page
 }  // namespace devtools
 }  // namespace content