[Webkit-unassigned] [Bug 127876] Extended background should only create margin tiles for pages with background images

[bugzilla-daemon at webkit.org]

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





--- Comment #17 from Beth Dakin <bdakin at apple.com>  2014-01-31 11:48:23 PST ---
(In reply to comment #16)
> (From update of attachment 222755 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=222755&action=review
> 
> Does this handle dynamic changes of root background color (e.g. via CSS animations)? I don't think it does, which suggests that the way you push the color onto the background is wrong. That should be fixed in a followup.
> 
> > Source/WebCore/ChangeLog:18
> > +        This patch makes callers that wonder only if the new setting is set always call 
> 
> callers that wonder!
> 

Haha! Okay, fine, will re-phrase. :-P

> > Source/WebCore/page/FrameView.cpp:2444
> > +    renderView->compositor().setRootExtendedBackgroundColor(extendBackground ? documentBackgroundColor() : Color());
> 
> I'm not sure this is the right place to grab the color. I'd expect setBackgroundExtendsBeyondPage(true) to just set some state, and for the color to be updated dynamically.
> 

Yeah, usually this is a no-op, but if we ever wanted to change the setting dynamically, I feel like this might be nice! I'm tempted to leave it in…but maybe that is silly.

> > Source/WebCore/page/FrameView.cpp:2448
> > +    RenderLayerBacking* backing = renderView->layer()->backing();
> > +    if (!backing)
> > +        return;
> 
> This is a bit odd since backing isn't used.
> 

Oops! Just some cruft. Will remove.

> > Source/WebCore/page/FrameView.cpp:2487
> > +    bool documentHasBackgroundImage = (documentElementRenderer && documentElementRenderer->style().hasBackgroundImage())
> 
> I think this should be called rootBackgroundHasImage.
> 

Will change!

> > Source/WebCore/rendering/RenderView.cpp:1041
> > +        if (frameView().hasExtendedBackgroundRectForPainting() != needsExtendedBackground)
> > +            frameView().setHasExtendedBackgroundRectForPainting(needsExtendedBackground);
> 
> It's bad that a getter on RenderView is changing state on FrameView! setHasExtendedBackgroundRectForPainting() needs to be called somewhere else (e.g. after style/layout changes or something).

Okay, I'll look for a new spot.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.