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

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Justin Novosad]

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

[Justin Novosad]

Created attachment 176050 [details]
Patch

[Simon Fraser (smfr)]

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]

@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]

(In reply to comment #2)
> 
> 
> You've made layer painting O(n^2) here.

D'Oh! I have an idea...

[Justin Novosad]

Created attachment 176067 [details]
Patch

[Justin Novosad]

(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 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]

(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]

Created attachment 176332 [details]
Patch

[Justin Novosad]

(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 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]

Created attachment 176475 [details]
Patch

[Peter Beverloo (cr-android ews)]

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]

Created attachment 176487 [details]
Patch

[Justin Novosad]

Pinging reviewers.

[Simon Fraser (smfr)]

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]

Created attachment 177005 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 177005 [details]
Patch for landing

Clearing flags on attachment: 177005

Committed r136326: <http://trac.webkit.org/changeset/136326>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.