103276 – Fix occlusion culling logic to handle css background layer clipping

RESOLVED FIXED 103276

Fix occlusion culling logic to handle css background layer clipping

https://bugs.webkit.org/show_bug.cgi?id=103276

Summary Fix occlusion culling logic to handle css background layer clipping

Justin Novosad

Reported 2012-11-26 10:59:51 PST

Fix clipping issue raised in comment 25: https://bugs.webkit.org/show_bug.cgi?id=102557

Attachments
Patch (6.69 KB, patch) 2012-11-26 13:10 PST, Justin Novosad	no flags	Details Formatted Diff Diff
Patch (9.20 KB, patch) 2012-11-26 14:27 PST, Justin Novosad	no flags	Details Formatted Diff Diff
Patch (8.50 KB, patch) 2012-11-27 13:11 PST, Justin Novosad	no flags	Details Formatted Diff Diff
Patch (8.50 KB, patch) 2012-11-28 06:22 PST, Justin Novosad	no flags	Details Formatted Diff Diff
Patch (8.58 KB, patch) 2012-11-28 08:07 PST, Justin Novosad	no flags	Details Formatted Diff Diff
Patch for landing (8.60 KB, patch) 2012-11-30 12:14 PST, Justin Novosad	no flags	Details Formatted Diff Diff
Show Obsolete (5) View All Add attachment proposed patch, testcase, etc.

Justin Novosad

Comment 1 2012-11-26 13:10:20 PST

Created attachment 176050 [details] Patch

Simon Fraser (smfr)

Comment 2 2012-11-26 13:18:33 PST

Comment on attachment 176050 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=176050&action=review > Source/WebCore/rendering/style/FillLayer.cpp:294 > +bool FillLayer::clipOccludesNextLayers() const > +{ > + switch (clip()) { > + case BorderFillBox: > + break; > + case PaddingFillBox: > + { > + for (const FillLayer* layer = this->next(); layer; layer = layer->next()) { > + if (layer->clip() == BorderFillBox) > + return false; > + } > + } > + break; > + case ContentFillBox: > + { > + for (const FillLayer* layer = this->next(); layer; layer = layer->next()) { > + if (layer->clip() == BorderFillBox || layer->clip() == PaddingFillBox) > + return false; > + } > + } > + break; > + case TextFillBox: > + { > + for (const FillLayer* layer = this->next(); layer; layer = layer->next()) { > + if (layer->clip() != TextFillBox) > + return false; > + } > + } > + break; > + default: > + ASSERT_NOT_REACHED(); > + } > + return true; > +} You've made layer painting O(n^2) here.

Justin Novosad

Comment 3 2012-11-26 13:25:43 PST

@simon.fraser: In your comment on bug 102557, you expressed concern that about a similar problem here: > Source/WebCore/rendering/RenderBoxModelObject.cpp:913 > + if (!bgLayer->next() && !(shouldPaintBackgroundImage && bgLayer->hasOpaqueImage(this) && bgLayer->hasRepeatXY())) { AFAIK, the background color gets the same clipping applied to it as the bottom-most layer, so I think that case is OK. I was unable to find a failing test case for it. Am I missing something?

Justin Novosad

Comment 4 2012-11-26 13:29:39 PST

(In reply to comment #2) > > > You've made layer painting O(n^2) here. D'Oh! I have an idea...

Justin Novosad

Comment 5 2012-11-26 14:27:29 PST

Created attachment 176067 [details] Patch

Justin Novosad

Comment 6 2012-11-27 10:19:29 PST

(In reply to comment #5) > Created an attachment (id=176067) [details] > Patch Pinging reviewers. This fixes a regression. IWBN to commit it promptly.

Simon Fraser (smfr)

Comment 7 2012-11-27 10:27:07 PST

Comment on attachment 176067 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=176067&action=review > Source/WebCore/rendering/RenderBox.cpp:1008 > +void RenderBox::recursivePaintFillLayers(const PaintInfo& paintInfo, const Color& c, const FillLayer* fillLayer, const LayoutRect& rect, > + BackgroundBleedAvoidance bleedAvoidance, CompositeOperator op, RenderObject* backgroundObject, bool firstLayer) I'd prefer this not be recursive. I see no reason why we can't just paint layers in a loop. > Source/WebCore/rendering/style/FillLayer.cpp:268 > + if (clipA == BorderFillBox || clipB == BorderFillBox) > + return BorderFillBox; > + if (clipA == PaddingFillBox || clipB == PaddingFillBox) > + return BorderFillBox; > + if (clipA == ContentFillBox || clipB == ContentFillBox) > + return BorderFillBox; Is this right? Why return BorderFillBox every time?

Justin Novosad

Comment 8 2012-11-27 10:44:01 PST

(In reply to comment #7) > > I'd prefer this not be recursive. I see no reason why we can't just paint layers in a loop. > BTW, it was already recursive before. It's easy to refactor, so I guess I can do it here... > > Source/WebCore/rendering/style/FillLayer.cpp:268 > > + if (clipA == BorderFillBox || clipB == BorderFillBox) > > + return BorderFillBox; > > + if (clipA == PaddingFillBox || clipB == PaddingFillBox) > > + return BorderFillBox; > > + if (clipA == ContentFillBox || clipB == ContentFillBox) > > + return BorderFillBox; > > Is this right? Why return BorderFillBox every time? Ooh, thanks for catching that.

Justin Novosad

Comment 9 2012-11-27 13:11:32 PST

Created attachment 176332 [details] Patch

Justin Novosad

Comment 10 2012-11-27 13:14:08 PST

(In reply to comment #9) > Created an attachment (id=176332) [details] > Patch Requesting another review because change to RenderBox::paintFillLayers are substantial. Because FillLayers not a doubly linked list, changing the recursion into a loop was non-trivial.

Peter Beverloo (cr-android ews)

Comment 11 2012-11-27 15:13:41 PST

Comment on attachment 176332 [details] Patch Attachment 176332 [details] did not pass cr-android-ews (chromium-android): Output: http://queues.webkit.org/results/15023187

Justin Novosad

Comment 12 2012-11-28 06:22:55 PST

Created attachment 176475 [details] Patch

Peter Beverloo (cr-android ews)

Comment 13 2012-11-28 07:54:01 PST

Comment on attachment 176475 [details] Patch Attachment 176475 [details] did not pass cr-android-ews (chromium-android): Output: http://queues.webkit.org/results/15026439

Justin Novosad

Comment 14 2012-11-28 08:07:27 PST

Created attachment 176487 [details] Patch

Justin Novosad

Comment 15 2012-11-29 13:59:21 PST

Pinging reviewers.

Simon Fraser (smfr)

Comment 16 2012-11-29 14:16:09 PST

Comment on attachment 176487 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=176487&action=review > Source/WebCore/ChangeLog:19 > + (WebCore): Useless line. > Source/WebCore/rendering/RenderBox.cpp:1001 > + Vector<const FillLayer*> layers; You should use some inline capacity here to avoid a malloc in most cases.

Justin Novosad

Comment 17 2012-11-30 12:14:13 PST

Created attachment 177005 [details] Patch for landing

WebKit Review Bot

Comment 18 2012-12-02 02:56:17 PST

Comment on attachment 177005 [details] Patch for landing Clearing flags on attachment: 177005 Committed r136326: <http://trac.webkit.org/changeset/136326>

WebKit Review Bot

Comment 19 2012-12-02 02:56:21 PST

All reviewed patches have been landed. Closing bug.

Note You need to log in before you can comment on or make changes to this bug.

Status RESOLVED

Resolution FIXED

Priority P2

Severity Normal

Classification Unclassified

Version 528+ (Nightly build)

Hardware Unspecified

OS Unspecified

Product WebKit

Component Layout and Rendering

Assignee

Justin Novosad

Reported

2012-11-26 10:59 PST

Modified

2012-12-02 02:56 PST History

CC List

7 users Show

URL

Keywords

Depends on

Blocks