[webkit-reviews] review denied: [Bug 128958] Add support for the isolation property : [Attachment 224479] Patch V2

[bugzilla-daemon at webkit.org]

Andreas Kling <akling at apple.com> has denied Mihai Tica <mitica at adobe.com>'s
request for review:
Bug 128958: Add support for the isolation property
https://bugs.webkit.org/show_bug.cgi?id=128958

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

------- Additional Comments from Andreas Kling <akling at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=224479&action=review


> Source/WebCore/css/CSSPrimitiveValueMappings.h:4158
> +template<> inline CSSPrimitiveValue::CSSPrimitiveValue(Isolation i)

Short variable names like "i" are not consistent with WebKit style. We tend to
prefer e.g like "isolation".

> Source/WebCore/css/CSSPrimitiveValueMappings.h:4169
> +    switch (i) {
> +    case IsolationAuto:
> +	   m_value.valueID = CSSValueAuto;
> +	   break;
> +    case IsolationIsolate:
> +	   m_value.valueID = CSSValueIsolate;
> +	   break;
> +    }

We should have a default case here with ASSERT_NOT_REACHED(), just like we do
when converting in the other direction.

> Source/WebCore/rendering/style/RenderStyle.h:974
> +    void setIsolation(Isolation v) {
rareNonInheritedData.access()->m_isolation = v; }

Using rareNonInheritedData.access() like this will force the RenderStyle to
create its own unique copy of the StyleRareNonInheritedData. If you look at
other setters in this file, they use a macro (SET_VAR) that first checks if the
value being written is identical to the existing one, and avoids a copy in that
case.