This bug is for implementing shape-outside: inset().

MozReview-Commit-ID: DmVOVArtzBZ

MozReview-Commit-ID: 3XbfW40AJCI

MozReview-Commit-ID: HaiDqNgE25P

MozReview-Commit-ID: Ag6V6XmlHIU

Make the interface consistent with the ConvertPhysicalToLogical() method added in the previous part. MozReview-Commit-ID: 1YARDzI3cyr

The radii can be computed when creating BoxShapeInfo. No need to compute them every time in the LineLeft() and LineRight(). MozReview-Commit-ID: GIDSLgickCT

The radii has been cached in the BoxShapeInfo in the previous part. Hence the rename. This class will be used to implement inset() in the next part, so the rect stored doesn't necessary be the rect of the <shape-box>. It could be the inset rectangle. Therefore I rename mShapeBoxRect to mRect to avoid any confusion. MozReview-Commit-ID: J0hpQDsbMyN

MozReview-Commit-ID: JZk1fo8SxgV

The reftests have passed layout/reftests/w3c-css/submitted/check-for-references.sh. MozReview-Commit-ID: JZk1fo8SxgV

David, the shape-outside: inset() implementation is ready for review. Would you please take a look at Part 1 through Part 8?

Comment on attachment 8835292 [details] [diff] [review] Part 1 - Extract a function to compute inset rect. (v1) >+/* static */ nsRect >+ShapeUtils::ComputeInsetRect(StyleBasicShape* const aBasicShape, >+ const nsRect& aRefBox) >+{ >+ const nsTArray<nsStyleCoord>& coords = aBasicShape->Coordinates(); >+ MOZ_ASSERT(coords.Length() == 4, "wrong number of arguments"); Perhaps also assert that the shape is the expected type? > struct ShapeUtils final > { > // Compute the length of a keyword <shape-radius>, i.e. closest-side or > // farthest-side, for a circle or an ellipse on a single dimension. The > // caller needs to call for both dimensions and combine the result. > // https://drafts.csswg.org/css-shapes/#typedef-shape-radius. >- // > // @return The length of the radius in app units. > static nscoord ComputeShapeRadius(const StyleShapeRadius aType, > const nscoord aCenter, > const nscoord aPosMin, > const nscoord aPosMax); > > // Compute the center of a circle or an ellipse. >- // > // @param aRefBox The reference box of the basic shape. > // @return The point of the center. I prefer the style with the blank line; it seems more readable to me. But I guess this is ok if you prefer without...

Comment on attachment 8835295 [details] [diff] [review] Part 4 - Extract a function to convert a rect to float manager's logical coordinate. >+ , mRect(ShapeInfo::ConvertToFloatLogical(aMarginRect, aWM, aContainerSize)) > { > MOZ_COUNT_CTOR(nsFloatManager::FloatInfo); > >+ mRect.MoveBy(aLineLeft, aBlockStart); Probably better to just put this all in the constructor, as: mRect(ShapeInfo::ConvertToFloatLogical(aMarginRect, aWM, aContainerSize) + nsPoint(aLineLeft, aBlockStart)) r=dbaron with that

Comment on attachment 8835296 [details] [diff] [review] Part 5 - Rewrite ConvertPhysicalToLogical(). It's not clear to me why you want the caller to construct the LogicalPoint, but maybe that will become clear in later patches. If not, maybe it's better to leave the construction of the LogicalPoint inside the function?

Comment on attachment 8835297 [details] [diff] [review] Part 6 - Cache the border radii in BoxShapeInfo. nsFloatManager.h: >+ BoxShapeInfo(const nsRect& aShapeBoxRect, >+ mozilla::UniquePtr<nscoord[]> aRadii) The implications of a UniquePtr<T> parameter aren't immediately clear to me, but is there a reason to prefer this over the more-common UniquePtr<T>&& ? I also wonder whether it would be worth creating a struct type containing an nscoord[8] array to make this clearer and perhaps safer. + // in the float manager's coordinate space. If there's no radii, it's "there's" -> "there are" nsFloatManager.cpp: >+ // This happens only if aWM is vertical-lr. We need to swap the top and >+ // bottom corners. I know you're only moving it, but maybe it's worth explaining this comment a little more: when IsLineInverted() is true, the relationship reverses between which corner comes first going clockwise, and which corner is block-start versus block-end. r=dbaron with that

Comment on attachment 8835298 [details] [diff] [review] Part 7 - Rename BoxShapeInfo to RoundedBoxShapeInfo. In the commit message: >stored doesn't necessary be the rect of the <shape-box>. It could be the "doesn't necessary be" -> "isn't necessarily" r=dbaron

Comment on attachment 8835293 [details] [diff] [review] Part 2 - Extract a function to compute inset radii. Actually, one further comment here: I think you should reverse the order of the aRefBox and aInsetRect parameters so that they match ComputeBorderRadii.

Comment on attachment 8835305 [details] [diff] [review] Part 8 - Implement shape-outside: inset(). (v2) >+ nsRect logicalInsetRect = >+ ConvertToFloatLogical(LogicalRect(aWM, insetRect, aContainerSize), >+ aWM, aContainerSize); Unless I missed something, this makes me think ConvertToFloatLogical is still better-off constructing the LogicalRect inside the function. r=dbaron

> Perhaps also assert that the shape is the expected type? Addedd assert in this patch. I'll also add some assertions to other ShapeUtils functions in Part 0. MozReview-Commit-ID: DmVOVArtzBZ

Address Bug 1326407 comment 11. MozReview-Commit-ID: 8Iy5q3f4yPd

Addressed comment 16: Reverse the order of the aRefBox and aInsetRect parameters so that they match ComputeBorderRadii. MozReview-Commit-ID: 3XbfW40AJCI

> mRect(ShapeInfo::ConvertToFloatLogical(aMarginRect, aWM, aContainerSize) + > nsPoint(aLineLeft, aBlockStart)) Addressed comment 12. MozReview-Commit-ID: Ag6V6XmlHIU

Re comment 13: > It's not clear to me why you want the caller to construct the LogicalPoint, > but maybe that will become clear in later patches. > > If not, maybe it's better to leave the construction of the LogicalPoint > inside the function? I was thinking about match the interface of ConvertPhysicalToLogical() added in Part 4, but it's also good to leaves construction of the LogicalPoint inside the function. As a result, this new patch becomes a simple rename and reorder of the arguments.

Addressed comment 14: correct the typo, and update comments. > The implications of a UniquePtr<T> parameter aren't immediately clear > to me, but is there a reason to prefer this over the more-common > UniquePtr<T>&& ? Yes, the two usage case have subtle differences. Using UniquePtr<T> in the function parameter means the caller wants to transfer the object's ownership to the callee, and callee *must* take the ownership anyway. On the other hand, if using UniquePtr<T>&&, the callee have the choice *not* to take the ownership, so the caller have to check whether the UniquePtr is empty afterwards. Here, we don't need the semantic of UniquePtr<T>&&. Using UniquePtr<T> is simpler. > I also wonder whether it would be worth creating a struct type containing > an nscoord[8] array to make this clearer and perhaps safer. This might worth doing. Filed bug 1339732.

Addressed comment 15.

Re comment 17: > Unless I missed something, this makes me think ConvertToFloatLogical > is still better-off constructing the LogicalRect inside the function. Actually, all the callers of ConvertToFloatLogical() except the one in Part 8 already have LogicalRect, not nsRect. That's why I choose the argument be LogicalRect.

David, thank you for the review. The lastest try results: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b83608fcdbcaf0a48353a343cea1b032fd3153f5

Pushed by tlin@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/ea8bf524e3c8 Part 0 - Add assertion to ShapeUtils functions. r=me https://hg.mozilla.org/integration/mozilla-inbound/rev/ecc715f72d61 Part 1 - Extract a function to compute inset rect. r=dbaron https://hg.mozilla.org/integration/mozilla-inbound/rev/2b9a5fad30b7 Part 2 - Extract a function to compute inset radii. r=dbaron https://hg.mozilla.org/integration/mozilla-inbound/rev/b0af1a8bdcaf Part 3 - Extract a function to compute <shape-box> rect. r=dbaron https://hg.mozilla.org/integration/mozilla-inbound/rev/366e6522665c Part 4 - Extract a function to convert a rect to float manager's logical coordinate. r=dbaron https://hg.mozilla.org/integration/mozilla-inbound/rev/f45cac0c1b91 Part 5 - Rename ConvertPhysicalToLogical(). r=dbaron https://hg.mozilla.org/integration/mozilla-inbound/rev/0058687f7813 Part 6 - Cache the border radii in BoxShapeInfo. r=dbaron https://hg.mozilla.org/integration/mozilla-inbound/rev/8a514a5851a4 Part 7 - Rename BoxShapeInfo to RoundedBoxShapeInfo. r=dbaron https://hg.mozilla.org/integration/mozilla-inbound/rev/16c77acfaa6e Part 8 - Implement shape-outside: inset(). r=dbaron

https://hg.mozilla.org/mozilla-central/rev/ea8bf524e3c8 https://hg.mozilla.org/mozilla-central/rev/ecc715f72d61 https://hg.mozilla.org/mozilla-central/rev/2b9a5fad30b7 https://hg.mozilla.org/mozilla-central/rev/b0af1a8bdcaf https://hg.mozilla.org/mozilla-central/rev/366e6522665c https://hg.mozilla.org/mozilla-central/rev/f45cac0c1b91 https://hg.mozilla.org/mozilla-central/rev/0058687f7813 https://hg.mozilla.org/mozilla-central/rev/8a514a5851a4 https://hg.mozilla.org/mozilla-central/rev/16c77acfaa6e

https://hg.mozilla.org/integration/mozilla-inbound/rev/6dfad3d738fed02f68223568f8ceb43df5a4e575 Rename (renumber) new mozilla-central-reftests shapes1 tests to avoid filename collisions with existing tests. Followup to bug 1311244, bug 1326406, and bug 1326407.

I've documented this, by added an entry and footnote to the browser support table at https://developer.mozilla.org/en-US/docs/Web/CSS/shape-outside I've not added a note to the release notes, as it is preffed off at the moment. I've checked that shape-outside is already covered in our Experimental features page: https://developer.mozilla.org/en-US/Firefox/Experimental_features Let me know if there's anything else you'd like me to do. Thanks!