Bug 5195 - drawing with cacheDisplayInRect:toBitmapImageRep: doesn't trigger layout on Leopard
Summary: drawing with cacheDisplayInRect:toBitmapImageRep: doesn't trigger layout on L...
Status: REOPENED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 420+
Hardware: Mac OS X 10.4
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2005-09-30 01:48 PDT by Duncan Wilcox
Modified: 2010-06-10 16:48 PDT (History)
3 users (show)

See Also:


Attachments
patch (2.04 KB, patch)
2005-09-30 02:05 PDT, Duncan Wilcox
darin: review-
Details | Formatted Diff | Diff
test case (2.80 KB, text/plain)
2005-09-30 02:07 PDT, Duncan Wilcox
no flags Details
patch (1.75 KB, patch)
2005-09-30 13:32 PDT, Duncan Wilcox
darin: review-
Details | Formatted Diff | Diff
patch doing what Duncan and I discussed in email (2.18 KB, patch)
2005-10-04 13:26 PDT, Darin Adler
darin: review-
Details | Formatted Diff | Diff
Tweaked test case (2.50 KB, text/plain)
2005-10-05 03:41 PDT, Duncan Wilcox
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Duncan Wilcox 2005-09-30 01:48:28 PDT
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.
Comment 1 Duncan Wilcox 2005-09-30 02:05:13 PDT
Created attachment 4102 [details]
patch
Comment 2 Duncan Wilcox 2005-09-30 02:07:38 PDT
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
Comment 3 Darin Adler 2005-09-30 07:42:39 PDT
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.
Comment 4 Darin Adler 2005-09-30 07:44:00 PDT
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.
Comment 5 Duncan Wilcox 2005-09-30 13:32:02 PDT
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.
Comment 6 Darin Adler 2005-10-02 20:32:35 PDT
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.
Comment 7 Darin Adler 2005-10-04 13:26:31 PDT
Created attachment 4199 [details]
patch doing what Duncan and I discussed in email
Comment 8 Duncan Wilcox 2005-10-05 03:41:41 PDT
Created attachment 4210 [details]
Tweaked test case

This slightly tweaked testcase demonstrates that Darin's latest patch fixes the
problem.
Comment 9 Darin Adler 2005-10-05 10:05:27 PDT
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.
Comment 10 Maciej Stachowiak 2005-10-09 16:35:14 PDT
Comment on attachment 4199 [details]
patch doing what Duncan and I discussed in email

r=me
Comment 11 Maciej Stachowiak 2005-10-09 17:01:02 PDT
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.
Comment 12 Adele Peterson 2005-10-11 12:08:11 PDT
This fix caused http://bugzilla.opendarwin.org/show_bug.cgi?id=5335
Comment 13 mitz 2005-10-11 13:06:07 PDT
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.
Comment 14 Darin Adler 2005-10-12 07:35:29 PDT
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.
Comment 15 Darin Adler 2007-10-04 19:47:38 PDT
I suspect this is fixed in Leopard.
Comment 16 Darin Adler 2008-03-14 11:57:26 PDT
For those with access to Radar, <rdar://problem/5668489> is related Radar bug.
Comment 17 mitz 2008-07-14 17:18:47 PDT
(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.
Comment 18 Duncan Wilcox 2008-07-14 23:46:44 PDT
I just tried the testcase on TOT on 10.5.4 and it is still not fixed.
Comment 19 John Sullivan 2008-07-31 10:38:12 PDT
This was fixed on Tiger a while back, but still not fixed on Leopard. I have a fix for Leopard.
Comment 20 John Sullivan 2008-07-31 10:50:55 PDT
Committed revision 35488.
Comment 21 mitz 2008-08-08 20:41:12 PDT
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.