Bug 87658 - [Gtk] Add support in DumpRenderTree for tracking repaints
Summary: [Gtk] Add support in DumpRenderTree for tracking repaints
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Zan Dobersek
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-05-28 06:38 PDT by Zan Dobersek
Modified: 2012-06-14 12:57 PDT (History)
2 users (show)

See Also:


Attachments
Patch (8.32 KB, patch)
2012-05-28 06:50 PDT, Zan Dobersek
no flags Details | Formatted Diff | Diff
Patch (65.74 KB, patch)
2012-06-01 03:12 PDT, Zan Dobersek
no flags Details | Formatted Diff | Diff
Patch (51.84 KB, patch)
2012-06-07 00:58 PDT, Zan Dobersek
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Zan Dobersek 2012-05-28 06:38:36 PDT
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.
Comment 1 Zan Dobersek 2012-05-28 06:41:56 PDT
(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.
Comment 2 Zan Dobersek 2012-05-28 06:50:43 PDT
Created attachment 144350 [details]
Patch
Comment 3 Martin Robinson 2012-05-29 08:28:23 PDT
(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?
Comment 4 Martin Robinson 2012-05-29 08:33:21 PDT
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.
Comment 5 Zan Dobersek 2012-05-29 09:16:44 PDT
(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 ...
Comment 6 Zan Dobersek 2012-06-01 03:12:59 PDT
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.
Comment 7 Martin Robinson 2012-06-01 09:02:38 PDT
(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?
Comment 8 Andrei Bucur 2012-06-06 06:49:27 PDT
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!
Comment 9 Zan Dobersek 2012-06-06 06:55:01 PDT
(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.
Comment 10 Zan Dobersek 2012-06-07 00:58:40 PDT
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.
Comment 11 Martin Robinson 2012-06-07 15:06:56 PDT
(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.
Comment 12 Martin Robinson 2012-06-08 15:53:26 PDT
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.
Comment 13 Zan Dobersek 2012-06-10 11:29:55 PDT
(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.
Comment 14 Martin Robinson 2012-06-10 16:49:10 PDT
(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.
Comment 15 Zan Dobersek 2012-06-14 12:53:09 PDT
Committed r120350: <http://trac.webkit.org/changeset/120350>
Comment 16 Zan Dobersek 2012-06-14 12:57:10 PDT
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