[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 #9 from Beth Dakin <bdakin at apple.com>  2014-01-30 11:00:32 PST ---
(In reply to comment #8)
> (From update of attachment 222623 [details])
> 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?
> 

You wouldn't for color, but in the future we might for a gradient, and if we ever tried to do something crazy like the "cnn.com" problem, then we might do it for other things too. I don't think we should get overly specific about background images here. My comment in FrameView attempts to explain the differentiation about how some extended backgrounds require extended background RECTs and others do not.

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

I saw similar logic in RenderLayerBacking, but it that logic was incorrect for my purposes, and it failed to catch a number of websites with background images. I don't know enough about that code to know if it should be fixed or if it's doing what it should do for its own purposes. 

Can you point to any other similar spots I should look at?

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

Yeah, Darin pointed this out too, and as I told him, I know that was a dirty trick. Working on a new patch now to disambiguate the getting and setter. 

> > Source/WebCore/rendering/RenderLayerCompositor.cpp:2973
> > +void RenderLayerCompositor::setExtendedBackgroundColor(const Color& color)
> 
> I feel like this name should have "root" in it somewhere.

Okay!

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