Use is<>() / downcast<>() for CSSValue subclasses
Created attachment 239180 [details] Patch
Attachment 239180 [details] did not pass style-queue: ERROR: Source/WebCore/editing/EditingStyle.cpp:1494: Multi line control clauses should use braces. [whitespace/braces] [4] ERROR: Source/WebCore/css/CSSValueList.h:99: Should be indented on a separate line, with the colon or comma first on that line. [whitespace/indent] [4] Total errors found: 2 in 63 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 239182 [details] Patch
Attachment 239182 [details] did not pass style-queue: ERROR: Source/WebCore/editing/EditingStyle.cpp:1494: Multi line control clauses should use braces. [whitespace/braces] [4] ERROR: Source/WebCore/css/CSSValueList.h:99: Should be indented on a separate line, with the colon or comma first on that line. [whitespace/indent] [4] Total errors found: 2 in 63 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 239208 [details] Patch
Attachment 239208 [details] did not pass style-queue: ERROR: Source/WebCore/editing/EditingStyle.cpp:1494: Multi line control clauses should use braces. [whitespace/braces] [4] ERROR: Source/WebCore/css/CSSValueList.h:99: Should be indented on a separate line, with the colon or comma first on that line. [whitespace/indent] [4] Total errors found: 2 in 63 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 239209 [details] Patch
Attachment 239209 [details] did not pass style-queue: ERROR: Source/WebCore/editing/EditingStyle.cpp:1494: Multi line control clauses should use braces. [whitespace/braces] [4] ERROR: Source/WebCore/css/CSSValueList.h:99: Should be indented on a separate line, with the colon or comma first on that line. [whitespace/indent] [4] Total errors found: 2 in 63 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 239209 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=239209&action=review > Source/WebCore/css/CSSGradientValue.h:237 > +SPECIALIZE_TYPE_TRAITS_CSS_VALUE(CSSGradientValue, isGradientValue()) > +SPECIALIZE_TYPE_TRAITS_CSS_VALUE(CSSLinearGradientValue, isLinearGradientValue()) > +SPECIALIZE_TYPE_TRAITS_CSS_VALUE(CSSRadialGradientValue, isRadialGradientValue()) > + IMHO it is better to keep the traits close to the declaration of the class. > Source/WebCore/css/CSSParser.cpp:5968 > RefPtr<CSSValue> centerX; > RefPtr<CSSValue> centerY; > parseFillPosition(args, centerX, centerY); It's weird that this is using CSSValue instead of CSSPrimitiveValue directly. > Source/WebCore/css/CSSToStyleMap.cpp:548 > + for (unsigned i = 0 ; i < borderImage.length() ; ++i) { Style: semi-color space. > Source/WebCore/css/CSSValueList.h:100 > + { } Shouldn't this be on two lines. > Source/WebCore/css/DeprecatedStyleBuilder.cpp:983 > + ASSERT(is<CSSPrimitiveValue>(item)); Gosh, I hate this. This should be an ASSERT_WITH_MESSAGE. > Source/WebCore/css/DeprecatedStyleBuilder.cpp:1181 > - int length = list ? list->length() : 0; > + CSSValueList& list = downcast<CSSValueList>(*value); > + int length = list.length(); Useless code, good catch. > Source/WebCore/css/DeprecatedStyleBuilder.cpp:1224 > + int len = list.length(); While you are fixing this cap, let's rename len to length. > Source/WebCore/css/DeprecatedStyleBuilder.cpp:1282 > + ASSERT_WITH_SECURITY_IMPLICATION(is<CSSPrimitiveValue>(item)); IIRC, downcast<> already does that (if not it should). You should be able to drop the assertion. > Source/WebCore/css/DeprecatedStyleBuilder.cpp:1447 > + ASSERT_WITH_SECURITY_IMPLICATION(is<CSSPrimitiveValue>(item)); ditto > Source/WebCore/css/DeprecatedStyleBuilder.cpp:1474 > + else if (primitiveValue.isLength()) { > + lineHeight = primitiveValue.computeLength<Length>(csstoLengthConversionDataWithTextZoomFactor(*styleResolver)); > + } else if (primitiveValue.isPercentage()) { Coding style!!! > Source/WebCore/css/DeprecatedStyleBuilder.cpp:1557 > + else if (primitiveValue.isLength()) { > + wordSpacing = primitiveValue.computeLength<Length>(csstoLengthConversionDataWithTextZoomFactor(*styleResolver)); > + } else if (primitiveValue.isPercentage()) coding style > Source/WebCore/css/DeprecatedStyleBuilder.cpp:1943 > EResize r = RESIZE_NONE; arrrrrrr... Probably coded on talk-like-a-pirate day. > Source/WebCore/css/DeprecatedStyleBuilder.cpp:2254 > + for (size_t i = 0; i < valueList.length(); i++) { ++i > Source/WebCore/css/DeprecatedStyleBuilder.cpp:2256 > + CSSValue* item = valueList.itemWithoutBoundsCheck(i); > + if (!is<CSSPrimitiveValue>(*item)) Why not use a reference from the beginning? CSSValue& item = *valueList.itemWithoutBoundsCheck(i); > Source/WebCore/css/MediaQueryEvaluator.cpp:200 > + if (is<CSSPrimitiveValue>(*value) > + && downcast<CSSPrimitiveValue>(*value).isNumber()) { Could be on one line. > Source/WebCore/css/StyleResolver.cpp:2274 > + ASSERT_WITH_SECURITY_IMPLICATION(is<CSSPrimitiveValue>(*first)); > + ASSERT_WITH_SECURITY_IMPLICATION(is<CSSPrimitiveValue>(*second)); Necessary? > Source/WebCore/css/StyleResolver.cpp:3541 > + CSSValue* argument = filterValue.itemWithoutBoundsCheck(0); Why not CSSValue& argument? > Source/WebCore/css/StyleResolver.cpp:3625 > + CSSValue* cssValue = filterValue.itemWithoutBoundsCheck(0); CSSValue& ? > Source/WebCore/css/TransformFunctions.cpp:93 > CSSValue* currValue = i.value(); ditto? > Source/WebCore/css/WebKitCSSMatrix.cpp:66 > + if (!value || (is<CSSPrimitiveValue>(*value) && downcast<CSSPrimitiveValue>(*value).getValueID() == CSSValueNone)) Couldn't you remove the !value and use is<CSSPrimitiveValue>(value)? > Source/WebCore/editing/ApplyStyleCommand.cpp:1513 > + ASSERT(is<CSSPrimitiveValue>(value.get())); This shouldn't be needed. > Source/WebCore/editing/EditingStyle.cpp:1082 > + value = nullptr; // text-decoration: none is equivalent to not having the property Missing period. > Source/WebCore/editing/EditingStyle.cpp:1511 > + for (size_t i = 0; i < valuesInRefTextDecoration.length(); i++) ++i > Source/WebCore/rendering/RenderTextControl.cpp:87 > + if (is<CSSPrimitiveValue>(value.get())) I would be nice to teach the is<> template about Ref and RefPtr. > Source/WebCore/svg/SVGFontFaceElement.cpp:259 > for (unsigned i = 0; i < srcLength; i++) { ++i
Created attachment 239243 [details] Patch
Created attachment 239245 [details] Patch
Attachment 239245 [details] did not pass style-queue: ERROR: Source/WebCore/editing/EditingStyle.cpp:1494: Multi line control clauses should use braces. [whitespace/braces] [4] ERROR: Source/WebCore/css/CSSValueList.h:99: Should be indented on a separate line, with the colon or comma first on that line. [whitespace/indent] [4] Total errors found: 2 in 63 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 239245 [details] Patch Clearing flags on attachment: 239245 Committed r174300: <http://trac.webkit.org/changeset/174300>
All reviewed patches have been landed. Closing bug.