Extended background should only create margin tiles for pages with background images

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Beth Dakin]

The new extended background mode doesn't actually need to create margin tiles for pages with simple background colors. For now, we should only create tiles when there is a background image. We may want to extend this to other types of complicated backgrounds in the future.

[Beth Dakin]

Created attachment 222614 [details]
Patch

[WebKit Commit Bot]

Attachment 222614 [details] did not pass style-queue:


ERROR: Source/WebCore/rendering/RenderView.cpp:566:  One line control clauses should not use braces.  [whitespace/braces] [4]
Total errors found: 1 in 13 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Darin Adler]

Comment on attachment 222614 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=222614&action=review

I have a few comments, but didn’t get all the way to review+ or review-

> Source/WebCore/page/FrameView.cpp:2510
> +    Element* htmlElement = frame().document()->documentElement();
> +    Element* bodyElement = frame().document()->body();

I would give these two local variables “auto” type rather than dumbing the type down to Element* (I think at least one of those functions returns a more specific type).

Also, I think documentElement is more pleasant than htmlElement, so I suggest using that name for it.

> Source/WebCore/page/FrameView.cpp:2512
> +    RenderObject* htmlRenderer = htmlElement ? htmlElement->renderer() : 0;
> +    RenderObject* bodyRenderer = bodyElement ? bodyElement->renderer() : 0;

nullptr

Also, I would use auto here since we possibly having a more specific renderer type than RenderObject.

I guess you probably won’t want to say documentElementRenderer, but that’s the name I would use.

> Source/WebCore/page/FrameView.cpp:2513
> +    bool doucmentHasBackgroundImage = (htmlRenderer && htmlRenderer->style().hasBackgroundImage())

Typo here, doucment.

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

What guarantees that renderView->layer() is non-null? I assume it’s guaranteed, but not sure by what.

> Source/WebCore/page/FrameView.cpp:2522
> +    if (!backing)
> +        return false;

I suggest moving the renderView ad backing parts earlier just so they can exit before we start looking at the document and body elements.

> Source/WebCore/page/FrameView.cpp:2524
> +    backing->setTiledBackingHasMargins(doucmentHasBackgroundImage);

It‘s strange that this function, which sounds like a getter, has a side effect of setting this flag on the backing.

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

You are calling this function just for its side effect? If so, then I think we should rename it. It really doesn’t sound like a function with a side effect.

> Source/WebCore/rendering/RenderView.cpp:566
> +    if (frameView().frame().settings().backgroundShouldExtendBeyondPage()) {
> +        compositor().setExtendedBackgroundColor(frameView().documentBackgroundColor());
> +    }

No braces for a one-line if statement like this.

Is there ever a case where we want to clear the extended background color because the setting changed?

[Beth Dakin]

(In reply to comment #3)
> (From update of attachment 222614 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=222614&action=review
> 
> I have a few comments, but didn’t get all the way to review+ or review-
> 

Thank you!

> > Source/WebCore/page/FrameView.cpp:2510
> > +    Element* htmlElement = frame().document()->documentElement();
> > +    Element* bodyElement = frame().document()->body();
> 
> I would give these two local variables “auto” type rather than dumbing the type down to Element* (I think at least one of those functions returns a more specific type).
> 

Done.

> Also, I think documentElement is more pleasant than htmlElement, so I suggest using that name for it.
> 

Done.

> > Source/WebCore/page/FrameView.cpp:2512
> > +    RenderObject* htmlRenderer = htmlElement ? htmlElement->renderer() : 0;
> > +    RenderObject* bodyRenderer = bodyElement ? bodyElement->renderer() : 0;
> 
> nullptr
> 

Done.

> Also, I would use auto here since we possibly having a more specific renderer type than RenderObject.
> 

Done.

> I guess you probably won’t want to say documentElementRenderer, but that’s the name I would use.
> 

No, I like it too! It's not terribly long. Changed it.

> > Source/WebCore/page/FrameView.cpp:2513
> > +    bool doucmentHasBackgroundImage = (htmlRenderer && htmlRenderer->style().hasBackgroundImage())
> 
> Typo here, doucment.
> 

Fixed.

> > Source/WebCore/page/FrameView.cpp:2520
> > +    RenderLayerBacking* backing = renderView->layer()->backing();
> 
> What guarantees that renderView->layer() is non-null? I assume it’s guaranteed, but not sure by what.
> 

This is inside a #if USE(ACCELERATED_COMPOSITING), and that guarantees that the RenderView has a non-null layer().

> > Source/WebCore/page/FrameView.cpp:2522
> > +    if (!backing)
> > +        return false;
> 
> I suggest moving the renderView ad backing parts earlier just so they can exit before we start looking at the document and body elements.
> 

Done.

> > Source/WebCore/page/FrameView.cpp:2524
> > +    backing->setTiledBackingHasMargins(doucmentHasBackgroundImage);
> 
> It‘s strange that this function, which sounds like a getter, has a side effect of setting this flag on the backing.
> 

Yeah, I do feel a little dirty about that. More on that in a second.

> > Source/WebCore/page/Settings.cpp:610
> > +    m_page->mainFrame().view()->shouldExtendBackgroundRect();
> 
> You are calling this function just for its side effect? If so, then I think we should rename it. It really doesn’t sound like a function with a side effect.
> 

Again, I agree this is a little dirty. It was just a matter convenience for me to turn this into a combination getter and setter. I would be open to a better name OR to figuring out which callers need the setter part and adding a separate function that does only that. I'll think it over, but input would be valued. I was puzzling this before I posted the patch, and decided just to post it in the meantime.

> > Source/WebCore/rendering/RenderView.cpp:566
> > +    if (frameView().frame().settings().backgroundShouldExtendBeyondPage()) {
> > +        compositor().setExtendedBackgroundColor(frameView().documentBackgroundColor());
> > +    }
> 
> No braces for a one-line if statement like this.
> 

Fixed.

> Is there ever a case where we want to clear the extended background color because the setting changed?

There could be! I will fix this. 

Updated patch coming soon.

[Beth Dakin]

Created attachment 222623 [details]
Patch

[WebKit Commit Bot]

Attachment 222623 [details] did not pass style-queue:


ERROR: Source/WebCore/rendering/RenderView.cpp:565:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
Total errors found: 1 in 13 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Antti Koivisto]

Comment on attachment 222623 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=222623&action=review

> Source/WebCore/rendering/RenderLayerBacking.cpp:244
> -    int marginSize = extendBackground ? 512 : 0;
> +    int marginSize = hasExtendedBackgroundRect ? 512 : 0;

What is the magical 512?  Could you make it a constant with informative name?

[Simon Fraser (smfr)]

Comment on attachment 222623 [details]
Patch

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.

[Beth Dakin]

(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!

[Beth Dakin]

(In reply to comment #7)
> (From update of attachment 222623 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=222623&action=review
> 
> > Source/WebCore/rendering/RenderLayerBacking.cpp:244
> > -    int marginSize = extendBackground ? 512 : 0;
> > +    int marginSize = hasExtendedBackgroundRect ? 512 : 0;
> 
> What is the magical 512?  Could you make it a constant with informative name?

Yeah, right now it's just mimicking the tile size, but those static const ints should probably live somewhere more share-able (they are currently in TileController.mm). Any suggestions for a good spot?

static const int defaultTileWidth = 512;
static const int defaultTileHeight = 512;

[Simon Fraser (smfr)]

Comment on attachment 222623 [details]
Patch

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?

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

[Beth Dakin]

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

[Beth Dakin]

Created attachment 222747 [details]
Patch

Here's my newest patch. This patch disambiguates my getter/setter into two separate functions. It does not attempt to combine with other similar root node background code…I couldn't think of any clean way to do that since all of the spots that look at html and body renders are all just slightly different.

[WebKit Commit Bot]

Attachment 222747 [details] did not pass style-queue:


ERROR: Source/WebCore/page/FrameView.cpp:2492:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 1 in 13 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Beth Dakin]

Created attachment 222755 [details]
Patch

Here's a patch with better names.

[Simon Fraser (smfr)]

Comment on attachment 222755 [details]
Patch

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

[Beth Dakin]

(In reply to comment #16)
> (From update of attachment 222755 [details])
> 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!
> 

Haha! Okay, fine, will re-phrase. :-P

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

Yeah, usually this is a no-op, but if we ever wanted to change the setting dynamically, I feel like this might be nice! I'm tempted to leave it in…but maybe that is silly.

> > Source/WebCore/page/FrameView.cpp:2448
> > +    RenderLayerBacking* backing = renderView->layer()->backing();
> > +    if (!backing)
> > +        return;
> 
> This is a bit odd since backing isn't used.
> 

Oops! Just some cruft. Will remove.

> > Source/WebCore/page/FrameView.cpp:2487
> > +    bool documentHasBackgroundImage = (documentElementRenderer && documentElementRenderer->style().hasBackgroundImage())
> 
> I think this should be called rootBackgroundHasImage.
> 

Will change!

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

Okay, I'll look for a new spot.

[Beth Dakin]

Committed http://trac.webkit.org/changeset/163190