[webkit-reviews] review granted: [Bug 127876] Extended background should only create margin tiles for pages with background images : [Attachment 222755] Patch

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has granted Beth Dakin
<bdakin at apple.com>'s request for review:
Bug 127876: Extended background should only create margin tiles for pages with
background images
https://bugs.webkit.org/show_bug.cgi?id=127876

Attachment 222755: Patch
https://bugs.webkit.org/attachment.cgi?id=222755&action=review

------- Additional Comments from Simon Fraser (smfr) <simon.fraser at apple.com>
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).