Need to add parsing of 3D transform functions and 3D related properties (perspective, perspective-origin, transform-style-3d, and backface-visibility).
Created attachment 27582 [details] Patch with test
Comment on attachment 27582 [details] Patch with test > Index: WebCore/css/CSSComputedStyleDeclaration.cpp > =================================================================== > - RefPtr<WebKitCSSTransformValue> transformVal = WebKitCSSTransformValue::create(WebKitCSSTransformValue::MatrixTransformOperation); > + RefPtr<WebKitCSSTransformValue> transformVal; Extra space there. > + if (transform.isAffine()) { > + transformVal = WebKitCSSTransformValue::create(WebKitCSSTransformValue::MatrixTransformOperation); > + > + transformVal->append(CSSPrimitiveValue::create(transform.a(), CSSPrimitiveValue::CSS_NUMBER)); > + transformVal->append(CSSPrimitiveValue::create(transform.b(), CSSPrimitiveValue::CSS_NUMBER)); > + transformVal->append(CSSPrimitiveValue::create(transform.c(), CSSPrimitiveValue::CSS_NUMBER)); > + transformVal->append(CSSPrimitiveValue::create(transform.d(), CSSPrimitiveValue::CSS_NUMBER)); > + transformVal->append(CSSPrimitiveValue::create(transform.e(), CSSPrimitiveValue::CSS_NUMBER)); > + transformVal->append(CSSPrimitiveValue::create(transform.f(), CSSPrimitiveValue::CSS_NUMBER)); > + } else { > + transformVal = WebKitCSSTransformValue::create(WebKitCSSTransformValue::Matrix3DTransformOperation); > + > + transformVal->append(CSSPrimitiveValue::create(transform.m11(), CSSPrimitiveValue::CSS_NUMBER)); > + transformVal->append(CSSPrimitiveValue::create(transform.m12(), CSSPrimitiveValue::CSS_NUMBER)); Add a FIXME comment about maybe printing out the individual operations if we can. > + case CSSPropertyWebkitBackfaceVisibility: > + return CSSPrimitiveValue::createIdentifier(style->backfaceVisibility() ? CSSValueVisible : CSSValueHidden); This should use the enum values you added for backfaceVisibility(). > + case CSSPropertyWebkitTransformStyle: > + return CSSPrimitiveValue::createIdentifier(style->transformStyle3D() ? CSSValuePreserve3d : CSSValueFlat); Weird spacing, or tabs there. Also should use the enum values for transformStyle3D(). > Index: WebCore/css/CSSParser.cpp > =================================================================== > --- WebCore/css/CSSParser.cpp (revision 40876) > +++ WebCore/css/CSSParser.cpp (working copy) > @@ -1298,11 +1298,52 @@ bool CSSParser::parseValue(int propId, b > break; > case CSSPropertyWebkitTransformOrigin: > case CSSPropertyWebkitTransformOriginX: > - case CSSPropertyWebkitTransformOriginY: { > + case CSSPropertyWebkitTransformOriginY: > + case CSSPropertyWebkitTransformOriginZ: > + { Brace should be on previous line. > + else if (equalIgnoringCase(name, "scalez(")) { > + m_unit = CSSParser::FNumber; > + m_type = WebKitCSSTransformValue::ScaleZTransformOperation; > + } else if (equalIgnoringCase(name, "scale3d(")) { > + m_type = WebKitCSSTransformValue::Scale3DTransformOperation; > + m_argCount = 5; > + m_unit = CSSParser::FNumber; > + } else if (equalIgnoringCase(name, "rotatex(") || > + equalIgnoringCase(name, "rotatey(") || > + equalIgnoringCase(name, "rotatez(")) { > + m_unit = CSSParser::FAngle; > + if (equalIgnoringCase(name, "rotatex(")) > + m_type = WebKitCSSTransformValue::RotateXTransformOperation; > + else if (equalIgnoringCase(name, "rotatey(")) > + m_type = WebKitCSSTransformValue::RotateYTransformOperation; > + else > + m_type = WebKitCSSTransformValue::RotateZTransformOperation; > + } else if (equalIgnoringCase(name, "rotate3d(")) { > + m_type = WebKitCSSTransformValue::Rotate3DTransformOperation; > + m_argCount = 7; > + m_unit = CSSParser::FNumber; > + } else if (equalIgnoringCase(name, "translatez(")) { > + m_unit = CSSParser::FLength | CSSParser::FPercent; > + m_type = WebKitCSSTransformValue::TranslateZTransformOperation; > + } else if (equalIgnoringCase(name, "translate3d(")) { > + m_type = WebKitCSSTransformValue::Translate3DTransformOperation; > + m_argCount = 5; > + m_unit = CSSParser::FLength | CSSParser::FPercent; > + } else if (equalIgnoringCase(name, "matrix3d(")) { > + m_type = WebKitCSSTransformValue::Matrix3DTransformOperation; > + m_argCount = 31; > + m_unit = CSSParser::FNumber; > + } else if (equalIgnoringCase(name, "perspective(")) { > + m_type = WebKitCSSTransformValue::PerspectiveTransformOperation; > + m_unit = CSSParser::FNumber; I think we should intersperse the 3d properties with the related 2d ones, rather than separating them out like this. > - if (!validUnit(a, unit, true)) > + if (info.type() == WebKitCSSTransformValue::Rotate3DTransformOperation && argNumber == 3) { Could use a comment to say why the 3rd param to Rotate3DTransformOperation is special. > - parseFillPosition(value, value2); > - // Unlike the other functions, parseFillPosition advances the m_valueList pointer > + parseTransformOrigin(value, value2, value3); > + // Unlike the other functions, parseTransformOrigin advances the m_valueList pointer So now there are two functions that advance the m_valueList pointer? Comments need updating. > + case CSSPropertyWebkitTransformOriginZ: { > + if (validUnit(m_valueList->current(), FLength, m_strict)) > + value = CSSPrimitiveValue::create(m_valueList->current()->fValue, > + (CSSPrimitiveValue::UnitTypes)m_valueList->current()->unit); Bad indentation. > - return value; > + return (value != 0); No need for this change. > + return (value != 0); "return value;" for consistency. > Index: WebCore/css/CSSParser.h > =================================================================== > --- WebCore/css/CSSParser.h (revision 40876) > +++ WebCore/css/CSSParser.h (working copy) > @@ -97,6 +97,7 @@ namespace WebCore { > PassRefPtr<CSSValue> parseAnimationProperty(); > PassRefPtr<CSSValue> parseAnimationTimingFunction(); > > + void parseTransformOrigin(RefPtr<CSSValue>&, RefPtr<CSSValue>&, RefPtr<CSSValue>&); > bool parseTimingFunctionValue(CSSParserValueList*& args, double& result); > bool parseAnimationProperty(int propId, RefPtr<CSSValue>&); > bool parseTransitionShorthand(bool important); > @@ -144,8 +145,8 @@ namespace WebCore { > bool parseGradient(RefPtr<CSSValue>&); > > PassRefPtr<CSSValueList> parseTransform(); > - bool parseTransformOrigin(int propId, int& propId1, int& propId2, RefPtr<CSSValue>&, RefPtr<CSSValue>&); > - > + bool parseTransformOrigin(int propId, int& propId1, int& propId2, int& propId3, RefPtr<CSSValue>&, RefPtr<CSSValue>&, RefPtr<CSSValue>&); > + bool parsePerspectiveOrigin(int propId, int& propId1, int& propId, RefPtr<CSSValue>&, RefPtr<CSSValue>&); > bool parseVariable(CSSVariablesDeclaration*, const String& variableName, const String& variableValue); > void parsePropertyWithResolvedVariables(int propId, bool important, CSSMutableStyleDeclaration*, CSSParserValueList*); I really don't like the overloading; maybe rename new methods to avoid adding more? > Index: WebCore/css/CSSStyleSelector.cpp > =================================================================== > + case CSSPropertyWebkitBackfaceVisibility: > + HANDLE_INHERIT_AND_INITIAL(backfaceVisibility, BackfaceVisibility) > + m_style->setBackfaceVisibility((primitiveValue && primitiveValue->getIdent() == CSSValueVisible) ? BackfaceVisibilityVisible : BackfaceVisibilityHidden); I'd do the if (primitiveValue) test separately like the other places do. > + case CSSPropertyWebkitTransformOriginZ: { > + HANDLE_INHERIT_AND_INITIAL(transformOriginZ, TransformOriginZ) > + CSSPrimitiveValue* primitiveValue = static_cast<CSSPrimitiveValue*>(value); > + float f; > + int type = primitiveValue->primitiveType(); > + if (type > CSSPrimitiveValue::CSS_PERCENTAGE && type < CSSPrimitiveValue::CSS_DEG) > + f = (float) primitiveValue->computeLengthIntForLength(style()); static_cast<float> > + case CSSPropertyWebkitTransformStyle: > + HANDLE_INHERIT_AND_INITIAL(transformStyle3D, TransformStyle3D) > + m_style->setTransformStyle3D((primitiveValue && primitiveValue->getIdent() == CSSValuePreserve3d) ? TransformStyle3DPreserve3D : TransformStyle3DFlat); Test primitiveValue separately. > + return; ^^^^^^ extra whitespace. > + float perspectiveValue = (float) primitiveValue->getDoubleValue(); static_cast<> > + case CSSPropertyWebkitPerspectiveOrigin: > + HANDLE_INHERIT_AND_INITIAL(perspectiveOriginX, PerspectiveOriginX) > + HANDLE_INHERIT_AND_INITIAL(perspectiveOriginY, PerspectiveOriginY) > + return; > + case CSSPropertyWebkitPerspectiveOriginX: { > + HANDLE_INHERIT_AND_INITIAL(perspectiveOriginX, PerspectiveOriginX) > + CSSPrimitiveValue* primitiveValue = static_cast<CSSPrimitiveValue*>(value); > + Length l; > + int type = primitiveValue->primitiveType(); > + if (type > CSSPrimitiveValue::CSS_PERCENTAGE && type < CSSPrimitiveValue::CSS_DEG) It's odd seeing the (type > CSSPrimitiveValue::CSS_PERCENTAGE && type < CSSPrimitiveValue::CSS_DEG) test everywhere. Seems like a good candidate for an inline function (isLengthType()), and replace all uses. > + case WebKitCSSTransformValue::ScaleZTransformOperation: > + case WebKitCSSTransformValue::Scale3DTransformOperation: { No need to separate this code from the 2d scale logic. Intersperse them, I think. > + case WebKitCSSTransformValue::TranslateZTransformOperation: > + case WebKitCSSTransformValue::Translate3DTransformOperation: { Ditto. > + case WebKitCSSTransformValue::RotateXTransformOperation: > + case WebKitCSSTransformValue::RotateYTransformOperation: > + case WebKitCSSTransformValue::RotateZTransformOperation: { Ditto. > @@ -5908,6 +6081,33 @@ bool CSSStyleSelector::createTransformOp > operations.operations().append(MatrixTransformOperation::create(a, b, c, d, e, f)); > break; > } > + case WebKitCSSTransformValue::Matrix3DTransformOperation: { > + TransformationMatrix matrix(static_cast<CSSPrimitiveValue*>(val->itemWithoutBoundsCheck(0))->getFloatValue(), Use getDoubleValue everywhere now? > Index: WebCore/css/WebKitCSSTransformValue.idl > =================================================================== > --- WebCore/css/WebKitCSSTransformValue.idl (revision 40876) > +++ WebCore/css/WebKitCSSTransformValue.idl (working copy) > @@ -48,6 +48,16 @@ module css { > const unsigned short CSS_SKEWX = 9; > const unsigned short CSS_SKEWY = 10; > const unsigned short CSS_MATRIX = 11; > + const unsigned short CSS_TRANSLATEZ = 12; > + const unsigned short CSS_TRANSLATE3D = 13; > + const unsigned short CSS_ROTATEX = 14; > + const unsigned short CSS_ROTATEY = 15; > + const unsigned short CSS_ROTATEZ = 16; Do we want CSS_ROTATEZ as a separate type from CSS_ROTATE? I think another round would be good, and I'd like hyatt to do the final review.
Created attachment 27609 [details] replacement patch This patch addresses all issues, except the last (ROTATE vs ROTATE_Z), which is a spec issue and requires more discussion.
Comment on attachment 27609 [details] replacement patch r=me
http://trac.webkit.org/changeset/40939