[webkit-reviews] review denied: [Bug 71488] [CSSRegions]Add support for background-color in region styling : [Attachment 113529] Patch

[bugzilla-daemon at webkit.org]

Dave Hyatt <hyatt at apple.com> has denied Mihnea Ovidenie <mihnea at adobe.com>'s
request for review:
Bug 71488: [CSSRegions]Add support for background-color in region styling
https://bugs.webkit.org/show_bug.cgi?id=71488

Attachment 113529: Patch
https://bugs.webkit.org/attachment.cgi?id=113529&action=review

------- Additional Comments from Dave Hyatt <hyatt at apple.com>
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.