cc: For cc pixel test, create data url for pngs and print bounding rect.

cc: For cc pixel test, create data url for pngs and print bounding rect.

This creates data url for both output and expected pngs, 
and prints the error bounding rect.

BUG=236196
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=282127

[sohanjg, 2014-07-07 07:50:39]

Can you please take a look.
Thanks.

[danakj, 2014-07-07 16:07:02]

https://codereview.chromium.org/371863002/diff/1/cc/test/pixel_comparator.cc
File cc/test/pixel_comparator.cc (right):

https://codereview.chromium.org/371863002/diff/1/cc/test/pixel_comparator.cc#...
cc/test/pixel_comparator.cc:22: int minimum_difference_x = -99;
Can you just use a gfx::Rect that starts empty and Union in pixels that are
wrong?

https://codereview.chromium.org/371863002/diff/1/cc/test/pixel_comparator.cc#...
cc/test/pixel_comparator.cc:62: << " Max X-Y Co-ordinates X = " <<
maximum_difference_x
You can just use ToString() on the Rect if you use one.

https://codereview.chromium.org/371863002/diff/1/cc/test/pixel_test_utils.cc
File cc/test/pixel_test_utils.cc (right):

https://codereview.chromium.org/371863002/diff/1/cc/test/pixel_test_utils.cc#...
cc/test/pixel_test_utils.cc:76: LOG(ERROR) << "Output Data URL =  " <<
op_png_url.c_str();
Let's output both pngs so it's easier to compare them from bot output.

LOG(ERROR) << "Pixels do not match!";
LOG(ERROR) << "Actual: " << gen_bmp_data_url;
LOG(ERROR) << "Expected: " << ref_bmp_data_url;

(you don't need c_str() to turn string into a pointer)

[sohanjg, 2014-07-08 07:31:08]

Please take a look.
Thanks.

https://codereview.chromium.org/371863002/diff/1/cc/test/pixel_comparator.cc
File cc/test/pixel_comparator.cc (right):

https://codereview.chromium.org/371863002/diff/1/cc/test/pixel_comparator.cc#...
cc/test/pixel_comparator.cc:22: int minimum_difference_x = -99;
On 2014/07/07 16:07:02, danakj wrote:
> Can you just use a gfx::Rect that starts empty and Union in pixels that are
> wrong?

Done.

https://codereview.chromium.org/371863002/diff/1/cc/test/pixel_comparator.cc#...
cc/test/pixel_comparator.cc:62: << " Max X-Y Co-ordinates X = " <<
maximum_difference_x
On 2014/07/07 16:07:02, danakj wrote:
> You can just use ToString() on the Rect if you use one.

Done.

https://codereview.chromium.org/371863002/diff/1/cc/test/pixel_test_utils.cc
File cc/test/pixel_test_utils.cc (right):

https://codereview.chromium.org/371863002/diff/1/cc/test/pixel_test_utils.cc#...
cc/test/pixel_test_utils.cc:76: LOG(ERROR) << "Output Data URL =  " <<
op_png_url.c_str();
On 2014/07/07 16:07:02, danakj wrote:
> Let's output both pngs so it's easier to compare them from bot output.
> 
> LOG(ERROR) << "Pixels do not match!";
> LOG(ERROR) << "Actual: " << gen_bmp_data_url;
> LOG(ERROR) << "Expected: " << ref_bmp_data_url;
> 
> (you don't need c_str() to turn string into a pointer)

Done.

[danakj, 2014-07-08 16:08:34]

https://codereview.chromium.org/371863002/diff/40001/cc/test/pixel_comparator.cc
File cc/test/pixel_comparator.cc (right):

https://codereview.chromium.org/371863002/diff/40001/cc/test/pixel_comparator...
cc/test/pixel_comparator.cc:22: // Bounding box for error
nit: comments are sentences including periods.

but how about making the variable name say what you wanna say instead of needing
a comment?

[sohanjg, 2014-07-08 16:20:34]

Can you please take a look.
Thanks.

https://codereview.chromium.org/371863002/diff/40001/cc/test/pixel_comparator.cc
File cc/test/pixel_comparator.cc (right):

https://codereview.chromium.org/371863002/diff/40001/cc/test/pixel_comparator...
cc/test/pixel_comparator.cc:22: // Bounding box for error
On 2014/07/08 16:08:34, danakj wrote:
> nit: comments are sentences including periods.
> 
> but how about making the variable name say what you wanna say instead of
needing
> a comment?

Done.

[danakj, 2014-07-08 16:46:47]

https://codereview.chromium.org/371863002/diff/60001/cc/test/pixel_comparator.cc
File cc/test/pixel_comparator.cc (right):

https://codereview.chromium.org/371863002/diff/60001/cc/test/pixel_comparator...
cc/test/pixel_comparator.cc:42: error_bounding_rect.set_x(std::min(x,
error_bounding_rect.x()));
Sorry, I had asked about this earlier and missed asking about it again in the
last patch set, but why not error_bounding_rect.Union(gfx::Rect(x, y, 1, 1))
instead of all this min/max math?

[sohanjg, 2014-07-09 05:25:52]

Can you please take a look,
Thanks.

https://codereview.chromium.org/371863002/diff/60001/cc/test/pixel_comparator.cc
File cc/test/pixel_comparator.cc (right):

https://codereview.chromium.org/371863002/diff/60001/cc/test/pixel_comparator...
cc/test/pixel_comparator.cc:42: error_bounding_rect.set_x(std::min(x,
error_bounding_rect.x()));
On 2014/07/08 16:46:47, danakj wrote:
> Sorry, I had asked about this earlier and missed asking about it again in the
> last patch set, but why not error_bounding_rect.Union(gfx::Rect(x, y, 1, 1))
> instead of all this min/max math?

Done.
Sorry i missed it, this essentially was doing the same thing that a union does.

[danakj, 2014-07-09 16:05:56]

Thanks this is great LGTM

[danakj, 2014-07-09 16:05:58]

The CQ bit was checked by danakj@chromium.org

[commit-bot: I haz the power, 2014-07-09 16:07:10]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/sohan.jyoti@samsung.com/371863002/80001

[commit-bot: I haz the power, 2014-07-09 21:23:49]

Change committed as 282127

[vmpstr, 2014-07-11 02:57:04]

https://codereview.chromium.org/371863002/diff/80001/cc/test/pixel_test_utils.cc
File cc/test/pixel_test_utils.cc (right):

https://codereview.chromium.org/371863002/diff/80001/cc/test/pixel_test_utils...
cc/test/pixel_test_utils.cc:77: LOG(ERROR) << "Pixels do not match!";
Is this meant to be printed only if comparator.Compare(gen_bmp, ref_bmp) returns
false?

[sohanjg, 2014-07-11 05:15:21]

https://codereview.chromium.org/371863002/diff/80001/cc/test/pixel_test_utils.cc
File cc/test/pixel_test_utils.cc (right):

https://codereview.chromium.org/371863002/diff/80001/cc/test/pixel_test_utils...
cc/test/pixel_test_utils.cc:77: LOG(ERROR) << "Pixels do not match!";
On 2014/07/11 02:57:04, vmpstr wrote:
> Is this meant to be printed only if comparator.Compare(gen_bmp, ref_bmp)
returns
> false?

Yeah, right! its better to log it based on Compare result.
Will update, thanks!