Bug 103276

Summary: Fix occlusion culling logic to handle css background layer clipping
Product: WebKit Reporter: Justin Novosad <junov>
Component: Layout and RenderingAssignee: Justin Novosad <junov>
Status: RESOLVED FIXED    
Severity: Normal CC: dglazkov, eric, ojan, peter+ews, senorblanco, simon.fraser, 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
Patch for landing none

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
DetailsFormatted DiffDiff
Patch (9.20 KB, patch)
2012-11-26 14:27 PST, Justin Novosad 		no flags
DetailsFormatted DiffDiff
Patch (8.50 KB, patch)
2012-11-27 13:11 PST, Justin Novosad 		no flags
DetailsFormatted DiffDiff
Patch (8.50 KB, patch)
2012-11-28 06:22 PST, Justin Novosad 		no flags
DetailsFormatted DiffDiff
Patch (8.58 KB, patch)
2012-11-28 08:07 PST, Justin Novosad 		no flags
DetailsFormatted DiffDiff
Patch for landing (8.60 KB, patch)
2012-11-30 12:14 PST, Justin Novosad 		no flags
DetailsFormatted DiffDiff
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.