Bug 121462 – Introduce CSS_VALUE_TYPE_CASTS macro in order to cast CSSValue type

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

[Gyuyoung Kim]

As r155429 introduced ELEMENT_TYPE_CASTS, CSS_VALUE_TYPE_CASTS can be used by css value type casting. This casting macro will help to detect bad-cast bugs. 

This patch adds the following methods,

 - CSSFooValue* toCSSFooValue(CSSValue*)
 - const CSSFooValue* toCSSFooValue(const CSSValue*)
 - CSSFooValue& toCSSFooValue(CSSValue&)
 - const CSSFooValue& toCSSFooValue(const CSSValue&)

[Gyuyoung Kim]

Created attachment 211845 [details]
WIP

[Gyuyoung Kim]

Created attachment 211865 [details]
Patch

[Gyuyoung Kim]

CC'ing kling, could you take a look this patch for CSSValue type casting ?

[Early Warning System Bot]

Comment on attachment 211865 [details]
Patch

Attachment 211865 [details] did not pass qt-ews (qt):
Output: http://webkit-queues.appspot.com/results/1872257

[Build Bot]

Comment on attachment 211865 [details]
Patch

Attachment 211865 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/1855248

[Build Bot]

Comment on attachment 211865 [details]
Patch

Attachment 211865 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/1893171

[Early Warning System Bot]

Comment on attachment 211865 [details]
Patch

Attachment 211865 [details] did not pass qt-wk2-ews (qt-wk2):
Output: http://webkit-queues.appspot.com/results/1859213

[Gyuyoung Kim]

Created attachment 211887 [details]
Patch

[Build Bot]

Comment on attachment 211887 [details]
Patch

Attachment 211887 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/1850365

New failing tests:
css3/filters/custom/custom-filter-matN.html
css3/filters/custom/custom-filter-u-tile-size.html
css3/filters/custom/custom-filter-property-computed-style.html
css3/filters/custom/custom-filter-blend-modes.html
css3/filters/custom/custom-filter-array.html
css3/filters/custom/custom-filter-u-texture-size.html
css3/filters/custom/custom-filter-shader-reuse.html
css3/filters/custom/custom-filter-property-parsing.html
css3/filters/custom/composited/custom-filter-blend-modes.html
css3/filters/custom/custom-filter-css-keyword-as-parameter-name.html
css3/filters/custom/custom-filter-clamp-css-mix-color.html
css3/filters/custom/custom-filter-composite-fractional-source-alpha.html
css3/filters/custom/custom-filter-u-mesh-size.html
css3/filters/custom/custom-filter-reload.html
css3/filters/custom/custom-filter-clamp-css-mix-color-negative.html
css3/filters/custom/custom-filter-clamp-css-color-matrix.html
css3/filters/custom/custom-filter-unavailable-varying.html
css3/filters/custom/custom-filter-change-blend-mode.html
css3/filters/custom/custom-filter-color-matrix.html
css3/filters/custom/custom-filter-color.html
css3/filters/custom/invalid-custom-filter-uniform-types.html
css3/filters/custom/custom-filter-mix-bindings.html
css3/filters/custom/custom-filter-u-mesh-box.html
css3/filters/custom/missing-custom-filter-shader.html
css3/filters/custom/invalid-custom-filter-shader.html
css3/filters/custom/custom-filter-clamp-css-color-matrix-negative.html

[Build Bot]

Created attachment 211894 [details]
Archive of layout-test-results from webkit-ews-10 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-10  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Andreas Kling]

Why do we need both CSS_VALUE_TYPE_CASTS and WEBKIT_CSS_VALUE_TYPE_CASTS?
Can't we just have one:

CSS_VALUE_TYPE_CASTS(CSSPrimitiveValue)
CSS_VALUE_TYPE_CASTS(WebKitCSSTransformValue)

[Build Bot]

Comment on attachment 211887 [details]
Patch

Attachment 211887 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/1893278

New failing tests:
css3/filters/custom/custom-filter-matN.html
css3/filters/custom/custom-filter-u-tile-size.html
css3/filters/custom/custom-filter-property-computed-style.html
css3/filters/custom/custom-filter-blend-modes.html
css3/filters/custom/custom-filter-array.html
css3/filters/custom/custom-filter-u-texture-size.html
css3/filters/custom/custom-filter-shader-reuse.html
css3/filters/custom/custom-filter-property-parsing.html
css3/filters/custom/composited/custom-filter-blend-modes.html
css3/filters/custom/custom-filter-css-keyword-as-parameter-name.html
css3/filters/custom/custom-filter-clamp-css-mix-color.html
css3/filters/custom/custom-filter-composite-fractional-source-alpha.html
css3/filters/custom/custom-filter-u-mesh-size.html
css3/filters/custom/custom-filter-reload.html
css3/filters/custom/custom-filter-clamp-css-mix-color-negative.html
css3/filters/custom/custom-filter-clamp-css-color-matrix.html
css3/filters/custom/custom-filter-unavailable-varying.html
css3/filters/custom/custom-filter-change-blend-mode.html
css3/filters/custom/custom-filter-color-matrix.html
css3/filters/custom/custom-filter-color.html
css3/filters/custom/invalid-custom-filter-uniform-types.html
css3/filters/custom/custom-filter-mix-bindings.html
css3/filters/custom/custom-filter-u-mesh-box.html
css3/filters/custom/missing-custom-filter-shader.html
css3/filters/custom/invalid-custom-filter-shader.html
css3/filters/custom/custom-filter-clamp-css-color-matrix-negative.html

[Build Bot]

Created attachment 211901 [details]
Archive of layout-test-results from webkit-ews-16 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-16  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 211887 [details]
Patch

Attachment 211887 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/1850383

New failing tests:
css3/filters/custom/custom-filter-matN.html
css3/filters/custom/custom-filter-u-tile-size.html
css3/filters/custom/custom-filter-property-computed-style.html
css3/filters/custom/custom-filter-blend-modes.html
css3/filters/custom/custom-filter-array.html
css3/filters/custom/custom-filter-u-texture-size.html
css3/filters/custom/custom-filter-shader-reuse.html
css3/filters/custom/custom-filter-property-parsing.html
css3/filters/custom/composited/custom-filter-blend-modes.html
css3/filters/custom/custom-filter-css-keyword-as-parameter-name.html
css3/filters/custom/custom-filter-clamp-css-mix-color.html
css3/filters/custom/custom-filter-composite-fractional-source-alpha.html
css3/filters/custom/custom-filter-u-mesh-size.html
css3/filters/custom/custom-filter-reload.html
css3/filters/custom/custom-filter-clamp-css-mix-color-negative.html
css3/filters/custom/custom-filter-clamp-css-color-matrix.html
css3/filters/custom/custom-filter-unavailable-varying.html
css3/filters/custom/custom-filter-change-blend-mode.html
css3/filters/custom/custom-filter-color-matrix.html
css3/filters/custom/custom-filter-color.html
css3/filters/custom/invalid-custom-filter-uniform-types.html
css3/filters/custom/custom-filter-mix-bindings.html
css3/filters/custom/custom-filter-u-mesh-box.html
css3/filters/custom/missing-custom-filter-shader.html
css3/filters/custom/invalid-custom-filter-shader.html
css3/filters/custom/custom-filter-clamp-css-color-matrix-negative.html

[Build Bot]

Created attachment 211905 [details]
Archive of layout-test-results from webkit-ews-02 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-02  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Gyuyoung Kim]

(In reply to comment #11)
> Why do we need both CSS_VALUE_TYPE_CASTS and WEBKIT_CSS_VALUE_TYPE_CASTS?
> Can't we just have one:
> 
> CSS_VALUE_TYPE_CASTS(CSSPrimitiveValue)
> CSS_VALUE_TYPE_CASTS(WebKitCSSTransformValue)

Because, naming style of is*Value() is different from isWebKitCSS*Value()

  bool isFilterImageValue() const { return m_classType == FilterImageClass; }
  bool isWebKitCSSFilterValue() const { return m_classType == WebKitCSSFilterClass; }

  ASSERT_WITH_SECURITY_IMPLICATION(!value || value->is##ValueTypeName()); \
  ASSERT_WITH_SECURITY_IMPLICATION(!value || value->isWebKitCSS##ValueTypeName()); \


I couldn't find a way to support both cases in CSS_VALUE_TYPE_CASTS macro. That's why I added two macros as below,Anyway, I will try to find a way to use one macro further. Any idea ?

[Gyuyoung Kim]

Created attachment 211978 [details]
Patch

[Early Warning System Bot]

Comment on attachment 211978 [details]
Patch

Attachment 211978 [details] did not pass qt-ews (qt):
Output: http://webkit-queues.appspot.com/results/1888184

[Early Warning System Bot]

Comment on attachment 211978 [details]
Patch

Attachment 211978 [details] did not pass qt-wk2-ews (qt-wk2):
Output: http://webkit-queues.appspot.com/results/1928112

[Build Bot]

Comment on attachment 211978 [details]
Patch

Attachment 211978 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/1851298

[Build Bot]

Comment on attachment 211978 [details]
Patch

Attachment 211978 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/1822482

[Gyuyoung Kim]

Created attachment 211994 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 211994 [details]
Patch

I only add CSS_VALUE_TYPE_CASTS at the moment.

[Gyuyoung Kim]

Created attachment 212037 [details]
Patch

[Andreas Kling]

Comment on attachment 212037 [details]
Patch

LGTM

[WebKit Commit Bot]

Comment on attachment 212037 [details]
Patch

Clearing flags on attachment: 212037

Committed r156253: <http://trac.webkit.org/changeset/156253>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 121778

[Gyuyoung Kim]

I feel this patch is too huge. It looks this patch needs to be separated. Patch will be prepared.

[Alexey Proskuryakov]

Rolled out in <http://trac.webkit.org/changeset/156260>, because this caused a bunch of assertion failures, and Gyuyoung agreed that this was the right course of action.

[Gyuyoung Kim]

Created attachment 212329 [details]
Patch

[Gyuyoung Kim]

(In reply to comment #31)
> Created an attachment (id=212329) [details]
> Patch

There is no layout test regression on latest patch. I will prepare following patches after landing this master patch.

[Darin Adler]

Comment on attachment 212329 [details]
Patch

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

> Source/WebCore/ChangeLog:16
> +        This patch support CSSImageSetValue and CSSREflectValue first. Other CSS*Values
> +        will use this macro step by step.

Typo: CSSReflectValue

> Source/WebCore/css/CSSValue.h:277
> +inline const CSS##ValueTypeName* toCSS##ValueTypeName(const CSSValue* value) \
> +{ \
> +    ASSERT_WITH_SECURITY_IMPLICATION(!value || value->is##ValueTypeName()); \
> +    return static_cast<const CSS##ValueTypeName*>(value); \
> +} \
> +inline CSS##ValueTypeName* toCSS##ValueTypeName(CSSValue* value) \
> +{ \
> +    ASSERT_WITH_SECURITY_IMPLICATION(!value || value->is##ValueTypeName()); \
> +    return static_cast<CSS##ValueTypeName*>(value); \
> +} \

Why no versions that take references? Maybe we should have versions that take RefPtr too?

[Gyuyoung Kim]

Created attachment 212408 [details]
Patch for landing

[Gyuyoung Kim]

(In reply to comment #33)

> > Source/WebCore/css/CSSValue.h:277
> > +inline const CSS##ValueTypeName* toCSS##ValueTypeName(const CSSValue* value) \
> > +{ \
> > +    ASSERT_WITH_SECURITY_IMPLICATION(!value || value->is##ValueTypeName()); \
> > +    return static_cast<const CSS##ValueTypeName*>(value); \
> > +} \
> > +inline CSS##ValueTypeName* toCSS##ValueTypeName(CSSValue* value) \
> > +{ \
> > +    ASSERT_WITH_SECURITY_IMPLICATION(!value || value->is##ValueTypeName()); \
> > +    return static_cast<CSS##ValueTypeName*>(value); \
> > +} \
> 
> Why no versions that take references? Maybe we should have versions that take RefPtr too?

Element.h also has similar inline functions. I think it would be good if we support version both together on new bug. I will file a bug.

[WebKit Commit Bot]

Comment on attachment 212408 [details]
Patch for landing

Rejecting attachment 212408 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.appspot.com', '--bot-id=webkit-cq-03', 'validate-changelog', '--check-oops', '--non-interactive', 212408, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

/Volumes/Data/EWS/WebKit/Source/WebCore/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

Full output: http://webkit-queues.appspot.com/results/1822648

[Gyuyoung Kim]

Created attachment 212412 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 212412 [details]
Patch for landing

Clearing flags on attachment: 212412

Committed r156313: <http://trac.webkit.org/changeset/156313>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.