[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 #12 from Beth Dakin <bdakin at apple.com>  2014-01-30 11:59:47 PST ---
(In reply to comment #11)
> (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.
> 
> How about just shouldExtendBackground() then?
> 

Definitely not this!!! Haha, maybe we should talk in person. There are three scenarios here:

1. We're in the "old mode," so the new setBackgroundShouldExtendBeyondPage Setting is set to false.
2. We're in the new mode, but we only have a background color, so we don't need to extend the background RECT. 
3. We're in the new mode, and we have a background image, so we DO need to extend the background RECT.

shouldExtendBackground() is a bad name for 3 because given the name of the Setting, it implies 2.

> >>> Source/WebCore/page/FrameView.cpp:2520
> >>> +        || (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?
> 
> There is related logic in these places:
> RenderLayerBacking::isSimpleContainerCompositingLayer()
> skipBodyBackground() in RenderBox.cpp
> RenderElement::styleWillChange()
> and RenderElement::rendererForRootBackground() is also related.

Will look.

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