[Gtk][LayoutTests] Repaint the complete WebKitWebView before dumping pixel results

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Zan Dobersek]

Currently some reftests fail (some consistently, some randomly) because of changes being made that have a visual impact are not immediately visible. In some tests, after these changes are committed, layoutTestController.notifyDone() is called, moving the test process straight do dumping the results, not giving the web view enough time to update these changes.

[Zan Dobersek]

Created attachment 141557 [details]
Patch

[Martin Robinson]

The harness already spins the main loop for a bit before dumping. Theoretically this should  cause the WebView to repaint if there are any pending paints. I'm worried that this is hiding a real bug by forcing the repaint.

[Zan Dobersek]

The problem here - I think - is the ChromeClient::paint. It paints into the backing store at around 60 frames per second which sometimes results in changes that were made not being visually visible in the pixel output.

For example, in the fast/css/tab-size.html reftest, the last part of that test[1] sets the tab size to 2 and the immediately calls notifyDone[2]. In the dump()[3] method the main loop only seems to be spun if dumping the render tree. But spinning the loop while events are pending even if calling gtk_widget_queue_draw before that doesn't help with the main problem, and to my understanding it's because calling that Gtk method only sends an expose event which is handled in webkit_web_view_expose_event if using the gtk2 API. If calling gtk_widget_draw on the WebView (which is actually how the pixel results are gathered), the process is similar - in webkit_web_view_draw we draw whatever is in the backing store into the passed-in Cairo context.

But the problem is that that backing store is not up-to-date - there are possibly visible changes pending for the timer to fire so the dirty rects can be updated in the backing store.

1: http://trac.webkit.org/browser/trunk/LayoutTests/fast/css/tab-size.html#L33
2: http://trac.webkit.org/browser/trunk/Tools/DumpRenderTree/gtk/LayoutTestControllerGtk.cpp#L185
3: http://trac.webkit.org/browser/trunk/Tools/DumpRenderTree/gtk/DumpRenderTree.cpp#L509

[Martin Robinson]

(In reply to comment #3)
> The problem here - I think - is the ChromeClient::paint. It paints into the backing store at around 60 frames per second which sometimes results in changes that were made not being visually visible in the pixel output.

I think I understand what you're getting at now. Perhaps the solution  is to disable the repaint throttle for the layout tests? Can you confirm that just removing the throttle manually fixes all the issues?

[Zan Dobersek]

(In reply to comment #4)
> (In reply to comment #3)
> > The problem here - I think - is the ChromeClient::paint. It paints into the backing store at around 60 frames per second which sometimes results in changes that were made not being visually visible in the pixel output.
> 
> I think I understand what you're getting at now. Perhaps the solution  is to disable the repaint throttle for the layout tests? Can you confirm that just removing the throttle manually fixes all the issues?

No, unfortunately it doesn't. Completely removing the throttle and calling ChromeClient::paint whenever there's nothing more important on the loop (i.e. calling m_displayTimer.startOneShot(0) at the end of ChromeClient::paint) just hogs the DumpRenderTree program.

[Martin Robinson]

(In reply to comment #5)

> No, unfortunately it doesn't. Completely removing the throttle and calling ChromeClient::paint whenever there's nothing more important on the loop (i.e. calling m_displayTimer.startOneShot(0) at the end of ChromeClient::paint) just hogs the DumpRenderTree program.

I'm not sure that you'd want to call m_displayTimer.startOneShot(0) ever from ChromeClient::paint. That seems like it would indeed starve the main loop, because relayouts / repaints are expensive. Instead maybe just removing the repaint throttle and spinning the main loop for reftests is enough. Perhaps I'm misunderstanding this whole situation though.

[Zan Dobersek]

(In reply to comment #6)
> I'm not sure that you'd want to call m_displayTimer.startOneShot(0) ever from ChromeClient::paint. That seems like it would indeed starve the main loop, because relayouts / repaints are expensive. Instead maybe just removing the repaint throttle and spinning the main loop for reftests is enough. Perhaps I'm misunderstanding this whole situation though.

No, just removing the repaint throttle wouldn't help either because ChromeClient::paint would still have to be called between LayoutTestController::notifyDone() and gtk_widget_draw() in PixelDumpSupportGtk.cpp to get the backing store updated.

[Zan Dobersek]

I've investigated a bit more why the call to ChromeClient::paint (i.e. the display timer being fired) won't occur when spinning the main loop. It turns out that because of some odd and unknown reason the shared timer for the main thread gets stopped, not firing WebCore timers anymore. As far as it is annoying I don't think it should be an obstacle.

I'm again leaning towards doing a forced paint before dumping the pixel results, but this time without updating the complete backing store but only the regions that were marked dirty until that moment. Given that in WebCore visual changes are synchronously producing dirty regions which are to be repainted, I don't think there would be any problem in that.

Another problem I've encountered is that probably the preferred way of either solution would be done through DumpRenderTreeSupport class, but including ChromeClientGtk.h in DumpRenderTreeSupportGtk.cpp causes name conflicts as it seems that xlib headers are included via the inclusion of FrameLoaderClientGtk.h. That's unfortunate and also the reason the current patch implements this through a private function in webkitwebview.cpp.

[Martin Robinson]

Thanks for looking at this. From what I understand, Alex was/is writing a patch that replaces the shared timer with a higher priority GLib timer. Perhaps that would solve most of the problems here. 

Take a look at my patch here: https://bugs.webkit.org/attachment.cgi?id=141086&action=prettypatch for how to deal with the name conflicts. Specifically search for "#undef"

[Zan Dobersek]

Created attachment 141747 [details]
Patch

[Zan Dobersek]

(In reply to comment #10)
> Created an attachment (id=141747) [details]
> Patch

Bypasses the compilation issues by removing the use of WebKit::FrameLoaderClient and using the kit() function instead (which uses the WebKit::FrameLoaderClient in the same way itself).

Otherwise this is still pushing for the instant and forced repaint, but now only of the regions that are marked dirty, not the complete backing store.

[Martin Robinson]

Comment on attachment 141747 [details]
Patch

Okay. Let's give this a try.

[Zan Dobersek]

Comment on attachment 141747 [details]
Patch

Clearing flags on attachment: 141747

Committed r117947: <http://trac.webkit.org/changeset/117947>

[Zan Dobersek]

(In reply to comment #13)
> (From update of attachment 141747 [details])
> Clearing flags on attachment: 141747
> 
> Committed r117947: <http://trac.webkit.org/changeset/117947>

I'll wait for the bots to process this commit to see if there are any regressions. After that I'll remove expectations for tests that now hopefully pass. If that goes well I'll close this bug.

[Dominik Röttsches (drott)]

Zan, there are some related cases in bug 73409. Do you have these?

I am not sure these reftest are designed 100% correctly. They work like repaint tests in fact. And for repaint tests Nikolas developed a harness to force a layout and call "display()". When we add this harness to those reftests (at least the ones mentioned in 73409, it worked as well.

[Zan Dobersek]

(In reply to comment #15)
> Zan, there are some related cases in bug 73409. Do you have these?
> 
> I am not sure these reftest are designed 100% correctly. They work like repaint tests in fact. And for repaint tests Nikolas developed a harness to force a layout and call "display()". When we add this harness to those reftests (at least the ones mentioned in 73409, it worked as well.

Two of those three tests pass now, I'll remove their test expectations. Still failing is fast/forms/textarea-placeholder-set-attribute.html. Loading it manually the 'Placeholder' text at first is not visible but that changes when I hover over the textarea. This seems as another, unrelated bug.

[Zan Dobersek]

Committed r117983: <http://trac.webkit.org/changeset/117983>

[Zan Dobersek]

... and closing.