Bug 69844

Summary: Disable border radius clips (r95239) for Chromium due to performance issues.
Product: WebKit Reporter: Tom Hudson <tomhudson>
Component: New BugsAssignee: 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 Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Tom Hudson 2011-10-11 08:41:50 PDT
Revert border radius clips (r95239) for Chromium due to performance issues.
Comment 1 Tom Hudson 2011-10-11 08:45:41 PDT
Created attachment 110517 [details]
Patch
Comment 2 WebKit Review Bot 2011-10-11 09:12:32 PDT
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
Comment 3 Tom Hudson 2011-10-11 09:27:48 PDT
Created attachment 110524 [details]
Patch
Comment 4 Tom Hudson 2011-10-11 09:29:22 PDT
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 5 Simon Fraser (smfr) 2011-10-11 12:02:05 PDT
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.
Comment 6 Tom Hudson 2011-10-11 12:25:54 PDT
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?
Comment 7 Simon Fraser (smfr) 2011-10-11 12:39:08 PDT
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?
Comment 8 Tom Hudson 2011-10-11 13:26:41 PDT
Created attachment 110563 [details]
Patch
Comment 9 Tom Hudson 2011-10-11 13:27:40 PDT
Oh, absolutely, that is so much better. Attached.
Comment 10 Simon Fraser (smfr) 2011-10-11 13:37:35 PDT
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.
Comment 11 Tom Hudson 2011-10-11 14:04:53 PDT
Created attachment 110571 [details]
Patch
Comment 12 Tom Hudson 2011-10-11 14:05:09 PDT
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 13 Simon Fraser (smfr) 2011-10-11 15:06:32 PDT
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.
Comment 14 Tom Hudson 2011-10-13 09:12:45 PDT
Created attachment 110857 [details]
Patch
Comment 15 WebKit Review Bot 2011-10-13 12:37:03 PDT
Comment on attachment 110857 [details]
Patch

Clearing flags on attachment: 110857

Committed r97386: <http://trac.webkit.org/changeset/97386>
Comment 16 WebKit Review Bot 2011-10-13 12:37:09 PDT
All reviewed patches have been landed.  Closing bug.