[webkit-reviews] review denied: [Bug 23908] Add CSS parsing and style selection for 3D properties : [Attachment 27582] Patch with test
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Wed Feb 11 18:45:13 PST 2009
Simon Fraser (smfr) <simon.fraser at apple.com> has denied Chris Marrin
<cmarrin at apple.com>'s request for review:
Bug 23908: Add CSS parsing and style selection for 3D properties
https://bugs.webkit.org/show_bug.cgi?id=23908
Attachment 27582: Patch with test
https://bugs.webkit.org/attachment.cgi?id=27582&action=review
------- Additional Comments from Simon Fraser (smfr) <simon.fraser at apple.com>
> Index: WebCore/css/CSSComputedStyleDeclaration.cpp
> ===================================================================
> - RefPtr<WebKitCSSTransformValue> transformVal =
WebKitCSSTransformValue::create(WebKitCSSTransformValue::MatrixTransformOperati
on);
> + RefPtr<WebKitCSSTransformValue> transformVal;
Extra space there.
> + if (transform.isAffine()) {
> + transformVal =
WebKitCSSTransformValue::create(WebKitCSSTransformValue::MatrixTransformOperati
on);
> +
> + 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::Matrix3DTransformOpera
tion);
> +
> + 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))->getFloa
tValue(),
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.
More information about the webkit-reviews
mailing list