[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 #3 from Darin Adler <darin at apple.com>  2014-01-29 18:05:00 PST ---
(From update of attachment 222614)
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?

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