Add test_runner hook to dump drag image.

Add test_runner hook to dump drag image.

This is used by:
https://codereview.chromium.org/886323005/

BUG=451759
Committed: https://crrev.com/3726d659908604350bc03ef02d1ccc5cdc097dd5
Cr-Commit-Position: refs/heads/master@{#315915}

[jackhou1, 2015-02-10 01:54:20]

jackhou@chromium.org changed reviewers:
+ junov@chromium.org

[jackhou1, 2015-02-10 01:54:21]

https://codereview.chromium.org/904833004/diff/1/content/shell/renderer/test_...
File content/shell/renderer/test_runner/web_test_proxy.cc (right):

https://codereview.chromium.org/904833004/diff/1/content/shell/renderer/test_...
content/shell/renderer/test_runner/web_test_proxy.cc:814:
base::Base64Encode(base::StringPiece(static_cast<char*>(bitmap.getPixels()),
Is there a better way to represent the image? This is sufficient for the test,
but there's no way to view the output as an image, and the expectations would
need updating if the pixel format changed.

[Justin Novosad, 2015-02-10 22:37:48]

https://codereview.chromium.org/904833004/diff/1/content/shell/renderer/test_...
File content/shell/renderer/test_runner/web_test_proxy.cc (right):

https://codereview.chromium.org/904833004/diff/1/content/shell/renderer/test_...
content/shell/renderer/test_runner/web_test_proxy.cc:814:
base::Base64Encode(base::StringPiece(static_cast<char*>(bitmap.getPixels()),
On 2015/02/10 01:54:20, jackhou1 wrote:
> Is there a better way to represent the image? This is sufficient for the test,
> but there's no way to view the output as an image, and the expectations would
> need updating if the pixel format changed.

Yeah this is not ideal. It would be much better to find a way to inject the drag
image into the test's pixel results.  Are your tests going to be outputting
multiple images, or always just one?

[jackhou1, 2015-02-11 00:08:25]

https://codereview.chromium.org/904833004/diff/1/content/shell/renderer/test_...
File content/shell/renderer/test_runner/web_test_proxy.cc (right):

https://codereview.chromium.org/904833004/diff/1/content/shell/renderer/test_...
content/shell/renderer/test_runner/web_test_proxy.cc:814:
base::Base64Encode(base::StringPiece(static_cast<char*>(bitmap.getPixels()),
On 2015/02/10 22:37:48, junov wrote:
> On 2015/02/10 01:54:20, jackhou1 wrote:
> > Is there a better way to represent the image? This is sufficient for the
test,
> > but there's no way to view the output as an image, and the expectations
would
> > need updating if the pixel format changed.
> 
> Yeah this is not ideal. It would be much better to find a way to inject the
drag
> image into the test's pixel results.  Are your tests going to be outputting
> multiple images, or always just one?

Just one. And I imagine that would be fine for any future tests of DragImage.

I'll look into dumping the image in place of pixel results. Let me know if you
have any suggestions.

[jackhou1, 2015-02-11 00:30:16]

PTAL

[Justin Novosad, 2015-02-11 04:23:25]

On 2015/02/11 00:30:16, jackhou1 wrote:
> PTAL

Ah! That's a lot more useful.  non-owner lgtm

[jackhou1, 2015-02-11 05:04:44]

jackhou@chromium.org changed reviewers:
+ tkent@chromium.org

[jackhou1, 2015-02-11 05:04:45]

tkent, could you please take a look?

[tkent, 2015-02-12 00:50:50]

lgtm

https://codereview.chromium.org/904833004/diff/20001/content/shell/renderer/t...
File content/shell/renderer/test_runner/test_runner.h (right):

https://codereview.chromium.org/904833004/diff/20001/content/shell/renderer/t...
content/shell/renderer/test_runner/test_runner.h:473: // This function sets a
flag which tells the test_shell to dump the drag image
test_shell -> content_shell or test_runner

We have no test_shell any longer.

[jackhou1, 2015-02-12 02:05:21]

New patchsets have been uploaded after l-g-t-m from
junov@chromium.org,tkent@chromium.org

[jackhou1, 2015-02-12 02:05:28]

https://codereview.chromium.org/904833004/diff/20001/content/shell/renderer/t...
File content/shell/renderer/test_runner/test_runner.h (right):

https://codereview.chromium.org/904833004/diff/20001/content/shell/renderer/t...
content/shell/renderer/test_runner/test_runner.h:473: // This function sets a
flag which tells the test_shell to dump the drag image
On 2015/02/12 00:50:49, tkent wrote:
> test_shell -> content_shell or test_runner
> 
> We have no test_shell any longer.

Changed to "WebTestProxy".

[jackhou1, 2015-02-12 02:05:35]

The CQ bit was checked by jackhou@chromium.org

[commit-bot: I haz the power, 2015-02-12 02:06:00]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/904833004/40001

[commit-bot: I haz the power, 2015-02-12 02:44:39]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-02-12 02:46:03]

Patchset 3 (id:??) landed as
https://crrev.com/3726d659908604350bc03ef02d1ccc5cdc097dd5
Cr-Commit-Position: refs/heads/master@{#315915}

[tkent, 2015-02-13 02:30:48]

A revert of this CL (patchset #3 id:40001) has been created in
https://codereview.chromium.org/925743002/ by tkent@chromium.org.

The reason for reverting is: Caused pixel test flakiness..

[Justin Novosad, 2015-02-13 03:00:05]

https://codereview.chromium.org/904833004/diff/40001/content/shell/renderer/t...
File content/shell/renderer/test_runner/web_test_proxy.cc (right):

https://codereview.chromium.org/904833004/diff/40001/content/shell/renderer/t...
content/shell/renderer/test_runner/web_test_proxy.cc:568: if
(!drag_image_.isNull()) {
Theory on why this this CL introduced flake: drag_image_ is never reset, so it
may contain remnants of a previous test. I don't know this class very well but I
am guessing WebTestProxyBase::Reset() might be the right place. 

Also, I think you should be checking the test runner flag here to avoid falling
back to the regular pixel dump in the case where dumpDragImage was called but no
drag ever happened.  That would be an error in the test, and it should produce
an error in the test result (blank output?)

[jackhou1, 2015-02-13 04:54:44]

https://codereview.chromium.org/904833004/diff/40001/content/shell/renderer/t...
File content/shell/renderer/test_runner/web_test_proxy.cc (right):

https://codereview.chromium.org/904833004/diff/40001/content/shell/renderer/t...
content/shell/renderer/test_runner/web_test_proxy.cc:568: if
(!drag_image_.isNull()) {
On 2015/02/13 03:00:05, junov wrote:
> Theory on why this this CL introduced flake: drag_image_ is never reset, so it
> may contain remnants of a previous test. I don't know this class very well but
I
> am guessing WebTestProxyBase::Reset() might be the right place. 

Yeah that does fix the flakiness: https://codereview.chromium.org/918313003/
That CL was failing on the CQ for some reason though.
I'll put together a new CL with that fix included.

> Also, I think you should be checking the test runner flag here to avoid
falling
> back to the regular pixel dump in the case where dumpDragImage was called but
no
> drag ever happened.  That would be an error in the test, and it should produce
> an error in the test result (blank output?)

Ah, good point, I'll make this change in the new CL.

[jackhou1, 2015-02-13 04:55:00]

New CL here: https://codereview.chromium.org/916893003/