Summary: | drawing with cacheDisplayInRect:toBitmapImageRep: doesn't trigger layout on Leopard | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Duncan Wilcox <duncan> | ||||||||||||
Component: | DOM | Assignee: | Nobody <webkit-unassigned> | ||||||||||||
Status: | RESOLVED CONFIGURATION CHANGED | ||||||||||||||
Severity: | Normal | CC: | ahmad.saleem792, ap, bfulgham, emacemac7, ian, mitz, rniwa, webkit-bug-importer | ||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||
Version: | 420+ | ||||||||||||||
Hardware: | Mac | ||||||||||||||
OS: | OS X 10.4 | ||||||||||||||
Attachments: |
|
Description
Duncan Wilcox
2005-09-30 01:48:28 PDT
Created attachment 4102 [details]
patch
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 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.
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. 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 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.
Created attachment 4199 [details]
patch doing what Duncan and I discussed in email
Created attachment 4210 [details]
Tweaked test case
This slightly tweaked testcase demonstrates that Darin's latest patch fixes the
problem.
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 on attachment 4199 [details]
patch doing what Duncan and I discussed in email
r=me
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.
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. 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.
I suspect this is fixed in Leopard. 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. I just tried the testcase on TOT on 10.5.4 and it is still not fixed. This was fixed on Tiger a while back, but still not fixed on Leopard. I have a fix for Leopard. 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. ap@webkit.org and rniwa@webkit.org - 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 We've re-rewriten code around this by now. |