[webkit-reviews] review denied: [Bug 109258] Web Inspector: [CSS Regions] InspectorCSSAgent::getMatchedStylesForNode should consider CSS region styles. : [Attachment 188979] Patch

[bugzilla-daemon at webkit.org]

Alexandru Chiculita <achicu at adobe.com> has denied Takashi Sakamoto
<tasak at google.com>'s request for review:
Bug 109258: Web Inspector: [CSS Regions]
InspectorCSSAgent::getMatchedStylesForNode should consider CSS region styles.
https://bugs.webkit.org/show_bug.cgi?id=109258

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

------- Additional Comments from Alexandru Chiculita <achicu at adobe.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=188979&action=review


Thanks for working on this patch!

> Source/WebCore/ChangeLog:12
> +	   getMatchedStyleForNode should provide a RenderRegionList, which is
> +	   obtained from a given node's render flow thread, for StyleResolver.
> +	   Without this, whether StyleResolver returns CSS region styles or not

> +	   depends on its internal state: regionForStyling. This would probably

> +	   make region-style-crash layout test fail or flaky.

I think regionForStyling is reset in the clear() method. Do you have a crash
for it? Maybe we should fix just that first.

> Source/WebCore/ChangeLog:14
> +	   No new tests. CSSStyleMode.js always disables obtaining matched
region

Why not add a test to check the collected rules?

> Source/WebCore/css/StyleResolver.cpp:2062
> +    if (regionList) {

Looks like this part changed recently. There's an ElementRuleCollector
allocated on the stack now.

> Source/WebCore/css/StyleResolver.h:237
> +    PassRefPtr<CSSRuleList> pseudoStyleRulesForElement(Element*, PseudoId,
unsigned rulesToInclude = AllButEmptyCSSRules, const RenderRegionList*
regionListForStyling = 0);

I think pseudo styles are not supported in the region styling @rules. For
example, I don't think that :hover would work with region styling at the
moment.

> Source/WebCore/inspector/InspectorCSSAgent.cpp:729
> +#if ENABLE(CSS_REGIONS)

You should not need the ENABLE(CSS_REGIONS) flag. It's only used when parsing
CSS properties related to regions.

> Source/WebCore/inspector/InspectorCSSAgent.cpp:731
> +	   if (element->renderer() &&
element->renderer()->inRenderFlowThread()) {

In this case renderer()->inRenderFlowThread() changed to
"renderer()->flowThreadState() == RenderObject::InsideOutOfFlowThread".

> Source/WebCore/inspector/InspectorCSSAgent.cpp:732
> +	       RenderFlowThread* flowThread =
element->renderer()->enclosingRenderFlowThread();

enclosingRenderFlowThread becomes locateFlowThreadContainingBlock

> Source/WebCore/inspector/InspectorCSSAgent.cpp:751
> +

nit: remove empty lines.