Split DumpPixelsAsync in pixel_dump.h into more granular functions.

Split DumpPixelsAsync in pixel_dump.h into more granular functions.

Before this CL, test_runner/pixel_dump.h would expose a single function:

  void DumpPixelsAsync(
      blink::WebView* web_view,
      const LayoutTestRuntimeFlags& layout_test_runtime_flags,
      float device_scale_factor_for_test,
      const base::Callback<void(const SkBitmap&)>& callback);

After this CL, three more granular functions are exposed instead:

  void DumpPixelsAsync(blink::WebLocalFrame* web_frame,
                       float device_scale_factor_for_test,
                       base::OnceCallback<void(const SkBitmap&)> callback);

  void PrintFrameAsync(blink::WebLocalFrame* web_frame,
                       base::OnceCallback<void(const SkBitmap&)> callback);

  base::OnceCallback<void(const SkBitmap&)>
  CreateSelectionBoundsRectDrawingCallback(
      blink::WebLocalFrame* web_frame,
      base::OnceCallback<void(const SkBitmap&)> original_callback);

This change means that it is easier to replace only the new DumpPixelsAsync
part, while retaining the old behavior for PrintFrameAsync and
CreateSelectionDrawingCallback.  In particular, a future CL should attempt to
delegate DumpPixelsAsync to the browser, so that pixels belonging to OOPIFs are
also captured.

BUG=667551
Review-Url: https://codereview.chromium.org/2963593002
Cr-Commit-Position: refs/heads/master@{#485774}
Committed: https://chromium.googlesource.com/chromium/src/+/20971a656acd1f320b5b27a5b6a895265e696bdf

[Łukasz Anforowicz, 2017-06-27 18:57:15]

The CQ bit was checked by lukasza@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-27 18:57:31]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Łukasz Anforowicz, 2017-06-27 19:13:34]

The CQ bit was checked by lukasza@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-27 19:13:48]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-27 20:44:46]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-27 20:44:47]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Łukasz Anforowicz, 2017-06-27 23:13:47]

The CQ bit was checked by lukasza@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-27 23:14:10]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-28 00:43:53]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-28 00:43:54]

Dry run: This issue passed the CQ dry run.

[Łukasz Anforowicz, 2017-07-10 19:36:07]

Description was changed from

==========
Modify pixel_dump.h functions to take WebLocalFrame instead of WebView pointer.

This CL enables taking a pixel dump of an individual frame (and is a
stepping stone toward stiching together pixel dumps coming from OOPIFs).

This CL can also be seen as a general clean-up of pixel_dump.cc:

- The code is cleaner and more generic if it works with WebLocalFrame,
  rather than with the main frame of a WebView.

- Instead of explicitly storing async callbacks state in
  PixelsDumpRequests, the state can be passed around by currying more
  generic base::Callbacks.  An example of this is seen in
  - introduction of CreateSelectionDrawingCallback,
  - removal of DidCapturePixels,
  - extraction of CapturePixels (out of DumpPixelsAsync).

BUG=667551
==========

to

==========
Split DumpPixelsAsync into more granular functions.

Before this CL, test_runner/pixel_dump.h would expose a single function:

  void DumpPixelsAsync(
      blink::WebView* web_view,
      const LayoutTestRuntimeFlags& layout_test_runtime_flags,
      float device_scale_factor_for_test,
      const base::Callback<void(const SkBitmap&)>& callback);

After this CL, three more granular functions are exposed instead:

  void DumpPixelsAsync(blink::WebLocalFrame* web_frame,
                       float device_scale_factor_for_test,
                       base::OnceCallback<void(const SkBitmap&)> callback);

  void PrintFrameAsync(blink::WebLocalFrame* web_frame,
                       base::OnceCallback<void(const SkBitmap&)> callback);

  base::OnceCallback<void(const SkBitmap&)> CreateSelectionDrawingCallback(
      blink::WebLocalFrame* web_frame,
      base::OnceCallback<void(const SkBitmap&)> original_callback);

This change means that it is easier to replace only the new DumpPixelsAsync
part, while retaining the old behavior for PrintFrameAsync and
CreateSelectionDrawingCallback.  In particular, a future CL should attempt to
delegate DumpPixelsAsync to the browser, so that pixels belonging to OOPIFs are
also captured.

BUG=667551
==========

[Łukasz Anforowicz, 2017-07-10 19:44:22]

The CQ bit was checked by lukasza@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-07-10 19:44:47]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Łukasz Anforowicz, 2017-07-10 19:49:25]

Description was changed from

==========
Split DumpPixelsAsync into more granular functions.

Before this CL, test_runner/pixel_dump.h would expose a single function:

  void DumpPixelsAsync(
      blink::WebView* web_view,
      const LayoutTestRuntimeFlags& layout_test_runtime_flags,
      float device_scale_factor_for_test,
      const base::Callback<void(const SkBitmap&)>& callback);

After this CL, three more granular functions are exposed instead:

  void DumpPixelsAsync(blink::WebLocalFrame* web_frame,
                       float device_scale_factor_for_test,
                       base::OnceCallback<void(const SkBitmap&)> callback);

  void PrintFrameAsync(blink::WebLocalFrame* web_frame,
                       base::OnceCallback<void(const SkBitmap&)> callback);

  base::OnceCallback<void(const SkBitmap&)> CreateSelectionDrawingCallback(
      blink::WebLocalFrame* web_frame,
      base::OnceCallback<void(const SkBitmap&)> original_callback);

This change means that it is easier to replace only the new DumpPixelsAsync
part, while retaining the old behavior for PrintFrameAsync and
CreateSelectionDrawingCallback.  In particular, a future CL should attempt to
delegate DumpPixelsAsync to the browser, so that pixels belonging to OOPIFs are
also captured.

BUG=667551
==========

to

==========
Split DumpPixelsAsync in pixel_dump.h into more granular functions.

Before this CL, test_runner/pixel_dump.h would expose a single function:

  void DumpPixelsAsync(
      blink::WebView* web_view,
      const LayoutTestRuntimeFlags& layout_test_runtime_flags,
      float device_scale_factor_for_test,
      const base::Callback<void(const SkBitmap&)>& callback);

After this CL, three more granular functions are exposed instead:

  void DumpPixelsAsync(blink::WebLocalFrame* web_frame,
                       float device_scale_factor_for_test,
                       base::OnceCallback<void(const SkBitmap&)> callback);

  void PrintFrameAsync(blink::WebLocalFrame* web_frame,
                       base::OnceCallback<void(const SkBitmap&)> callback);

  base::OnceCallback<void(const SkBitmap&)> CreateSelectionDrawingCallback(
      blink::WebLocalFrame* web_frame,
      base::OnceCallback<void(const SkBitmap&)> original_callback);

This change means that it is easier to replace only the new DumpPixelsAsync
part, while retaining the old behavior for PrintFrameAsync and
CreateSelectionDrawingCallback.  In particular, a future CL should attempt to
delegate DumpPixelsAsync to the browser, so that pixels belonging to OOPIFs are
also captured.

BUG=667551
==========

[commit-bot: I haz the power, 2017-07-10 21:29:29]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-07-10 21:29:30]

Dry run: This issue passed the CQ dry run.

[Łukasz Anforowicz, 2017-07-10 21:32:05]

lukasza@chromium.org changed reviewers:
+ alexmos@chromium.org

[Łukasz Anforowicz, 2017-07-10 21:32:06]

Alex, can you PTAL?

[alexmos, 2017-07-11 15:01:07]

Thanks, LGTM with nits

https://codereview.chromium.org/2963593002/diff/100001/content/shell/test_run...
File content/shell/test_runner/pixel_dump.cc (right):

https://codereview.chromium.org/2963593002/diff/100001/content/shell/test_run...
content/shell/test_runner/pixel_dump.cc:58: blink::WebRect wr,
const &?

https://codereview.chromium.org/2963593002/diff/100001/content/shell/test_run...
content/shell/test_runner/pixel_dump.cc:171: return original_callback;
does this need std::move?

https://codereview.chromium.org/2963593002/diff/100001/content/shell/test_run...
File content/shell/test_runner/pixel_dump.h (right):

https://codereview.chromium.org/2963593002/diff/100001/content/shell/test_run...
content/shell/test_runner/pixel_dump.h:18: // Asks |web_frame|'s widget to dumps
its pixels and calls |callback| with the
nit: s/dumps/dump/

https://codereview.chromium.org/2963593002/diff/100001/content/shell/test_run...
content/shell/test_runner/pixel_dump.h:28: // Creates a new callback that will
first draw the current selection rect of
This is called "selection bounds rect" elsewhere, maybe use that here as well? 
Also, would it be useful to preserve some of this old comment about it?
"Selection bounds rect is the rect enclosing the (possibly transformed)
selection.  It should be drawn after everything is laid out and painted"

https://codereview.chromium.org/2963593002/diff/100001/content/shell/test_run...
content/shell/test_runner/pixel_dump.h:30: base::OnceCallback<void(const
SkBitmap&)> CreateSelectionDrawingCallback(
The name sounds like it draws the selection rather than the bounds rect - could
we include "Rect"  in the name?

https://codereview.chromium.org/2963593002/diff/100001/content/shell/test_run...
File content/shell/test_runner/test_runner.cc (right):

https://codereview.chromium.org/2963593002/diff/100001/content/shell/test_run...
content/shell/test_runner/test_runner.cc:1789: if
(layout_test_runtime_flags_.dump_selection_rect()) {
nit: { not needed

[Łukasz Anforowicz, 2017-07-11 16:29:23]

The CQ bit was checked by lukasza@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-07-11 16:29:43]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Łukasz Anforowicz, 2017-07-11 16:30:24]

Thanks!

https://codereview.chromium.org/2963593002/diff/100001/content/shell/test_run...
File content/shell/test_runner/pixel_dump.cc (right):

https://codereview.chromium.org/2963593002/diff/100001/content/shell/test_run...
content/shell/test_runner/pixel_dump.cc:58: blink::WebRect wr,
On 2017/07/11 15:01:06, alexmos wrote:
> const &?

Done.

https://codereview.chromium.org/2963593002/diff/100001/content/shell/test_run...
content/shell/test_runner/pixel_dump.cc:171: return original_callback;
On 2017/07/11 15:01:06, alexmos wrote:
> does this need std::move?

1. It compiles, therefore it has to be a move (base::OnceCallback<> doesn't have
a copy constructor).

2. I don't quite understand it myself, but I hear |return foo| is preferrable
over |return std::move(foo)| because the latter can in some cases undesirably
prevent RVO / ellision.  I found a bit of conflicting advise out there, so I am
not sure if it is fair for me to cherry-pick one that fits my narrative, but
maybe I can leave this here:
https://isocpp.org/blog/2013/02/no-really-moving-a-return-value-is-easy-stack...

https://codereview.chromium.org/2963593002/diff/100001/content/shell/test_run...
File content/shell/test_runner/pixel_dump.h (right):

https://codereview.chromium.org/2963593002/diff/100001/content/shell/test_run...
content/shell/test_runner/pixel_dump.h:18: // Asks |web_frame|'s widget to dumps
its pixels and calls |callback| with the
On 2017/07/11 15:01:06, alexmos wrote:
> nit: s/dumps/dump/

Done.

https://codereview.chromium.org/2963593002/diff/100001/content/shell/test_run...
content/shell/test_runner/pixel_dump.h:28: // Creates a new callback that will
first draw the current selection rect of
On 2017/07/11 15:01:06, alexmos wrote:
> This is called "selection bounds rect" elsewhere, maybe use that here as well?

> Also, would it be useful to preserve some of this old comment about it?
> "Selection bounds rect is the rect enclosing the (possibly transformed)
> selection.  It should be drawn after everything is laid out and painted"

Done (I also named the parameters of the callbacks, so that I can refer to them
in the comment - this makes the declarations a bit verbose, but I think this is
okay, as I think it does make it easier to understand what this does).

https://codereview.chromium.org/2963593002/diff/100001/content/shell/test_run...
content/shell/test_runner/pixel_dump.h:30: base::OnceCallback<void(const
SkBitmap&)> CreateSelectionDrawingCallback(
On 2017/07/11 15:01:06, alexmos wrote:
> The name sounds like it draws the selection rather than the bounds rect -
could
> we include "Rect"  in the name?

Done.

https://codereview.chromium.org/2963593002/diff/100001/content/shell/test_run...
File content/shell/test_runner/test_runner.cc (right):

https://codereview.chromium.org/2963593002/diff/100001/content/shell/test_run...
content/shell/test_runner/test_runner.cc:1789: if
(layout_test_runtime_flags_.dump_selection_rect()) {
On 2017/07/11 15:01:07, alexmos wrote:
> nit: { not needed

After renaming the function to CreateSelectionBoundsRectDrawingCallback, the
body of the if statement takes 2 lines now.  So - I think it might be
okay/appropriate to leave the curly braces here.

[Łukasz Anforowicz, 2017-07-11 16:31:26]

Description was changed from

==========
Split DumpPixelsAsync in pixel_dump.h into more granular functions.

Before this CL, test_runner/pixel_dump.h would expose a single function:

  void DumpPixelsAsync(
      blink::WebView* web_view,
      const LayoutTestRuntimeFlags& layout_test_runtime_flags,
      float device_scale_factor_for_test,
      const base::Callback<void(const SkBitmap&)>& callback);

After this CL, three more granular functions are exposed instead:

  void DumpPixelsAsync(blink::WebLocalFrame* web_frame,
                       float device_scale_factor_for_test,
                       base::OnceCallback<void(const SkBitmap&)> callback);

  void PrintFrameAsync(blink::WebLocalFrame* web_frame,
                       base::OnceCallback<void(const SkBitmap&)> callback);

  base::OnceCallback<void(const SkBitmap&)> CreateSelectionDrawingCallback(
      blink::WebLocalFrame* web_frame,
      base::OnceCallback<void(const SkBitmap&)> original_callback);

This change means that it is easier to replace only the new DumpPixelsAsync
part, while retaining the old behavior for PrintFrameAsync and
CreateSelectionDrawingCallback.  In particular, a future CL should attempt to
delegate DumpPixelsAsync to the browser, so that pixels belonging to OOPIFs are
also captured.

BUG=667551
==========

to

==========
Split DumpPixelsAsync in pixel_dump.h into more granular functions.

Before this CL, test_runner/pixel_dump.h would expose a single function:

  void DumpPixelsAsync(
      blink::WebView* web_view,
      const LayoutTestRuntimeFlags& layout_test_runtime_flags,
      float device_scale_factor_for_test,
      const base::Callback<void(const SkBitmap&)>& callback);

After this CL, three more granular functions are exposed instead:

  void DumpPixelsAsync(blink::WebLocalFrame* web_frame,
                       float device_scale_factor_for_test,
                       base::OnceCallback<void(const SkBitmap&)> callback);

  void PrintFrameAsync(blink::WebLocalFrame* web_frame,
                       base::OnceCallback<void(const SkBitmap&)> callback);

  base::OnceCallback<void(const SkBitmap&)>
  CreateSelectionBoundsRectDrawingCallback(
      blink::WebLocalFrame* web_frame,
      base::OnceCallback<void(const SkBitmap&)> original_callback);

This change means that it is easier to replace only the new DumpPixelsAsync
part, while retaining the old behavior for PrintFrameAsync and
CreateSelectionDrawingCallback.  In particular, a future CL should attempt to
delegate DumpPixelsAsync to the browser, so that pixels belonging to OOPIFs are
also captured.

BUG=667551
==========

[alexmos, 2017-07-11 16:48:11]

LGTM

https://codereview.chromium.org/2963593002/diff/100001/content/shell/test_run...
File content/shell/test_runner/pixel_dump.cc (right):

https://codereview.chromium.org/2963593002/diff/100001/content/shell/test_run...
content/shell/test_runner/pixel_dump.cc:171: return original_callback;
On 2017/07/11 16:30:23, Łukasz A. wrote:
> On 2017/07/11 15:01:06, alexmos wrote:
> > does this need std::move?
> 
> 1. It compiles, therefore it has to be a move (base::OnceCallback<> doesn't
have
> a copy constructor).
> 
> 2. I don't quite understand it myself, but I hear |return foo| is preferrable
> over |return std::move(foo)| because the latter can in some cases undesirably
> prevent RVO / ellision.  I found a bit of conflicting advise out there, so I
am
> not sure if it is fair for me to cherry-pick one that fits my narrative, but
> maybe I can leave this here:
>
https://isocpp.org/blog/2013/02/no-really-moving-a-return-value-is-easy-stack...

Ack.  Thanks for the pointers, I was confused about this myself, as I also saw
some conflicting examples in our codebase (e.g.,
https://cs.chromium.org/chromium/src/android_webview/browser/net/aw_cookie_st...).

https://codereview.chromium.org/2963593002/diff/100001/content/shell/test_run...
File content/shell/test_runner/test_runner.cc (right):

https://codereview.chromium.org/2963593002/diff/100001/content/shell/test_run...
content/shell/test_runner/test_runner.cc:1789: if
(layout_test_runtime_flags_.dump_selection_rect()) {
On 2017/07/11 16:30:24, Łukasz A. wrote:
> On 2017/07/11 15:01:07, alexmos wrote:
> > nit: { not needed
> 
> After renaming the function to CreateSelectionBoundsRectDrawingCallback, the
> body of the if statement takes 2 lines now.  So - I think it might be
> okay/appropriate to leave the curly braces here.

Acknowledged.

[commit-bot: I haz the power, 2017-07-11 17:27:16]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-07-11 17:27:20]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Łukasz Anforowicz, 2017-07-11 18:07:47]

The CQ bit was checked by lukasza@chromium.org

[commit-bot: I haz the power, 2017-07-11 18:08:05]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-07-11 19:19:50]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-07-11 19:19:52]

Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Łukasz Anforowicz, 2017-07-11 20:43:30]

The CQ bit was checked by lukasza@chromium.org

[commit-bot: I haz the power, 2017-07-11 20:43:56]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-07-11 22:45:14]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-07-11 22:45:15]

Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, build
has not started yet; builder either lacks capacity or does not exist
(misspelled?))

[Łukasz Anforowicz, 2017-07-12 00:00:57]

The CQ bit was checked by lukasza@chromium.org

[commit-bot: I haz the power, 2017-07-12 00:01:35]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-07-12 01:31:29]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1499817657197230,
"parent_rev": "dcbb1b6ea802c3490dab3ee1ff1ab2ce1c58e156", "commit_rev":
"20971a656acd1f320b5b27a5b6a895265e696bdf"}

[commit-bot: I haz the power, 2017-07-12 01:31:46]

Description was changed from

==========
Split DumpPixelsAsync in pixel_dump.h into more granular functions.

Before this CL, test_runner/pixel_dump.h would expose a single function:

  void DumpPixelsAsync(
      blink::WebView* web_view,
      const LayoutTestRuntimeFlags& layout_test_runtime_flags,
      float device_scale_factor_for_test,
      const base::Callback<void(const SkBitmap&)>& callback);

After this CL, three more granular functions are exposed instead:

  void DumpPixelsAsync(blink::WebLocalFrame* web_frame,
                       float device_scale_factor_for_test,
                       base::OnceCallback<void(const SkBitmap&)> callback);

  void PrintFrameAsync(blink::WebLocalFrame* web_frame,
                       base::OnceCallback<void(const SkBitmap&)> callback);

  base::OnceCallback<void(const SkBitmap&)>
  CreateSelectionBoundsRectDrawingCallback(
      blink::WebLocalFrame* web_frame,
      base::OnceCallback<void(const SkBitmap&)> original_callback);

This change means that it is easier to replace only the new DumpPixelsAsync
part, while retaining the old behavior for PrintFrameAsync and
CreateSelectionDrawingCallback.  In particular, a future CL should attempt to
delegate DumpPixelsAsync to the browser, so that pixels belonging to OOPIFs are
also captured.

BUG=667551
==========

to

==========
Split DumpPixelsAsync in pixel_dump.h into more granular functions.

Before this CL, test_runner/pixel_dump.h would expose a single function:

  void DumpPixelsAsync(
      blink::WebView* web_view,
      const LayoutTestRuntimeFlags& layout_test_runtime_flags,
      float device_scale_factor_for_test,
      const base::Callback<void(const SkBitmap&)>& callback);

After this CL, three more granular functions are exposed instead:

  void DumpPixelsAsync(blink::WebLocalFrame* web_frame,
                       float device_scale_factor_for_test,
                       base::OnceCallback<void(const SkBitmap&)> callback);

  void PrintFrameAsync(blink::WebLocalFrame* web_frame,
                       base::OnceCallback<void(const SkBitmap&)> callback);

  base::OnceCallback<void(const SkBitmap&)>
  CreateSelectionBoundsRectDrawingCallback(
      blink::WebLocalFrame* web_frame,
      base::OnceCallback<void(const SkBitmap&)> original_callback);

This change means that it is easier to replace only the new DumpPixelsAsync
part, while retaining the old behavior for PrintFrameAsync and
CreateSelectionDrawingCallback.  In particular, a future CL should attempt to
delegate DumpPixelsAsync to the browser, so that pixels belonging to OOPIFs are
also captured.

BUG=667551

Review-Url: https://codereview.chromium.org/2963593002
Cr-Commit-Position: refs/heads/master@{#485774}
Committed:
https://chromium.googlesource.com/chromium/src/+/20971a656acd1f320b5b27a5b6a8...
==========

[commit-bot: I haz the power, 2017-07-12 01:31:50]

Committed patchset #7 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/20971a656acd1f320b5b27a5b6a8...