Skip to content

Commit

Permalink
Implement API for shorthand properties animation and transition.
Browse files Browse the repository at this point in the history 
This cl is a resubmission of an earlier cl (crrev.com/c/549675), which
was reverted to fix a crash on one specific version of Google Pixel.
The difference between this cl and the previous cl is in the
implementation of CSSPropertyAnimationUtils::ConsumeAnimationShorthand:
- The old version is a templated function that uses function callback
  and variable number of args.
- The new version uses a function pointer, which is simpler than a
  callback.

The original crash was not reproducible using the old patch. 
But it is probably better to use function pointers instead of 
a callback function with variable number of bugs. 

Have run this new patch on a pixel phone and there was not crash. 

Bug: 668012
Change-Id: I183df5f3a168fffbed253a8b47b177fa72cd06b4
Reviewed-on: https://chromium-review.googlesource.com/569505
Commit-Queue: Jia Meng <jiameng@chromium.org>
Reviewed-by: Alan Cutter <alancutter@chromium.org>
Cr-Commit-Position: refs/heads/master@{#487372}
  • Loading branch information
@jm318
jm318 authored and Commit Bot committed Jul 18, 2017
1 parent b7d775d commit 51c2bff
Show file tree
Hide file tree
Showing 11 changed files with 423 additions and 201 deletions.
2 changes: 2 additions & 0 deletions third_party/WebKit/Source/core/BUILD.gn
View file
Original file line number Diff line number Diff line change
Expand Up @@ -592,6 +592,7 @@ css_properties("make_core_generated_css_property_apis") {
"$blink_core_output_dir/css/properties/CSSPropertyAPIZoom.h",
"$blink_core_output_dir/css/properties/CSSPropertyDescriptor.cpp",
"$blink_core_output_dir/css/properties/CSSPropertyDescriptor.h",
"$blink_core_output_dir/css/properties/CSSShorthandPropertyAPIAnimation.h",
"$blink_core_output_dir/css/properties/CSSShorthandPropertyAPIBorderImage.h",
"$blink_core_output_dir/css/properties/CSSShorthandPropertyAPIBorderRadius.h",
"$blink_core_output_dir/css/properties/CSSShorthandPropertyAPIBorderSpacing.h",
Expand All @@ -608,6 +609,7 @@ css_properties("make_core_generated_css_property_apis") {
"$blink_core_output_dir/css/properties/CSSShorthandPropertyAPIScrollSnapMarginBlock.h",
"$blink_core_output_dir/css/properties/CSSShorthandPropertyAPIScrollSnapMarginInline.h",
"$blink_core_output_dir/css/properties/CSSShorthandPropertyAPITextDecoration.h",
"$blink_core_output_dir/css/properties/CSSShorthandPropertyAPITransition.h",
"$blink_core_output_dir/css/properties/CSSShorthandPropertyAPIWebkitBorderAfter.h",
"$blink_core_output_dir/css/properties/CSSShorthandPropertyAPIWebkitBorderBefore.h",
"$blink_core_output_dir/css/properties/CSSShorthandPropertyAPIWebkitBorderEnd.h",
Expand Down
6 changes: 6 additions & 0 deletions third_party/WebKit/Source/core/css/BUILD.gn
View file
Original file line number Diff line number Diff line change
Expand Up @@ -506,6 +506,10 @@ blink_core_sources("css") {
"properties/CSSPropertyAnimationIterationCountUtils.h",
"properties/CSSPropertyAnimationNameUtils.cpp",
"properties/CSSPropertyAnimationNameUtils.h",
"properties/CSSPropertyAnimationTimingFunctionUtils.cpp",
"properties/CSSPropertyAnimationTimingFunctionUtils.h",
"properties/CSSPropertyAnimationUtils.cpp",
"properties/CSSPropertyAnimationUtils.h",
"properties/CSSPropertyBorderImageUtils.cpp",
"properties/CSSPropertyBorderImageUtils.h",
"properties/CSSPropertyBoxShadowUtils.cpp",
Expand Down Expand Up @@ -535,6 +539,7 @@ blink_core_sources("css") {
"properties/CSSPropertyTransitionPropertyUtils.h",
"properties/CSSPropertyWebkitBorderWidthUtils.cpp",
"properties/CSSPropertyWebkitBorderWidthUtils.h",
"properties/CSSShorthandPropertyAPIAnimation.cpp",
"properties/CSSShorthandPropertyAPIBorderImage.cpp",
"properties/CSSShorthandPropertyAPIBorderRadius.cpp",
"properties/CSSShorthandPropertyAPIBorderSpacing.cpp",
Expand All @@ -551,6 +556,7 @@ blink_core_sources("css") {
"properties/CSSShorthandPropertyAPIScrollSnapMarginBlock.cpp",
"properties/CSSShorthandPropertyAPIScrollSnapMarginInline.cpp",
"properties/CSSShorthandPropertyAPITextDecoration.cpp",
"properties/CSSShorthandPropertyAPITransition.cpp",
"properties/CSSShorthandPropertyAPIWebkitBorderAfter.cpp",
"properties/CSSShorthandPropertyAPIWebkitBorderBefore.cpp",
"properties/CSSShorthandPropertyAPIWebkitBorderEnd.cpp",
Expand Down
4 changes: 4 additions & 0 deletions third_party/WebKit/Source/core/css/CSSProperties.json5
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3663,6 +3663,8 @@
{
name: "animation",
longhands: ["animation-name", "animation-duration", "animation-timing-function", "animation-delay", "animation-iteration-count", "animation-direction", "animation-fill-mode", "animation-play-state"],
api_class: true,
api_methods: ["parseShorthand"],
},
{
name: "background",
Expand Down Expand Up @@ -3885,6 +3887,8 @@
{
name: "transition",
longhands: ["transition-property", "transition-duration", "transition-timing-function", "transition-delay"],
api_class: true,
api_methods: ["parseShorthand"],
},
{
name: "-webkit-border-after",
Expand Down
201 changes: 4 additions & 197 deletions third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@
#include "core/css/CSSReflectValue.h"
#include "core/css/CSSShadowValue.h"
#include "core/css/CSSStringValue.h"
#include "core/css/CSSTimingFunctionValue.h"
#include "core/css/CSSURIValue.h"
#include "core/css/CSSUnicodeRangeValue.h"
#include "core/css/CSSUnsetValue.h"
Expand All @@ -35,8 +34,7 @@
#include "core/css/parser/CSSPropertyParserHelpers.h"
#include "core/css/parser/CSSVariableParser.h"
#include "core/css/properties/CSSPropertyAlignmentUtils.h"
#include "core/css/properties/CSSPropertyAnimationIterationCountUtils.h"
#include "core/css/properties/CSSPropertyAnimationNameUtils.h"
#include "core/css/properties/CSSPropertyAnimationTimingFunctionUtils.h"
#include "core/css/properties/CSSPropertyBorderImageUtils.h"
#include "core/css/properties/CSSPropertyBoxShadowUtils.h"
#include "core/css/properties/CSSPropertyDescriptor.h"
Expand Down Expand Up @@ -311,192 +309,6 @@ static CSSValue* ConsumeLocale(CSSParserTokenRange& range) {
return ConsumeString(range);
}

static CSSValue* ConsumeSteps(CSSParserTokenRange& range) {
DCHECK_EQ(range.Peek().FunctionId(), CSSValueSteps);
CSSParserTokenRange range_copy = range;
CSSParserTokenRange args = ConsumeFunction(range_copy);

CSSPrimitiveValue* steps = ConsumePositiveInteger(args);
if (!steps)
return nullptr;

StepsTimingFunction::StepPosition position =
StepsTimingFunction::StepPosition::END;
if (ConsumeCommaIncludingWhitespace(args)) {
switch (args.ConsumeIncludingWhitespace().Id()) {
case CSSValueMiddle:
if (!RuntimeEnabledFeatures::WebAnimationsAPIEnabled())
return nullptr;
position = StepsTimingFunction::StepPosition::MIDDLE;
break;
case CSSValueStart:
position = StepsTimingFunction::StepPosition::START;
break;
case CSSValueEnd:
position = StepsTimingFunction::StepPosition::END;
break;
default:
return nullptr;
}
}

if (!args.AtEnd())
return nullptr;

range = range_copy;
return CSSStepsTimingFunctionValue::Create(steps->GetIntValue(), position);
}

static CSSValue* ConsumeFrames(CSSParserTokenRange& range) {
DCHECK_EQ(range.Peek().FunctionId(), CSSValueFrames);
CSSParserTokenRange range_copy = range;
CSSParserTokenRange args = ConsumeFunction(range_copy);

CSSPrimitiveValue* frames = ConsumePositiveInteger(args);
if (!frames)
return nullptr;

int frames_int = frames->GetIntValue();
if (frames_int <= 1)
return nullptr;

if (!args.AtEnd())
return nullptr;

range = range_copy;
return CSSFramesTimingFunctionValue::Create(frames_int);
}

static CSSValue* ConsumeCubicBezier(CSSParserTokenRange& range) {
DCHECK_EQ(range.Peek().FunctionId(), CSSValueCubicBezier);
CSSParserTokenRange range_copy = range;
CSSParserTokenRange args = ConsumeFunction(range_copy);

double x1, y1, x2, y2;
if (ConsumeNumberRaw(args, x1) && x1 >= 0 && x1 <= 1 &&
ConsumeCommaIncludingWhitespace(args) && ConsumeNumberRaw(args, y1) &&
ConsumeCommaIncludingWhitespace(args) && ConsumeNumberRaw(args, x2) &&
x2 >= 0 && x2 <= 1 && ConsumeCommaIncludingWhitespace(args) &&
ConsumeNumberRaw(args, y2) && args.AtEnd()) {
range = range_copy;
return CSSCubicBezierTimingFunctionValue::Create(x1, y1, x2, y2);
}

return nullptr;
}

static CSSValue* ConsumeAnimationTimingFunction(CSSParserTokenRange& range) {
CSSValueID id = range.Peek().Id();
if (id == CSSValueEase || id == CSSValueLinear || id == CSSValueEaseIn ||
id == CSSValueEaseOut || id == CSSValueEaseInOut ||
id == CSSValueStepStart || id == CSSValueStepEnd ||
id == CSSValueStepMiddle)
return ConsumeIdent(range);

CSSValueID function = range.Peek().FunctionId();
if (function == CSSValueSteps)
return ConsumeSteps(range);
if (RuntimeEnabledFeatures::FramesTimingFunctionEnabled() &&
function == CSSValueFrames) {
return ConsumeFrames(range);
}
if (function == CSSValueCubicBezier)
return ConsumeCubicBezier(range);
return nullptr;
}

static CSSValue* ConsumeAnimationValue(CSSPropertyID property,
CSSParserTokenRange& range,
const CSSParserContext* context,
bool use_legacy_parsing) {
switch (property) {
case CSSPropertyAnimationDelay:
case CSSPropertyTransitionDelay:
return ConsumeTime(range, kValueRangeAll);
case CSSPropertyAnimationDirection:
return ConsumeIdent<CSSValueNormal, CSSValueAlternate, CSSValueReverse,
CSSValueAlternateReverse>(range);
case CSSPropertyAnimationDuration:
case CSSPropertyTransitionDuration:
return ConsumeTime(range, kValueRangeNonNegative);
case CSSPropertyAnimationFillMode:
return ConsumeIdent<CSSValueNone, CSSValueForwards, CSSValueBackwards,
CSSValueBoth>(range);
case CSSPropertyAnimationIterationCount:
return CSSPropertyAnimationIterationCountUtils::
ConsumeAnimationIterationCount(range);
case CSSPropertyAnimationName:
return CSSPropertyAnimationNameUtils::ConsumeAnimationName(
range, context, use_legacy_parsing);
case CSSPropertyAnimationPlayState:
return ConsumeIdent<CSSValueRunning, CSSValuePaused>(range);
case CSSPropertyTransitionProperty:
return CSSPropertyTransitionPropertyUtils::ConsumeTransitionProperty(
range);
case CSSPropertyAnimationTimingFunction:
case CSSPropertyTransitionTimingFunction:
return ConsumeAnimationTimingFunction(range);
default:
NOTREACHED();
return nullptr;
}
}

bool CSSPropertyParser::ConsumeAnimationShorthand(
const StylePropertyShorthand& shorthand,
bool use_legacy_parsing,
bool important) {
const unsigned longhand_count = shorthand.length();
CSSValueList* longhands[8];
DCHECK_LE(longhand_count, 8u);
for (size_t i = 0; i < longhand_count; ++i)
longhands[i] = CSSValueList::CreateCommaSeparated();

do {
bool parsed_longhand[8] = {false};
do {
bool found_property = false;
for (size_t i = 0; i < longhand_count; ++i) {
if (parsed_longhand[i])
continue;

if (CSSValue* value =
ConsumeAnimationValue(shorthand.properties()[i], range_,
context_, use_legacy_parsing)) {
parsed_longhand[i] = true;
found_property = true;
longhands[i]->Append(*value);
break;
}
}
if (!found_property)
return false;
} while (!range_.AtEnd() && range_.Peek().GetType() != kCommaToken);

// TODO(timloh): This will make invalid longhands, see crbug.com/386459
for (size_t i = 0; i < longhand_count; ++i) {
if (!parsed_longhand[i])
longhands[i]->Append(*CSSInitialValue::Create());
parsed_longhand[i] = false;
}
} while (ConsumeCommaIncludingWhitespace(range_));

for (size_t i = 0; i < longhand_count; ++i) {
// TODO(bugsnash): Refactor out the need to check for
// CSSPropertyTransitionProperty here when this is method implemented in the
// property APIs
if (shorthand.properties()[i] == CSSPropertyTransitionProperty &&
!CSSPropertyTransitionPropertyUtils::IsValidPropertyList(*longhands[i]))
return false;
}

for (size_t i = 0; i < longhand_count; ++i) {
AddParsedProperty(shorthand.properties()[i], shorthand.id(), *longhands[i],
important);
}
return range_.AtEnd();
}

static CSSFunctionValue* ConsumeFilterFunction(
CSSParserTokenRange& range,
const CSSParserContext* context) {
Expand Down Expand Up @@ -1216,7 +1028,9 @@ const CSSValue* CSSPropertyParser::ParseSingleValue(
kValueRangeNonNegative);
case CSSPropertyAnimationTimingFunction:
case CSSPropertyTransitionTimingFunction:
return ConsumeCommaSeparatedList(ConsumeAnimationTimingFunction, range_);
return ConsumeCommaSeparatedList(CSSPropertyAnimationTimingFunctionUtils::
ConsumeAnimationTimingFunction,
range_);
case CSSPropertyGridColumnGap:
case CSSPropertyGridRowGap:
return ConsumeLengthOrPercent(range_, context_->Mode(),
Expand Down Expand Up @@ -2321,13 +2135,6 @@ bool CSSPropertyParser::ParseShorthand(CSSPropertyID unresolved_property,
}

switch (property) {
case CSSPropertyAnimation:
return ConsumeAnimationShorthand(
animationShorthandForParsing(),
unresolved_property == CSSPropertyAliasWebkitAnimation, important);
case CSSPropertyTransition:
return ConsumeAnimationShorthand(transitionShorthandForParsing(), false,
important);
case CSSPropertyTextDecoration:
DCHECK(RuntimeEnabledFeatures::CSS3TextDecorationsEnabled());
return ConsumeShorthandGreedily(textDecorationShorthand(), important);
Expand Down
4 changes: 0 additions & 4 deletions third_party/WebKit/Source/core/css/parser/CSSPropertyParser.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -88,10 +88,6 @@ class CSSPropertyParser {
bool Consume2Values(const StylePropertyShorthand&, bool important);
bool Consume4Values(const StylePropertyShorthand&, bool important);

// Legacy parsing allows <string>s for animation-name
bool ConsumeAnimationShorthand(const StylePropertyShorthand&,
bool use_legacy_parsing,
bool important);
bool ConsumeBackgroundShorthand(const StylePropertyShorthand&,
bool important);

Expand Down
Loading

0 comments on commit 51c2bff

Please sign in to comment.