Bug 121776

Summary: Add toWebKitCSS*Value functions to cast from CSSValue
Product: WebKit Reporter: Gyuyoung Kim <gyuyoung.kim>
Component: CSSAssignee: Gyuyoung Kim <gyuyoung.kim>
Status: RESOLVED FIXED    
Severity: Normal CC: allan.jensen, commit-queue, esprehn+autocc, glenn, kling, macpherson, menard
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 121820, 121894    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch for ews none

Description Gyuyoung Kim 2013-09-22 20:33:56 PDT
CSS_VALUE_TYPE_CASTS can't cover WebKitCSS*Value classes. So, this patch adds toWebKitCSS*Value manually.
Comment 1 Gyuyoung Kim 2013-09-22 20:36:42 PDT
Created attachment 212318 [details]
Patch
Comment 2 Gyuyoung Kim 2013-09-22 21:20:11 PDT
CC'ing Kling. Could you review this patch ?
Comment 3 Darin Adler 2013-09-23 09:58:48 PDT
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.
Comment 4 Darin Adler 2013-09-23 09:59:24 PDT
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?
Comment 5 Gyuyoung Kim 2013-09-23 16:54:16 PDT
(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.
Comment 6 Gyuyoung Kim 2013-09-23 16:57:47 PDT
(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.
Comment 7 Gyuyoung Kim 2013-09-24 17:51:41 PDT
(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.
Comment 8 Gyuyoung Kim 2013-09-24 17:53:09 PDT
Comment on attachment 212318 [details]
Patch

I'd like to go to clean-up static_cast<CSS*Value>. So, this patch is landed.
Comment 9 WebKit Commit Bot 2013-09-24 18:16:44 PDT
Comment on attachment 212318 [details]
Patch

Clearing flags on attachment: 212318

Committed r156379: <http://trac.webkit.org/changeset/156379>
Comment 10 WebKit Commit Bot 2013-09-24 18:16:47 PDT
All reviewed patches have been landed.  Closing bug.
Comment 11 WebKit Commit Bot 2013-09-24 22:18:39 PDT
Re-opened since this is blocked by bug 121894
Comment 12 Alexey Proskuryakov 2013-09-24 22:23:36 PDT
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>.
Comment 13 Gyuyoung Kim 2013-09-24 22:34:28 PDT
(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.
Comment 14 Gyuyoung Kim 2013-09-27 01:28:52 PDT
Created attachment 212791 [details]
Patch for ews
Comment 15 Gyuyoung Kim 2013-09-27 05:35:19 PDT
I think Bug 121886 fixed assertion fails. So, I'd like to try to land this patch again.
Comment 16 WebKit Commit Bot 2013-09-27 07:30:30 PDT
Comment on attachment 212791 [details]
Patch for ews

Clearing flags on attachment: 212791

Committed r156542: <http://trac.webkit.org/changeset/156542>
Comment 17 WebKit Commit Bot 2013-09-27 07:30:34 PDT
All reviewed patches have been landed.  Closing bug.