Make reftests work for sky.

Make reftests work for sky.

-Add a --testing flag to sky_viewer and cause it to paint into an
SkBitmap instead of a ganesh surface so we can get the pixels out.
-Add GetPixelsForTesting to layer.cc to actually grab out the pixels.
-Add a reftest and a mismatch reftest. They need a setTimeout after
the load event. Unclear why or what the right fix is. Maybe we should
give internals some way to force the paint? If we don't have the
setTimeout, we paint a white page (so we do a paint, but with no
content).
-Add a DisplayDelegate to Layer so that Viewer can decide whether
to use the real ganesh backend or the SkBitmap one without littering
the whole code-base with is_testing bools and logic.

R=esprehn@chromium.org

Committed: https://chromium.googlesource.com/external/mojo/+/9e174d2084be1ffeb32952b9ac91009f8af6e1a7

[esprehn, 2014-12-11 23:18:58]

esprehn@chromium.org changed reviewers:
+ esprehn@chromium.org

[esprehn, 2014-12-11 23:18:59]

Instead you might want a display delegate, and have Layer do something like 

DisplayDelegate::GetCurrent()

and tests swap the delegate to one that does pixel dumping?

https://codereview.chromium.org/797063002/diff/20001/sky/compositor/layer.cc
File sky/compositor/layer.cc (right):

https://codereview.chromium.org/797063002/diff/20001/sky/compositor/layer.cc#...
sky/compositor/layer.cc:33: std::string Layer::GetPixelsForTesting() {
Returning a string is weird, I'd return either a vector<> or a special type. A
special type makes more sense since you're making copies of a massive string as
you pass this thing around. I think you want to return scoped_ptr (or OwnPtr)?

https://codereview.chromium.org/797063002/diff/20001/sky/compositor/layer.cc#...
sky/compositor/layer.cc:53: if (is_testing_) {
Instead I think you want a LayerDelegate interface that has a method on it.
Testing would pass the TestingLayerDelegate, otherwise we pass the
DisplayLayerDelegate.

https://codereview.chromium.org/797063002/diff/20001/sky/tools/tester/tester.cc
File sky/tools/tester/tester.cc (right):

https://codereview.chromium.org/797063002/diff/20001/sky/tools/tester/tester....
sky/tools/tester/tester.cc:26: std::string WaitForURL(bool*
enable_pixel_dumping,
Maybe return a struct of results? Having so many out params is a little weird.

[ojan, 2014-12-12 03:48:39]

Uploaded a new patch. It doesn't quite work yet. It compiles, but gets an
undefined symbol error when trying to run tests.

[25/105] layout/fixed-width.sky failed unexpectedly (test timed out, text diff)

Saw a stderr lines between the end of the previous test and the start of the
current one:
[ERROR:dynamic_service_runner.cc(50)] Failed to load app library (error:
/work/sky/src/out/Debug/sky_viewer.mojo: undefined symbol:
_ZN3sky15DisplayDelegate32setDisplayDelegateCreateFunctionEPFPS0_PNS_11LayerClientEE)

https://codereview.chromium.org/797063002/diff/1/sky/compositor/layer.cc
File sky/compositor/layer.cc (right):

https://codereview.chromium.org/797063002/diff/1/sky/compositor/layer.cc#newc...
sky/compositor/layer.cc:58: bitmapCanvas.drawColor(SK_ColorRED);
This lets us see if if we don't paint at all. Not sure if we want to keep this.
It was useful in debugging as I put this patch together.

https://codereview.chromium.org/797063002/diff/1/sky/tools/tester/tester.cc
File sky/tools/tester/tester.cc (right):

https://codereview.chromium.org/797063002/diff/1/sky/tools/tester/tester.cc#n...
sky/tools/tester/tester.cc:26: std::string WaitForURL(bool*
enable_pixel_dumping,
This code is cargo-culted from
https://code.google.com/p/chromium/codesearch#chromium/src/content/shell/brow...

https://codereview.chromium.org/797063002/diff/1/sky/tools/tester/tester.cc#n...
sky/tools/tester/tester.cc:137: std::string expected_pixel_hash;
This is unused at the moment, but we will need it when we enable regular pixel
tests.

[ojan, 2014-12-12 03:49:57]

weird...those comments are from earlier....i guess i forgot to send them before.

in either case, done for the night. will take this up again tomorrow. i'm sure
i'm just doing something silly.

[ojan, 2014-12-13 00:18:33]

OK. This is ready for review.

https://codereview.chromium.org/797063002/diff/20001/sky/compositor/layer.cc
File sky/compositor/layer.cc (right):

https://codereview.chromium.org/797063002/diff/20001/sky/compositor/layer.cc#...
sky/compositor/layer.cc:33: std::string Layer::GetPixelsForTesting() {
On 2014/12/11 23:18:59, esprehn wrote:
> Returning a string is weird, I'd return either a vector<> or a special type. A
> special type makes more sense since you're making copies of a massive string
as
> you pass this thing around. I think you want to return scoped_ptr (or OwnPtr)?

I made it a std::vector<unsigned char> that you pass in as an out-param to avoid
excessive copies.

https://codereview.chromium.org/797063002/diff/20001/sky/compositor/layer.cc#...
sky/compositor/layer.cc:53: if (is_testing_) {
On 2014/12/11 23:18:59, esprehn wrote:
> Instead I think you want a LayerDelegate interface that has a method on it.
> Testing would pass the TestingLayerDelegate, otherwise we pass the
> DisplayLayerDelegate.

Done.

https://codereview.chromium.org/797063002/diff/20001/sky/tools/tester/tester.cc
File sky/tools/tester/tester.cc (right):

https://codereview.chromium.org/797063002/diff/20001/sky/tools/tester/tester....
sky/tools/tester/tester.cc:26: std::string WaitForURL(bool*
enable_pixel_dumping,
On 2014/12/11 23:18:59, esprehn wrote:
> Maybe return a struct of results? Having so many out params is a little weird.

Done. I made it an out param.

[esprehn, 2014-12-13 00:24:42]

lgtm

https://codereview.chromium.org/797063002/diff/60001/sky/compositor/display_d...
File sky/compositor/display_delegate_bitmap.h (right):

https://codereview.chromium.org/797063002/diff/60001/sky/compositor/display_d...
sky/compositor/display_delegate_bitmap.h:14: class DisplayDelegateBitmap :
public DisplayDelegate {
final

https://codereview.chromium.org/797063002/diff/60001/sky/compositor/display_d...
sky/compositor/display_delegate_bitmap.h:16: DisplayDelegateBitmap(LayerClient*
client);
explicit

https://codereview.chromium.org/797063002/diff/60001/sky/compositor/display_d...
File sky/compositor/display_delegate_ganesh.h (right):

https://codereview.chromium.org/797063002/diff/60001/sky/compositor/display_d...
sky/compositor/display_delegate_ganesh.h:14: class DisplayDelegateGanesh :
public DisplayDelegate {
final

https://codereview.chromium.org/797063002/diff/60001/sky/compositor/display_d...
sky/compositor/display_delegate_ganesh.h:16: DisplayDelegateGanesh(LayerClient*
client) : client_(client) {}
explicit

https://codereview.chromium.org/797063002/diff/60001/sky/tests/harness/reftes...
File sky/tests/harness/reftest-expected.sky (right):

https://codereview.chromium.org/797063002/diff/60001/sky/tests/harness/reftes...
sky/tests/harness/reftest-expected.sky:5: // TODO(ojan): Why is this setTimeout
needed to get
We might, but it's racing.

https://codereview.chromium.org/797063002/diff/60001/sky/tests/harness/reftes...
sky/tests/harness/reftest-expected.sky:8: setTimeout(function() {
requestAnimationFrame() ?

[ojan, 2014-12-13 00:45:53]

Committed patchset #5 (id:80001) manually as
9e174d2084be1ffeb32952b9ac91009f8af6e1a7 (presubmit successful).