[Webkit-unassigned] [Bug 71488] [CSSRegions]Add support for background-color in region styling

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=71488


Dave Hyatt <hyatt at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #113529|review?, commit-queue?      |review-, commit-queue-
               Flag|                            |




--- Comment #2 from Dave Hyatt <hyatt at apple.com>  2011-11-04 09:31:07 PST ---
(From update of attachment 113529)
View in context: https://bugs.webkit.org/attachment.cgi?id=113529&action=review

> Source/WebCore/rendering/RenderBoxRegionInfo.cpp:59
> +        // FIXME: assume for now that a box with region styling always has box decorations.
> +        (const_cast<RenderBox*>(box))->setHasBoxDecorations(true);

You should be able to set this accurately. Do the same checks that were done for the real style.

> Source/WebCore/rendering/RenderBoxRegionInfo.h:56
> +    RenderStyle* regionStyle() const { return m_regionStyle.get(); }
> +    void setRegionStyle(PassRefPtr<RenderStyle>);
> +    void computeStyleInRegion(const RenderBox*, RenderRegion*);

I think this should just be in RenderFlowThread in a separate map rather than part of RenderBoxRegionInfo.h. It has to work for all RenderObjects, so that's my mistake suggesting to put it here.

> Source/WebCore/rendering/RenderFlowThread.cpp:751
> +            view()->setCurrentRenderRegion(startRegion);

Let's just remove this for now. I don't think it's the right place to be setting the current render region, and it's not relevant to getting background color working, so you can revisit this in a future patch.

> Source/WebCore/rendering/RenderFlowThread.cpp:769
> +        view()->setCurrentRenderRegion(startRegion);

Same. Remove for now.

> Source/WebCore/rendering/RenderLayer.cpp:2548
> +    renderer()->view()->setCurrentRenderRegion(region);

Who clears this when you're finished? It seems like this should be more like a stack... where you push the region and then pop back to the old one when finished. A little helper class might be good for this. I suspect this might be a problem with the current flow thread stuff in RenderView as well... 

It seems like you could eliminate all of the RenderRegion* arguments I added to RenderLayer by just using this current region instead. Then you could set this in RenderRegion's painting and hit testing instead.

> Source/WebCore/rendering/RenderObject.cpp:1696
> +    if (view() && view()->currentRenderRegion() && view()->currentRenderRegion()->hasCustomRegionStyle() && isBox() && !isAnonymous()) {

Check inRenderFlowThread(). That is a really fast check you can do right up front. I would also bias towards the common case first so:

if (!inRenderFlowThread() || ....)
    return m_style.get();

// Now run your code.

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