Overlay expected and actual repaint rects for LayoutTests.

Overlay expected and actual repaint rects for LayoutTests.

For the new world of text based repaint tests it can be hard
to visualize how the repaint rects appear on the screen. This
is a first pass at visualizing the difference between the old
and new repaint rects when one of the text based repaint tests
fail.

BUG=

[dsinclair, 2014-02-21 01:54:06]

esprehn, PTAL.

Talking to jamesr@ he mentioned that you maybe interested in this. I wanted to
be able to visualize the difference between the old and new repaint rects with
the text based tests.

This will generate a <test>-overlay.html file for the text based tests that
failed. This will, using the file name passed in the query string, iFrame in the
test content then overlay the difference between the old and new rects on top.

There is one issue with this in that, this won't work correctly on the bots,
currently. When the test framework decides that the path to the test file is in
svn things go a bit wonky.

Does this line up with what you were thinking in this area? Is this worth
polishing off and trying to land?

[esprehn, 2014-02-21 02:00:58]

Yeah this is what I was hoping for, lets finish this up and land it. It's fine
if it doesn't work on the bots to start, it's no worse than what we have today.

https://codereview.chromium.org/174073002/diff/1/Tools/Scripts/webkitpy/layou...
File Tools/Scripts/webkitpy/layout_tests/controllers/test_result_writer.py
(right):

https://codereview.chromium.org/174073002/diff/1/Tools/Scripts/webkitpy/layou...
Tools/Scripts/webkitpy/layout_tests/controllers/test_result_writer.py:274: var
overlay = document.querySelector('.overlay');
Use an id and getElementById

https://codereview.chromium.org/174073002/diff/1/Tools/Scripts/webkitpy/layou...
Tools/Scripts/webkitpy/layout_tests/controllers/test_result_writer.py:291: var
expected_canvas = document.querySelector('#expected')
same

https://codereview.chromium.org/174073002/diff/1/Tools/Scripts/webkitpy/layou...
Tools/Scripts/webkitpy/layout_tests/controllers/test_result_writer.py:311: var
type = document.querySelector('#type');
getElementById?

[dsinclair, 2014-02-21 02:24:55]

https://codereview.chromium.org/174073002/diff/1/Tools/Scripts/webkitpy/layou...
File Tools/Scripts/webkitpy/layout_tests/controllers/test_result_writer.py
(right):

https://codereview.chromium.org/174073002/diff/1/Tools/Scripts/webkitpy/layou...
Tools/Scripts/webkitpy/layout_tests/controllers/test_result_writer.py:274: var
overlay = document.querySelector('.overlay');
On 2014/02/21 02:00:59, esprehn wrote:
> Use an id and getElementById

Done.

https://codereview.chromium.org/174073002/diff/1/Tools/Scripts/webkitpy/layou...
Tools/Scripts/webkitpy/layout_tests/controllers/test_result_writer.py:291: var
expected_canvas = document.querySelector('#expected')
On 2014/02/21 02:00:59, esprehn wrote:
> same

Done.

https://codereview.chromium.org/174073002/diff/1/Tools/Scripts/webkitpy/layou...
Tools/Scripts/webkitpy/layout_tests/controllers/test_result_writer.py:311: var
type = document.querySelector('#type');
On 2014/02/21 02:00:59, esprehn wrote:
> getElementById?

Done.

[dsinclair, 2014-02-24 21:14:08]

ping.

[esprehn, 2014-02-24 22:24:13]

lgtm to me, but I don't think I can approve stuff in Source/Tools, ojan and
dpranke should look at the python change to make sure that stuff matches the
style down there.

[Dirk Pranke, 2014-02-24 23:31:34]

On 2014/02/24 22:24:13, esprehn wrote:
> lgtm to me, but I don't think I can approve stuff in Source/Tools, ojan and
> dpranke should look at the python change to make sure that stuff matches the
> style down there.

I definitely need to go through this ... I talked to esprehn and jamesr a bit
for the context, so I'll try to look at this later today. Normally we'd want at
least some unit tests for this, and it would be good if there was some way to
see what the visual effect of this change will be, but it's probably hard to do
that w/o actually applying the patch locally, so I'll probably end up doing
that.

[ojan, 2014-02-25 00:43:42]

This needs a test. See LayoutTests/fast/harness/resources/results-test.js. It's
just run as a regular layout test.

The idea all looks good to me. Just some minor comments.

https://codereview.chromium.org/174073002/diff/60001/LayoutTests/fast/harness...
File LayoutTests/fast/harness/results.html (right):

https://codereview.chromium.org/174073002/diff/60001/LayoutTests/fast/harness...
LayoutTests/fast/harness/results.html:537: html += resultLink(prefix,
'-overlay.html?' + encodeURIComponent(testLinkTarget(test)), 'overlay');
It's a bit of a bummer to have the (broken) overlay links even for non-repaint
tests. Can we add a bit to the json (e.g. like has_wdiff) for repaint tests?

https://codereview.chromium.org/174073002/diff/60001/Tools/Scripts/webkitpy/l...
File Tools/Scripts/webkitpy/layout_tests/controllers/test_result_writer.py
(right):

https://codereview.chromium.org/174073002/diff/60001/Tools/Scripts/webkitpy/l...
Tools/Scripts/webkitpy/layout_tests/controllers/test_result_writer.py:311:
type.innerText = 'Actual Difference';
Nit: innerText is crazy. Use textContent.

[Dirk Pranke, 2014-02-27 20:57:44]

The general idea, while a bit painful, seems fine.

We need  unit tests for both the python change and the results.html change;
splitting the python change out into a separate file should make the former easy
enough.

I've added a couple of comments on the coding style. I didn't see any outright
bugs, so add the tests and this be lgt'able.

https://codereview.chromium.org/174073002/diff/60001/LayoutTests/fast/harness...
File LayoutTests/fast/harness/results.html (right):

https://codereview.chromium.org/174073002/diff/60001/LayoutTests/fast/harness...
LayoutTests/fast/harness/results.html:537: html += resultLink(prefix,
'-overlay.html?' + encodeURIComponent(testLinkTarget(test)), 'overlay');
On 2014/02/25 00:43:42, ojan wrote:
> It's a bit of a bummer to have the (broken) overlay links even for non-repaint
> tests. Can we add a bit to the json (e.g. like has_wdiff) for repaint tests?

Agreed. I don't want to display a link that is irrelevant.

https://codereview.chromium.org/174073002/diff/60001/Tools/Scripts/webkitpy/l...
File Tools/Scripts/webkitpy/layout_tests/controllers/test_result_writer.py
(right):

https://codereview.chromium.org/174073002/diff/60001/Tools/Scripts/webkitpy/l...
Tools/Scripts/webkitpy/layout_tests/controllers/test_result_writer.py:208: def
create_overlay_invalidation_result(self, actual_text, expected_text):
Can you pull this whole function out of this file and create a new file for it?
I would like to keep it separate from the other logic in this file (we should do
that for write_image_diff_files, as well, but not as part of this change).

https://codereview.chromium.org/174073002/diff/60001/Tools/Scripts/webkitpy/l...
Tools/Scripts/webkitpy/layout_tests/controllers/test_result_writer.py:210:
repaintMatch = repaintPattern.search(expected_text)
this can probably be just re.search('^\(repaint rects$', expected_text).

compilling the re doesn't buy you anything if you're only going to use re once
in the function and recompile it every time.

https://codereview.chromium.org/174073002/diff/60001/Tools/Scripts/webkitpy/l...
Tools/Scripts/webkitpy/layout_tests/controllers/test_result_writer.py:218: for m
in re.finditer(rect_pattern, input_str):
hm. didn't know about finditer(); nifty.

https://codereview.chromium.org/174073002/diff/60001/Tools/Scripts/webkitpy/l...
Tools/Scripts/webkitpy/layout_tests/controllers/test_result_writer.py:227:
return ret + ']'
hm. didn't know about finditer(); nifty.

This can probably be written as 

def make_js_rect(input_str):
    rect_pattern = '\(rect\s+(\d+)\s+(\d+)\s+(\d+)\s+(\d+)\)'
    rects = []
    for m in re.finditer(rect_pattern, input_str):
        rects.append('[' + ','.join(m.groups()) + ']')
    return '[' + ','.join(rects) + ']'

(You could probably do it w/ a nested list comprehension or two, even, but I
wouldn't).

https://codereview.chromium.org/174073002/diff/60001/Tools/Scripts/webkitpy/l...
Tools/Scripts/webkitpy/layout_tests/controllers/test_result_writer.py:232: html
= """<!DOCTYPE HTML>
I defer to ojan re: the html template, but it looks more or less plausible to me
:).