Implement webview.captureVisibleRegion()

content/browser/readback_request_helpers.h

+// Copyright 2016 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.
+
+#ifndef CONTENT_BROWSER_READBACK_REQUEST_HELPERS_H_
+#define CONTENT_BROWSER_READBACK_REQUEST_HELPERS_H_

[Charlie Reis, 2016/01/19 22:45:36]

As a non-graphics person, "readback" isn't an obvious term.  Seems like this
file should have some comments saying what this is for.

[ncarter (slow), 2016/01/20 00:16:18]

'Readback' as a term is already part of the content public api (in the form of
ReadbackRequestCallback and ReadbackResponse, defined in readback_types.h).

[wjmaclean, 2016/01/20 15:32:36]

Acknowledged.

[wjmaclean, 2016/01/20 15:32:36]

Acknowledged.

+
+#include "base/memory/scoped_ptr.h"
+#include "content/public/browser/readback_types.h"
+#include "third_party/skia/include/core/SkImageInfo.h"
+#include "ui/gfx/geometry/size.h"
+
+class SkAutoLockPixels;
+class SkBitmap;
+
+namespace cc {
+class CopyOutputResult;
+class SingleReleaseCallback;
+}
+
+namespace content {

[Charlie Reis, 2016/01/19 22:45:36]

Having this float around the content namespace seems weird to me.  For one thing
(as Nick pointed out to me), it would be tough to know where to look for this
code without doing a codesearch.

Is there a reason that RWHVCF can't call these methods on DelegatedFrameHost if
they were made public?  Or perhaps they should be in utils somewhere?

[kenrb, 2016/01/20 01:43:04]

DelegatedFrameHost manages compositor frame state for top-level
RenderWidgetHostViews, and interacts with the compositor on their behalf.
RenderWidgetHostViewChildFrame does not have access to one.

[wjmaclean, 2016/01/20 15:32:36]

We could consider making content::surface_utils or content::compositor::utils.
Since there are other unrelated functions in surface_utils that would be
affected by this, perhaps the namespace change should be done separately?

+
+void CopyFromCompositingSurfaceFinished(

[ncarter (slow), 2016/01/20 00:16:18]

This is just a helper used to implement PrepareTextureCopyOutputResult, right?
It's confusing for that to be exposed in the header file.

[wjmaclean, 2016/01/20 15:32:36]

Done.

+    const ReadbackRequestCallback& callback,
+    scoped_ptr<cc::SingleReleaseCallback> release_callback,
+    scoped_ptr<SkBitmap> bitmap,
+    scoped_ptr<SkAutoLockPixels> bitmap_pixels_lock,
+    bool result);

[ncarter (slow), 2016/01/20 00:16:18]

Functions in a big namespace like 'content' hurt readability, since you
basically have to use codesearch or grep to find where they are. That's why we
usually either put them in a class and make them static methods, or put them in
a util namespace, so that call sites give some hint as to where to look to find
the definition.

But an even better option, perhaps, would be to make this actually a stateful
class (e.g. 'class ReadbackRequestHelper', which DelegatedFrameHost has a
member?) I have no idea if that's a suitable solution or not, these copy paths
are a twisty maze of chained callbacks, but I think it's worth the due diligence
to try to arrange these in OO style if possible.

[wjmaclean, 2016/01/20 15:32:36]

The functions all appear to be stateless, though they could still be grouped
into a class as static functions? For now I've moved them into
content/browser/compositor/surface_utils.{h|cc} with some related surface
functionality. 

+
+void PrepareTextureCopyOutputResult(const gfx::Size& dst_size_in_pixel,

[ncarter (slow), 2016/01/20 00:16:18]

Similar to CopyFromCompositingSurfaceFinished, PrepareTextureCopyOutputResult
and PrepareBitmapCopyOutputResult seem like they're just helpers used to
implement CopyFromCompositingSurfaceHasResult. Is that right? If so I'd look for
a way to restrict their visibility so that it's clearer that
CopyFromCompositingSurfaceHasResult is the bit of code we're actually trying to
share.

+                                    const SkColorType color_type,
+                                    const ReadbackRequestCallback& callback,
+                                    scoped_ptr<cc::CopyOutputResult> result);
+
+void PrepareBitmapCopyOutputResult(const gfx::Size& dst_size_in_pixel,
+                                   const SkColorType preferred_color_type,
+                                   const ReadbackRequestCallback& callback,
+                                   scoped_ptr<cc::CopyOutputResult> result);
+
+void CopyFromCompositingSurfaceHasResult(

[ncarter (slow), 2016/01/20 00:16:19]

My interpretation is that the reusable functionality here, is all about bridging
between cc::CopyOutputResult and the content-public ReadbackRequestCallback. We
can't declare this helper in cc because of their content/public dependency, so
they need to be in content/browser somewhere.

Why not a _util file in content/browser/compositor, then? Isn't that a natural
place to put code that's converting cc-isms into content-isms?

[wjmaclean, 2016/01/20 15:32:36]

Acknowledged.

+    const gfx::Size& dst_size_in_pixel,
+    const SkColorType color_type,
+    const ReadbackRequestCallback& callback,
+    scoped_ptr<cc::CopyOutputResult> result);

[ncarter (slow), 2016/01/20 00:16:18]

+= blank line

[wjmaclean, 2016/01/20 15:32:36]

Done.

+}

[ncarter (slow), 2016/01/20 00:16:18]

}  // namespace content

[wjmaclean, 2016/01/20 15:32:36]

Done.

+
+#endif  // CONTENT_BROWSER_READBACK_REQUEST_HELPERS_H_