The shape-margin property should be implemented as defined in https://drafts.csswg.org/css-shapes/#shape-margin-property. Sebastian

What's the status on this? It seems `shape-outside` has been implemented in Nightly (behind a flag), but `shape-margin` isn't working yet. Is that right? What's the plan here?

This will be my next thing to do after implementing bug 1404222.

Sorry, I'm not able to work on this bug as promised.

I'm still working on shape-margin implementation for polygon. I would appreciate some spec interpretation on what to do about margins applied to polygons that enclose no area. https://drafts.csswg.org/css-shapes/#funcdef-polygon > At least three vertices are required to define a polygon with an area. This means that (for this specification) polygons with less than three vertices (or with three or more vertices arranged to enclose no area) result in an empty float area. https://drafts.csswg.org/css-shapes/#empty-float-area > An empty float area (where the shape encloses no area) has no effect on line boxes. I'm assuming the correct thing to do in such a case is to inflate the degenerate polygon into a lozenge shape. Otherwise we could have bad effects with an animated polygon that had vertices moved into colinear positions -- the margin would suddenly disappear. dbaron, do you agree that this would be spec-compliant?

(In reply to Brad Werth [:bradwerth] from comment #15) > I'm assuming the correct thing to do in such a case is to inflate the > degenerate polygon into a lozenge shape. Otherwise we could have bad effects > with an animated polygon that had vertices moved into colinear positions -- > the margin would suddenly disappear. dbaron, do you agree that this would be > spec-compliant? I think you should raise this explicitly as a spec issue in https://github.com/w3c/csswg-drafts/issues and see what happens.

(In reply to David Baron :dbaron: ⌚️UTC-8 from comment #20) > I think you should raise this explicitly as a spec issue in > https://github.com/w3c/csswg-drafts/issues and see what happens. https://github.com/w3c/csswg-drafts/issues/2375

Comment on attachment 8953643 [details] Bug 1265342 Part 4: Complete the implementation of shape-margin for shape-outside: image, for shape-margin: > 0. https://reviewboard.mozilla.org/r/222878/#review232104 ::: layout/generic/nsFloatManager.cpp:1304 (Diff revision 3) > + // We've finished with the difference field. > + delete[] df; Drive-by nit: use UniquePtr to manage this memory for you, rather than "new"/"delete". (Then this'll be more robust against leaks or double-frees, in cases of early-returns, minor logic bugs, etc.) I believe you'd allocate it like so: auto df = MakeUnique<dfType[]>(dfSize); ...and then you don't need to bother deleting it at all -- you can just let it go out of scope. And the rest of your code ("df[index]") etc. should still Just Work, according to https://dxr.mozilla.org/mozilla-central/rev/a6a32fb286fa9e5d5f6d5b3b77423ab6b96c9502/mfbt/UniquePtr.h#151-156

Comment on attachment 8953643 [details] Bug 1265342 Part 4: Complete the implementation of shape-margin for shape-outside: image, for shape-margin: > 0. https://reviewboard.mozilla.org/r/222878/#review232104 > Drive-by nit: use UniquePtr to manage this memory for you, rather than "new"/"delete". (Then this'll be more robust against leaks or double-frees, in cases of early-returns, minor logic bugs, etc.) > > I believe you'd allocate it like so: > auto df = MakeUnique<dfType[]>(dfSize); > ...and then you don't need to bother deleting it at all -- you can just let it go out of scope. > > And the rest of your code ("df[index]") etc. should still Just Work, according to > https://dxr.mozilla.org/mozilla-central/rev/a6a32fb286fa9e5d5f6d5b3b77423ab6b96c9502/mfbt/UniquePtr.h#151-156 Good idea; thank you.

Bug 1451499 has been split out for the shape-margin implementation for polygon.

Comment on attachment 8959327 [details] Bug 1265342 Part 0: Servo changes to add shape-margin. https://reviewboard.mozilla.org/r/228166/#review239538 r=me with comments addressed. ::: servo/components/style/properties/longhand/box.mako.rs:654 (Diff revision 3) > + "NonNegativeLengthOrPercentage", > + "computed::NonNegativeLengthOrPercentage::zero()", > + products="gecko", > + gecko_pref="layout.css.shape-outside.enabled", > + animation_value_type="NonNegativeLengthOrPercentage", > + spec="https://drafts.csswg.org/css-shapes/#shape-margin", This url is wrong. It seems to be "https://drafts.csswg.org/css-shapes/#shape-margin-property". ::: servo/components/style/properties/longhand/box.mako.rs:655 (Diff revision 3) > + "computed::NonNegativeLengthOrPercentage::zero()", > + products="gecko", > + gecko_pref="layout.css.shape-outside.enabled", > + animation_value_type="NonNegativeLengthOrPercentage", > + spec="https://drafts.csswg.org/css-shapes/#shape-margin", > +)} `shape-outside` has the flag `APPLIES_TO_FIRST_LETTER`, maybe this (and probably `shape-image-threshold` as well) should also have this flag? `::first-letter` can be float, so I would assume they would work there.

Comment on attachment 8959328 [details] Bug 1265342 Part 1: Add shape-margin to style system (Gecko bindings). https://reviewboard.mozilla.org/r/228168/#review239540 ::: layout/style/nsStyleStruct.cpp:3820 (Diff revision 3) > > if (mShapeOutside != aNewData.mShapeOutside || > + mShapeMargin != aNewData.mShapeMargin || > mShapeImageThreshold != aNewData.mShapeImageThreshold) { > if (aNewData.mFloat != StyleFloat::None) { > - // If we are floating, and our shape-outside or shape-image-threshold > + // If we are floating, and our shape-outside or or shape-margin nit: duplicate "or"s here. Actually, since you are listing three properties, it should probably be > shape-outside, shape-margin, or shape-image-threshold

Comment on attachment 8953641 [details] Bug 1265342 Part 10: Change a bunch of web-platform tests to expected pass. https://reviewboard.mozilla.org/r/222874/#review240330

Comment on attachment 8959329 [details] Bug 1265342 Part 2b: Refactor interval creation for shape-outside:image. https://reviewboard.mozilla.org/r/228170/#review240334 ::: commit-message-675b8:1 (Diff revision 3) > +Bug 1265342 Part 2: Refactor interval creation in shape-outside:image. Optional nit: s/in/for/

Comment on attachment 8959330 [details] Bug 1265342 Part 3: Stub in shape-margin for shape-outside: image, by implementing only for shape-margin: 0. https://reviewboard.mozilla.org/r/228172/#review240342 ::: layout/generic/nsFloatManager.cpp:1249 (Diff revision 3) > +static bool > +IsPercentOfIndefiniteSize(const nsStyleCoord& aCoord, nscoord aPercentBasis) > +{ > + return aPercentBasis == NS_UNCONSTRAINEDSIZE && aCoord.HasPercent(); > +} > + > +static nscoord > +ResolveToDefiniteSize(const nsStyleCoord& aCoord, nscoord aPercentBasis) > +{ > + MOZ_ASSERT(aCoord.IsCoordPercentCalcUnit()); > + if (::IsPercentOfIndefiniteSize(aCoord, aPercentBasis)) { > + return nscoord(0); > + } Two concerns here: (1) IsPercentOfIndefiniteSize() & ResolveToDefiniteSize() seem to be cribbed from nsGridContainerFrame. It'd probably be better to move the existing versions to a central location (maybe nsLayoutUtils or nsStyleCoord), to share code. (2) Right now, this will resolve "calc(100px + 1%)" to 0. Is that really what you want for this case? (maybe hold off on addressing these concerns until we've got a response on bug 1279182 comment 14) ::: layout/generic/nsFloatManager.cpp:1299 (Diff revision 3) > + nscoord shapeMargin = > + ::ResolveToDefiniteSize(mFrame->StyleDisplay()->mShapeMargin, > + aContainerSize.width); Is aContainerSize.width correct here in a vertical writing mode? Or should we really be using its logical inline axis to be writing-mode-independent? i.e. this should maybe be something like "LogicalSize(aWM, aContainerSize).ISize(aWM)"? (an expression that I cribbed from nsFloatManager::GetFlowArea)

Comment on attachment 8959331 [details] Bug 1265342 Part 4a: Complete the implementation of shape-margin for shape-outside: image (handling shape-margin: > 0). https://reviewboard.mozilla.org/r/228174/#review240348 Here are review notes on my first pass through part 4 -- I haven't really reviewed the the math/code yet, but I'll take a closer at that soon. For now, just editorial nits: ::: commit-message-f8f7b:1 (Diff revision 3) > +Bug 1265342 Part 4: Complete the implementation of shape-margin for shape-outside: image, for shape-margin: > 0. nit: let's put parens around ", for shape-margin: > 0" - that makes this sentence parse a bit better. (The current phrasing -- "complete impl of X for Y, for Z" -- reads as if each "for" clause is an further narrowing the space of what you're implementing here. Which it kind of is, but it's really more meant as a clarification on what wasn't previously complete, IIUC.) Anyway -- maybe tweak to something like: ==== Complete the implementation of shape-margin for shape-outside: image (handling shape-margin: > 0) ::: layout/generic/nsFloatManager.cpp:1099 (Diff revision 3) > + // With a positive aShapeMargin, we have to calculate a distance > + // field from the opaque pixels, then build intervals based on > + // them being within aShapeMargin distance to an edge pixel. > + > + // Computing the difference field is a two-pass O(n) operation. You use two different terms here -- "distance field" in the first paragraph, vs. "difference field" in the second paragraph. I think the first one (distance field) is the correct one...? ::: layout/generic/nsFloatManager.cpp:1111 (Diff revision 3) > + // Our distance field has to be able to hold values equal to the > + // maximum allowed shape-margin value, times 5. A 16-bit unsigned > + // int is ~ 65K which can handle a margin up to ~ 13K. That's good s/is ~65K/can represent values up to ~ 65K/ ::: layout/generic/nsFloatManager.cpp:1114 (Diff revision 3) > + // pass which builds the intervals. > + > + // Our distance field has to be able to hold values equal to the > + // maximum allowed shape-margin value, times 5. A 16-bit unsigned > + // int is ~ 65K which can handle a margin up to ~ 13K. That's good > + // enough for practical usage. What does "good enough for practical usage" mean here? (It's not obvious what coordinate-space 13K is in here, or why it's a reasonable limit.) In particular, is this 13K device pixels, or 13K CSS pixels, or 13K app units? (If it's some form of pixel, then I can buy that that's a reasonable limit. If it's app units, then of course it's much smaller & perhaps too small of a limit...) Anyway, please clarify this comment to make the coordinate space clearer & some justification for why this is an OK practical limit. ::: layout/generic/nsFloatManager.cpp:1157 (Diff revision 3) > + auto df = MakeUnique<dfType[]>(dfSize); > + > + const int32_t bSize = aWM.IsVertical() ? wEx : hEx; > + const int32_t iSize = aWM.IsVertical() ? hEx : wEx; > + > + // First pass setting difference field, top-to-bottom, three cases: (As above, "difference field" here might be a typo here -- s/difference/distance/?) ::: layout/generic/nsFloatManager.cpp:1280 (Diff revision 3) > + MOZ_ASSERT(iMin); > + iMin = i; Looks like we're asserting that iMin is nonzero, but it's not obvious to me why. (Also, we're about to assign iMin to something else, so it's not clear to me why we care a lot its old value, other than maybe as an internal consistency thing.) Anyway -- assuming that the assertion is useful, please include an assertion message here, to give a hint at why we care about this condition and/or why we're justified in asserting it.

Comment on attachment 8959332 [details] Bug 1265342 Part 5a: Implement shape-margin for shape-outside: circle and ellipse (ellipse only for shape-margin: 0). https://reviewboard.mozilla.org/r/228176/#review240358 ::: layout/generic/nsFloatManager.cpp:1795 (Diff revision 3) > - radii = nsSize(logicalRadii.ISize(aWM), logicalRadii.BSize(aWM)); > + radii = nsSize(logicalRadii.ISize(aWM) + aShapeMargin, > + logicalRadii.BSize(aWM) + aShapeMargin); It looks like you're assuming that this: ellipse(rX,rY) with a shape-margin ...creates float area that is functionally equivalent to this: ellipse(rX + shape-margin, rY + shape-margin) Is that really a valid assumption? It seems like it might be roughly true, but it's not obvious to me that it's necessarily the case for an ellipse in general. It makes obvious sense at rightmost tip of the ellipse (right where the ellipse is 2* rX wide). But consider e.g. an arbitrary point on the upper-right "\" curve of the ellipse. shape-margin:10px says we should define our float as being exactly 10px to the right of the point on the curve. Would an ellipse with 10px-larger-radii really produce a curve that's got the correct measurement at that y-value? (with a curve intercepting that y-value at an x position precisely 10px to the right of the original ellipse's point for that y-value) I don't know for sure that this assumption is invalid, but I think it should probably be clearly stated & justified somewhere. :) whether that's in a code comment, in the commit message, or in the bug.

Comment on attachment 8959333 [details] Bug 1265342 Part 2a: Move interval binary search method into ShapeInfo. https://reviewboard.mozilla.org/r/228178/#review240360 ::: layout/generic/nsFloatManager.cpp:1763 (Diff revision 3) > return MakeUnique<RoundedBoxShapeInfo>(logicalInsetRect, > UniquePtr<nscoord[]>()); > } > > + // Add aShapeMargin to each of the radii. > + for (int32_t i = 0; i < 0; ++i) { This looks like a typo... "i = 0; i < 0" will immediately fail. :) I think you mean i < ArrayLength(physicalRadii) (which is 8, but better to let the compiler statically figure that out for you rather than hardcoding magic number 8 multiple times))). Please add a testcase for "inset" if we don't already have one... (I'm guessing we might not have any, if this bug didn't trigger any failures)

Comment on attachment 8959334 [details] Bug 1265342 Part 7: Implement shape-margin for shape-outside: shape-box. https://reviewboard.mozilla.org/r/228180/#review240364 ::: layout/generic/nsFloatManager.cpp:1713 (Diff revision 3) > return MakeUnique<RoundedBoxShapeInfo>(logicalShapeBoxRect, > UniquePtr<nscoord[]>()); > } > > + // Add aShapeMargin to each of the radii. > + for (int32_t i = 0; i < 8; ++i) { s/8/ArrayLength(physicalRadii)/ , assuming that works (I'm pretty sure it should). Note you have to #include "mozilla/ArrayUtils.h" to provide the mozilla::ArrayLength function: https://dxr.mozilla.org/mozilla-central/rev/62c2508f27a865dd0ac5ca3f0485cb20afafbbd0/mfbt/ArrayUtils.h#48-56

Comment on attachment 8959335 [details] Bug 1265342 Part 8: Update shape-outside inset tests to explicitly set a line-height of 1, and not rely on a UA-specific value. https://reviewboard.mozilla.org/r/228182/#review240366

Comment on attachment 8959332 [details] Bug 1265342 Part 5a: Implement shape-margin for shape-outside: circle and ellipse (ellipse only for shape-margin: 0). https://reviewboard.mozilla.org/r/228176/#review240358 > It looks like you're assuming that this: > ellipse(rX,rY) with a shape-margin > ...creates float area that is functionally equivalent to this: > ellipse(rX + shape-margin, > rY + shape-margin) > > Is that really a valid assumption? It seems like it might be roughly true, but it's not obvious to me that it's necessarily the case for an ellipse in general. > > It makes obvious sense at rightmost tip of the ellipse (right where the ellipse is 2* rX wide). But consider e.g. an arbitrary point on the upper-right "\" curve of the ellipse. shape-margin:10px says we should define our float as being exactly 10px to the right of the point on the curve. Would an ellipse with 10px-larger-radii really produce a curve that's got the correct measurement at that y-value? (with a curve intercepting that y-value at an x position precisely 10px to the right of the original ellipse's point for that y-value) > > > I don't know for sure that this assumption is invalid, but I think it should probably be clearly stated & justified somewhere. :) whether that's in a code comment, in the commit message, or in the bug. Phrasing this another way -- it looks like the spec text says we're supposed to include "all the points that are the shape-margin distance outward in the perpendicular direction from a point on the underlying shape." https://drafts.csswg.org/css-shapes-1/#shape-margin-property And "ellipse(rX + shape-margin, rY + shape-margin)" does *not* seem to produce the correct larger shape for this, in general, by my visual judgement. Here's an example with a tall-and-skinny ellipse, centered inside of an ellipse with 40px larger radii: https://jsfiddle.net/fphn5k16/ You can try measuring the (approximate) perpendicular distance at various points using the devtools "Measure a Portion of the Page" tool (which has to be enabled in the gear-icon section, under "Available Toolbox Buttons"). At the "equator" of the ellipse, you can see that the green border is indeed 40px wide. But close to the top, where the perpendicular juts out at an angle, you can see visually (and verify with devtools measurement tool) that the larger ellipse's edge is only ~27px away from the smaller ellipse. Here's what I'm seeing https://i.imgur.com/g2I75bP.png (I probably don't have the normal drawn exactly right, but I can't imagine that the correct normal has 13px *more distance* between the two ellipses than what I've drawn here...) Basically, it looks to me like there is less perpendicular distance between the original vs. inflated ellipse at its "corners" vs. at its actual extremes. I think the correct inflated shape is a bit different from ellipse(rX+shape-margin, rY+shape-margin), and indeed might not really even be representable mathematically as an ellipse...

(In reply to Daniel Holbert [:dholbert] (away 4/24 - 5/11) from comment #97) > Basically, it looks to me like there is less perpendicular distance between > the original vs. inflated ellipse at its "corners" vs. at its actual > extremes. I think the correct inflated shape is a bit different from > ellipse(rX+shape-margin, rY+shape-margin), and indeed might not really even > be representable mathematically as an ellipse... Yes, good catch. The best counter-example would be a long, then ellipse that's basically a line. The correct fixed-width margin around it would create a lozenge/capsule shape. Merely extending the ellipse axes would make a rounded diamond shape. I'll have to rework the math on this.

(In reply to Daniel Holbert [:dholbert] (away 4/24 - 5/11) from comment #91) > Comment on attachment 8959330 [details] > Two concerns here: > (1) IsPercentOfIndefiniteSize() & ResolveToDefiniteSize() seem to be > cribbed from nsGridContainerFrame. It'd probably be better to move the > existing versions to a central location (maybe nsLayoutUtils or > nsStyleCoord), to share code. Thank you for explaining this. Looks like Bug 1434478 will provide a solution, which I'll use here when it's available.

Comment on attachment 8959336 [details] Bug 1265342 Part 9: Update shape-outside ellipse tests to correct invalid use of only one radius parameter. https://reviewboard.mozilla.org/r/228184/#review240810 ::: commit-message-941d8:6 (Diff revision 4) > +ellipse() = ellipse( [<shape-radius>{2}]? [at <position>]? ) > + > +This permits either 0 or 2 shape-radius tokens, but not 1. These two tests > +use 1 shape-radius token and do not parse correctly. Chrome and Safari seem to allow for 1 shape-radius values, e.g. with this testcase: https://jsfiddle.net/pLs3cunm/ Could you file bugs on them[1][2]? (or be sure they've got a bug open on this) [1] https://code.google.com/p/chromium/issues/ [2] https://bugs.webkit.org/ Otherwise this will likely lead to interop issues.

Comment on attachment 8959333 [details] Bug 1265342 Part 2a: Move interval binary search method into ShapeInfo. https://reviewboard.mozilla.org/r/228178/#review241640 D'oh, looks like MozReview got a little confused about mapping old-patches to new-patches here... In reality this is a brand-new patch, but MozReview thinks there are 3 prior revisions of this patch -- the and the previous "revision" has commit message for "Bug 1265342 Part 6". Hopefully not a big deal -- I'm guessing that later patches may have the same issue and interdiffs might be a little broken as a result, but we'll manage. ::: layout/generic/nsFloatManager.cpp:625 (Diff revision 4) > + // on increasing y values. This function uses a binary search to find the > + // first interval that contains aTargetY. Probably worth stating at the end here: "If no such interval exists, this function returns aIntervals.Length()" Otherwise this edge-case behavior is left kind of undefined/ambiguous. (We do mention this in the function's implementation, but it's worth hinting at in its main documentation too.)

Comment on attachment 8959331 [details] Bug 1265342 Part 4a: Complete the implementation of shape-margin for shape-outside: image (handling shape-margin: > 0). https://reviewboard.mozilla.org/r/228174/#review241646 Overall this is really nice! The chamfer technique seems like a cool trick. This is basically r+ with nits, but I'd like to skim the interdiff of the final changes, so marking r- for now. ::: layout/generic/nsFloatManager.cpp:1107 (Diff revision 4) > + // With a positive aShapeMargin, we have to calculate a distance > + // field from the opaque pixels, then build intervals based on > + // them being within aShapeMargin distance to an edge pixel. It would help to motivate the rest of this code (and orient the reader around what the chamfer 5-7-11 process is for) if you added something like this here: "Roughly: for each pixel in the margin box, we need to determine the distance to the nearest opaque image-pixel. If that distance is less than aShapeMargin, we consider this margin-box pixel as being part of the float area." ::: layout/generic/nsFloatManager.cpp:1119 (Diff revision 4) > + // Our distance field has to be able to hold values equal to the > + // maximum allowed shape-margin value, times 5. A 16-bit unsigned This sentence feels a little bit backwards / incorrect -- because from the CSS parser's perspective, the "maximum allowed shape-margin value" is actually *much larger* than what our distance field can represent. Maybe rephrase: "maximum allowed shape-margin value" --> "maximum shape-margin value that we care about faithfully rendering" or something like that? ::: layout/generic/nsFloatManager.cpp:1121 (Diff revision 4) > + // int can represent up to ~ 65K which means we can handle a margin > + // up to ~ 13K device pixels. That's good enough for practical usage. Please add a final note to this comment, to say that any larger shape-margin lengths will be clamped to 13K device pixels. (Right now that edge-case behavior is left unspecified in this comment, and I initially wasn't sure what the fallback would be & whether it'd be coherent vs. random. But after reading a bit further, I see that we do clamp via std::min, which is nice & worth hinting at.) ::: layout/generic/nsFloatManager.cpp:1124 (Diff revision 4) > + const dfType MAX_MARGIN = 13000; > + const dfType MAX_MARGIN_5X = MAX_MARGIN * 5; MAX_MARGIN should probably be defined as: std::numeric_limits<dfType>::max()/5 The hardcoded number 13000 feels a bit too arbitrary/magic. That will also make it clearer that MAX_MARGIN_5X is definitely representable in our dfType. (Note that it won't be *exactly* the max value, due to flooring in the /5 calculation; and that's fine.) ::: layout/generic/nsFloatManager.cpp:1127 (Diff revision 4) > + // Calculate aShapeMargin upper bounded by MAX_MARGIN and > + // multiplied by 5 in a typesafe fashion. > + float shapeMarginDevPixels = > + NSAppUnitsToFloatPixels(aShapeMargin, aAppUnitsPerDevPixel); This phrasing ("Calculate aShapeMargin upper bounded by MAX_MARGIN") is a bit confusing. How about: // Convert aShapeMargin to dev pixels, bounded by MAX_MARGIN // (and then convert that result to 5x-dev-pixel space) ::: layout/generic/nsFloatManager.cpp:1129 (Diff revision 4) > + float shapeMarginDevPixels = > + NSAppUnitsToFloatPixels(aShapeMargin, aAppUnitsPerDevPixel); > + NS_WARNING_ASSERTION(shapeMarginDevPixels <= MAX_MARGIN, > + "shape-margin is too large and is being clamped."); > + dfType usedMargin5X = 5 * (dfType)std::min((int32_t)MAX_MARGIN, > + NSToIntRound(shapeMarginDevPixels)); There are a few issues with the math here. (1) The comparison in the warning is subtly different from the actual std::min clamping in the code. (The former is in float space, the latter is in rounded-int space.) (2) This code is rounding the shape-margin to the nearest dev pixel (quantizing to whole dev pixels), but then scaling up by 5x -- so it's needlessly throwing away precision. It could just as easily do the rounding *after* scaling up by 5x, and then be able to differentiate between e.g. 1.2 dev pixels vs. 1.4 dev pixels. (3) The last line here is too long (but it might be rewritten a bit anyway to address (2)). So: - Please make sure the warning and the code are doing an equivalent comparison (so that we do actually warn in precisely the cases where we clamp). - Please do the rounding *after* scaling up, to avoid throwing away precision for no good reason. ::: layout/generic/nsFloatManager.cpp:1148 (Diff revision 4) > + LayoutDevicePixel::FromAppUnitsRounded(offsetPoint, > + aAppUnitsPerDevPixel); > + > + // Since our distance field is computed with a 5x5 neighborhood, > + // we need to expand our distance field by a further 4 pixels in > + // both axes. We call this border area the "expanded region". s/border area/broader area/, perhaps? Or "fringe area" if you want to emphasize the outside-ness? (Better to avoid the term "border", since that's a term for an unrelated CSS concept that's very closely tied to margin/content-boxes etc. that we're working with here.) Also, why are we only expanding by 4 (2px per side), rather than by 8 (4px per side)? ::: layout/generic/nsFloatManager.cpp:1154 (Diff revision 4) > + const LayoutDeviceIntSize margin = > + LayoutDevicePixel::FromAppUnitsRounded(aMarginRect.Size(), > + aAppUnitsPerDevPixel); "margin" is a bit of a confusing name here, since we have "margin"-flavored types (nsMargin, BaseMargin, etc), and this is not one of those types. Maybe call this "marginRectSizeDev" or something like that? ::: layout/generic/nsFloatManager.cpp:1159 (Diff revision 4) > + const int32_t dfSize = wEx * hEx; > + auto df = MakeUnique<dfType[]>(dfSize); The variable "dfSize" is only used once; maybe just collapse it into the MakeUnique expression, to reduce the number of "size" variables in play here (dfSize/bSize/iSize/wEx/hEx), and to make it clearer up-front that df is a 2D array. e.g. consider just doing: auto df = MakeUnique<dfType[]>(wEx * hEx); ::: layout/generic/nsFloatManager.cpp:1164 (Diff revision 4) > + > + // First pass setting distance field, top-to-bottom, three cases: > + // 1) Expanded region pixel: set to MAX_MARGIN_5X. > + // 2) Image pixel with alpha greater than threshold: set to 0. > + // 3) Other pixel: set to minimum backward-looking neighborhood > + // distance value, computed with 5-7-11 chamfer. > + > + // Scan the pixels in a double loop. For horizontal writing modes, we do > + // this row by row, from top to bottom. For vertical writing modes, we do > + // column by column, from left to right. We define the two loops There's a semi-contradiction here: the first paragraph here declares "top-to-bottom" but the second paragraph says we might be walking top to bottom, or might be walking left to right. Maybe clearer: // First pass setting distance field, starting at top-left, three cases: ::: layout/generic/nsFloatManager.cpp:1188 (Diff revision 4) > + } else if (col >= (dfOffset.x + 2) && > + col < (dfOffset.x + 2 + w) && > + row >= (dfOffset.y + 2) && > + row < (dfOffset.y + 2 + h) && > + aAlphaPixels[col - (dfOffset.x + 2) + > + (row - (dfOffset.y + 2)) * aStride] > threshold) { Could you just set up dfOffset so that it includes the "+2" in both axes up-front? i.e. it should *truly* be an offset into the full "df" matrix, rather than being an offset from the 2,2 point of the "df" matrix. That will make it easier to reason about, and it'll make this logic easier to follow -- particularly the aAlphaPixels indexing. Also: I confess I had no idea what "w" and" h" are here. Scrolling way far up, I see they're image width and height, but they're easy to confuse with all the other similarly-named variables. Could you perhaps add a followup patch to give them clearer names? (e.g. imageDevW/imageDevH) ::: layout/generic/nsFloatManager.cpp:1226 (Diff revision 4) > + // Okay, time for the second pass. This pass is bottom-to-top. > + // All of our opaque pixels have been set to 0, and all of our Similar to above: the term "bottom-to-top" is kind of confusing here, since we don't actually know whether we'll be iterating in row-major or column-major way. Maybe replace that sentence here with: "This pass starts at the bottom-right and iterates in reverse." ::: layout/generic/nsFloatManager.cpp:1234 (Diff revision 4) > + // This time, we constrain our outer and inner loop to ignore > + // the expanded region pixels. For each pixel we iterate, we > + // set the df value to the minimum backward-looking neighborhood > + // distance value, computed with 5-7-11 chamfer. We also check s/backward-looking/forward-looking/, I think? (for this second loop) (The first loop already did the backward-looking examination; now we're doing the forward-looking examination.) ::: layout/generic/nsFloatManager.cpp:1245 (Diff revision 4) > + // Set iMin and iMax in preparation for this row or column. > + int32_t iMin = iSize; > + int32_t iMax = -1; Might help to say what these are used for, to make their initial extremes clearer. (Their first use is pretty far away.) Maybe add to the comment: "(to track first/last pixel we find whose df[] value is less than usedMargin5X)" ::: layout/generic/nsFloatManager.cpp:1282 (Diff revision 4) > + // Finally, we can check the df value and see if its less than > + // or equal to the usedMargin5X value. s/its/it's/ ::: layout/generic/nsFloatManager.cpp:1294 (Diff revision 4) > + // Our col and row are calculated from the margin rect. > + // We likewise need to subtract 2 from iMin, iMax, and b > + // because those indices are relative to the expanded > + // pixel area. > + CreateInterval(iMin - 2, iMax - 2, b - 2, aAppUnitsPerDevPixel, > + aMarginRect.TopLeft(), aWM, aContainerSize); RE "Our col and row are calculated from the margin rect", two nits: (1) "calculated from" is too vague. Perhaps "relative to" or "offsets from" would be clearer? (2) I didn't initially see where the margin-box was accounted for here (we don't subtract it in the way that we subtract "2"). Maybe just clarify this by adding at the end of that first sentence: "(hence we pass in aMarginRect.TopLeft()" ...to anchor this statement to the relevant code a bit more clearly.

Comment on attachment 8959331 [details] Bug 1265342 Part 4a: Complete the implementation of shape-margin for shape-outside: image (handling shape-margin: > 0). https://reviewboard.mozilla.org/r/228174/#review241646 > s/border area/broader area/, perhaps? Or "fringe area" if you want to emphasize the outside-ness? > > (Better to avoid the term "border", since that's a term for an unrelated CSS concept that's very closely tied to margin/content-boxes etc. that we're working with here.) > > Also, why are we only expanding by 4 (2px per side), rather than by 8 (4px per side)? Sorry, disregard my "why are we only expanding by 4" question here; I figured that out after reading further, and forgot to update this nit. (To enhance grokkability, might be worth adding "(2px per side)" at the end of this sentence, to make it clearer what the extra 4px represent, and to help set the stage for all the "+2" / "-2" calculations that are coming up in subsequent code.)

Comment on attachment 8959331 [details] Bug 1265342 Part 4a: Complete the implementation of shape-margin for shape-outside: image (handling shape-margin: > 0). https://reviewboard.mozilla.org/r/228174/#review242006 Sorry, one more nit I just noticed (applies both to the "df" allocation for both the image and ellipse patch): ::: layout/generic/nsFloatManager.cpp:1160 (Diff revision 4) > + LayoutDevicePixel::FromAppUnitsRounded(aMarginRect.Size(), > + aAppUnitsPerDevPixel); > + const int32_t wEx = margin.width + 4; > + const int32_t hEx = margin.height + 4; > + const int32_t dfSize = wEx * hEx; > + auto df = MakeUnique<dfType[]>(dfSize); I think we need to make this a fallible allocation. Which I think means you need to use nsAutoPtr and "new" via https://dxr.mozilla.org/mozilla-central/source/memory/fallible/fallible.h , rather than UniquePtr/MakeUnique (which IIUC does not support fallible allocation). I think the rule of thumb for fallible allocation is: - is the size of the allocation controlled by an author? - can they make that allocation-size be arbitrarily large without needing to scale up the amount of data they're sending us? If the answers are yes/yes, then we should be using fallible allocation, to avoid introducing avenues for trivial DOS attacks. And here, I think the answers are indeed yes/yes -- I'm assuming authors could use CSS and/or a large blank image to produce a large "aMarginRect" here, which would in turn produce huge wEx & hEx values, which would in turn produce huge df allocation. If allocation fails, I think you'd be fine to just return immediately without creating any intervals, or something like that. Broken-ish layout is a fine graceful fallback - it's preferred to OOM-crashing. :)

Comment on attachment 8959331 [details] Bug 1265342 Part 4a: Complete the implementation of shape-margin for shape-outside: image (handling shape-margin: > 0). https://reviewboard.mozilla.org/r/228174/#review242006 > I think we need to make this a fallible allocation. Which I think means you need to use nsAutoPtr and "new" via https://dxr.mozilla.org/mozilla-central/source/memory/fallible/fallible.h , rather than UniquePtr/MakeUnique (which IIUC does not support fallible allocation). > > I think the rule of thumb for fallible allocation is: > - is the size of the allocation controlled by an author? > - can they make that allocation-size be arbitrarily large without needing to scale up the amount of data they're sending us? > > If the answers are yes/yes, then we should be using fallible allocation, to avoid introducing avenues for trivial DOS attacks. And here, I think the answers are indeed yes/yes -- I'm assuming authors could use CSS and/or a large blank image to produce a large "aMarginRect" here, which would in turn produce huge wEx & hEx values, which would in turn produce huge df allocation. > > If allocation fails, I think you'd be fine to just return immediately without creating any intervals, or something like that. Broken-ish layout is a fine graceful fallback - it's preferred to OOM-crashing. :) There's MakeUniqueFallible fwiw.

Comment on attachment 8967199 [details] Bug 1265342 Part 5b: Complete the implementation of shape-margin for ellipse (handling shape-margin: > 0). https://reviewboard.mozilla.org/r/235860/#review241996 Partial review on the ellipse patch. Note that many/most of my comments for Part 4 apply to this patch as well (since there's a lot of similar code/comments). Please be sure to address those comments here as applicable -- I didn't bother restating them on this patch. ::: layout/generic/nsFloatManager.cpp:826 (Diff revision 1) > ShapeMarginIsNegligible(aShapeMargin)), > "This constructor should not be called when margin is negligible or " > "radii are roughly equal."); > > - MOZ_ASSERT_UNREACHABLE("shape-margin > 0 not yet implemented for ellipse."); > + // We have to calculate a distance field from the ellipse edge, then build > + // intervals based on pixels with less than aShapeMargin distance to an edge pixel. This line (and a number of comment lines below this) is longer than 80 characters long. Please rewrap to 80 columns. ::: layout/generic/nsFloatManager.cpp:862 (Diff revision 1) > + // Since our distance field is computed with a 5x5 neighborhood, but only looks > + // in the negative block and negative inline directions, it is effectively a 3x3 > + // neighborhood. We need to expand our distance field by a further 2 pixels in both > + // axes. We call this border area the "expanded region". > + nsSize radiiPlusShapeMargin(mRadii.width + aShapeMargin, > + mRadii.height + aShapeMargin); > + const LayoutDeviceIntSize bounds = > + LayoutDevicePixel::FromAppUnitsRounded(radiiPlusShapeMargin, > + aAppUnitsPerDevPixel); > + static const int32_t iExpand = 2; > + static const int32_t bExpand = 2; This comment should probably move down to be above iExpand/bExpand, rather than being above the unrelated radiiPlusShapeMargin declaration. ::: layout/generic/nsFloatManager.cpp:890 (Diff revision 1) > + // Find the i intercept of the ellipse edge for this block row, and adjust it > + // to compensate for the expansion of the inline dimension. If we're in the > + // expanded region, or if we're using a b that's less than the bStart of the > + // ellipse, the intercept is 0 before adjustment. > + bool bIsInExpandedRegion(b >= bSize - bExpand); > + bool bInAppUnits = b * aAppUnitsPerDevPixel; It looks like bInAppUnits wants to have type "nscoord", right? But right now it's bool. That means all the subsequent math/comparisons about "bInAppUnits" etc. must be doing the wrong thing much of the time.... Please be sure we've got some automated testcases that exercise this code sufficiently so that they would be able to catch this sort of bug. ::: layout/generic/nsFloatManager.cpp:891 (Diff revision 1) > + // to compensate for the expansion of the inline dimension. If we're in the > + // expanded region, or if we're using a b that's less than the bStart of the > + // ellipse, the intercept is 0 before adjustment. > + bool bIsInExpandedRegion(b >= bSize - bExpand); > + bool bInAppUnits = b * aAppUnitsPerDevPixel; > + bool bIsLessThanEllipseBStart(bInAppUnits < bounds.height - mRadii.height); This arithmetic/comparison seems bogus, because: - bInAppUnits seems is in nscoord units (or will be, after it's no longer a bool :)) - "bounds" is in dev pixels - mRadii.height is in nscoord units. ::: layout/generic/nsFloatManager.cpp:892 (Diff revision 1) > + const int32_t iIntercept = iExpand + > + bIsInExpandedRegion || bIsLessThanEllipseBStart ? 0 : > + XInterceptAtY(radiiPlusShapeMargin.height - bInAppUnits, > + mRadii.width, > + mRadii.height); Please add a layer of parens around everything after the "+" here. Otherwise, this superficially looks like it's doing "iExpand + bIsInExpandedRegion", which would be <int> + <bool> which is bogus. (Really it's not doing that, because the || has higher precedence I think. But that's non-obvious & we might as well make it unambiguous.) ::: layout/generic/nsFloatManager.cpp:926 (Diff revision 1) > + // X should be set to the minimum of its current value and > + // the values of all of the numbered neighbors summed with > + // the value in that chamfer cell. > + df[index] = std::min<dfType>(df[index], "its current value" here is actually uninitialized, I think! (i.e. I don't think any particular value has been stored in df[index] yet.) In the <image> version of this code in Part 4, we use MAX_MARGIN_5X here (in the first time through the matrix). Perhaps that's what we should use here, too? ::: layout/generic/nsFloatManager.cpp:936 (Diff revision 1) > + std::min<dfType>(df[index + iSize] + 5, > + std::min<dfType>(df[index + iSize - 1] + 7, > + std::min<dfType>(df[index + iSize - 2] + 11, > + df[index + (iSize * 2) - 1] + 11))))); > + > + // Check the df value and see if its less than or equal to the s/its/it's/ ::: layout/generic/nsFloatManager.cpp:953 (Diff revision 1) > + // Origin is at the minimum value in both axes (which is the top center > + // for horizontal-tb writing mode). > + nsPoint origin(aCenter.x, aCenter.y - radiiPlusShapeMargin.height + > + bInAppUnits); (1) Logic nit: I don't understand the y-component of 'origin' here. "Top center" sounds like it'd be a constant point, which I'd imagine would be at "aCenter.y - mRadii.height". I don't understand why shape-margin comes into play in determining this point, nor do I understand why bInAppUnits would come into play. (Having said that, bInAppUnits has the wrong type in this patch version -- it's a bool -- so it's negligible right now.) (2) Stylistic nit: if linewrapping is required here, let's just put each arg on its own line, so that "bInAppUnits" (or whatever is wrapped) doesn't end up with a kinda arbitrary indentation. something like this: nsPoint origin(aCenter.x, aCenter.y - radiiPlusShapeMargin.height + bInAppUnits); ...though the actual args might end up changing, depending on your thoughts on (1) Logic Nit. ::: layout/generic/nsFloatManager.cpp:969 (Diff revision 1) > nscoord > -nsFloatManager::EllipseShapeInfo::LineLeft(const nscoord aBStart, > - const nscoord aBEnd) const > +nsFloatManager::EllipseShapeInfo::LineEdge(const nscoord aBStart, > + const nscoord aBEnd, > + bool aLeft) const > { s/aLeft/aIsLineLeft/ to disambiguate. ("left" and "line-left" are subtly different.) Also, it might be nice to spin this "LineEdge" helper-function-refactoring off as a (relatively small) helper patch -- and then this bigger patch here would just replace the MOZ_ASSERT_UNREACHABLE in that already-existing helper-function. (That helper patch could be on another bug even, if you like, because IIUC it's non-functional and independent of the rest of this bug -- it just needs to land before this part.) Up to you, though -- it's OK like this, too, I suppose. ::: layout/generic/nsFloatManager.cpp:985 (Diff revision 1) > - return 0; > + // We are checking against our intervals. Make sure we have some. > + MOZ_ASSERT(!mIntervals.IsEmpty(), > + "With mShapeMargin > 0, we should have generated intervals."); You should soften this assertion -- because, once you start using fallible allocation for the df[] matrix, it'll become possible for us to get here without ever having populated mIntervals. And actually, you need to add an early-return here to handle that scenario. (It looks like the code below assumes mIntervals is non-empty.) So this should perhaps be: if (mIntervals.IsEmpty()) { NS_WARNING(...); return 0; } (I'm assuming 0 is a reasonable fallback value to return in that scenario) ::: layout/generic/nsFloatManager.cpp:1739 (Diff revision 1) > - // Per spec, a float area defined by a shape is clipped to the float’s > + // Per spec, a float area defined by a shape is clipped to the floats > // margin box. Therefore, no need to create a shape info if the float's This patch is incorrectly removing an apostrophe on the first line here - please revert this chagne. s/floats/float's/ (Looks like it was a fancy UTF apostrophe (’) in the old version of this file, before this patch. Maybe your editor or VCS stripped it out for that reason? Anyway, maybe we should swap in a plain single-quote, but that's wholly unrelated to the rest of this patch so it shouldn't happen here.) ::: layout/generic/nsFloatManager.cpp:1840 (Diff revision 1) > // Clip the flow area to the margin-box because > // https://drafts.csswg.org/css-shapes-1/#relation-to-box-model-and-float-behavior > // says "When a shape is used to define a float area, the shape is clipped > - // to the float’s margin box." > + // to the floats margin box." Here as well, this patch is removing an an apostrophe. Please revert this change.

(In reply to Daniel Holbert [:dholbert] (away 4/24 - 5/11) from comment #115) > Comment on attachment 8967199 [details] > Bug 1265342 Part 5b: Complete the implementation of shape-margin for ellipse > (handling shape-margin: > 0). > > https://reviewboard.mozilla.org/r/235860/#review241996 Thanks for this. I hadn't marked this part for review yet because I know it's not yet working. I uploaded it now because I had a very scary local repository corruption where I lost this code and I had to manually recover it from a bzipped mercurial histedit diff. This is what caused the apostrophes to disappear -- I had to zap a lot of non-ASCII characters. Anyway, I appreciate the early review and I'll incorporate your notes (as well as the relevant issues on the similar part 4) into the version I mark up as being ready for review.

Comment on attachment 8959331 [details] Bug 1265342 Part 4a: Complete the implementation of shape-margin for shape-outside: image (handling shape-margin: > 0). https://reviewboard.mozilla.org/r/228174/#review240670 ::: layout/generic/nsFloatManager.cpp:1114 (Diff revision 3) > + // pass which builds the intervals. > + > + // Our distance field has to be able to hold values equal to the > + // maximum allowed shape-margin value, times 5. A 16-bit unsigned > + // int is ~ 65K which can handle a margin up to ~ 13K. That's good > + // enough for practical usage. It's device pixels. I've made the comment more clear. ::: layout/generic/nsFloatManager.cpp:1148 (Diff revision 4) > + LayoutDevicePixel::FromAppUnitsRounded(offsetPoint, > + aAppUnitsPerDevPixel); > + > + // Since our distance field is computed with a 5x5 neighborhood, > + // we need to expand our distance field by a further 4 pixels in > + // both axes. We call this border area the "expanded region". I decided to use the term "edge area" since I changed the comment on adding pixels to refer to to "leading edge" and "trailing edge".

Comment on attachment 8967199 [details] Bug 1265342 Part 5b: Complete the implementation of shape-margin for ellipse (handling shape-margin: > 0). https://reviewboard.mozilla.org/r/235860/#review242286 ::: layout/generic/nsFloatManager.cpp:926 (Diff revision 1) > + // X should be set to the minimum of its current value and > + // the values of all of the numbered neighbors summed with > + // the value in that chamfer cell. > + df[index] = std::min<dfType>(df[index], Ah, yes. We shouldn't consider the current value at all in this case. ::: layout/generic/nsFloatManager.cpp:953 (Diff revision 1) > + // Origin is at the minimum value in both axes (which is the top center > + // for horizontal-tb writing mode). > + nsPoint origin(aCenter.x, aCenter.y - radiiPlusShapeMargin.height + > + bInAppUnits); I updated the comment to make clear that this is the origin _for this interval_, which explains why it is adjusted by bInAppUnits.

Ugh, a mishap with my local repository has caused MozReview to disconnect the existing reviews on parts 6+ with my local patches. I've re-imported from MozReview and the MozReview-Commit-ID values match, but it's treating my copy as an additional, unreviewed patch. I'm giving up for now., but I'll try again to clean everything up when the patch is ready for landing.

Comment on attachment 8959332 [details] Bug 1265342 Part 5a: Implement shape-margin for shape-outside: circle and ellipse (ellipse only for shape-margin: 0). https://reviewboard.mozilla.org/r/228176/#review242346 ::: layout/generic/nsFloatManager.cpp:717 (Diff revision 5) > + // Construct the float area using math to calculate the shape boundary. This > + // is the fast path and should be used when shape-margin is negligible, or when > + // the two values of aRadii are roughly equal. Those two conditions are defined > + // by ShapeMarginIsNegligible() and RadiiAreRoughlyEqual(). Two nits: (1) This paragraph needs to be rewrapped to 80 characters. (The middle 2 lines overflow in my emacs buffer which is 80 characters wide -- those lines are each 81 characters long. :)) (2) Towards the end here, consider adding something like: "In those cases, we can conveniently represent the entire float area using an ellipse." (That makes it clearer what's special about the shape-margin-negligible and the radii-roughly-equal cases, and why they get their own constructor.) ::: layout/generic/nsFloatManager.cpp:725 (Diff revision 5) > - {} > + // Construct the float area using rasterization to calculate the shape boundary. > + EllipseShapeInfo(const nsPoint& aCenter, > + const nsSize& aRadii, (1) This line is a bit too long (82 chars) -- please wrap it at 80 columns or less. (2) In symmetry with my nit for the other constructor: perhaps add a clarification at the end here -- something like the following perhaps: "(This version accounts for the fact that applying 'shape-margin' to an ellipse can produce a shape that's ellipse-like, but not actually representable as an ellipse.)" ::: layout/generic/nsFloatManager.cpp:733 (Diff revision 5) > + static bool ShapeMarginIsNegligible(nscoord aShapeMargin) { > + static const nscoord SHAPE_MARGIN_NEGLIGIBLE_MAX(5); > + return aShapeMargin <= SHAPE_MARGIN_NEGLIGIBLE_MAX; > + } > > + static bool RadiiAreRoughlyEqual(const nsSize& aRadii) { > + static const float RADII_RATIO_ROUGHLY_EQUAL_MIN = 0.9f; > + static const float RADII_RATIO_ROUGHLY_EQUAL_MAX = > + 1.0f / RADII_RATIO_ROUGHLY_EQUAL_MIN; > + float radiiRatio = aRadii.width / aRadii.height; > + return (radiiRatio >= RADII_RATIO_ROUGHLY_EQUAL_MIN && > + radiiRatio <= RADII_RATIO_ROUGHLY_EQUAL_MAX); > + } > nscoord LineLeft(const nscoord aBStart, These seem like premature & oddly specific optimization targets -- "shape-margin" from 1 through 5 nscoord units (which is 1/60th - 1/20th of a CSS pixel), and ellipses that have only 10% difference between their radii (and also need shape-margin). I'm not sure this is worth doing at this point, unless we have knowledge that content within these constraints is common, and we have confidence on how far off our ellipse approximation will be in the worst case for these scenarios. Could we just start out by going for strict correctness, i.e. require exactness to trigger the fast path? (so: require a shape-margin of *exactly* 0, or radii that are exactly-equal). Down the line, if we discover there are use cases where people are having rasterization perf issues from content that uses an ellipse with nonzero-but-negligible shape-margin, or not-quite-equal radii, then we can optimize at that point (and we might well want a subtler optimization that takes *both* the shape-margin *and* the radii-ratio into consideration simultaneously, and only uses the fast path if they collectively satisfy some requirements). (Side note: the existence of this "ShapeMarginIsNegligible()" check also makes it harder to reason about exact-zero comparisons elsewhere, like "if (mShapeMargin == 0) {" in LineLeft/LineRight. It seems unnecessarily confusing to have different concepts of "basically-zero" vs. "actually zero" in different chunks of the code -- and while it's possible the different usages are justified & reasonable, I'm also not sure it's worth it to document why they're justified & maintain the invariants around them being justified, just so that we can get an optimization for 1/20th-of-a-pixel shape-margins.) ::: layout/generic/nsFloatManager.cpp:1607 (Diff revision 5) > } > > case StyleShapeSourceType::Shape: { > const UniquePtr<StyleBasicShape>& basicShape = shapeOutside.GetBasicShape(); > + nscoord shapeMargin = > + ::ResolveToDefiniteSize(mFrame->StyleDisplay()->mShapeMargin, (As in part 3, we should be using the nsLayoutUtils function added in Bug 1434478 here, rather than a call to a local percent-resolution function.) ::: layout/generic/nsFloatManager.cpp:1858 (Diff revision 5) > if (type == StyleBasicShapeType::Circle) { > nscoord radius = ShapeUtils::ComputeCircleRadius(aBasicShape, physicalCenter, > physicalShapeBoxRect); > + // Circles can use the three argument, math constructor for EllipseShapeInfo. > radii = nsSize(radius, radius); > + return MakeUnique<EllipseShapeInfo>(logicalCenter, radii, aShapeMargin); > } else { > MOZ_ASSERT(type == StyleBasicShapeType::Ellipse); This patch is turning this code into using "else-after-return", which the coding style guide discourages: https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#General_C.2FC.2B.2B_Practices Just drop the "else {" and deindent the code that follows.

Comment on attachment 8959330 [details] Bug 1265342 Part 3: Stub in shape-margin for shape-outside: image, by implementing only for shape-margin: 0. [canceling bugzilla-attachment review requests on part 3 and part 4, as those have open issues in MozReview and I think there was just a bugzilla<-->mozreview review-state-preservation mixup there. Let me know if I misunderstood & you're waiting on review feedback for either of these though.]

Comment on attachment 8959332 [details] Bug 1265342 Part 5a: Implement shape-margin for shape-outside: circle and ellipse (ellipse only for shape-margin: 0). https://reviewboard.mozilla.org/r/228176/#review242346 > These seem like premature & oddly specific optimization targets -- "shape-margin" from 1 through 5 nscoord units (which is 1/60th - 1/20th of a CSS pixel), and ellipses that have only 10% difference between their radii (and also need shape-margin). > > I'm not sure this is worth doing at this point, unless we have knowledge that content within these constraints is common, and we have confidence on how far off our ellipse approximation will be in the worst case for these scenarios. > > Could we just start out by going for strict correctness, i.e. require exactness to trigger the fast path? (so: require a shape-margin of *exactly* 0, or radii that are exactly-equal). > > Down the line, if we discover there are use cases where people are having rasterization perf issues from content that uses an ellipse with nonzero-but-negligible shape-margin, or not-quite-equal radii, then we can optimize at that point (and we might well want a subtler optimization that takes *both* the shape-margin *and* the radii-ratio into consideration simultaneously, and only uses the fast path if they collectively satisfy some requirements). > > (Side note: the existence of this "ShapeMarginIsNegligible()" check also makes it harder to reason about exact-zero comparisons elsewhere, like "if (mShapeMargin == 0) {" in LineLeft/LineRight. It seems unnecessarily confusing to have different concepts of "basically-zero" vs. "actually zero" in different chunks of the code -- and while it's possible the different usages are justified & reasonable, I'm also not sure it's worth it to document why they're justified & maintain the invariants around them being justified, just so that we can get an optimization for 1/20th-of-a-pixel shape-margins.) The 5 nscoord units was scale confusion on my part -- I think 5 dev pixels would be a reasonable maximum. I'm making the change that you propose here -- strict equality -- but I continue to believe that some fuzziness is the right thing to do. For low-eccentricity ellipses ("round" ones), there's just not going to be a meaningful difference in the results between the fast path and the slow path. And it's not like the slow path is mathematically perfect -- it's using a chamfer method that will result in mismeasurements at some angles around the ellipse.