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)

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

Tracking

()

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

People

(Reporter: TYLin, Assigned: TYLin)

References

(Blocks 1 open bug)

Details

Attachments

(8 files)

Bug 1288626 Part 8 - Add shape-outside support to style system.
58 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1288626 Part 1 - Move nsBasicShape and nsStyleClipPath into mozilla namespace.
58 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1288626 Part 2 - Use basic shape enum class defined in nsStyleConsts.h.
58 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1288626 Part 3 - Rename nsStyleBasicShape to StyleBasicShape.
58 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1288626 Part 4 - Rename nsStyleClipPath to StyleClipPath.
58 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1288626 Part 5 - Rename StyleClipPathType to StyleShapeSourceType.
58 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1288626 Part 7 - Rename StyleClipShapeSizing to StyleClipPathGeometryBox.
58 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1288626 Part 6 - Generalize StyleClipPath to be template struct StyleShapeSource.
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.
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)
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-
Blocks: 1289049
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.
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.)
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.
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.
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)
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/
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/
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/
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.
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/
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
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/
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/
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/
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/
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/
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/
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/
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/
Latest try results looks good. Ready for the review.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=17db89ab4aee
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.
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
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.
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/
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/
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/
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/
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/
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/
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/
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+
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+
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.
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)
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
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)
See Also: → 1266316
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: