Closed
Bug 1288626
Opened 8 years ago
Closed 8 years ago
Add shape-outside support to the style system
Categories
(Core :: CSS Parsing and Computation, defect)
Core
CSS Parsing and Computation
Tracking
()
RESOLVED
FIXED
mozilla51
People
(Reporter: TYLin, Assigned: TYLin)
References
(Blocks 1 open bug)
Details
Attachments
(8 files)
58 bytes,
text/x-review-board-request
|
heycam
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
heycam
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
heycam
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
heycam
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
heycam
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
heycam
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
heycam
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
heycam
:
review+
|
Details |
Style system are changing rapidly these days. I'd like to land the shape-outside support to prevent my patch bitrot.
Assignee | ||
Comment 1•8 years ago
|
||
The style system support of shape-outside property is very similar to the clip-path property, and the logic to deal with the basic-shape is the same. Review commit: https://reviewboard.mozilla.org/r/66388/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/66388/
Attachment #8773643 -
Flags: review?(dbaron)
Assignee | ||
Comment 2•8 years ago
|
||
https://reviewboard.mozilla.org/r/66386/#review63456 dbaron is on vacation. Try heycam.
Assignee | ||
Updated•8 years ago
|
Attachment #8773643 -
Flags: review?(dbaron) → review?(cam)
Comment 3•8 years ago
|
||
Comment on attachment 8773643 [details] Bug 1288626 Part 8 - Add shape-outside support to style system. https://reviewboard.mozilla.org/r/66388/#review63490 This looks good, just a couple of comments. ::: layout/style/nsCSSPropList.h:3704 (Diff revision 1) > + CSS_PROPERTY_PARSE_VALUE | > + CSS_PROPERTY_VALUE_PARSER_FUNCTION | I don't think it matters much, but I *believe* CSS_PROPERTY_VALUE_PARSER_FUNCTION is for properties that have a parsing function and which can be parsed as part of a shorthand property. (The functions which parse the shorthands then loop around calling ParseSingleValueProperty for each of the longhand components.) There are probably a couple of properties handled in ParseSingleValueProperty that aren't in this category (e.g. image-orientation). To try to keep this consistent with (most) other function-parsed properties, let's make this CSS_PROPERTY_PARSE_FUNCTION and move the ParseShapeOutside() call to ParsePropertyByFunction. ::: layout/style/nsCSSPropList.h:3711 (Diff revision 1) > + CSS_PROPERTY_APPLIES_TO_FIRST_LETTER, > + "layout.css.shape-outside.enabled", > + 0, > + nullptr, > + CSS_PROP_NO_OFFSET, > + eStyleAnimType_None) // FIXME: basic-shapes are animatable. Please file a followup for this. ::: layout/style/nsComputedDOMStyle.cpp:3255 (Diff revision 1) > + RefPtr<nsROCSSPrimitiveValue> val = new nsROCSSPrimitiveValue; > + val->SetIdent(eCSSKeyword_none); > + return val.forget(); Should this be doing something else? ::: layout/style/nsRuleNode.h:806 (Diff revision 1) > + void SetStyleShapeOutsideToCSSValue(mozilla::StyleShapeOutside* aShapeOutside, > + const nsCSSValue* aValue, > + nsStyleContext* aStyleContext, > + nsPresContext* aPresContext, > + mozilla::RuleNodeCacheConditions& aConditions); I think this method should be a static function in nsRuleNode.cpp, rather than a public non-static method on nsRuleNode itself, since it doesn't need to access anything on nsRuleNode. Can you move that, and file a followup to move these other methods (GetShadowData, GetStyleBasicShapeFromCSSValue, SetStyleFilterToCSSValue, SetStyleClipPathToCSSValue) too? ::: layout/style/nsRuleNode.cpp:6417 (Diff revision 1) > + if (url) { > + display->mShapeOutside.SetURL(url); > + } I think if url is null it indicates a bug (e.g. we called CSSParserImpl::SetValueToURL incorrectly), so I think it's worth asserting as well that it's not null. (And in the clip-path handling too.) ::: layout/style/nsStyleStruct.h:2487 (Diff revision 1) > +// shape-box for shape-outside > +enum class StyleShapeBox : uint8_t { > + NoBox, > + Margin, > + Border, > + Padding, > + Content > +}; This should go in nsStyleConsts.h. ::: layout/style/test/property_database.js:5795 (Diff revision 1) > + other_values: [ > + "url(#my-shape-outside)", > + ].concat(basicShapeOtherValues), This entry should cause some tests to fail, given the bug in nsComputedDOMStyle.cpp, which makes me think we're not running these tests for shape-outside. Maybe you need to set the pref in testing/profiles/prefs_general.js.
Attachment #8773643 -
Flags: review?(cam) → review-
Assignee | ||
Comment 4•8 years ago
|
||
https://reviewboard.mozilla.org/r/66388/#review63490 > Please file a followup for this. Filed Bug 1289049 for this. > I think this method should be a static function in nsRuleNode.cpp, rather than a public non-static method on nsRuleNode itself, since it doesn't need to access anything on nsRuleNode. Can you move that, and file a followup to move these other methods (GetShadowData, GetStyleBasicShapeFromCSSValue, SetStyleFilterToCSSValue, SetStyleClipPathToCSSValue) too? File Bug 1289052 for this.
Comment 5•8 years ago
|
||
https://reviewboard.mozilla.org/r/66388/#review63568 ::: layout/style/nsCSSPropList.h:3704 (Diff revision 1) > + CSS_PROPERTY_PARSE_VALUE | > + CSS_PROPERTY_VALUE_PARSER_FUNCTION | TY points out that this contradicts the comment in nsCSSProps.h, so we should indeed use CSS_PROPERTY_VALUE_PARSER_FUNCTION here.
Since this was in my review queue, I reviewed it on the airplane, and had the following comments: nsCSSPropList.h: Please put a bug number (as in "FIXME (bug NNNNNNN)"), and file the bug if it doesn't exist yet. Should StyleShapeBox and StyleClipShapeSizing share their common constants? (Can one enum class inherit from another?) nsComputedDOMStyle.cpp: >+ val->SetString(boxString); This should be val->SetIdent() rather than SetString(). And so should nsComputedDOMStyle::CreatePrimitiveValueForClipPath. And, really, this should share rather more code with the clip-path code. nsRuleNode.cpp: >+ display->mShapeOutside = StyleShapeOutside(); >+ nsIURI* url = shapeOutsideValue->GetURLValue(); >+ if (url) { >+ display->mShapeOutside.SetURL(url); >+ } eCSSUnit_URL doesn't allow null pointers. So you don't need the null-check or the initialization to StyleShapeOutside(). (And clip-path should be fixed in the same way.) SetStyleShapeOutsideToCSSValue also looks a lot like SetStyleBasicShapeToCSSValue. Can they share code? nsStyleStruct.h: It also seems like there's too much copied code here (relative to nsStyleClipPath). (I didn't review it closely.)
Assignee | ||
Comment 7•8 years ago
|
||
Per dbaron's comment 6, I'd like to refactor the code involving basic shape and clip-path before adding support for shape-outside so that the code can be shared as much as possible.
Assignee | ||
Comment 8•8 years ago
|
||
Re comment 6: > Should StyleShapeBox and StyleClipShapeSizing share their common constants? > (Can one enum class inherit from another?) No. C++ does not support enum class inheritance.
Assignee | ||
Comment 9•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/67692/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/67692/
Attachment #8773643 -
Attachment description: Bug 1288626 - Add shape-outside support to style system. → Bug 1288626 Part 8 - Add shape-outside support to style system.
Attachment #8775875 -
Flags: review?(cam)
Attachment #8775876 -
Flags: review?(cam)
Attachment #8775877 -
Flags: review?(cam)
Attachment #8775878 -
Flags: review?(cam)
Attachment #8775879 -
Flags: review?(cam)
Attachment #8775880 -
Flags: review?(cam)
Attachment #8775881 -
Flags: review?(cam)
Attachment #8773643 -
Flags: review- → review?(cam)
Assignee | ||
Comment 10•8 years ago
|
||
Rename StyleBasicShape to StyleBasicShapeType in nsStyleConsts.h, and replace the old enum nsStyleBasicShape::Type by the enum class StyleBasicShapeType. Also, replace NS_ASSERTION() by MOZ_ASSERT(). Review commit: https://reviewboard.mozilla.org/r/67694/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/67694/
Assignee | ||
Comment 11•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/67696/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/67696/
Assignee | ||
Comment 12•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/67698/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/67698/
Assignee | ||
Comment 13•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/67700/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/67700/
Assignee | ||
Comment 14•8 years ago
|
||
Change to geometry box to match the name in the spec. https://drafts.fxtf.org/css-masking-1/#the-clip-path Review commit: https://reviewboard.mozilla.org/r/67702/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/67702/
Assignee | ||
Comment 15•8 years ago
|
||
The only difference between clip-path and shape-outside is the reference box enum, so I make StyleBasicShapeOrURL a template struct to accommodate both. I'll have to move all the method definition to the header to make the linker happy. Also, rename SizingBox to ReferenceBox. Review commit: https://reviewboard.mozilla.org/r/67704/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/67704/
Assignee | ||
Comment 16•8 years ago
|
||
Comment on attachment 8773643 [details] Bug 1288626 Part 8 - Add shape-outside support to style system. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/66388/diff/1-2/
Assignee | ||
Comment 17•8 years ago
|
||
https://reviewboard.mozilla.org/r/66388/#review63490 > Should this be doing something else? Should be fixed in patch set 2 since shape-outside now shares the code with clip-path. > I think if url is null it indicates a bug (e.g. we called CSSParserImpl::SetValueToURL incorrectly), so I think it's worth asserting as well that it's not null. (And in the clip-path handling too.) Fixed for both shape-outside and clip-path code in patch set 2. > This entry should cause some tests to fail, given the bug in nsComputedDOMStyle.cpp, which makes me think we're not running these tests for shape-outside. Maybe you need to set the pref in testing/profiles/prefs_general.js. Turn on the pref for testing in patch set 2. BTW, the clip-path pref is *not* turned on due to bug 1269990. Shape-ouside does not support animation yet, so even if we use the same code, we won't be affected for now.
Assignee | ||
Comment 18•8 years ago
|
||
Comment on attachment 8773643 [details] Bug 1288626 Part 8 - Add shape-outside support to style system. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/66388/diff/2-3/
Assignee | ||
Comment 19•8 years ago
|
||
https://reviewboard.mozilla.org/r/66388/#review65258 ::: layout/style/nsCSSValue.cpp:1357 (Diff revision 3) > nsCSSProps::kClipPathGeometryBoxKTable), > aResult); > break; > > + case eCSSProperty_shape_outside: > + AppendASCIItoUTF16(nsCSSProps::ValueToKeyword(intValue, This needs more work. I got parse+serialize test failure like https://treeherder.mozilla.org/logviewer.html#?job_id=24823462&repo=try#L3185
Assignee | ||
Comment 20•8 years ago
|
||
Comment on attachment 8775875 [details] Bug 1288626 Part 1 - Move nsBasicShape and nsStyleClipPath into mozilla namespace. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67692/diff/1-2/
Assignee | ||
Comment 21•8 years ago
|
||
Comment on attachment 8775876 [details] Bug 1288626 Part 2 - Use basic shape enum class defined in nsStyleConsts.h. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67694/diff/1-2/
Assignee | ||
Comment 22•8 years ago
|
||
Comment on attachment 8775877 [details] Bug 1288626 Part 3 - Rename nsStyleBasicShape to StyleBasicShape. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67696/diff/1-2/
Assignee | ||
Comment 23•8 years ago
|
||
Comment on attachment 8775878 [details] Bug 1288626 Part 4 - Rename nsStyleClipPath to StyleClipPath. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67698/diff/1-2/
Assignee | ||
Comment 24•8 years ago
|
||
Comment on attachment 8775879 [details] Bug 1288626 Part 5 - Rename StyleClipPathType to StyleShapeSourceType. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67700/diff/1-2/
Assignee | ||
Comment 25•8 years ago
|
||
Comment on attachment 8775880 [details] Bug 1288626 Part 7 - Rename StyleClipShapeSizing to StyleClipPathGeometryBox. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67702/diff/1-2/
Assignee | ||
Comment 26•8 years ago
|
||
Comment on attachment 8775881 [details] Bug 1288626 Part 6 - Generalize StyleClipPath to be template struct StyleShapeSource. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67704/diff/1-2/
Assignee | ||
Comment 27•8 years ago
|
||
Comment on attachment 8773643 [details] Bug 1288626 Part 8 - Add shape-outside support to style system. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/66388/diff/3-4/
Assignee | ||
Comment 28•8 years ago
|
||
Latest try results looks good. Ready for the review. https://treeherder.mozilla.org/#/jobs?repo=try&revision=17db89ab4aee
Updated•8 years ago
|
Attachment #8775875 -
Flags: review?(cam) → review+
Comment 30•8 years ago
|
||
Comment on attachment 8775875 [details] Bug 1288626 Part 1 - Move nsBasicShape and nsStyleClipPath into mozilla namespace. https://reviewboard.mozilla.org/r/67692/#review65862
Comment 31•8 years ago
|
||
Comment on attachment 8775877 [details] Bug 1288626 Part 3 - Rename nsStyleBasicShape to StyleBasicShape. https://reviewboard.mozilla.org/r/67696/#review65864
Attachment #8775877 -
Flags: review?(cam) → review+
Comment 32•8 years ago
|
||
Comment on attachment 8775876 [details] Bug 1288626 Part 2 - Use basic shape enum class defined in nsStyleConsts.h. https://reviewboard.mozilla.org/r/67694/#review65866
Attachment #8775876 -
Flags: review?(cam) → review+
Comment 33•8 years ago
|
||
Comment on attachment 8775878 [details] Bug 1288626 Part 4 - Rename nsStyleClipPath to StyleClipPath. https://reviewboard.mozilla.org/r/67698/#review65868
Attachment #8775878 -
Flags: review?(cam) → review+
Comment 34•8 years ago
|
||
Comment on attachment 8775879 [details] Bug 1288626 Part 5 - Rename StyleClipPathType to StyleShapeSourceType. https://reviewboard.mozilla.org/r/67700/#review65874 ::: layout/style/nsStyleConsts.h:67 (Diff revision 2) > Inset, > }; > > +// Basic shapes or URL type > +// X11 has a #define for None causing conflicts, so we use None_ here > +enum class StyleBasicShapeOrURLType : uint8_t { I think "Box" isn't a basic shape or a URL. What do you think of "StyleShapeSourceType"? To me that sounds like the kind of source of shape data -- given by a url(), a basic shape specification, or a box.
Attachment #8775879 -
Flags: review?(cam) → review+
Updated•8 years ago
|
Attachment #8775880 -
Flags: review?(cam) → review+
Comment 35•8 years ago
|
||
Comment on attachment 8775880 [details] Bug 1288626 Part 7 - Rename StyleClipShapeSizing to StyleClipPathGeometryBox. https://reviewboard.mozilla.org/r/67702/#review65876 ::: layout/style/nsStyleStruct.h:3522 (Diff revision 2) > union { > StyleBasicShape* mBasicShape; > nsIURI* mURL; > }; > StyleBasicShapeOrURLType mType; > - StyleClipShapeSizing mSizingBox; > + StyleClipPathGeometryBox mSizingBox; Should we rename mSizingBox to mGeometryBox too?
Comment 36•8 years ago
|
||
https://reviewboard.mozilla.org/r/67702/#review65876 > Should we rename mSizingBox to mGeometryBox too? Never mind, I see it is being renamed in the next patch.
Comment 37•8 years ago
|
||
By the way, I think this patch will conflict with the changes in bug 652991 to use FragmentOrURL instead of nsIURI for the url() values of clip-path.
Comment 38•8 years ago
|
||
https://reviewboard.mozilla.org/r/67704/#review65880 ::: layout/style/nsStyleStruct.h:3476 (Diff revision 2) > nsTArray<nsStyleCoord> mCoordinates; > Position mPosition; > nsStyleCorners mRadius; > }; > > -struct StyleClipPath > +template <typename ReferenceBox> Small nit: I think the prevailing style is not to have a space between "template" and "<". ::: layout/style/nsStyleStruct.h:3477 (Diff revision 2) > Position mPosition; > nsStyleCorners mRadius; > }; > > -struct StyleClipPath > +template <typename ReferenceBox> > +struct StyleBasicShapeOrURL I see the naming of StyleBasicShapeOrURLType matches StyleBasicShapeOrURL here. Feel free to leave the Type type named that way, or to rename the class here to StyleShapeSource. ::: layout/style/nsStyleStruct.h:3522 (Diff revision 2) > + ReleaseRef(); > + mReferenceBox = ReferenceBox::NoBox; > + mType = StyleBasicShapeOrURLType::None_; > + } > + return *this; > + }; Drop ";". ::: layout/style/nsStyleStruct.h:3531 (Diff revision 2) > + if (mType != aOther.mType) { > + return false; > + } > + > + if (mType == StyleBasicShapeOrURLType::URL) { > + return EqualURIs(mURL, aOther.mURL); I guess the reference to EqualURIs here is OK because the template gets instantiated within nsStyleStruct.cpp? And calling operator== from another file probably won't work. Maybe add a comment to that effect, or copy the body of EqualURIs in here.
Assignee | ||
Comment 39•8 years ago
|
||
https://reviewboard.mozilla.org/r/67700/#review65874 > I think "Box" isn't a basic shape or a URL. What do you think of "StyleShapeSourceType"? To me that sounds like the kind of source of shape data -- given by a url(), a basic shape specification, or a box. StyleShapeSourceType sounds good to me, and shorter.
Assignee | ||
Comment 40•8 years ago
|
||
Comment on attachment 8775875 [details] Bug 1288626 Part 1 - Move nsBasicShape and nsStyleClipPath into mozilla namespace. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67692/diff/2-3/
Attachment #8775879 -
Attachment description: Bug 1288626 Part 5 - Rename StyleClipPathType to StyleBasicShapeOrURLType. → Bug 1288626 Part 5 - Rename StyleClipPathType to StyleShapeSourceType.
Attachment #8775881 -
Attachment description: Bug 1288626 Part 7 - Generalize StyleClipPath to be template struct StyleBasicShapeOrURL. → Bug 1288626 Part 6 - Generalize StyleClipPath to be template struct StyleShapeSource.
Attachment #8775880 -
Attachment description: Bug 1288626 Part 6 - Rename StyleClipShapeSizing to StyleClipPathGeometryBox. → Bug 1288626 Part 7 - Rename StyleClipShapeSizing to StyleClipPathGeometryBox.
Assignee | ||
Comment 41•8 years ago
|
||
Comment on attachment 8775876 [details] Bug 1288626 Part 2 - Use basic shape enum class defined in nsStyleConsts.h. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67694/diff/2-3/
Assignee | ||
Comment 42•8 years ago
|
||
Comment on attachment 8775877 [details] Bug 1288626 Part 3 - Rename nsStyleBasicShape to StyleBasicShape. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67696/diff/2-3/
Assignee | ||
Comment 43•8 years ago
|
||
Comment on attachment 8775878 [details] Bug 1288626 Part 4 - Rename nsStyleClipPath to StyleClipPath. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67698/diff/2-3/
Assignee | ||
Comment 44•8 years ago
|
||
Comment on attachment 8775879 [details] Bug 1288626 Part 5 - Rename StyleClipPathType to StyleShapeSourceType. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67700/diff/2-3/
Assignee | ||
Comment 45•8 years ago
|
||
Comment on attachment 8775881 [details] Bug 1288626 Part 6 - Generalize StyleClipPath to be template struct StyleShapeSource. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67704/diff/2-3/
Assignee | ||
Comment 46•8 years ago
|
||
Comment on attachment 8775880 [details] Bug 1288626 Part 7 - Rename StyleClipShapeSizing to StyleClipPathGeometryBox. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67702/diff/2-3/
Assignee | ||
Comment 47•8 years ago
|
||
Comment on attachment 8773643 [details] Bug 1288626 Part 8 - Add shape-outside support to style system. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/66388/diff/4-5/
Assignee | ||
Comment 48•8 years ago
|
||
https://reviewboard.mozilla.org/r/67704/#review65880 > I guess the reference to EqualURIs here is OK because the template gets instantiated within nsStyleStruct.cpp? And calling operator== from another file probably won't work. > > Maybe add a comment to that effect, or copy the body of EqualURIs in here. mURL has been changed to the FragmentOrURL type. Using its operator== should be sufficient.
Comment 49•8 years ago
|
||
Comment on attachment 8775881 [details] Bug 1288626 Part 6 - Generalize StyleClipPath to be template struct StyleShapeSource. https://reviewboard.mozilla.org/r/67704/#review66320 ::: layout/style/nsStyleStruct.h:3643 (Diff revision 3) > + mBasicShape->AddRef(); > + mReferenceBox = aReferenceBox; > + mType = StyleShapeSourceType::Shape; > + } > > - StyleClipShapeSizing GetSizingBox() const { return mSizingBox; } > + ReferenceBox GetReferenceBox() const { return mReferenceBox; } Should we assert in here that our type isn't URL? ::: layout/style/nsStyleStruct.h:3661 (Diff revision 3) > + mBasicShape->Release(); > + } else if (mType == StyleShapeSourceType::URL) { > + NS_ASSERTION(mURL, "expected pointer"); > + delete mURL; > + } > + // mBasicShap, mURL, etc. are all pointers in a union of pointers. Nulling *mBasicShape (I realise you're just moving the code though)
Attachment #8775881 -
Flags: review?(cam) → review+
Assignee | ||
Comment 50•8 years ago
|
||
https://reviewboard.mozilla.org/r/67704/#review66320 > Should we assert in here that our type isn't URL? The reference box is meaningful only if the type is Box or Shape. I think we can assert that the type should be one of them. > *mBasicShape (I realise you're just moving the code though) I'll fix it in next patch set.
Comment 51•8 years ago
|
||
mozreview-review |
Comment on attachment 8773643 [details] Bug 1288626 Part 8 - Add shape-outside support to style system. https://reviewboard.mozilla.org/r/66388/#review67104 ::: layout/style/nsCSSPropList.h:3722 (Diff revision 5) > + CSS_PROPERTY_APPLIES_TO_FIRST_LETTER, > + "layout.css.shape-outside.enabled", > + 0, > + nullptr, > + CSS_PROP_NO_OFFSET, > + eStyleAnimType_None) // FIXME: Bug 1289019 for adding animation support This doesn't look like the right bug number? ::: layout/style/nsStyleStruct.cpp:3079 (Diff revision 5) > , mAnimationNameCount(aSource.mAnimationNameCount) > , mAnimationDirectionCount(aSource.mAnimationDirectionCount) > , mAnimationFillModeCount(aSource.mAnimationFillModeCount) > , mAnimationPlayStateCount(aSource.mAnimationPlayStateCount) > , mAnimationIterationCountCount(aSource.mAnimationIterationCountCount) > + , mShapeOutside(aSource.mShapeOutside) Please also initialize mShapeOutside in the other constructor. ::: layout/style/nsStyleStruct.cpp:3161 (Diff revision 5) > || mBreakBefore != aNewData.mBreakBefore > || mBreakAfter != aNewData.mBreakAfter > || mAppearance != aNewData.mAppearance > || mOrient != aNewData.mOrient > - || mOverflowClipBox != aNewData.mOverflowClipBox) { > + || mOverflowClipBox != aNewData.mOverflowClipBox > + || mShapeOutside != aNewData.mShapeOutside) { It's not clear yet exactly what change hints will be needed to handle shape-outside changes. It would be fine to handle this as an nsChangeHint_NeutralChange for now.
Attachment #8773643 -
Flags: review?(cam) → review+
Assignee | ||
Comment 52•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8773643 [details] Bug 1288626 Part 8 - Add shape-outside support to style system. https://reviewboard.mozilla.org/r/66388/#review67104 > This doesn't look like the right bug number? It should be bug 1289049. Nice catch. > Please also initialize mShapeOutside in the other constructor. mShapeOutside have a default constructor. I think it's OK not to write it explicitly in other constructors of nsStyleDisplay. > It's not clear yet exactly what change hints will be needed to handle shape-outside changes. It would be fine to handle this as an nsChangeHint_NeutralChange for now. Fair enough. We can always change this when implementing the rendering part if we need something else.
Comment 53•8 years ago
|
||
(In reply to Ting-Yu Lin [:TYLin] (UTC+8) from comment #52) > > Please also initialize mShapeOutside in the other constructor. > > mShapeOutside have a default constructor. I think it's OK not to write it > explicitly in other constructors of nsStyleDisplay. You are right of course. I was thinking that its type was the enum, but it's the struct.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 62•8 years ago
|
||
Pushed by tlin@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/be2c56decd65 Part 1 - Move nsBasicShape and nsStyleClipPath into mozilla namespace. r=heycam https://hg.mozilla.org/integration/mozilla-inbound/rev/654556e45f5a Part 2 - Use basic shape enum class defined in nsStyleConsts.h. r=heycam https://hg.mozilla.org/integration/mozilla-inbound/rev/17f63bb7be11 Part 3 - Rename nsStyleBasicShape to StyleBasicShape. r=heycam https://hg.mozilla.org/integration/mozilla-inbound/rev/74633b2735e0 Part 4 - Rename nsStyleClipPath to StyleClipPath. r=heycam https://hg.mozilla.org/integration/mozilla-inbound/rev/c5e4865819ff Part 5 - Rename StyleClipPathType to StyleShapeSourceType. r=heycam https://hg.mozilla.org/integration/mozilla-inbound/rev/07e9cb8a46fd Part 6 - Generalize StyleClipPath to be template struct StyleShapeSource. r=heycam https://hg.mozilla.org/integration/mozilla-inbound/rev/eebda25d85cd Part 7 - Rename StyleClipShapeSizing to StyleClipPathGeometryBox. r=heycam https://hg.mozilla.org/integration/mozilla-inbound/rev/caee616ea09c Part 8 - Add shape-outside support to style system. r=heycam
I had to back these out because it conflicted with the changes from bug 1291280, and is stopping me from merging stuff around. Feel free to just rebase and reland. Sorry about this. https://hg.mozilla.org/integration/mozilla-inbound/rev/34002422988b
Flags: needinfo?(tlin)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 80•8 years ago
|
||
Pushed by tlin@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/bd21cb3d7d2f Part 1 - Move nsBasicShape and nsStyleClipPath into mozilla namespace. r=heycam https://hg.mozilla.org/integration/autoland/rev/6002c44767ba Part 2 - Use basic shape enum class defined in nsStyleConsts.h. r=heycam https://hg.mozilla.org/integration/autoland/rev/12ff52a5c3e7 Part 3 - Rename nsStyleBasicShape to StyleBasicShape. r=heycam https://hg.mozilla.org/integration/autoland/rev/ebe6ae09a5e4 Part 4 - Rename nsStyleClipPath to StyleClipPath. r=heycam https://hg.mozilla.org/integration/autoland/rev/15b183a994b0 Part 5 - Rename StyleClipPathType to StyleShapeSourceType. r=heycam https://hg.mozilla.org/integration/autoland/rev/fbff11330a7f Part 6 - Generalize StyleClipPath to be template struct StyleShapeSource. r=heycam https://hg.mozilla.org/integration/autoland/rev/8b1d83675347 Part 7 - Rename StyleClipShapeSizing to StyleClipPathGeometryBox. r=heycam https://hg.mozilla.org/integration/autoland/rev/3444b7c5c08f Part 8 - Add shape-outside support to style system. r=heycam
Assignee | ||
Comment 81•8 years ago
|
||
I've rebased against bug 1291280 and bug 1288812 (which is still in autoland for now), and landed my patches in autoland.
Flags: needinfo?(tlin)
Comment 82•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/bd21cb3d7d2f https://hg.mozilla.org/mozilla-central/rev/6002c44767ba https://hg.mozilla.org/mozilla-central/rev/12ff52a5c3e7 https://hg.mozilla.org/mozilla-central/rev/ebe6ae09a5e4 https://hg.mozilla.org/mozilla-central/rev/15b183a994b0 https://hg.mozilla.org/mozilla-central/rev/fbff11330a7f https://hg.mozilla.org/mozilla-central/rev/8b1d83675347 https://hg.mozilla.org/mozilla-central/rev/3444b7c5c08f
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox51:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
You need to log in
before you can comment on or make changes to this bug.
Description
•