Bug 69844 – Disable border radius clips (r95239) for Chromium due to performance issues.

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Tom Hudson]

Revert border radius clips (r95239) for Chromium due to performance issues.

[Tom Hudson]

Created attachment 110517 [details]
Patch

[WebKit Review Bot]

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

[Tom Hudson]

Created attachment 110524 [details]
Patch

[Tom Hudson]

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.

[Simon Fraser (smfr)]

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.

[Tom Hudson]

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?

[Simon Fraser (smfr)]

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?

[Tom Hudson]

Created attachment 110563 [details]
Patch

[Tom Hudson]

Oh, absolutely, that is so much better. Attached.

[Simon Fraser (smfr)]

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.

[Tom Hudson]

Created attachment 110571 [details]
Patch

[Tom Hudson]

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

[Simon Fraser (smfr)]

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.

[Tom Hudson]

Created attachment 110857 [details]
Patch

[WebKit Review Bot]

Comment on attachment 110857 [details]
Patch

Clearing flags on attachment: 110857

Committed r97386: <http://trac.webkit.org/changeset/97386>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.