[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


Simon Fraser (smfr) <simon.fraser at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #222755|review?                     |review+
               Flag|                            |




--- Comment #16 from Simon Fraser (smfr) <simon.fraser at apple.com>  2014-01-30 17:28:09 PST ---
(From update of attachment 222755)
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!

> 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.

> Source/WebCore/page/FrameView.cpp:2448
> +    RenderLayerBacking* backing = renderView->layer()->backing();
> +    if (!backing)
> +        return;

This is a bit odd since backing isn't used.

> Source/WebCore/page/FrameView.cpp:2487
> +    bool documentHasBackgroundImage = (documentElementRenderer && documentElementRenderer->style().hasBackgroundImage())

I think this should be called rootBackgroundHasImage.

> 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).

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