Bug 87658 – [Gtk] Add support in DumpRenderTree for tracking repaints

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Zan Dobersek]

Gtk's DumpRenderTree lacks proper support for tracking repaints. This would make repaint tests function properly, but would also require (re)generated pixel results for these tests along with pixel tests actually being run on at least some of the builders.

[Zan Dobersek]

(In reply to comment #0)
> Gtk's DumpRenderTree lacks proper support for tracking repaints. This would make repaint tests function properly, but would also require (re)generated pixel results for these tests along with pixel tests actually being run on at least some of the builders.

To clarify, regenerating pixel results should be done when the decision to run pixel tests on the builders is made. I don't think there are any brave men out there running pixel tests on a regular basis.

[Zan Dobersek]

Created attachment 144350 [details]
Patch

[Martin Robinson]

(In reply to comment #1)
 
> To clarify, regenerating pixel results should be done when the decision to run pixel tests on the builders is made. I don't think there are any brave men out there running pixel tests on a regular basis.

I actually do update the results in cases where it makes sense to verify that a particular change was a progression. Perhaps you could include repaint results in your patch?

[Martin Robinson]

Comment on attachment 144350 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=144350&action=review

Looks good though I think it makes sense to include pixel results with this patch. r- only because I'd like to take a look at the pixel results to manually verify the correctness.

> Tools/DumpRenderTree/gtk/PixelDumpSupportGtk.cpp:41
> +    cairo_set_source_rgba(context, 0.0, 0.0, 0.0, 0.66);

It makes sense to include a comment about where these numbers come from.

> Tools/DumpRenderTree/gtk/PixelDumpSupportGtk.cpp:90
> +        cairo_surface_t* overlaySurface = cairo_image_surface_create(CAIRO_FORMAT_ARGB32, width, height);
> +        paintOverlay(overlaySurface);
> +
> +        cairo_set_source_surface(context, overlaySurface, 0, 0);
> +        cairo_rectangle(context, 0, 0, width, height);
> +        cairo_fill(context);
> +
> +        cairo_surface_destroy(overlaySurface);

I think it'd be nice to split out this code into another helper as well.

[Zan Dobersek]

(In reply to comment #3)
> (In reply to comment #1)
> 
> > To clarify, regenerating pixel results should be done when the decision to run pixel tests on the builders is made. I don't think there are any brave men out there running pixel tests on a regular basis.
> 
> I actually do update the results in cases where it makes sense to verify that a particular change was a progression. Perhaps you could include repaint results in your patch?

A habit that makes sense. I'll upload a proper patch. I'd start how we should update and run pixel tests, but I'll leave this for someone else to pick it up or for some time later ...

[Zan Dobersek]

Created attachment 145257 [details]
Patch

Here's a patch that also contains pixel rebaselines for the tests in fast/repaint. Note that there might be some tests of which the new baselines are not affected by enabling the track repainting and are just being updated to their latest status.

[Martin Robinson]

(In reply to comment #6)
> Created an attachment (id=145257) [details]
> Patch
> 
> Here's a patch that also contains pixel rebaselines for the tests in fast/repaint. Note that there might be some tests of which the new baselines are not affected by enabling the track repainting and are just being updated to their latest status.

If I'm not mistaken the tests in fast/repaint should show a gray overlay over top the areas that were not repainted. Is this still the case? If so, I guess this means that some of the tests repainted the entire view?

[Andrei Bucur]

Any updates for this patch? I'd love to experiment the work for repaint rectangles test type on the GTK port  (see the "Using ref tests for repaint bugs" thread on webkit-dev).

Thx!

[Zan Dobersek]

(In reply to comment #7)
> (In reply to comment #6)
> > Created an attachment (id=145257) [details] [details]
> > Patch
> > 
> > Here's a patch that also contains pixel rebaselines for the tests in fast/repaint. Note that there might be some tests of which the new baselines are not affected by enabling the track repainting and are just being updated to their latest status.
> 
> If I'm not mistaken the tests in fast/repaint should show a gray overlay over top the areas that were not repainted. Is this still the case? If so, I guess this means that some of the tests repainted the entire view?

I don't think all the tests in fast/repaint operate like this. If it's OK I'll go ahead and just update pixel results only for these tests. It would be easier to review the changes and also compare to other ports' pixel results.

[Zan Dobersek]

Created attachment 146225 [details]
Patch

Here's a patch updating only pixel baselines for tests that use the repaint.js harness. Omitted are baselines that differ too much from the Mac port's baselines, they might be indicating bugs.

[Martin Robinson]

(In reply to comment #10)
> Created an attachment (id=146225) [details]
> Patch
> 
> Here's a patch updating only pixel baselines for tests that use the repaint.js harness. Omitted are baselines that differ too much from the Mac port's baselines, they might be indicating bugs.

I think it's useful to check those in as well. That way one simply has to look at the repository to see the state of GTK+ rendering.

[Martin Robinson]

Comment on attachment 146225 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=146225&action=review

When landing please update all pixel results in the repaint directory.

> Tools/DumpRenderTree/gtk/PixelDumpSupportGtk.cpp:44
> +    cairo_set_source_rgba(context, 0.0, 0.0, 0.0, 0.66);

Just use 0 here instead of 0.0, as that's the WebKit style.

[Zan Dobersek]

(In reply to comment #11)
> (In reply to comment #10)
> > Created an attachment (id=146225) [details] [details]
> > Patch
> > 
> > Here's a patch updating only pixel baselines for tests that use the repaint.js harness. Omitted are baselines that differ too much from the Mac port's baselines, they might be indicating bugs.
> 
> I think it's useful to check those in as well. That way one simply has to look at the repository to see the state of GTK+ rendering.

I'll generate all the pixel baselines later this week. I'll note somewhere all the tests with baselines that significantly differ from the Mac baselines - they're probably bugs. It might be even better to use the Mac baseline for these tests instead and add an IMAGE test expectation, filing bug entries for each, just so the bug is marked as noted and logged.

[Martin Robinson]

(In reply to comment #13)
> I'll generate all the pixel baselines later this week. I'll note somewhere all the tests with baselines that significantly differ from the Mac baselines - they're probably bugs. It might be even better to use the Mac baseline for these tests instead and add an IMAGE test expectation, filing bug entries for each, just so the bug is marked as noted and logged.

The big benefit of pixel results is that, even if they show a bug, you can detect when a change you made locally is causing a regression. It's always possible to see if a result is correct or not by cross-referencing it with the results of other platforms and by running the test in other browsers.

[Zan Dobersek]

Committed r120350: <http://trac.webkit.org/changeset/120350>

[Zan Dobersek]

Here's the promised list of tests of which baselines differ significantly when compared to Mac's baselines (in some cases baselines for Chromium Mac were used). Besides each is also a short description of what exactly seems to be incorrect.

https://docs.google.com/spreadsheet/ccc?key=0AvP3bJqGv0uXdEN0cmh3X3F2dHlkZzBoc050Zkg1ZFE#gid=0