Bug 69844

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.