Bug 5195 – drawing with cacheDisplayInRect:toBitmapImageRep: doesn't trigger layout on Leopard

• Component: DOM
• Status: RESOLVED CONFIGURATION CHANGED

[Duncan Wilcox]

Updates to the DOM are rendered after 250ms:

WebCore/khtml/xml/dom_docimpl.cpp:106:const int cLayoutScheduleThreshold = 250;

When a WebView is used for the purpose of making screenshots (or perhaps even when less latency is 
desired), and the caller is manipulating the DOM and it knows that there are no more updates, a way to 
explicitly force the rendering of such changes would be desirable.

The attached patch adds a "forceLayout" method to WebView, that flushes rendering by calling the 
underlying view's layout method.

The attached DumpRenderTree-derived source demonstrates how a bitmap grabbed from a WebView is 
white without the forced layout, but is correct when the new method is called.

[Duncan Wilcox]

Created attachment 4102 [details]
patch

[Duncan Wilcox]

Created attachment 4103 [details]
test case

Build with something like:

gcc -g -o dump dump.m -F<path to webkit build dir> -framework WebKit -framework
WebCore -framework JavaScriptCore -framework Cocoa

run with something like:

DYLD_FRAMEWORK_PATH=<path to webkit build dir> ./dump

[Darin Adler]

Comment on attachment 4102 [details]
patch

I don't think we want this. We do want to perform layout, bypassing the timer,
but as a direct response to another method call that requires layout.

For example, if there's a call to render the view into a buffer, then that call
should force layout.

We should only add a call like this to API if there is no way to accomplish
this automatically, and I believe there is.

[Darin Adler]

Now having looked at this test case, I think that we have to figure out how cacheDisplayInRect will force 
layout; I think there's code that tries to do it and is buggy.

[Duncan Wilcox]

Created attachment 4119 [details]
patch

cacheDisplayInRect:toBitmapImageRep: doesn't trigger any notification, so I
override it in WebView and force a layout before calling the super method.

[Darin Adler]

Comment on attachment 4119 [details]
patch

I still don't think this is quite right. We should not be overriding this
particular method, but rather the code path it follows that requires layout. In
particular, we want this to work properly even if you make the call directly on
the WebHTMLView.

Is there really a need to support repainting without relayout? I don't think
there is. Any painting should presumably create layout. The comment on the
ChangeLog indicates an idea that somehow we would paint but still wait for a
layout scheduling threshold; I think that's wrong.

[Darin Adler]

Created attachment 4199 [details]
patch doing what Duncan and I discussed in email

[Duncan Wilcox]

Created attachment 4210 [details]
Tweaked test case

This slightly tweaked testcase demonstrates that Darin's latest patch fixes the
problem.

[Darin Adler]

Comment on attachment 4199 [details]
patch doing what Duncan and I discussed in email

Since Duncan confirms this code fixes the problem, lets get it reviewed and
checked in.

[Maciej Stachowiak]

Comment on attachment 4199 [details]
patch doing what Duncan and I discussed in email

r=me

[Maciej Stachowiak]

Comment on attachment 4199 [details]
patch doing what Duncan and I discussed in email

Eric points out that this patch violats the style guidelines - single-line if
blocks are put in braces and they should not be. Please fix before landing.

[Adele Peterson]

This fix caused http://bugzilla.opendarwin.org/show_bug.cgi?id=5335

More accurately, the WebHTMLView patch attached to this bug resulted in subviews (such as form widgets) 
not being rendered in the pixel tests. The additional patch that fixed that (which is not in bugzilla) is what 
caused bug 5335.

[Darin Adler]

Comment on attachment 4199 [details]
patch doing what Duncan and I discussed in email

I'll need to come up with a version of this that doesn't break drawing controls
in the layout tests or drawing text field insertion points.

[Darin Adler]

I suspect this is fixed in Leopard.

[Darin Adler]

For those with access to Radar, <rdar://problem/5668489> is related Radar bug.

(In reply to comment #15)
> I suspect this is fixed in Leopard.

And I think <http://trac.webkit.org/changeset/35176> fixed this for Tiger.

[Duncan Wilcox]

I just tried the testcase on TOT on 10.5.4 and it is still not fixed.

[John Sullivan]

This was fixed on Tiger a while back, but still not fixed on Leopard. I have a fix for Leopard.

[John Sullivan]

Committed revision 35488.

Unfortunately the Leopard fix had to be rolled out in <http://trac.webkit.org/changeset/35650>. I am re-opening the bug, but I am not sure there is a way to fix it in Leopard.

[Ahmad Saleem]

[e-mail] and [e-mail] - Is this needed any more?

I can only found one reference about "cacheDisplayInRect":

https://github.com/WebKit/WebKit/blob/a742d4b71e27e2f1e82e90c3b26150aa0366fdcd/Tools/DumpRenderTree/mac/DumpRenderTree.mm#L801

[Ryosuke Niwa]

We've re-rewriten code around this by now.

[Radar WebKit Bug Importer]

<rdar://problem/98392152>