Summary: | Disable border radius clips (r95239) for Chromium due to performance issues. | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Tom Hudson <tomhudson> | ||||||||||||
Component: | New Bugs | Assignee: | Tom Hudson <tomhudson> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | dglazkov, hyatt, mitz, simon.fraser, tomhudson, webkit.review.bot | ||||||||||||
Priority: | P2 | ||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||
Hardware: | Unspecified | ||||||||||||||
OS: | Unspecified | ||||||||||||||
Attachments: |
|
Description
Tom Hudson
2011-10-11 08:41:50 PDT
Created attachment 110517 [details]
Patch
Comment on attachment 110517 [details] Patch Attachment 110517 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/10029549 New failing tests: fast/clip/overflow-border-radius-composited.html fast/clip/overflow-border-radius-fixed-position.html fast/clip/overflow-border-radius-transformed.html fast/clip/overflow-border-radius-combinations.html Created attachment 110524 [details]
Patch
Performance issues are discussed at https://bugs.webkit.org/show_bug.cgi?id=68733 and http://code.google.com/p/chromium/issues/detail?id=97716. Layout test failures were expected; they were added to test border radius clipping. New patch adds a test_expectations change. Comment on attachment 110524 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=110524&action=review > Source/WebCore/rendering/RenderLayer.cpp:2588 > +#if !PLATFORM(CHROMIUM) > // If the clip rect has been tainted by a border radius, then we have to walk up our layer chain applying the clips from > // any layers with overflow. The condition for being able to apply these clips is that the overflow object be in our > // containing block chain so we check that also. Can't you just #ifdef out the one line that sets the taint bit? We really dislike seemingly arbitrary platform #ifdefs. It's impossible to tell, by reading the code, that this was disabled for perf reasons. It would be much better to work on a solution to the performance issue. hasBorderRadius() is used many places in the renderer; faking the value of that seems much more dangerous to me. This (arbitrary platform #ifdef) is the approach I thought Dave Hyatt was suggesting as a stopgap measure in https://bugs.webkit.org/show_bug.cgi?id=68733. His change to add the new feature caused performance regressions - as I understand it mild for Safari, but cripplingly severe for Chrome. We're working on addressing the underlying performance issues, but the short term approach he suggested in https://bugs.webkit.org/show_bug.cgi?id=68733 has (1) proved nontrivial for me to implement and (2) is high-risk; we expect to find web pages it still performs poorly on. A long term solution to the underlying performance issues ran into multiple Windows kernel/GDI-related issues and I expect to be weeks away. Chromium needs some solution in a shorter timeframe. The WebKit coding standards suggest using FIXME; would a "FIXME: triggers unacceptably slow soft-clipping path on Chromium (WebKit bug 68733)" be better documentation? How about: #if PLATFORM(CHROMIUM) // FIXME: blah blah Chromium blah https://bugs.webkit.org/show_bug.cgi?id=69844 #define DISABLE_ROUNDED_CORNER_CLIPPING #endif at the top of RenderLayer.cpp? Created attachment 110563 [details]
Patch
Oh, absolutely, that is so much better. Attached. Comment on attachment 110563 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=110563&action=review > Source/WebCore/rendering/RenderLayer.cpp:103 > +// https://bugs.webkit.org/show_bug.cgi?id=69844 Actually you should reference a bug that stays open until the hack can be removed. Created attachment 110571 [details]
Patch
That was sloppy on my part - I had meant to reference 68733, but on reflection it's probably better to create a new bug, since 68733 might not be sufficient. https://bugs.webkit.org/show_bug.cgi?id=69866 Comment on attachment 110571 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=110571&action=review > Source/WebCore/ChangeLog:8 > + No new tests. (OOPS!) Remove this. Created attachment 110857 [details]
Patch
Comment on attachment 110857 [details] Patch Clearing flags on attachment: 110857 Committed r97386: <http://trac.webkit.org/changeset/97386> All reviewed patches have been landed. Closing bug. |