Bug 121776 – Add toWebKitCSS*Value functions to cast from CSSValue

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

[Gyuyoung Kim]

CSS_VALUE_TYPE_CASTS can't cover WebKitCSS*Value classes. So, this patch adds toWebKitCSS*Value manually.

[Gyuyoung Kim]

Created attachment 212318 [details]
Patch

[Gyuyoung Kim]

CC'ing Kling. Could you review this patch ?

[Darin Adler]

Comment on attachment 212318 [details]
Patch

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

r=me but we have follow-up work to do on toWebKitCSSShaderValue

> Source/WebCore/css/WebKitCSSShaderValue.h:76
>  inline WebKitCSSShaderValue* toWebKitCSSShaderValue(CSSValue* value)
>  {
> +    ASSERT_WITH_SECURITY_IMPLICATION(!value || value->isWebKitCSSShaderValue());
>      return value->isWebKitCSSShaderValue() ? static_cast<WebKitCSSShaderValue*>(value) : 0;
>  }

This is not right. The existing version of this function is doing a type check and returning nullptr. That does not fit the pattern. We should visit the call sites and add isWebKitCSSShaderValue checks as needed, and then remove that feature. We don’t want to leave things so that one of these toXXX functions does type checking, and lots of others do not.

[Darin Adler]

Comment on attachment 212318 [details]
Patch

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

> Source/WebCore/ChangeLog:8
> +        CSS_VALUE_TYPE_CASTS can't cover WebKitCSS*Value classes. So, this patch adds toWebKitCSS*Value manually.

Can’t we make a second macro for these ones with WebKit prefixes instead of writing them all by hand?

[Gyuyoung Kim]

(In reply to comment #3)
> (From update of attachment 212318 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=212318&action=review
> 
> r=me but we have follow-up work to do on toWebKitCSSShaderValue
> 
> > Source/WebCore/css/WebKitCSSShaderValue.h:76
> >  inline WebKitCSSShaderValue* toWebKitCSSShaderValue(CSSValue* value)
> >  {
> > +    ASSERT_WITH_SECURITY_IMPLICATION(!value || value->isWebKitCSSShaderValue());
> >      return value->isWebKitCSSShaderValue() ? static_cast<WebKitCSSShaderValue*>(value) : 0;
> >  }
> 
> This is not right. The existing version of this function is doing a type check and returning nullptr. That does not fit the pattern. We should visit the call sites and add isWebKitCSSShaderValue checks as needed, and then remove that feature. We don’t want to leave things so that one of these toXXX functions does type checking, and lots of others do not.

Ok, I will prepare a patch regarding your concern.

[Gyuyoung Kim]

(In reply to comment #4)
> (From update of attachment 212318 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=212318&action=review
> 
> > Source/WebCore/ChangeLog:8
> > +        CSS_VALUE_TYPE_CASTS can't cover WebKitCSS*Value classes. So, this patch adds toWebKitCSS*Value manually.
> 
> Can’t we make a second macro for these ones with WebKit prefixes instead of writing them all by hand?

In fact, I introduced WEBKIT_CSS_VALUE_TYPE_CASTS on https://bugs.webkit.org/attachment.cgi?id=211887&action=review as well. However,  two macros may confuse webkit developers. But, if there is no objection to support two macros, I can prepare "WEBKIT_CSS_VALUE_TYPE_CASTS" again.

[Gyuyoung Kim]

(In reply to comment #5)
> (In reply to comment #3)
> > (From update of attachment 212318 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=212318&action=review
> > 
> > r=me but we have follow-up work to do on toWebKitCSSShaderValue
> > 
> > > Source/WebCore/css/WebKitCSSShaderValue.h:76
> > >  inline WebKitCSSShaderValue* toWebKitCSSShaderValue(CSSValue* value)
> > >  {
> > > +    ASSERT_WITH_SECURITY_IMPLICATION(!value || value->isWebKitCSSShaderValue());
> > >      return value->isWebKitCSSShaderValue() ? static_cast<WebKitCSSShaderValue*>(value) : 0;
> > >  }
> > 
> > This is not right. The existing version of this function is doing a type check and returning nullptr. That does not fit the pattern. We should visit the call sites and add isWebKitCSSShaderValue checks as needed, and then remove that feature. We don’t want to leave things so that one of these toXXX functions does type checking, and lots of others do not.
> 
> Ok, I will prepare a patch regarding your concern.

I file a Bug 121886 for this issue.

[Gyuyoung Kim]

Comment on attachment 212318 [details]
Patch

I'd like to go to clean-up static_cast<CSS*Value>. So, this patch is landed.

[WebKit Commit Bot]

Comment on attachment 212318 [details]
Patch

Clearing flags on attachment: 212318

Committed r156379: <http://trac.webkit.org/changeset/156379>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 121894

[Alexey Proskuryakov]

This caused multiple assertion failures: <http://build.webkit.org/results/Apple%20MountainLion%20Debug%20WK1%20(Tests)/r156381%20(10381)/results.html>. Something is still wrong with shader values.

Rolled out in <http://trac.webkit.org/changeset/156383>.

[Gyuyoung Kim]

(In reply to comment #12)
> This caused multiple assertion failures: <http://build.webkit.org/results/Apple%20MountainLion%20Debug%20WK1%20(Tests)/r156381%20(10381)/results.html>. Something is still wrong with shader values.
> 
> Rolled out in <http://trac.webkit.org/changeset/156383>.

Let me check what is wrong.

[Gyuyoung Kim]

Created attachment 212791 [details]
Patch for ews

[Gyuyoung Kim]

I think Bug 121886 fixed assertion fails. So, I'd like to try to land this patch again.

[WebKit Commit Bot]

Comment on attachment 212791 [details]
Patch for ews

Clearing flags on attachment: 212791

Committed r156542: <http://trac.webkit.org/changeset/156542>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.