Add shape-outside support to the style system

RESOLVED FIXED in Firefox 51

Status

()

Core
CSS Parsing and Computation
RESOLVED FIXED
a year ago
10 months ago

People

(Reporter: TYLin, Assigned: TYLin)

Tracking

(Blocks: 1 bug)

Trunk
mozilla51
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox50 affected, firefox51 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(8 attachments)

(Assignee)

Description

a year ago
Style system are changing rapidly these days. I'd like to land the shape-outside support to prevent my patch bitrot.
(Assignee)

Comment 1

a year ago
Created attachment 8773643 [details]
Bug 1288626 Part 8 - Add shape-outside support to style system.

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

a year ago
https://reviewboard.mozilla.org/r/66386/#review63456

dbaron is on vacation. Try heycam.
(Assignee)

Updated

a year ago
Attachment #8773643 - Flags: review?(dbaron) → review?(cam)
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)

Updated

a year ago
Blocks: 1289049
(Assignee)

Comment 4

a year 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.
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.
(Assignee)

Updated

a year ago
Depends on: 1289052
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

a year 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

a year 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

a year ago
Created attachment 8775875 [details]
Bug 1288626 Part 1 - Move nsBasicShape and nsStyleClipPath into mozilla namespace.

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

a year ago
Created attachment 8775876 [details]
Bug 1288626 Part 2 - Use basic shape enum class defined in nsStyleConsts.h.

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

a year ago
Created attachment 8775877 [details]
Bug 1288626 Part 3 - Rename nsStyleBasicShape to StyleBasicShape.

Review commit: https://reviewboard.mozilla.org/r/67696/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/67696/
(Assignee)

Comment 12

a year ago
Created attachment 8775878 [details]
Bug 1288626 Part 4 - Rename nsStyleClipPath to StyleClipPath.

Review commit: https://reviewboard.mozilla.org/r/67698/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/67698/
(Assignee)

Comment 13

a year ago
Created attachment 8775879 [details]
Bug 1288626 Part 5 - Rename StyleClipPathType to StyleShapeSourceType.

Review commit: https://reviewboard.mozilla.org/r/67700/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/67700/
(Assignee)

Comment 14

a year ago
Created attachment 8775880 [details]
Bug 1288626 Part 7 - Rename StyleClipShapeSizing to StyleClipPathGeometryBox.

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

a year ago
Created attachment 8775881 [details]
Bug 1288626 Part 6 - Generalize StyleClipPath to be template struct StyleShapeSource.

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

a year 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

a year 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

a year 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

a year 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

a year 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

a year 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

a year 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

a year 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

a year 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

a year 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

a year 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

a year 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

a year ago
Latest try results looks good. Ready for the review.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=17db89ab4aee
https://reviewboard.mozilla.org/r/67692/#review65860
Attachment #8775875 - Flags: review?(cam) → review+
Comment on attachment 8775875 [details]
Bug 1288626 Part 1 - Move nsBasicShape and nsStyleClipPath into mozilla namespace.

https://reviewboard.mozilla.org/r/67692/#review65862
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 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 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 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+
Attachment #8775880 - Flags: review?(cam) → review+
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?
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.
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.
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

a year 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.
Depends on: 1291962
(Assignee)

Comment 40

a year 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

a year 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

a year 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

a year 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

a year 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

a year 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

a year 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

a year 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

a year 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 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

a year 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 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

a year 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.
(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

a year 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

a year 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

a year 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

a year 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
Last Resolved: a year ago
status-firefox51: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51

Updated

10 months ago
See Also: → bug 1266316
You need to log in before you can comment on or make changes to this bug.