Closed Bug 1290864 Opened 8 years ago Closed 8 years ago

basic-shape serialization is too conservative

Categories

(Core :: CSS Parsing and Computation, defect)

Product:
Core
Component:
CSS Parsing and Computation
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla51
Tracking Flags:
Tracking Status
firefox51 --- fixed

People

(Reporter: manishearth, Assigned: manishearth)

References

(Blocks 1 open bug)

Details

Attachments

(4 files, 1 obsolete file)

Bug 1290864 - Omit the default closest-side when serializing circle() and ellipse();
58 bytes, text/x-review-board-request
xidorn
: review+
Details
Bug 1290864 - Standardize serialization of <position> values in <basic-shape>;
58 bytes, text/x-review-board-request
xidorn
: review+
Details
testcase.html
945 bytes, text/plain
Details
Bug 1290864 - Add WPT test for serialization of basic-shapes;
58 bytes, text/x-review-board-request
xidorn
: review+
Details
Attached file testcase.html (obsolete) —
(needs layout.css.clip-path-shapes.enabled) According to https://drafts.csswg.org/css-shapes/#basic-shape-serialization, we should be omitting values in the serialization of basic-shape whenever they are the defaults. There are also some tricky rules about serializing <position>. We're not following these. I haven't investigated the full extent of this, but some examples include not ignoring closest-side when given inside |circle()|, and instead of serializing |circle(at center)| to |circle(at 50% 50%)|, we serialize to |circle(at center center)|. The spec contradicts itself here (https://github.com/w3c/csswg-drafts/issues/365), but we're wrong either way.
I'm adding shape-outside support to the style system in bug 1288626, which uses the same basic shape parsing, serialization as clip-path. BTW, this is probably related to bug 1266316.
Attachment #8776549 - Attachment mime type: text/plain → text/html
Still WIP, need to test it more and then add tests. But the attached testcase now passes. Any idea where these tests could go? This is specced behavior so it could go in wpt, but it seems like it better belongs in the csswg tests, and we don't seem to use those. layout/style/test? I would love to be able to share these tests between browsers.
Assignee: nobody → manishearth
Status: NEW → ASSIGNED
Depends on: 1291962
Comment on attachment 8777371 [details] Bug 1290864 - Omit the default closest-side when serializing circle() and ellipse(); https://reviewboard.mozilla.org/r/68942/#review66226 r=me with the nits addressed. ::: layout/style/nsCSSValue.cpp:932 (Diff revision 1) > + (aFunctionId == eCSSKeyword_circle || > + (array->Item(2).GetUnit() == eCSSUnit_Enumerated && > + array->Item(2).GetIntValue() == NS_RADIUS_CLOSEST_SIDE))) { The "(array" should be aligned with "aFunctionId" as they are in the same nesting level. ::: layout/style/nsCSSValue.cpp:937 (Diff revision 1) > - AppendPositionCoordinateToString(array->Item(1), aProperty, > + AppendPositionCoordinateToString(array->Item(1), aProperty, > - aResult, aSerialization); > + aResult, aSerialization); As far as you're here, could you reindent the second line to make "aResult" align with "array" to match our code style?
Attachment #8777371 - Flags: review?(xidorn+moz) → review+
(In reply to Manish Goregaokar [:manishearth] from comment #4) > Still WIP, need to test it more and then add tests. But the attached > testcase now passes. > > Any idea where these tests could go? This is specced behavior so it could go > in wpt, but it seems like it better belongs in the csswg tests, and we don't > seem to use those. layout/style/test? I would love to be able to share these > tests between browsers. We don't run non-reftests from CSSWG test repo. Traditionally, we put this kind of tests in layout/style/test, but yeah I agree it would be great if the tests could be shared between browsers, so I guess they should go in wpt. But I'm not sure where should they be added. jgraham, any suggestion?
Flags: needinfo?(james)
Comment on attachment 8777372 [details] Bug 1290864 - Standardize serialization of <position> values in <basic-shape>; https://reviewboard.mozilla.org/r/68944/#review66238 I think most of the simplification (e.g. simple keywords -> percent, keyword + percent -> percent, etc.) should happen in the parser side. The parser should yield consistent and simplified result in the 4-value form with every value presents (so no null). And the serialization code should be straightforward to reflect how the value is stored, just with some additional code to remove default bits. (e.g. if edges are left and top, produce 2-value form, otherwise 4-value). Storing simplified values would potentially simplify its usage elsewhere across the codebase. Also having serialization code straightforward makes it easier to examine the value stored inside. So please fix the parser first. Also please take care of the style issues I raised below when you write your new patch, thanks. ::: layout/style/nsCSSValue.cpp:961 (Diff revision 1) > } > aResult.AppendLiteral("at "); > - array->Item(count).AppendToString(eCSSProperty_object_position, > - aResult, aSerialization); > + array->Item(count).AppendBasicShapePositionToString(aResult, aSerialization); > +} > + > +static float PositionKeywordAsPercent(const nsCSSValue& aValue) { In general the function names and the start braces of functions always starts a new line (unless inside class/struct body). So it should be > static float > PositionKeywordAsPercent(...) > { ::: layout/style/nsCSSValue.cpp:963 (Diff revision 1) > - array->Item(count).AppendToString(eCSSProperty_object_position, > - aResult, aSerialization); > + array->Item(count).AppendBasicShapePositionToString(aResult, aSerialization); > +} > + > +static float PositionKeywordAsPercent(const nsCSSValue& aValue) { > + MOZ_ASSERT(aValue.GetUnit() == eCSSUnit_Enumerated, > + "PositionKeywordAsFloat must be called on keywords only"); This should be aligned after the open bracket. ::: layout/style/nsCSSValue.cpp:974 (Diff revision 1) > + return 0; > + case NS_STYLE_IMAGELAYER_POSITION_BOTTOM: > + case NS_STYLE_IMAGELAYER_POSITION_RIGHT: > + return 1.0; > + } > + MOZ_ASSERT(false, "unexpected position keyword"); You can use MOZ_ASSERT_UNREACHABLE("...") here. ::: layout/style/nsCSSValue.cpp:979 (Diff revision 1) > + MOZ_ASSERT(false, "unexpected position keyword"); > + return 0; > +} > + > +// Spec asks us to treat zero values as 0% > +static bool IsPercentageOrZero(const nsCSSValue& aValue) { Same style issue here. Probably rename it to IsPercentageOrZeroLength() and asserts that aValue.IsLengthPercentCalcUnit(). ::: layout/style/nsCSSValue.cpp:985 (Diff revision 1) > +static void AppendPositionOffsetMakePercent(const nsCSSValue& aValue, > + nsAString& aResult, > + nsCSSValue::Serialization aSerialization) { Same style issue here. ::: layout/style/nsCSSValue.cpp:998 (Diff revision 1) > +static void AppendPositionEdgeRemoveKeywords(const nsCSSValue& aValue, > + nsAString& aResult, > + nsCSSValue::Serialization aSerialization) { Same style issue here. ::: layout/style/nsCSSValue.cpp:1023 (Diff revision 1) > + const nsCSSValue &xEdge = array->Item(0), > + &xOffset = array->Item(1), > + &yEdge = array->Item(2), > + &yOffset = array->Item(3); Please give each variable a separate declaration, rather than putting them together. So it should be > const nsCSSValue& xEdge = ...; > const nsCSSValue& xOffset = ...; > ... ::: layout/style/nsCSSValue.cpp:1030 (Diff revision 1) > + // > Since <position> keywords stand in for percentages, > + // > keywords without an offset turn into percentages. What's the "> " for at the beginning of each line? I suppose you should remove them. ::: layout/style/nsCSSValue.cpp:1065 (Diff revision 1) > + // Fall back to regular behavior (See eCSSUnit_Array case in > + // nsCSSValue::AppendToString, with one small exception: We still > + // replace zero offsets with 0% > + > + bool mark = false; > + for (size_t i = 0, i_end = array->Count(); i < i_end; ++i) { You don't really need an i_end. "i < array->Count()" should be fine as far as Count() is a simple inline function.
Attachment #8777372 - Flags: review?(xidorn+moz)
https://reviewboard.mozilla.org/r/68944/#review66238 I don't think this really fits in the parser, though, the spec explicitly talks about serialization. Both Gecko and Servo often *insert* defaults while parsing (Servo does this pretty consistently), and in general expand shortforms into their representations. We'd be going in the reverse direction. Handling the serialization spec in the serialization code means that it's easier to maintain and it doesn't need to be rewritten for stylo. Besides, we'd still need to handle these cases in serialization to deal with values from `getComputedStyle()`, no? Or introduce the same checks in style computation. Seems redundant. > Same style issue here. > > Probably rename it to IsPercentageOrZeroLength() and asserts that aValue.IsLengthPercentCalcUnit(). We don't want calc units, only lengths or percentages. But I added a check that it's a length. > What's the "> " for at the beginning of each line? I suppose you should remove them. I'm quoting the spec. Removed. > You don't really need an i_end. "i < array->Count()" should be fine as far as Count() is a simple inline function. Heh, I just copied this loop from `AppendToString`. Fixed
(In reply to Manish Goregaokar [:manishearth] from comment #8) > https://reviewboard.mozilla.org/r/68944/#review66238 > > I don't think this really fits in the parser, though, the spec explicitly > talks about serialization. I believe the spec is written that way in the hope of making the serializer simpler, not more complicated, since engines generally want to simplify and unify values as early as possible. > Both Gecko and Servo often *insert* defaults > while parsing (Servo does this pretty consistently), and in general expand > shortforms into their representations. We'd be going in the reverse > direction. Why do you think we are going in the reverse direction? I was asking you to make the parser do expanding shortforms and inserting defaults, so that you can simplify the serializer. That matches what we generally do, doesn't it? > Handling the serialization spec in the serialization code means > that it's easier to maintain and it doesn't need to be rewritten for stylo. IIRC, Stylo doesn't return nsCSSValue to Gecko. Stylo only outputs style structs. So this code would need to be written independently in the Servo side anyway. > Besides, we'd still need to handle these cases in serialization to deal with > values from `getComputedStyle()`, no? Or introduce the same checks in style > computation. Seems redundant. No. Computed style are generated from computed values, which is not stored as nsCSSValue, but stored in the style structs. We need to construct the string from scratch anyway. They usually cannot share any code with CSS value serializer.
Comment on attachment 8777371 [details] Bug 1290864 - Omit the default closest-side when serializing circle() and ellipse(); Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68942/diff/1-2/
Attachment #8777372 - Flags: review?(xidorn+moz)
Comment on attachment 8777372 [details] Bug 1290864 - Standardize serialization of <position> values in <basic-shape>; Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68944/diff/1-2/
Attached file testcase.html —
Updated testcase with more examples. All pass.
Attachment #8776549 - Attachment is obsolete: true
Comment on attachment 8777372 [details] Bug 1290864 - Standardize serialization of <position> values in <basic-shape>; Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68944/diff/2-3/
Refactored to move half the logic into the serialization. This way is much neater :) Still need to figure out where the tests go. These are tests that can be run on a simple CSSStyleDeclaration object, so they *could* work within wpt, they just don't belong there :)
(In reply to Manish Goregaokar [:manishearth] from comment #14) > Refactored to move half the logic into the serialization. This way is much > neater :) I suppose you mean into the parser?
Yes, that :)
Your comment on the `circle()` issue made me take a second look at empty values; I wasn't handling them correctly. Fixed and added a `checkEquals("circle()", "circle(at 50% 50%)");` testcase
Comment on attachment 8777372 [details] Bug 1290864 - Standardize serialization of <position> values in <basic-shape>; Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68944/diff/3-4/
Attachment #8777372 - Flags: review?(xidorn+moz)
Comment on attachment 8777372 [details] Bug 1290864 - Standardize serialization of <position> values in <basic-shape>; https://reviewboard.mozilla.org/r/68944/#review66328 Overall it looks much better. Some nits below. I'd like to recheck after the they are addressed. ::: layout/style/nsCSSParser.cpp:910 (Diff revision 3) > // ParsePositionValue parses a CSS <position> value, which is used by > // the 'background-position' property. > bool ParsePositionValue(nsCSSValue& aOut); > + // We parse position values differently for basic-shape, by expanding defaults > + // and replacing keywords with percentages > + bool ParsePositionValueBasicShape(nsCSSValue& aOut); I'd prefer calling it ParsePositionValueForBasicShape. You may want to put the declaration of this together with other basic shape parsing function below, under comment "/* Functions for basic shapes */". ::: layout/style/nsCSSParser.cpp:12582 (Diff revision 3) > // Spec reference: http://www.w3.org/TR/css3-background/#ltpositiongt > +// Invariants: > +// - Always produces a four-value array on a successful parse. > +// - The values are: X edge, X offset, Y edge, Y offset > +// - Edges are always keywords or null > +// - A |center| edge will not have an offset Please remove the tailing spaces. ::: layout/style/nsCSSParser.cpp:12760 (Diff revision 3) > > return true; > } > > +static void > +ZeroLengthToPercentage(nsCSSValue& aValue) { Function name should generally be a verb phase. I guess we can call ConvertZeroLengthToPercentage? Actually I think this function can be merged with the function below. Also, the open brace of function definition should be in a newline (unlike the convention in Rust). ::: layout/style/nsCSSParser.cpp:12768 (Diff revision 3) > + aValue.SetPercentValue(0); > + } > +} > + > +static void > +ContractEdgeOffsetPair(nsCSSValue& aEdge, I'm not pretty sure what's the meaning of "contract" here. Could we use some simpler word, e.g. AdjustEdgeOffsetPairForBasicShape? Also I think we could merge this function and the function above given they are both adjusting the values. ::: layout/style/nsCSSParser.cpp:12770 (Diff revision 3) > +} > + > +static void > +ContractEdgeOffsetPair(nsCSSValue& aEdge, > + nsCSSValue& aOffset, > + uint8_t aZeroVal) { "zero val" sounds confusing. Probably aDefaultEdge? ::: layout/style/nsCSSParser.cpp:12790 (Diff revision 3) > + case NS_STYLE_IMAGELAYER_POSITION_BOTTOM: > + case NS_STYLE_IMAGELAYER_POSITION_RIGHT: Probably add an assertion that the default edge is the opposite edge here. ::: layout/style/nsCSSParser.cpp:12799 (Diff revision 3) > + } > + } > +} > + > +// https://drafts.csswg.org/css-shapes/#basic-shape-serialization > +// We set values to defaults while parsing for these "while parsing basic shapes" ::: layout/style/nsCSSParser.cpp:12803 (Diff revision 3) > +// https://drafts.csswg.org/css-shapes/#basic-shape-serialization > +// We set values to defaults while parsing for these > +// Invariants: > +// - Always produces a four-value array on a successful parse. > +// - The values are: X edge, X offset, Y edge, Y offset > +// - Edges are always keywords And never be center, right? ::: layout/style/nsCSSParser.cpp:12806 (Diff revision 3) > +// - Always produces a four-value array on a successful parse. > +// - The values are: X edge, X offset, Y edge, Y offset > +// - Edges are always keywords > +// - Offsets are nonnull > +// - Percentage offsets have keywords folded into them, > +// so "bottom 40%" or "right 20%" will not exist Please remove the tailing space. ::: layout/style/nsCSSParser.cpp:12808 (Diff revision 3) > +// - Edges are always keywords > +// - Offsets are nonnull > +// - Percentage offsets have keywords folded into them, > +// so "bottom 40%" or "right 20%" will not exist > +bool > +CSSParserImpl::ParsePositionValueBasicShape(nsCSSValue& aOut) { The open brace should be in new line. ::: layout/style/nsCSSParser.cpp:12813 (Diff revision 3) > + nsCSSValue &xEdge = value->Item(0), > + &xOffset = value->Item(1), > + &yEdge = value->Item(2), > + &yOffset = value->Item(3); Please give each variable its own declaration line. So > nsCSSValue& xEdge = ...; > nsCSSValue& xOffset = ...; writing them together could be confusing, especially for references and pointers. ::: layout/style/nsCSSParser.cpp:12821 (Diff revision 3) > + &yOffset = value->Item(3); > + // Zero offsets are 0% > + ZeroLengthToPercentage(xOffset); > + ZeroLengthToPercentage(yOffset); > + // A keyword edge + percent offset pair can be contracted > + // into the percentage with the default value in the The sentence isn't complete here. What do you want to say? ::: layout/style/nsCSSValue.cpp:980 (Diff revision 3) > + // Ensure invariants from ParsePositionValueBasicShape > + // haven't been modified > + MOZ_ASSERT(xEdge.GetUnit() == eCSSUnit_Enumerated && > + yEdge.GetUnit() == eCSSUnit_Enumerated && > + xOffset.GetUnit() != eCSSUnit_Null && > + yOffset.GetUnit() != eCSSUnit_Null); Also edges are not "center", and offsets should be IsLengthPercentCalcUnit(). It'd be better to move the comment to be description of the assertion as its second argument, so that people would know more about the assertion without checking the code when that happens. ::: layout/style/nsCSSValue.cpp:989 (Diff revision 3) > + xOffset.GetUnit() != eCSSUnit_Null && > + yOffset.GetUnit() != eCSSUnit_Null); > + if (xEdge.GetIntValue() == NS_STYLE_IMAGELAYER_POSITION_LEFT && > + yEdge.GetIntValue() == NS_STYLE_IMAGELAYER_POSITION_TOP) { > + // We can omit these defaults > + xOffset.AppendToString(eCSSProperty_object_position, aResult, aSerialization); We need to specify object-position for edges because we need its keyword table (probably worth a comment for this). But we don't need the keyword table for offsets. I think it'd be better to just use eCSSProperty_UNKNOWN here and other three places below. Also this line exceeds the limit of 80 chars, and changing to eCSSProperty_UNKNOWN would make it short enough.
Attachment #8777789 - Flags: review?(xidorn+moz)
Comment on attachment 8777789 [details] Bug 1290864 - Add WPT test for serialization of basic-shapes; https://reviewboard.mozilla.org/r/69264/#review66332 The existing tests look good, but I think it's worth adding some more tests as described below. ::: testing/web-platform/meta/MANIFEST.json:37241 (Diff revision 1) > + "css-shapes/basic-shape-serialization.html": [ > + { > + "path": "css-shapes/basic-shape-serialization.html", > + "url": "/css-shapes/basic-shape-serialization.html" > + } This test is called "basic-shape-serialization" but it actually only checks circle() function. Probably it is better to call it "circle-function-serialization", and probably consider adding another test for ellipse (or adding those tests to the same file). ::: testing/web-platform/tests/css-shapes/basic-shape-serialization.html:24 (Diff revision 1) > +checkEquals("circle(at bottom left)", "circle(at 0% 100%)"); > +checkEquals("circle(at right calc(10% + 5px))", > + "circle(at 100% calc(10% + 5px))"); > + > +// Only 2 or 4 value form allowed > +checkEquals("circle()", "circle(at 50% 50%)"); The function allows radius only, please add tests to check those cases as well. ::: testing/web-platform/tests/css-shapes/basic-shape-serialization.html:28 (Diff revision 1) > +// Only 2 or 4 value form allowed > +checkEquals("circle()", "circle(at 50% 50%)"); > + > +checkEquals("circle(at right 5px top)", "circle(at right 5px top 0%)"); > +// Remove defaults like closest-side > +checkEquals("circle(closest-side at center)", "circle(at 50% 50%)"); Please also add some tests to ensure that non-defaults are not stripped.
Comment on attachment 8777789 [details] Bug 1290864 - Add WPT test for serialization of basic-shapes; Review request updated; see interdiff: https://reviewboard.mozilla.org/r/69264/diff/1-2/
Attachment #8777789 - Flags: review?(xidorn+moz)
Attachment #8777789 - Flags: review?(xidorn+moz)
Comment on attachment 8777372 [details] Bug 1290864 - Standardize serialization of <position> values in <basic-shape>; Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68944/diff/4-5/
Attachment #8777372 - Flags: review?(xidorn+moz)
Attachment #8777789 - Flags: review?(xidorn+moz)
Comment on attachment 8777789 [details] Bug 1290864 - Add WPT test for serialization of basic-shapes; Review request updated; see interdiff: https://reviewboard.mozilla.org/r/69264/diff/2-3/
Updated. I got the test location by asking Ms2ger, no need for the needinfo anymore. Apparently WPT is okay with in-tree css tests and even reftests. Works for me!
Flags: needinfo?(james)
https://reviewboard.mozilla.org/r/68944/#review66368 Mostly looks fine, with some nits. It is going late here... I'll continue reviewing tomorrow. ::: layout/style/nsCSSParser.cpp:12819 (Diff revisions 4 - 5) > - nsCSSValue &xEdge = value->Item(0), > - &xOffset = value->Item(1), > - &yEdge = value->Item(2), > - &yOffset = value->Item(3); > + nsCSSValue &xEdge = value->Item(0); > + nsCSSValue &xOffset = value->Item(1); > + nsCSSValue &yEdge = value->Item(2); > + nsCSSValue &yOffset = value->Item(3); Our coding style suggests "nsCSSValue& var" rather than "nsCSSValue &var". ::: layout/style/nsCSSParser.cpp:12763 (Diff revision 5) > + uint8_t aDefaultEdge) { > + The open brace should be in the new line, and you don't need a empty line at the beginning of the function. ::: layout/style/nsCSSValue.cpp:984 (Diff revision 5) > + // Ensure invariants from ParsePositionValueBasicShape > + // haven't been modified Please move this comment to be the description of the assertion below, like > MOZ_ASSERT(conditions, "Ensure invariants...");
Comment on attachment 8777789 [details] Bug 1290864 - Add WPT test for serialization of basic-shapes; https://reviewboard.mozilla.org/r/69264/#review66372 ::: testing/web-platform/meta/MANIFEST.json:37241 (Diff revision 3) > + "css-shapes/basic-shape-serialization.html": [ > + { > + "path": "css-shapes/basic-shape-serialization.html", > + "url": "/css-shapes/basic-shape-serialization.html" > + } > + ], This is still not testing all basic shapes, so it should probably be something like circle-ellipse-function-serialization.html ::: testing/web-platform/tests/css-shapes/basic-shape-serialization.html:34 (Diff revision 3) > +checkEquals("circle(closest-side at center)", > + "circle(at 50% 50%)"); Should add a test for non-default radius in circle() function.
Attachment #8777789 - Flags: review?(xidorn+moz)
Comment on attachment 8777372 [details] Bug 1290864 - Standardize serialization of <position> values in <basic-shape>; Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68944/diff/5-6/
Attachment #8777789 - Flags: review?(xidorn+moz)
Comment on attachment 8777789 [details] Bug 1290864 - Add WPT test for serialization of basic-shapes; Review request updated; see interdiff: https://reviewboard.mozilla.org/r/69264/diff/3-4/
Comment on attachment 8777372 [details] Bug 1290864 - Standardize serialization of <position> values in <basic-shape>; Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68944/diff/6-7/
Comment on attachment 8777789 [details] Bug 1290864 - Add WPT test for serialization of basic-shapes; Review request updated; see interdiff: https://reviewboard.mozilla.org/r/69264/diff/4-5/
https://reviewboard.mozilla.org/r/69264/#review66372 > This is still not testing all basic shapes, so it should probably be something like circle-ellipse-function-serialization.html The other basic shapes don't have special serialization rules, though.
https://reviewboard.mozilla.org/r/69264/#review66372 > The other basic shapes don't have special serialization rules, though. Minor issue anyway; renamed.
Comment on attachment 8777789 [details] Bug 1290864 - Add WPT test for serialization of basic-shapes; Review request updated; see interdiff: https://reviewboard.mozilla.org/r/69264/diff/5-6/
Comment on attachment 8777372 [details] Bug 1290864 - Standardize serialization of <position> values in <basic-shape>; https://reviewboard.mozilla.org/r/68944/#review66528 Looks good, thanks. r=me with two nits addressed below. ::: layout/style/nsCSSValue.cpp:987 (Diff revision 7) > + xOffset.GetUnit() != eCSSUnit_Null && > + yOffset.GetUnit() != eCSSUnit_Null && Looks like these two lines are redundant now, as IsLengthPercentCalcUnit() does not include Null. ::: layout/style/nsCSSValue.cpp:993 (Diff revision 7) > + yOffset.GetUnit() != eCSSUnit_Null && > + xOffset.IsLengthPercentCalcUnit() && > + yOffset.IsLengthPercentCalcUnit() && > + xEdge.GetIntValue() != NS_STYLE_IMAGELAYER_POSITION_CENTER && > + yEdge.GetIntValue() != NS_STYLE_IMAGELAYER_POSITION_CENTER, > + "Ensure invariants from ParsePositionValueBasicShape haven't been modified"); Please align this with the lines above. In C++ you can break a string literal this way: > "Ensure invariants from ParsePositionValueBasicShape " > "haven't been modified" and they would be concatenated automatically (unlike Rust which requires a macro IIRC).
Attachment #8777372 - Flags: review?(xidorn+moz) → review+
Comment on attachment 8777789 [details] Bug 1290864 - Add WPT test for serialization of basic-shapes; https://reviewboard.mozilla.org/r/69264/#review66530
Attachment #8777789 - Flags: review?(xidorn+moz) → review+
Looks like you need to reslve the crash in test. It seems the invariant that aOffset is null when aEdge is center is broken somehow.
https://reviewboard.mozilla.org/r/68944/#review66528 > Please align this with the lines above. > > In C++ you can break a string literal this way: > > "Ensure invariants from ParsePositionValueBasicShape " > > "haven't been modified" > and they would be concatenated automatically (unlike Rust which requires a macro IIRC). (Rust lets you use a backslash at the end of the line, and it automatically strips out indentation for the following lines so it still looks neat. You can also use "raw strings", `r##"foo"##`.)
Comment on attachment 8777372 [details] Bug 1290864 - Standardize serialization of <position> values in <basic-shape>; Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68944/diff/7-8/
Comment on attachment 8777789 [details] Bug 1290864 - Add WPT test for serialization of basic-shapes; Review request updated; see interdiff: https://reviewboard.mozilla.org/r/69264/diff/6-7/
Comment on attachment 8777372 [details] Bug 1290864 - Standardize serialization of <position> values in <basic-shape>; Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68944/diff/8-9/
Comment on attachment 8777789 [details] Bug 1290864 - Add WPT test for serialization of basic-shapes; Review request updated; see interdiff: https://reviewboard.mozilla.org/r/69264/diff/7-8/
https://reviewboard.mozilla.org/r/68944/#review66626 ::: layout/style/nsCSSParser.cpp:12766 (Diff revision 8) > + if (nsCSSValue::IsFloatUnit(aOffset.GetUnit()) && > + aOffset.GetFloatValue() == 0.0 && aOffset.IsLengthUnit()) { I think you only need to check that aOffset.IsLengthUnit(). Alternatively, check IsFloatUnit, but asserts if not IsLengthUnit.
Comment on attachment 8777372 [details] Bug 1290864 - Standardize serialization of <position> values in <basic-shape>; Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68944/diff/9-10/
Comment on attachment 8777789 [details] Bug 1290864 - Add WPT test for serialization of basic-shapes; Review request updated; see interdiff: https://reviewboard.mozilla.org/r/69264/diff/8-9/
https://reviewboard.mozilla.org/r/68944/#review66654 ::: layout/style/nsCSSParser.cpp:12766 (Diff revision 10) > + if (aOffset.IsLengthUnit() && > + aOffset.GetFloatValue() == 0.0 && aOffset.IsLengthUnit()) { Two IsLengthUnit()... You only need the first one. ::: layout/style/nsCSSValue.cpp:991 (Diff revision 10) > + "Ensure invariants from ParsePositionValueBasicShape" > + "haven't been modified"); need a whitespace at the end of the first line of the string.
Comment on attachment 8777372 [details] Bug 1290864 - Standardize serialization of <position> values in <basic-shape>; Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68944/diff/10-11/
Comment on attachment 8777789 [details] Bug 1290864 - Add WPT test for serialization of basic-shapes; Review request updated; see interdiff: https://reviewboard.mozilla.org/r/69264/diff/9-10/
(Can't autoland in this case because reviewboard is showing two phantom issues that don't actually exist)
has problems to apply : 7151d7e42849 transplanted to 5ed43997ad1a applying 61ed1ea2620c patching file testing/web-platform/meta/MANIFEST.json Hunk #1 FAILED at 37232 1 out of 1 hunks FAILED -- saving rejects to file testing/web-platform/meta/MANIFEST.json.rej patch failed to apply
Flags: needinfo?(manishearth)
Keywords: checkin-needed
Comment on attachment 8777371 [details] Bug 1290864 - Omit the default closest-side when serializing circle() and ellipse(); Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68942/diff/2-3/
Comment on attachment 8777372 [details] Bug 1290864 - Standardize serialization of <position> values in <basic-shape>; Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68944/diff/11-12/
Comment on attachment 8777789 [details] Bug 1290864 - Add WPT test for serialization of basic-shapes; Review request updated; see interdiff: https://reviewboard.mozilla.org/r/69264/diff/10-11/
Updated
Flags: needinfo?(manishearth)
Keywords: checkin-needed
Pushed by fdesre@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/d5ffe00049c1 Omit the default closest-side when serializing circle() and ellipse(); r=xidorn https://hg.mozilla.org/integration/mozilla-inbound/rev/09ec7c8502e3 Standardize serialization of <position> values in <basic-shape>; r=xidorn https://hg.mozilla.org/integration/mozilla-inbound/rev/ae557997e0a6 Add WPT test for serialization of basic-shapes; r=xidorn
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/d5ffe00049c1 https://hg.mozilla.org/mozilla-central/rev/09ec7c8502e3 https://hg.mozilla.org/mozilla-central/rev/ae557997e0a6
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox51: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Blocks: basic-shape
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: