Bug 128958 – Add support for the isolation property

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Mihai Tica]

The isolation property is part of the CSS Blending specification: http://dev.w3.org/fxtf/compositing-1/#isolation

Parse and implement a prefixed version of the property. The allowed values for -webkit-isolation are isolate and auto(default).

[Mihai Tica]

Created attachment 224476 [details]
Patch V1

[Mihai Tica]

Created attachment 224479 [details]
Patch V2

[Andreas Kling]

Comment on attachment 224479 [details]
Patch V2

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.

[Mihai Tica]

Created attachment 224608 [details]
Patch V3

Addressed issues highlighted by Andreas Kling.

[Simon Fraser (smfr)]

Comment on attachment 224608 [details]
Patch V3

View in context: https://bugs.webkit.org/attachment.cgi?id=224608&action=review

> Source/WebCore/rendering/style/RenderStyle.h:975
> +    bool hasIsolation() const { return rareNonInheritedData->m_isolation != IsolationAuto; }

Won't this crash if rareNonInheritedData hasn't been allocated?

[Mihai Tica]

(In reply to comment #5)
> (From update of attachment 224608 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=224608&action=review
> 
> > Source/WebCore/rendering/style/RenderStyle.h:975
> > +    bool hasIsolation() const { return rareNonInheritedData->m_isolation != IsolationAuto; }
> 
> Won't this crash if rareNonInheritedData hasn't been allocated?

All of RenderStyle's constructors initialise a valid rareNonInheritedData member, so I don't think this will crash.

[Mihai Tica]

Created attachment 225055 [details]
Patch V4

Add SVG implementation. Add additional tests for HTML and SVG. Please have another look.

[Dirk Schulze]

Comment on attachment 225055 [details]
Patch V4

View in context: https://bugs.webkit.org/attachment.cgi?id=225055&action=review

The patch looks good to me. Just two snippets. I give the other reviewers some time to review the patch as well before giving my r+. Maybe Simon has more comments.

> Source/WebCore/rendering/style/RenderStyle.h:974
> +    void setBlendMode(BlendMode v) { SET_VAR(rareNonInheritedData, m_effectiveBlendMode, v); }
>      bool hasBlendMode() const { return static_cast<BlendMode>(rareNonInheritedData->m_effectiveBlendMode) != BlendModeNormal; }
> +
> +    Isolation isolation() const { return static_cast<Isolation>(rareNonInheritedData->m_isolation); }
> +    void setIsolation(Isolation v) { SET_VAR(rareNonInheritedData, m_isolation, v); }

should still have longer names like blendMode and isolation instead of v. Regardless of the use elsewhere.

[Mihai Tica]

Created attachment 225068 [details]
Patch V4. Addressed Dirk's comments

[Dirk Schulze]

Comment on attachment 225068 [details]
Patch V4. Addressed Dirk's comments

LGTM. Since no further review comments came from Simon and kling: r=me

[WebKit Commit Bot]

Comment on attachment 225068 [details]
Patch V4. Addressed Dirk's comments

Clearing flags on attachment: 225068

Committed r164795: <http://trac.webkit.org/changeset/164795>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Alexey Proskuryakov]

css3/compositing/isolation-isolate-blended-child.html test added in this patch fails every time on one of the bots. Filed bug 129468.