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

[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 114470: Patch 3
https://bugs.webkit.org/attachment.cgi?id=114470&action=review

------- Additional Comments from Dave Hyatt <hyatt at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=114470&action=review


Getting close!

> Source/WebCore/rendering/RenderRegion.cpp:155
> +    bool clearRegionStyle = true;
>      if (m_flowThread && isValid()) {
>	   if (regionRect().width() != contentWidth() || regionRect().height()
!= contentHeight())
>	       m_flowThread->invalidateRegions();
> +	   else
> +	       clearRegionStyle = false;
>      }
> -    
> +
> +    if (clearRegionStyle)
> +	   clearRegionStyleInRegion();

This isn't really right. You should not clear styles just because layout
changed. I would expect styles to be cleared out when the start/end regions
change for a given object, or when that object goes away. I think at a bare
minimum you should work with RenderObject::destroy to clear out any custom
region styles. This could possibly be done by clearing the start/end region and
having that function just delete the styles.

I'd also file a followup bug about keeping the styles up to date when something
dynamically changes with style.