Bug 127021 - Repeating background images should continue into margin tiles
Summary: Repeating background images should continue into margin tiles
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Beth Dakin
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2014-01-14 16:51 PST by Beth Dakin
Modified: 2014-01-16 11:30 PST (History)
10 users (show)

See Also:


Attachments
Patch (18.32 KB, patch)
2014-01-14 17:20 PST, Beth Dakin
simon.fraser: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Beth Dakin 2014-01-14 16:51:50 PST
When the TileController is configured to have margin tiles, repeating background images should continue to repaint in those tiles.

<rdar://problem/15571300>
Comment 1 Beth Dakin 2014-01-14 17:20:57 PST
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.
Comment 2 Simon Fraser (smfr) 2014-01-15 14:53:15 PST
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.
Comment 3 Beth Dakin 2014-01-15 15:43:28 PST
(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!
Comment 4 Beth Dakin 2014-01-15 15:47:36 PST
http://trac.webkit.org/changeset/162098
Comment 5 Csaba Osztrogonác 2014-01-16 07:26:12 PST
(In reply to comment #4)
> http://trac.webkit.org/changeset/162098

It broke the WinCairo build.
Comment 6 Beth Dakin 2014-01-16 11:30:29 PST
(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