Closed
Bug 1074528
Opened 10 years ago
Closed 9 years ago
Implement inset shape function for clip-path
Categories
(Core :: Layout, defect)
Core
Layout
Tracking
()
RESOLVED
FIXED
mozilla36
People
(Reporter: krit, Assigned: krit, Mentored)
References
Details
Attachments
(1 file, 4 obsolete files)
36.49 KB,
patch
|
Details | Diff | Splinter Review |
No description provided.
Assignee | ||
Comment 1•10 years ago
|
||
Basic shapes make use of some primitives introduced in other specs. circle() and ellipse() use <position> as specified in CSS Background and Borders. With ParsePositionValue() it was possible to reuse the parsing code from background-position. inset() makes use of the syntax of the 'border-radius' shorthand. Unlike ParsePositionValue(), ParseBoxCornerRadii() sets the nsCSSValues for the individual longhand properties of border-radius directly. This way I can not reuse the code. What is the best way forward here? Rewrite the radius parsing code just for inset()? Refactor the existing code to take nsCSSValue*s and don't set the long hand properties directly? Are there and implications in performance with such a refactoring?
Depends on: 1072894
Assignee | ||
Updated•10 years ago
|
Assignee | ||
Comment 2•10 years ago
|
||
The patch requires ellipse/circle patch to land first and is not meant for landing. I would like to get some early feedback to the refactoring of ParseBoxCornerRadii to ParseBoxCornerRadiiInternals and taking an Array* as argument. Also, I use an bitfield in nsStyleBasicShape now. IIRC GCC is able to put enumerations and integer values into one bitfield. Don't know what happens on Windows. I could turn mType to an int32_t type as well if it should be necessary but it wouldn't be great though. Furthermore, I moved AppendSidesShorthandToString from Declaration to nsCSSValue as a static member function. nsStyleUtils might be the right place, but it would require to include nsCSSValue.h which seems to be a bigger tradeoff. So nsCSSValue seemed to be the better choice. I would like to do the real nsComputedDOMStyle string creation for border-radius in a different patch. Right now it returns the right value but doesn't optimize it by reducing the length of the string where possible. Instead it returns 8 length/percentage values all the time, separated with a '/'.
Flags: needinfo?(dbaron)
Assignee | ||
Comment 3•10 years ago
|
||
I am working on a patch for review based on the feedback to circle/ellipse parsing.
Flags: needinfo?(dbaron)
Assignee | ||
Comment 4•10 years ago
|
||
Instead of the bit field I use a union. I also followed the advices from bug 1074522.
Attachment #8501100 -
Attachment is obsolete: true
Attachment #8505718 -
Flags: review?(dbaron)
Assignee | ||
Comment 5•10 years ago
|
||
Initialize nsStyleCorners if no border radius was specified.
Attachment #8505718 -
Attachment is obsolete: true
Attachment #8505718 -
Flags: review?(dbaron)
Attachment #8506199 -
Flags: review?(dbaron)
Spec links: http://dev.w3.org/fxtf/css-masking-1/#the-clip-path http://dev.w3.org/csswg/css-shapes/#basic-shape-functions
OS: Mac OS X → All
Hardware: x86 → All
Comment on attachment 8506199 [details] [diff] [review] clip-path-inset.patch Looks good, but I'd like to have a look at the revised patch with these comments addressed, so review-: It might have been nice to structure this as a series of patches. For example, moving AppendSidesShorthandToString to nsCSSValue could have been a separate patch. (Don't change it now, though.) Declaration.cpp: >+ nsCSSValue::AppendSidesShorthandToString(subprops, vals, aValue, aSerialization); ... >+ nsCSSValue::AppendSidesShorthandToString(subprops, xVals, aValue, aSerialization); ... >+ nsCSSValue::AppendSidesShorthandToString(subprops, yVals, aValue, aSerialization); Wrap before 80th column. nsCSSParser.cpp: Could you make ParseBoxCornerRadiiInternal take an nsCSSValue[] instead of an nsCSSValue::Array*, so that the main caller doesn't have to heap-allocate an array? (Probably best to add an ItemStorage() accessor to nsCSSValue::Array so it's clear that some callers depend on the items being contiguous.) (Even without that change, ParseBoxCornerRadii would have no need for |value| or for the ".get()".) >+ if (ParseVariant(functionArray->Item(2), VARIANT_LPCALC, nullptr) && >+ ParseVariant(functionArray->Item(3), VARIANT_LPCALC, nullptr) && >+ ParseVariant(functionArray->Item(4), VARIANT_LPCALC, nullptr)) { >+ } I'd just put this as a statement, but add a comment: // Consume up to 4, but only require one. ParseVariant(functionArray->Item(2), VARIANT_LPCALC, nullptr) && ParseVariant(functionArray->Item(3), VARIANT_LPCALC, nullptr) && ParseVariant(functionArray->Item(4), VARIANT_LPCALC, nullptr); (I assume you can do that, though it's rare in C++, I admit.) + if (!ExpectSymbol(')', true)) { + if (!GetToken(true)) { + REPORT_UNEXPECTED_EOF(PEBorderRadiusEOF); Instead of this REPORT_UNEXPECTED_EOF, how about: NS_NOTREACHED("ExpectSymbol should have returned true"); And delete PEBorderRadiusEOF too. >+ !ParseBoxCornerRadiiInternals(radiusArray.get())) { No .get() needed. nsCSSValue.cpp: >+nsCSSValue::AppendSidesShorthandToString(const nsCSSProperty aProperties[], >+ const nsCSSValue* aValues[], >+ nsAString& aString, >+ nsCSSValue::Serialization aSerialization) Fix indentation of the later lines in this quote to line up. nsCSSValue::AppendBasicShapeRadiusToString could perhaps share a little more code with the border_radius case in Declaration::GetValue. (If it does, perhaps what isn't shared could move into AppendInsetToString.) AppendInsetToString could call AppendSidesShorthandToString; it just needs to construct a dummy array of the same property repeated 4 times, and then use the array storage getter I mentioned adding above. nsComputedDOMStyle.cpp: Please spell BashicShapeRadiiToString correctly, i.e., as BasicShapeRadiiToString. >+ for (uint8_t i = 0; i < 8; i += 2) { >+ horizontal.AppendElement(aCorners.Get(i)); >+ vertical.AppendElement(aCorners.Get(i + 1)); >+ } Please write this as: NS_FOR_CSS_FULL_CORNERS(corner) { horizontal.AppendElement( aCorners.Get(NS_FULL_TO_HALF_CORNER(corner, false)); vertical.AppendElement( aCorners.Get(NS_FULL_TO_HALF_CORNER(corner, true)); } >+ if (horizontal == vertical) { Cheaper to compare horizontalString and verticalString. nsRuleNode.cpp: >+ int32_t mask = SETCOORD_PERCENT | SETCOORD_LENGTH | >+ SETCOORD_STORE_CALC; const int32_t (and the same for the existing two chunks, too!) >+ inset = coords[0]; Please assert that j != 1. >+ if (SetPairCoords(radius, coordX, coordY, >+ nsStyleCoord(), nsStyleCoord(), mask, >+ aStyleContext, aPresContext, >+ aCanStoreInRuleTree)) { What if SetPairCoords fails? I'm guessing that it can't, and that you should assert it can't. (Though it's still ok to test it at runtime.) nsStyleConsts.h: >+ >+// Basic Shape has radius >+#define NS_STYLE_BASIC_SHAPE_NO_RADIUS 0 >+#define NS_STYLE_BASIC_SHAPE_RADIUS 1 This is unused; remove it. I'm not sure why nsStyleBasicShape has an mFillRule, although given that it's there, initializing it is good. But maybe it can be removed? >+ bool HasRadius() const { This should probably assert that type is inset just like the following 2 functions do.
Attachment #8506199 -
Flags: review?(dbaron) → review-
Component: Graphics → Layout
Assignee: nobody → krit
Assignee | ||
Comment 8•10 years ago
|
||
(In reply to David Baron [:dbaron] (UTC-4) (needinfo? for questions) (away/busy Oct. 24-31) from comment #7) > Comment on attachment 8506199 [details] [diff] [review] > clip-path-inset.patch > I fixed all comments. > I'm not sure why nsStyleBasicShape has an mFillRule, although > given that it's there, initializing it is good. But maybe it > can be removed? > mFillRule is not used for inset() but needed for polygon(). polygon() has an argument nonzero or evenodd which is encoded in this variable.
Attachment #8506199 -
Attachment is obsolete: true
Attachment #8510644 -
Flags: review?(dbaron)
Comment on attachment 8510644 [details] [diff] [review] clip-path-inset.patch Sorry for dropping this one on the floor during TPAC. Declaration.cpp new changes? >+ REPORT_UNEXPECTED_EOF(PEBorderRadiusEOF); You need to delete this line. >+ // caller depend on the items being contiguous caller -> callers >+ void BasicShapeRadiiToString(nsAString& aCssText, >+ const nsStyleCorners& aCorners); Fix indent (one space less on the second line). r=dbaron with that fixed
Attachment #8510644 -
Flags: review?(dbaron) → review+
Don't forget about getting this landed, please.
Flags: needinfo?(krit)
Assignee | ||
Comment 11•10 years ago
|
||
I'll prepare the patch when I have a bit more time. Won't forget it :)
Flags: needinfo?(krit)
Assignee | ||
Comment 12•10 years ago
|
||
Patch for landing. Try bots results here: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=a140bc133044
Attachment #8510644 -
Attachment is obsolete: true
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Comment 13•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/3c993904ea4f
Keywords: checkin-needed
Comment 14•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/3c993904ea4f
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
You need to log in
before you can comment on or make changes to this bug.
Description
•