[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 #4 from Beth Dakin <bdakin at apple.com>  2014-01-29 18:21:01 PST ---
(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.

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