Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2097383002:
Added support of calc() for SVGLength (Closed)
|
Created:
4 years, 5 months ago by Shanmuga Pandi Modified:
4 years, 5 months ago CC:
blink-reviews, blink-reviews-css, chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdded support of calc() for SVGLength
Added Support of calc() for SVGLength and with supporting
all combination of length, number and percentage.
BUG=558304
Committed: https://crrev.com/e52ba8d44cfb51a514ef337c0d06a3e28d4d288d
Cr-Commit-Position: refs/heads/master@{#406502}
Patch Set 1 #
Total comments: 15
Patch Set 2 : Fixed animation issue and added more TCs #Patch Set 3 : nits #
Total comments: 28
Patch Set 4 : Align with review comments #
Total comments: 1
Patch Set 5 : nits #
Total comments: 2
Patch Set 6 : nits #Patch Set 7 : Rebased #Messages
Total messages: 36 (7 generated)
Description was changed from ========== Added support of calc() for SVGLength Added Support of calc() for SVGLength and with supporting all combination of length, number and percentage. BUG=558304 ========== to ========== Added support of calc() for SVGLength Added Support of calc() for SVGLength and with supporting all combination of length, number and percentage. BUG=558304 ==========
shanmuga.m@samsung.com changed reviewers: + rob.buis@samsung.com
PTAL!!! Thanks :)
https://codereview.chromium.org/2097383002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/css/CSSCalculationValue.h (right): https://codereview.chromium.org/2097383002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/css/CSSCalculationValue.h:63: CalcLengthNumber, Are you sure you need this one? The Number should default to 'px' so it basically is a Length too AFAICS.
https://codereview.chromium.org/2097383002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/css/CSSCalculationValue.h (right): https://codereview.chromium.org/2097383002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/css/CSSCalculationValue.h:63: CalcLengthNumber, On 2016/06/27 18:00:01, rwlbuis wrote: > Are you sure you need this one? The Number should default to 'px' so it > basically is a Length too AFAICS. I think it is needed. for eg. calc(10 + 20px). In this 10 is parsed as CalcNumber and 20px is parsed as CalcLength. So the result category will be CalcLengthNumber. CalcLengthNumber should be valid only for SVG. So we need to have unique category to identfiy.
On 2016/06/28 05:15:01, Shanmuga Pandi wrote: > https://codereview.chromium.org/2097383002/diff/1/third_party/WebKit/Source/c... > File third_party/WebKit/Source/core/css/CSSCalculationValue.h (right): > > https://codereview.chromium.org/2097383002/diff/1/third_party/WebKit/Source/c... > third_party/WebKit/Source/core/css/CSSCalculationValue.h:63: CalcLengthNumber, > On 2016/06/27 18:00:01, rwlbuis wrote: > > Are you sure you need this one? The Number should default to 'px' so it > > basically is a Length too AFAICS. > > I think it is needed. for eg. calc(10 + 20px). In this 10 is parsed as > CalcNumber and 20px is parsed as CalcLength. So the result category will be > CalcLengthNumber. CalcLengthNumber should be valid only for SVG. So we need to > have unique category to identfiy. shall we use calcNumber for instead of all these ?
On 2016/06/28 11:36:48, Shanmuga Pandi wrote: > On 2016/06/28 05:15:01, Shanmuga Pandi wrote: > > > https://codereview.chromium.org/2097383002/diff/1/third_party/WebKit/Source/c... > > File third_party/WebKit/Source/core/css/CSSCalculationValue.h (right): > > > > > https://codereview.chromium.org/2097383002/diff/1/third_party/WebKit/Source/c... > > third_party/WebKit/Source/core/css/CSSCalculationValue.h:63: CalcLengthNumber, > > On 2016/06/27 18:00:01, rwlbuis wrote: > > > Are you sure you need this one? The Number should default to 'px' so it > > > basically is a Length too AFAICS. > > > > I think it is needed. for eg. calc(10 + 20px). In this 10 is parsed as > > CalcNumber and 20px is parsed as CalcLength. So the result category will be > > CalcLengthNumber. CalcLengthNumber should be valid only for SVG. So we need to > > have unique category to identfiy. > > shall we use calcNumber for instead of all these ? I feel, if we use calcNumber instead of these new categories. It may break other things.. for ex determineCategory() in case of multiply/divide.
On 2016/06/28 11:43:15, Shanmuga Pandi wrote: > https://codereview.chromium.org/2097383002/diff/1/third_party/WebKit/Source/c... > > > third_party/WebKit/Source/core/css/CSSCalculationValue.h:63: > CalcLengthNumber, > > > On 2016/06/27 18:00:01, rwlbuis wrote: > > > > Are you sure you need this one? The Number should default to 'px' so it > > > > basically is a Length too AFAICS. > > > > > > I think it is needed. for eg. calc(10 + 20px). In this 10 is parsed as > > > CalcNumber and 20px is parsed as CalcLength. So the result category will be > > > CalcLengthNumber. CalcLengthNumber should be valid only for SVG. So we need > to > > > have unique category to identfiy. > > > > shall we use calcNumber for instead of all these ? > I feel, if we use calcNumber instead of these new categories. It may break other > things.. for ex determineCategory() in case of multiply/divide. Ok, I now remember that the original expression needs to be kept for later use, so maybe my comment was wrong. Peer looks good to me. alancutter is probably the best person for review.
shanmuga.m@samsung.com changed reviewers: + alancutter@chromium.org, fs@opera.com, pdr@chromium.org
On 2016/06/28 20:42:15, rwlbuis wrote: > On 2016/06/28 11:43:15, Shanmuga Pandi wrote: > > > https://codereview.chromium.org/2097383002/diff/1/third_party/WebKit/Source/c... > > > > third_party/WebKit/Source/core/css/CSSCalculationValue.h:63: > > CalcLengthNumber, > > > > On 2016/06/27 18:00:01, rwlbuis wrote: > > > > > Are you sure you need this one? The Number should default to 'px' so it > > > > > basically is a Length too AFAICS. > > > > > > > > I think it is needed. for eg. calc(10 + 20px). In this 10 is parsed as > > > > CalcNumber and 20px is parsed as CalcLength. So the result category will > be > > > > CalcLengthNumber. CalcLengthNumber should be valid only for SVG. So we > need > > to > > > > have unique category to identfiy. > > > > > > shall we use calcNumber for instead of all these ? > > I feel, if we use calcNumber instead of these new categories. It may break > other > > things.. for ex determineCategory() in case of multiply/divide. > > Ok, I now remember that the original expression needs to be kept for later use, > so maybe my comment was wrong. > > Peer looks good to me. alancutter is probably the best person for review. Added more reviewers!!! PTAL!! Thanks
SVGLengthList will also need some work, since it uses SVGLength to do parsing and currently uses a very simple non-whitespace tokenization strategy which will no longer work with calc(...). Could be done as a preparatory CL depending on the strategy taken. Would also like to see some tests for interactions with the SVGLength DOM interface. The various setters are likely the most interesting ones to test. It's possible the spec has something to say here, but it may require some clarification and/or fixing. We also need to test a non-presentation attribute length attribute, since that uss slightly different resolving code. (I also notice that floatValueForLength converts maximumValue to LayoutUnit since nonNanCalculatedValue wants. I suspect we want to fix that as well, but maybe not in this CL.) Probably also need to audit the uses of SVGLength::typeWithCalcResolved() (and maybe the corresponding setter.) https://codereview.chromium.org/2097383002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/svg/dom/SVGLength-calc-in-attr.html (right): https://codereview.chromium.org/2097383002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/svg/dom/SVGLength-calc-in-attr.html:8: var rectElement = document.querySelector("rect"); Doesn't need to be global. https://codereview.chromium.org/2097383002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/svg/dom/SVGLength-calc-in-attr.html:17: var rect; Neither do these two. https://codereview.chromium.org/2097383002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/svg/dom/SVGLength-calc-in-attr.html:20: { Move to previous line. Alternatively just open-code this - I don't think this adds much clarity with 'svgWidth' renamed to 'viewportWidth' or some such. https://codereview.chromium.org/2097383002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/svg/dom/SVGLength-calc-in-attr.html:21: return (svgWidth / 100); Drop parenthesis. https://codereview.chromium.org/2097383002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/svg/dom/SVGLength-calc-in-attr.html:28: assert_approx_equals(rect.width, expected, EPSILON); For some reduction in boilerplate, I think a structure like: function assert_calc_expression(expression, expected) { var rect = document.querySelector('rect'); rect.setAttribute('width', expression); var rectWidth = rect.getBoundingClientRect().width; assert_approx_equals(rectWidth, expected, EPSILON); } and then: assert_calc_expression("calc(10 + 10)", 20); ... https://codereview.chromium.org/2097383002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/svg/dom/SVGLength-calc-in-attr.html:49: Drop blank line. https://codereview.chromium.org/2097383002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/css/parser/CSSPropertyParserHelpers.cpp (right): https://codereview.chromium.org/2097383002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/css/parser/CSSPropertyParserHelpers.cpp:227: switch (calculation->category()) { Maybe move this to a helper (predicate) function, and keep the function looking similar to before. https://codereview.chromium.org/2097383002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/svg/SVGLength.cpp (right): https://codereview.chromium.org/2097383002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/svg/SVGLength.cpp:89: return (CSSPrimitiveValue::isLength(type) || type == CSSPrimitiveValue::UnitType::Number || type == CSSPrimitiveValue::UnitType::Percentage || (type >= CSSPrimitiveValue::UnitType::Calc && type <= CSSPrimitiveValue::UnitType::CalcPercentageWithLengthAndNumber)) Suggest you reformat this function, to maybe make it more readable, by adding some line-breaks (so at least the new part of the condition goes on a new line. Feel free to reformat the old pat too though.)
Did someone remove the automatically cc-ed lists? A patch like this should have at least blink-reviews and blink-reviews-css cc-ed (I just re-added these), and probably a couple more folks. Please don't remove these.
On 2016/06/29 09:28:17, fs wrote: > SVGLengthList will also need some work, since it uses SVGLength to do parsing > and currently uses a very simple non-whitespace tokenization strategy which will > no longer work with calc(...). Could be done as a preparatory CL depending on > the strategy taken. Will add TODO for SVGLengthList. > Would also like to see some tests for interactions with the SVGLength DOM > interface. The various setters are likely the most interesting ones to test. > It's possible the spec has something to say here, but it may require some > clarification and/or fixing. unitType/valueAsString wont do anything , because it will work for only exposedLengthUnits. value() , we can get accumulateLengthArray() and convert the values to pixels and send the result. setValue(), we can throw expeption if unit type is calculted. > We also need to test a non-presentation attribute length attribute, since that > uss slightly different resolving code. (I also notice that floatValueForLength > converts maximumValue to LayoutUnit since nonNanCalculatedValue wants. I suspect > we want to fix that as well, but maybe not in this CL.) For non-presentaion attributes, Parsing mode may not come as "SVGAttributeMode" (for ex: width/hight). In that case, it fails to apply. Other cases like Rx,Ry will parse successfully , since its parser mode is SVGAttributeMode. > Probably also need to audit the uses of SVGLength::typeWithCalcResolved() (and > maybe the corresponding setter.) Agree. Will check more..
On 2016/06/30 at 12:22:44, shanmuga.m wrote: > On 2016/06/29 09:28:17, fs wrote: > > SVGLengthList will also need some work, since it uses SVGLength to do parsing > > and currently uses a very simple non-whitespace tokenization strategy which will > > no longer work with calc(...). Could be done as a preparatory CL depending on > > the strategy taken. > > Will add TODO for SVGLengthList. > > > Would also like to see some tests for interactions with the SVGLength DOM > > interface. The various setters are likely the most interesting ones to test. > > It's possible the spec has something to say here, but it may require some > > clarification and/or fixing. > > unitType/valueAsString wont do anything , because it will work for only exposedLengthUnits. By "won't do anything" you mean "return SVG_LENGTHTYPE_UNKNOWN" that sounds about right. For unitType that is - valueAsString should actually do something, but I suspect that is inconsistent with how the current implementation is, so we can fix that up separately. (I think the spec moved in a different direction from our initial implementation of additional units in some regards.) > value() , we can get accumulateLengthArray() and convert the values to pixels and send the result. > setValue(), we can throw expeption if unit type is calculted. value is fairly well-defined in the spec, and should not pose a problem. valueInSpecifiedUnits is the more interesting case (but it's also specified.) In general though, check https://svgwg.org/svg2-draft/types.html#InterfaceSVGLength and follow that where it makes sense. Also, add tests for whatever ends up being implemented. (This last bit is really the most interesting one.) > > > > > We also need to test a non-presentation attribute length attribute, since that > > uss slightly different resolving code. (I also notice that floatValueForLength > > converts maximumValue to LayoutUnit since nonNanCalculatedValue wants. I suspect > > we want to fix that as well, but maybe not in this CL.) > > For non-presentaion attributes, Parsing mode may not come as "SVGAttributeMode" (for ex: width/hight). > In that case, it fails to apply. Other cases like Rx,Ry will parse successfully , since its parser mode is SVGAttributeMode. SVGAttributeMode is used in both case (they all use SVGLength::setValueAsString after all.)
>
> Probably also need to audit the uses of SVGLength::typeWithCalcResolved() (and
> maybe the corresponding setter.)
>
When I audit the usage of SVGLength::typeWithCalcResolved(), I found like one
below.
InterpolationValue SVGLengthInterpolationType::convertSVGLength(const SVGLength&
length)
{
double value = length.valueInSpecifiedUnits();
LengthInterpolatedUnit unitType =
convertToInterpolatedUnit(length.typeWithCalcResolved(), value);
double values[numLengthInterpolatedUnits] = { };
values[unitType] = value;
OwnPtr<InterpolableList> listOfValues =
InterpolableList::create(numLengthInterpolatedUnits);
for (size_t i = 0; i < numLengthInterpolatedUnits; ++i)
listOfValues->set(i, InterpolableNumber::create(values[i]));
return InterpolationValue(std::move(listOfValues));
}
What should be the expected behaviour of calc() on Interpolation/Animations ?
Shall we convert to absolute value and Number type here?
If yes, then SVGLenghContext needed here to conver to value().
Please suggest some idea.
Thank you!
On 2016/07/04 at 09:01:15, shanmuga.m wrote:
> >
> > Probably also need to audit the uses of SVGLength::typeWithCalcResolved()
(and
> > maybe the corresponding setter.)
> >
>
> When I audit the usage of SVGLength::typeWithCalcResolved(), I found like one
below.
>
> InterpolationValue SVGLengthInterpolationType::convertSVGLength(const
SVGLength& length)
> {
> double value = length.valueInSpecifiedUnits();
> LengthInterpolatedUnit unitType =
convertToInterpolatedUnit(length.typeWithCalcResolved(), value);
>
> double values[numLengthInterpolatedUnits] = { };
> values[unitType] = value;
>
> OwnPtr<InterpolableList> listOfValues =
InterpolableList::create(numLengthInterpolatedUnits);
> for (size_t i = 0; i < numLengthInterpolatedUnits; ++i)
> listOfValues->set(i, InterpolableNumber::create(values[i]));
>
> return InterpolationValue(std::move(listOfValues));
> }
>
> What should be the expected behaviour of calc() on Interpolation/Animations ?
> Shall we convert to absolute value and Number type here?
> If yes, then SVGLenghContext needed here to conver to value().
> Please suggest some idea.
> Thank you!
You should be able to look at CSSLengthInterpolationType I think. There are
already traces in the code above of using a list with one entry per type of
unit. I think CSSPrimitiveValue::accumulateLengthArray is the method you want.
On 2016/07/04 at 09:11:03, fs wrote:
> On 2016/07/04 at 09:01:15, shanmuga.m wrote:
> > >
> > > Probably also need to audit the uses of SVGLength::typeWithCalcResolved()
(and
> > > maybe the corresponding setter.)
> > >
> >
> > When I audit the usage of SVGLength::typeWithCalcResolved(), I found like
one below.
> >
> > InterpolationValue SVGLengthInterpolationType::convertSVGLength(const
SVGLength& length)
> > {
> > double value = length.valueInSpecifiedUnits();
> > LengthInterpolatedUnit unitType =
convertToInterpolatedUnit(length.typeWithCalcResolved(), value);
> >
> > double values[numLengthInterpolatedUnits] = { };
> > values[unitType] = value;
> >
> > OwnPtr<InterpolableList> listOfValues =
InterpolableList::create(numLengthInterpolatedUnits);
> > for (size_t i = 0; i < numLengthInterpolatedUnits; ++i)
> > listOfValues->set(i, InterpolableNumber::create(values[i]));
> >
> > return InterpolationValue(std::move(listOfValues));
> > }
> >
> > What should be the expected behaviour of calc() on Interpolation/Animations
?
> > Shall we convert to absolute value and Number type here?
> > If yes, then SVGLenghContext needed here to conver to value().
> > Please suggest some idea.
> > Thank you!
>
> You should be able to look at CSSLengthInterpolationType I think. There are
already traces in the code above of using a list with one entry per type of
unit. I think CSSPrimitiveValue::accumulateLengthArray is the method you want.
(I'd also expect these two [SVGLengthInterpolationType and
CSSLengthInterpolationType] to be largely the same when we're done here - modulo
SVGLength->CSSPrimitiveValue unwrapping and how the value is applied.)
Are user units in SVG lengths always equivalent to pixels or can they be scaled by some factor?
On 2016/07/05 05:19:53, alancutter wrote: > Are user units in SVG lengths always equivalent to pixels or can they be scaled > by some factor? It will be equivalent to pixels finally.
On 2016/07/04 09:15:38, fs wrote:
> On 2016/07/04 at 09:11:03, fs wrote:
> > On 2016/07/04 at 09:01:15, shanmuga.m wrote:
> > > >
> > > > Probably also need to audit the uses of
SVGLength::typeWithCalcResolved()
> (and
> > > > maybe the corresponding setter.)
> > > >
> > >
> > > When I audit the usage of SVGLength::typeWithCalcResolved(), I found like
> one below.
> > >
> > > InterpolationValue SVGLengthInterpolationType::convertSVGLength(const
> SVGLength& length)
> > > {
> > > double value = length.valueInSpecifiedUnits();
> > > LengthInterpolatedUnit unitType =
> convertToInterpolatedUnit(length.typeWithCalcResolved(), value);
> > >
> > > double values[numLengthInterpolatedUnits] = { };
> > > values[unitType] = value;
> > >
> > > OwnPtr<InterpolableList> listOfValues =
> InterpolableList::create(numLengthInterpolatedUnits);
> > > for (size_t i = 0; i < numLengthInterpolatedUnits; ++i)
> > > listOfValues->set(i, InterpolableNumber::create(values[i]));
> > >
> > > return InterpolationValue(std::move(listOfValues));
> > > }
> > >
> > > What should be the expected behaviour of calc() on
Interpolation/Animations
> ?
> > > Shall we convert to absolute value and Number type here?
> > > If yes, then SVGLenghContext needed here to conver to value().
> > > Please suggest some idea.
> > > Thank you!
> >
> > You should be able to look at CSSLengthInterpolationType I think. There are
> already traces in the code above of using a list with one entry per type of
> unit. I think CSSPrimitiveValue::accumulateLengthArray is the method you want.
>
> (I'd also expect these two [SVGLengthInterpolationType and
> CSSLengthInterpolationType] to be largely the same when we're done here -
modulo
> SVGLength->CSSPrimitiveValue unwrapping and how the value is applied.)
I agree. Should we have any issue if we convert into canonical units like below
for calc() ?
Since SVGLengthInterpolationType may not be needed many things of
CSSLengthInterpolationType.
InterpolationValue SVGLengthInterpolationType::convertSVGLength(const SVGLength&
length)
{
const CSSPrimitiveValue* primitiveValue = length.asCSSPrimitiveValue();
CSSLengthArray lengthArray;
primitiveValue->accumulateLengthArray(lengthArray);
double values[numLengthInterpolatedUnits] = { };
for (size_t i = 0; i < CSSPrimitiveValue::LengthUnitTypeCount; i++) {
double value = lengthArray.values[i];
LengthInterpolatedUnit unitType =
convertToInterpolatedUnit(toUnitType(i), value);
values[unitType] = value;
}
OwnPtr<InterpolableList> listOfValues =
InterpolableList::create(numLengthInterpolatedUnits);
for (size_t i = 0; i < numLengthInterpolatedUnits; ++i)
listOfValues->set(i, InterpolableNumber::create(values[i]));
return InterpolationValue(std::move(listOfValues));
}
SVGLength* SVGLengthInterpolationType::resolveInterpolableSVGLength(const
InterpolableValue& interpolableValue, const SVGLengthContext& lengthContext,
SVGLengthMode unitMode, bool negativeValuesForbidden)
{
const InterpolableList& listOfValues =
toInterpolableList(interpolableValue);
double value = 0;
CSSPrimitiveValue::UnitType unitType =
CSSPrimitiveValue::UnitType::UserUnits;
unsigned unitTypeCount = 0;
for (size_t i = 0; i < numLengthInterpolatedUnits; i++) {
double entry = toInterpolableNumber(listOfValues.get(i))->value();
if (!entry)
continue;
unitTypeCount++;
value = entry;
unitType = unitTypes[i];
}
if (unitTypeCount > 1) {
value = 0;
unitType = CSSPrimitiveValue::UnitType::UserUnits;
// SVGLength does not support calc expressions, so we convert to
canonical units.
for (size_t i = 0; i < numLengthInterpolatedUnits; i++) {
double entry = toInterpolableNumber(listOfValues.get(i))->value();
if (entry)
value += lengthContext.convertValueToUserUnits(entry, unitMode,
unitTypes[i]);
}
}
if (negativeValuesForbidden && value < 0)
value = 0;
SVGLength* result = SVGLength::create(unitMode); // defaults to the length 0
result->newValueSpecifiedUnits(unitType, value);
return result;
}
On 2016/07/05 at 13:58:48, shanmuga.m wrote:
> On 2016/07/04 09:15:38, fs wrote:
> > On 2016/07/04 at 09:11:03, fs wrote:
> > > On 2016/07/04 at 09:01:15, shanmuga.m wrote:
> > > > >
> > > > > Probably also need to audit the uses of
SVGLength::typeWithCalcResolved()
> > (and
> > > > > maybe the corresponding setter.)
> > > > >
> > > >
> > > > When I audit the usage of SVGLength::typeWithCalcResolved(), I found
like
> > one below.
> > > >
> > > > InterpolationValue SVGLengthInterpolationType::convertSVGLength(const
> > SVGLength& length)
> > > > {
> > > > double value = length.valueInSpecifiedUnits();
> > > > LengthInterpolatedUnit unitType =
> > convertToInterpolatedUnit(length.typeWithCalcResolved(), value);
> > > >
> > > > double values[numLengthInterpolatedUnits] = { };
> > > > values[unitType] = value;
> > > >
> > > > OwnPtr<InterpolableList> listOfValues =
> > InterpolableList::create(numLengthInterpolatedUnits);
> > > > for (size_t i = 0; i < numLengthInterpolatedUnits; ++i)
> > > > listOfValues->set(i, InterpolableNumber::create(values[i]));
> > > >
> > > > return InterpolationValue(std::move(listOfValues));
> > > > }
> > > >
> > > > What should be the expected behaviour of calc() on
Interpolation/Animations
> > ?
> > > > Shall we convert to absolute value and Number type here?
> > > > If yes, then SVGLenghContext needed here to conver to value().
> > > > Please suggest some idea.
> > > > Thank you!
> > >
> > > You should be able to look at CSSLengthInterpolationType I think. There
are
> > already traces in the code above of using a list with one entry per type of
> > unit. I think CSSPrimitiveValue::accumulateLengthArray is the method you
want.
> >
> > (I'd also expect these two [SVGLengthInterpolationType and
> > CSSLengthInterpolationType] to be largely the same when we're done here -
modulo
> > SVGLength->CSSPrimitiveValue unwrapping and how the value is applied.)
>
>
> I agree. Should we have any issue if we convert into canonical units like
below for calc() ?
> Since SVGLengthInterpolationType may not be needed many things of
CSSLengthInterpolationType.
I'll defer any finer points of core/animation review to alancutter et al, but I
don't see any strong reason to diverge unless strictly necessary. Somewhat by
extension, I'd like to see SVGLengthContext and SVGLengthMode be eliminated from
this code.
The suggested animation code seems reasonable though it would be easier to verify as a diff of the original code. https://codereview.chromium.org/2097383002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/css/CSSCalculationValue.cpp (right): https://codereview.chromium.org/2097383002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/css/CSSCalculationValue.cpp:210: case CalcNumber: This is not true for border-image-width (which doesn't support numbers in calc currently). Numbers represent multiples of the computed border-width, see: https://drafts.csswg.org/css-backgrounds-3/#the-border-image-width We should add a TODO here to stop treating numbers like pixels unconditionally in calcs to be able to accomodate border-image-width.
Please have a look !! Thanks https://codereview.chromium.org/2097383002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/svg/dom/SVGLength-calc-in-attr.html (right): https://codereview.chromium.org/2097383002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/svg/dom/SVGLength-calc-in-attr.html:8: var rectElement = document.querySelector("rect"); On 2016/06/29 09:28:17, fs (ooo to July 18) wrote: > Doesn't need to be global. Acknowledged. https://codereview.chromium.org/2097383002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/svg/dom/SVGLength-calc-in-attr.html:17: var rect; On 2016/06/29 09:28:17, fs (ooo to July 18) wrote: > Neither do these two. Acknowledged. https://codereview.chromium.org/2097383002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/css/parser/CSSPropertyParserHelpers.cpp (right): https://codereview.chromium.org/2097383002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/css/parser/CSSPropertyParserHelpers.cpp:227: switch (calculation->category()) { On 2016/06/29 09:28:17, fs (ooo to July 18) wrote: > Maybe move this to a helper (predicate) function, and keep the function looking > similar to before. Done. https://codereview.chromium.org/2097383002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/svg/SVGLength.cpp (right): https://codereview.chromium.org/2097383002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/svg/SVGLength.cpp:89: return (CSSPrimitiveValue::isLength(type) || type == CSSPrimitiveValue::UnitType::Number || type == CSSPrimitiveValue::UnitType::Percentage || (type >= CSSPrimitiveValue::UnitType::Calc && type <= CSSPrimitiveValue::UnitType::CalcPercentageWithLengthAndNumber)) On 2016/06/29 09:28:17, fs (ooo to July 18) wrote: > Suggest you reformat this function, to maybe make it more readable, by adding > some line-breaks (so at least the new part of the condition goes on a new line. > Feel free to reformat the old pat too though.) Done.
https://codereview.chromium.org/2097383002/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/svg/dom/SVGLength-calc-in-attr.html (right): https://codereview.chromium.org/2097383002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/svg/dom/SVGLength-calc-in-attr.html:18: { Move to previous line https://codereview.chromium.org/2097383002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/svg/dom/SVGLength-calc-in-attr.html:64: marker.setAttribute('width', expression); // reset to calc value s/'width'/'markerWidth'/ https://codereview.chromium.org/2097383002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/svg/dom/SVGLength-calc-in-attr.html:91: }, "Tests calc() on presentation attr in svgLength"); This is a bit misleading since it also tests the non-presattr case. https://codereview.chromium.org/2097383002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/animation/SVGLengthInterpolationType.cpp (right): https://codereview.chromium.org/2097383002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/animation/SVGLengthInterpolationType.cpp:115: LengthInterpolatedUnit unitType = convertToInterpolatedUnit(SVGLength::toUnitType(i), value); Couldn't we take this opportunity to transition to LengthUnitType from CSSPrimitiveValue? (And potentially gain 'ch' support as well?) Then I think this loop would fold with the loop below, and all the remapping code would go away. https://codereview.chromium.org/2097383002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/parser/CSSPropertyParserHelpers.cpp (right): https://codereview.chromium.org/2097383002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/parser/CSSPropertyParserHelpers.cpp:220: || (cssParserMode == SVGAttributeMode Suggested formatting: if (<old predicate>) return true; if (cssParserMode != SVGAttributeMode) return false; return <additional categories predicate>; https://codereview.chromium.org/2097383002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/svg/SVGLength.cpp (right): https://codereview.chromium.org/2097383002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/svg/SVGLength.cpp:74: bool isCalcCSSUnitType(CSSPrimitiveValue::UnitType type) Make this static (and isSupportedCSSUnitType too, while at it) and move to down just before it is used. https://codereview.chromium.org/2097383002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/svg/SVGLength.cpp:76: return (type >= CSSPrimitiveValue::UnitType::Calc && type <= CSSPrimitiveValue::UnitType::CalcPercentageWithLengthAndNumber); Drop parenthesis. https://codereview.chromium.org/2097383002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/svg/SVGLength.cpp:79: float SVGLength::resolveCalcValue(const SVGLengthContext& context) const We could consider going through CSSPrimitiveValue::convertToLength here instead to avoid special-case resolving code. (This should hopefully allow us to get rid of m_unitMode eventually.) https://codereview.chromium.org/2097383002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/svg/SVGLength.cpp:83: float value = 0; If we end up keeping this I suggest making this 'double', and then converting (narrowing) to float when returning. https://codereview.chromium.org/2097383002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/svg/SVGLength.cpp:173: Drop. https://codereview.chromium.org/2097383002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/svg/SVGLength.cpp:278: if (!fromLength->isCalculated()) Test for this code? (Preferably i think this would just construct a calc() expression if the units fall in different categories et.c, but we can fix that up later I guess...) https://codereview.chromium.org/2097383002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/svg/SVGLength.h (right): https://codereview.chromium.org/2097383002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/svg/SVGLength.h:105: return static_cast<CSSPrimitiveValue::UnitType>(CSSPrimitiveValue::lengthUnitTypeToUnitType(static_cast<CSSPrimitiveValue::LengthUnitType>(lengthUnitType))); Shouldn't need to cast what lengthUnitTypeToUnitType returns and I think we might as well just use the CSSPrimitiveValue method directly - potentially with a cast on the input. https://codereview.chromium.org/2097383002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/svg/SVGLengthTearOff.cpp (right): https://codereview.chromium.org/2097383002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/svg/SVGLengthTearOff.cpp:151: target()->setValueAsNumber(value); Per the other CL (and current spec text) this would be the only case necessary?
PTAL!! https://codereview.chromium.org/2097383002/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/svg/dom/SVGLength-calc-in-attr.html (right): https://codereview.chromium.org/2097383002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/svg/dom/SVGLength-calc-in-attr.html:18: { On 2016/07/16 21:13:02, fs (ooo to July 18) wrote: > Move to previous line Done. https://codereview.chromium.org/2097383002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/svg/dom/SVGLength-calc-in-attr.html:64: marker.setAttribute('width', expression); // reset to calc value On 2016/07/16 21:13:02, fs (ooo to July 18) wrote: > s/'width'/'markerWidth'/ Done. https://codereview.chromium.org/2097383002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/svg/dom/SVGLength-calc-in-attr.html:91: }, "Tests calc() on presentation attr in svgLength"); On 2016/07/16 21:13:01, fs (ooo to July 18) wrote: > This is a bit misleading since it also tests the non-presattr case. Done. https://codereview.chromium.org/2097383002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/animation/SVGLengthInterpolationType.cpp (right): https://codereview.chromium.org/2097383002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/animation/SVGLengthInterpolationType.cpp:115: LengthInterpolatedUnit unitType = convertToInterpolatedUnit(SVGLength::toUnitType(i), value); On 2016/07/16 21:13:02, fs (ooo to July 18) wrote: > Couldn't we take this opportunity to transition to LengthUnitType from > CSSPrimitiveValue? (And potentially gain 'ch' support as well?) Then I think > this loop would fold with the loop below, and all the remapping code would go > away. That's good thought.. :) Done!! https://codereview.chromium.org/2097383002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/parser/CSSPropertyParserHelpers.cpp (right): https://codereview.chromium.org/2097383002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/parser/CSSPropertyParserHelpers.cpp:220: || (cssParserMode == SVGAttributeMode On 2016/07/16 21:13:02, fs (ooo to July 18) wrote: > Suggested formatting: > > if (<old predicate>) > return true; > if (cssParserMode != SVGAttributeMode) > return false; > return <additional categories predicate>; Done. https://codereview.chromium.org/2097383002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/svg/SVGLength.cpp (right): https://codereview.chromium.org/2097383002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/svg/SVGLength.cpp:74: bool isCalcCSSUnitType(CSSPrimitiveValue::UnitType type) On 2016/07/16 21:13:02, fs (ooo to July 18) wrote: > Make this static (and isSupportedCSSUnitType too, while at it) and move to down > just before it is used. Done. https://codereview.chromium.org/2097383002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/svg/SVGLength.cpp:76: return (type >= CSSPrimitiveValue::UnitType::Calc && type <= CSSPrimitiveValue::UnitType::CalcPercentageWithLengthAndNumber); On 2016/07/16 21:13:02, fs (ooo to July 18) wrote: > Drop parenthesis. Done. https://codereview.chromium.org/2097383002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/svg/SVGLength.cpp:79: float SVGLength::resolveCalcValue(const SVGLengthContext& context) const On 2016/07/16 21:13:02, fs wrote: > We could consider going through CSSPrimitiveValue::convertToLength here instead > to avoid special-case resolving code. (This should hopefully allow us to get rid > of m_unitMode eventually.) Done. https://codereview.chromium.org/2097383002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/svg/SVGLength.cpp:173: On 2016/07/16 21:13:02, fs wrote: > Drop. Done. https://codereview.chromium.org/2097383002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/svg/SVGLength.cpp:278: if (!fromLength->isCalculated()) On 2016/07/16 21:13:02, fs (ooo to July 18) wrote: > Test for this code? (Preferably i think this would just construct a calc() > expression if the units fall in different categories et.c, but we can fix that > up later I guess...) Yes. We could just construct a calc_expression later. Have added some test cases for this on svg_calc_interpolation.html https://codereview.chromium.org/2097383002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/svg/SVGLength.h (right): https://codereview.chromium.org/2097383002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/svg/SVGLength.h:105: return static_cast<CSSPrimitiveValue::UnitType>(CSSPrimitiveValue::lengthUnitTypeToUnitType(static_cast<CSSPrimitiveValue::LengthUnitType>(lengthUnitType))); On 2016/07/16 21:13:02, fs (ooo to July 18) wrote: > Shouldn't need to cast what lengthUnitTypeToUnitType returns and I think we > might as well just use the CSSPrimitiveValue method directly - potentially with > a cast on the input. Done. https://codereview.chromium.org/2097383002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/svg/SVGLengthTearOff.cpp (right): https://codereview.chromium.org/2097383002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/svg/SVGLengthTearOff.cpp:151: target()->setValueAsNumber(value); On 2016/07/16 21:13:02, fs (ooo to July 18) wrote: > Per the other CL (and current spec text) this would be the only case necessary? As per the spec, It should always "Set the SVGLength's value to a <number> whose value is value.". Since my previous patch is reverted, I added just isCalculated() check to handle calc cases. But eventually, it should be as per spec. I will investigate why that patch is reverted, if I get the testCase. https://codereview.chromium.org/2097383002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/CSSCalculationValue.cpp (right): https://codereview.chromium.org/2097383002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/CSSCalculationValue.cpp:212: value.pixels += m_value->getDoubleValue() * conversionData.zoom() * multiplier; Added test case for this!!
LGTM w/ a minor nit. Let's wait for alancutter to chime in again though. https://codereview.chromium.org/2097383002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/animation/SVGLengthInterpolationType.cpp (right): https://codereview.chromium.org/2097383002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/animation/SVGLengthInterpolationType.cpp:115: LengthInterpolatedUnit unitType = convertToInterpolatedUnit(SVGLength::toUnitType(i), value); On 2016/07/18 at 13:35:32, Shanmuga Pandi wrote: > On 2016/07/16 21:13:02, fs (ooo to July 18) wrote: > > Couldn't we take this opportunity to transition to LengthUnitType from > > CSSPrimitiveValue? (And potentially gain 'ch' support as well?) Then I think > > this loop would fold with the loop below, and all the remapping code would go > > away. > > That's good thought.. :) > Done!! Looks great, thanks! https://codereview.chromium.org/2097383002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/svg/SVGLength.cpp (right): https://codereview.chromium.org/2097383002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/svg/SVGLength.cpp:278: if (!fromLength->isCalculated()) On 2016/07/18 at 13:35:32, Shanmuga Pandi wrote: > On 2016/07/16 21:13:02, fs (ooo to July 18) wrote: > > Test for this code? (Preferably i think this would just construct a calc() > > expression if the units fall in different categories et.c, but we can fix that > > up later I guess...) > > Yes. We could just construct a calc_expression later. > Have added some test cases for this on svg_calc_interpolation.html Please add a TODO about constructing a calc() expression here. https://codereview.chromium.org/2097383002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/svg/SVGLengthTearOff.cpp (right): https://codereview.chromium.org/2097383002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/svg/SVGLengthTearOff.cpp:151: target()->setValueAsNumber(value); On 2016/07/18 at 13:35:33, Shanmuga Pandi wrote: > On 2016/07/16 21:13:02, fs (ooo to July 18) wrote: > > Per the other CL (and current spec text) this would be the only case necessary? > > As per the spec, It should always "Set the SVGLength's value to a <number> whose value is value.". > Since my previous patch is reverted, I added just isCalculated() check to handle calc cases. > But eventually, it should be as per spec. I will investigate why that patch is reverted, if I get the testCase. Fair enough. https://codereview.chromium.org/2097383002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/svg/SVGLengthContext.h (right): https://codereview.chromium.org/2097383002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/svg/SVGLengthContext.h:80: float resolveValue(const CSSPrimitiveValue*, SVGLengthMode = SVGLengthMode::Other) const; Nit: In think this should be const CSSPrimitiveValue&. Also, I don't think we need to have a default argument for the mode.
https://codereview.chromium.org/2097383002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/svg/SVGLengthContext.h (right): https://codereview.chromium.org/2097383002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/svg/SVGLengthContext.h:80: float resolveValue(const CSSPrimitiveValue*, SVGLengthMode = SVGLengthMode::Other) const; On 2016/07/18 13:56:49, fs wrote: > Nit: In think this should be const CSSPrimitiveValue&. Also, I don't think we > need to have a default argument for the mode. Done.
lgtm. I would like to see calc type querying get a refactor, I'm not too pleased with the way they're represented as CSSPrimitiveValue::UnitTypes but that's something to tackle in some other patch.
The CQ bit was checked by shanmuga.m@samsung.com
The patchset sent to the CQ was uploaded after l-g-t-m from fs@opera.com Link to the patchset: https://codereview.chromium.org/2097383002/#ps120001 (title: "Rebased")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Added support of calc() for SVGLength Added Support of calc() for SVGLength and with supporting all combination of length, number and percentage. BUG=558304 ========== to ========== Added support of calc() for SVGLength Added Support of calc() for SVGLength and with supporting all combination of length, number and percentage. BUG=558304 ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
CQ bit was unchecked.
Message was sent while issue was closed.
Description was changed from ========== Added support of calc() for SVGLength Added Support of calc() for SVGLength and with supporting all combination of length, number and percentage. BUG=558304 ========== to ========== Added support of calc() for SVGLength Added Support of calc() for SVGLength and with supporting all combination of length, number and percentage. BUG=558304 Committed: https://crrev.com/e52ba8d44cfb51a514ef337c0d06a3e28d4d288d Cr-Commit-Position: refs/heads/master@{#406502} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/e52ba8d44cfb51a514ef337c0d06a3e28d4d288d Cr-Commit-Position: refs/heads/master@{#406502} |
