Revert border radius clips (r95239) for Chromium due to performance issues.
Created attachment 110517 [details]
Comment on attachment 110517 [details]
Attachment 110517 [details] did not pass chromium-ews (chromium-xvfb):
New failing tests:
Created attachment 110524 [details]
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]
View in context: https://bugs.webkit.org/attachment.cgi?id=110524&action=review
> +#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?
// FIXME: blah blah Chromium blah https://bugs.webkit.org/show_bug.cgi?id=69844
at the top of RenderLayer.cpp?
Created attachment 110563 [details]
Oh, absolutely, that is so much better. Attached.
Comment on attachment 110563 [details]
View in context: https://bugs.webkit.org/attachment.cgi?id=110563&action=review
> +// 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]
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]
View in context: https://bugs.webkit.org/attachment.cgi?id=110571&action=review
> + No new tests. (OOPS!)
Created attachment 110857 [details]
Comment on attachment 110857 [details]
Clearing flags on attachment: 110857
Committed r97386: <http://trac.webkit.org/changeset/97386>
All reviewed patches have been landed. Closing bug.