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

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has denied  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 222623: Patch
https://bugs.webkit.org/attachment.cgi?id=222623&action=review

------- Additional Comments from Simon Fraser (smfr) <simon.fraser at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=222623&action=review


> Source/WebCore/page/FrameView.cpp:2488
> +bool FrameView::shouldExtendBackgroundRect() const

This name doesn't imply to me that it's about extending for background image;
why would you not extend the background rect to show color?

> Source/WebCore/page/FrameView.cpp:2520
> +    auto documentElement = document->documentElement();
> +    auto bodyElement = document->body();
> +    auto documentElementRenderer = documentElement ?
documentElement->renderer() : nullptr;
> +    auto bodyRenderer = bodyElement ? bodyElement->renderer() : nullptr;
> +    bool documentHasBackgroundImage = (documentElementRenderer &&
documentElementRenderer->style().hasBackgroundImage())
> +	   || (bodyRenderer && bodyRenderer->style().hasBackgroundImage());

We have similar logic in maybe 3 places; it would be nice to share code.

> Source/WebCore/page/Settings.cpp:610
> +    m_page->mainFrame().view()->shouldExtendBackgroundRect();

Is this a setter? It reads like a getter, which makes this line seem like a
no-op.

> Source/WebCore/rendering/RenderLayerCompositor.cpp:2973
> +void RenderLayerCompositor::setExtendedBackgroundColor(const Color& color)

I feel like this name should have "root" in it somewhere.