Bug 127021 – Repeating background images should continue into margin tiles

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Beth Dakin]

When the TileController is configured to have margin tiles, repeating background images should continue to repaint in those tiles.

<rdar://problem/15571300>

[Beth Dakin]

Created attachment 221215 [details]
Patch

I did a ton of manual testing. I think it would be really good to finally add LayoutTests for this stuff, but I figured I would post this patch in the meantime.

[Simon Fraser (smfr)]

Comment on attachment 221215 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=221215&action=review

> Source/WebCore/page/FrameView.h:197
> +    bool hasExtendedBackground();
> +    IntRect extendedBackgroundRect();

Both should be const. I would like to see extendedBackgroundRect() have a comment saying what coordinate system the rect is in.

> Source/WebCore/rendering/RenderBox.cpp:1603
> +                if (rectToRepaint.width() == rendererRect.width()) {
> +                    rectToRepaint.move(extendedBackgroundRect.x(), 0);
> +                    rectToRepaint.setWidth(extendedBackgroundRect.width());
> +                }
> +                if (rectToRepaint.height() == rendererRect.height()) {
> +                    rectToRepaint.move(0, extendedBackgroundRect.y());
> +                    rectToRepaint.setHeight(extendedBackgroundRect.height());
> +                }

It seems a bit arbitrary to say "if we're invalidating the width, just invalidate the extended width". What about a non-repeating animating background image that is projecting into the left margin but not the right?

I think the background geometry code needs to know about extended rects.

[Beth Dakin]

(In reply to comment #2)
> (From update of attachment 221215 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=221215&action=review
> 
> > Source/WebCore/page/FrameView.h:197
> > +    bool hasExtendedBackground();
> > +    IntRect extendedBackgroundRect();
> 
> Both should be const. I would like to see extendedBackgroundRect() have a comment saying what coordinate system the rect is in.
> 

Done.

> > Source/WebCore/rendering/RenderBox.cpp:1603
> > +                if (rectToRepaint.width() == rendererRect.width()) {
> > +                    rectToRepaint.move(extendedBackgroundRect.x(), 0);
> > +                    rectToRepaint.setWidth(extendedBackgroundRect.width());
> > +                }
> > +                if (rectToRepaint.height() == rendererRect.height()) {
> > +                    rectToRepaint.move(0, extendedBackgroundRect.y());
> > +                    rectToRepaint.setHeight(extendedBackgroundRect.height());
> > +                }
> 
> It seems a bit arbitrary to say "if we're invalidating the width, just invalidate the extended width". What about a non-repeating animating background image that is projecting into the left margin but not the right?
> 
> I think the background geometry code needs to know about extended rects.

I will file a follow-up bug for this. The performance penalty for over-invalidation is probably pretty small in the long run since a repeating background image like you've described is probably unlikely to be very common in the real world. But it would be nice to be fully-optimized here.

Thanks for the review!

[Beth Dakin]

http://trac.webkit.org/changeset/162098

[Csaba Osztrogonác]

(In reply to comment #4)
> http://trac.webkit.org/changeset/162098

It broke the WinCairo build.

[Beth Dakin]

(In reply to comment #5)
> (In reply to comment #4)
> > http://trac.webkit.org/changeset/162098
> 
> It broke the WinCairo build.

Speculative build fix: http://trac.webkit.org/changeset/162138