Closed Bug 1265342 Opened 8 years ago Closed 6 years ago

Implement 'shape-margin' for all shapes except polygon

Categories

(Core :: CSS Parsing and Computation, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox61 --- fixed

People

(Reporter: sebo, Assigned: bradwerth)

References

(Depends on 1 open bug, Blocks 1 open bug, )

Details

(Keywords: dev-doc-complete, DevAdvocacy, Whiteboard: [designer-tools][wptsync upstream])

Attachments

(20 files, 12 obsolete files)

59 bytes, text/x-review-board-request
xidorn
: review+
Details
59 bytes, text/x-review-board-request
xidorn
: review+
Details
59 bytes, text/x-review-board-request
dholbert
: review+
Details
59 bytes, text/x-review-board-request
dholbert
: review+
Details
59 bytes, text/x-review-board-request
dholbert
: review+
Details
59 bytes, text/x-review-board-request
dholbert
: review+
Details
59 bytes, text/x-review-board-request
dholbert
: review+
Details
59 bytes, text/x-review-board-request
dholbert
: review+
Details
59 bytes, text/x-review-board-request
dholbert
: review+
Details
59 bytes, text/x-review-board-request
dholbert
: review+
Details
59 bytes, text/x-review-board-request
dholbert
: review+
Details
59 bytes, text/x-review-board-request
dholbert
: review+
Details
59 bytes, text/x-review-board-request
dholbert
: review+
Details
59 bytes, text/x-review-board-request
dholbert
: review+
Details
59 bytes, text/x-review-board-request
dholbert
: review+
Details
59 bytes, text/x-review-board-request
dholbert
: review+
Details
59 bytes, text/x-review-board-request
dholbert
: review+
Details
59 bytes, text/x-review-board-request
dholbert
: review+
Details
59 bytes, text/x-review-board-request
dholbert
: review+
Details
59 bytes, text/x-review-board-request
dholbert
: review+
Details
The shape-margin property should be implemented as defined in https://drafts.csswg.org/css-shapes/#shape-margin-property.

Sebastian
Keywords: dev-doc-needed
Keywords: DevAdvocacy
Whiteboard: [DevRel:P1]
Flags: platform-rel?
platform-rel: --- → ?
platform-rel: ? → ---
Whiteboard: [DevRel:P1]
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.
Assignee: nobody → tlin
Status: NEW → ASSIGNED
Sorry, I'm not able to work on this bug as promised.
Assignee: aethanyc → nobody
Status: ASSIGNED → NEW
Assignee: nobody → bwerth
Status: NEW → ASSIGNED
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?
Flags: needinfo?(dbaron)
(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.
Flags: needinfo?(dbaron)
(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.
Attachment #8953642 - Attachment is obsolete: true
Attachment #8955343 - Attachment is obsolete: true
Attachment #8958614 - Attachment is obsolete: true
Attachment #8953643 - Attachment is obsolete: true
Attachment #8954209 - Attachment is obsolete: true
Attachment #8954210 - Attachment is obsolete: true
Attachment #8954211 - Attachment is obsolete: true
Blocks: 1451499
Bug 1451499 has been split out for the shape-margin implementation for polygon.
Summary: Implement 'shape-margin' → Implement 'shape-margin' for all shapes except polygon
Attachment #8959327 - Flags: review?(xidorn+moz)
Attachment #8959328 - Flags: review?(xidorn+moz)
Attachment #8959329 - Flags: review?(dholbert)
Attachment #8959330 - Flags: review?(dholbert)
Attachment #8959331 - Flags: review?(dholbert)
Attachment #8959332 - Flags: review?(dholbert)
Attachment #8959333 - Flags: review?(dholbert)
Attachment #8959334 - Flags: review?(dholbert)
Attachment #8959335 - Flags: review?(dholbert)
Attachment #8959336 - Flags: review?(dholbert)
Attachment #8953641 - Flags: review?(dholbert)
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.
Attachment #8959327 - Flags: review?(xidorn+moz) → review+
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
Attachment #8959328 - Flags: review?(xidorn+moz) → review+
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
Attachment #8953641 - Flags: review?(dholbert) → review+
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/
Attachment #8959329 - Flags: review?(dholbert) → review+
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)
Attachment #8959330 - Flags: review?(dholbert) → 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/#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)
Attachment #8959333 - Flags: review?(dholbert) → review-
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
Attachment #8959334 - Flags: review?(dholbert) → review+
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
Attachment #8959335 - Flags: review?(dholbert) → review+
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.
Depends on: 1434478
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.
Attachment #8959336 - Flags: review?(dholbert) → review+
Attachment #8959334 - Attachment is obsolete: true
Attachment #8959335 - Attachment is obsolete: true
Attachment #8959336 - Attachment is obsolete: true
Attachment #8953641 - Attachment is obsolete: true
Attachment #8954210 - Attachment is obsolete: false
Attachment #8959334 - Attachment is obsolete: false
Attachment #8959335 - Attachment is obsolete: false
Attachment #8959336 - Attachment is obsolete: false
Attachment #8953641 - Attachment is obsolete: false
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.)
Attachment #8959333 - Flags: review?(dholbert) → 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/#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.
Attachment #8959331 - Flags: review?(dholbert) → 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/#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.
Attachment #8967199 - Flags: review-
(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.
Attachment #8967813 - Attachment is obsolete: true
Attachment #8967814 - Attachment is obsolete: true
Attachment #8967815 - Attachment is obsolete: true
Attachment #8967816 - Attachment is obsolete: true
Attachment #8967817 - Attachment is obsolete: true
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.
Attachment #8959332 - Flags: review?(dholbert) → review-
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.]
Attachment #8959330 - Flags: review?(dholbert)
Attachment #8959331 - Flags: review?(dholbert)
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.
Attachment #8954210 - Attachment is obsolete: true
Attachment #8959334 - Attachment is obsolete: true
Attachment #8959335 - Attachment is obsolete: true
Attachment #8959336 - Attachment is obsolete: true
Attachment #8953641 - Attachment is obsolete: true
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/#review243116

r=me with the following:

::: layout/generic/nsFloatManager.cpp:1179
(Diff revisions 4 - 6)
> -    auto df = MakeUnique<dfType[]>(dfSize);
> +    // Since the shape-margin value is CSS controlled, and large values will
> +    // generate large wEx and hEx values, we do a falliable allocation for
> +    // the distance field. If allocation fails, we early exit and layout will
> +    // be wrong, but we'll avoid memory allocation errors.
> +    auto df = MakeUniqueFallible<dfType[]>(wEx * hEx);

Two nits:
 (1) s/shape-margin value/margin-box size/

(IIUC, wEx and hEx aren't influenced by shape-margin -- they come from the margin-box size, i.e. "height/width" properties + "margin-*" properties.)

 (2) s/memory allocation errors/aborting from OOM/  (just to be a bit more precise about what sorts of "errors" we're avoiding)

::: layout/generic/nsFloatManager.cpp:1323
(Diff revisions 4 - 6)
> -        // 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.
> +        // Our interval values, iMin, iMax, and b are all calculated from
> +        // the expanded region, which is based on the margin rect. To create
> +        // our interval, we have to subtract 2 from iMin, iMax, and b to
> +        // account for the expanded region edges, and we pass in
> +        // aMarginRect.TopLeft() as the offset from the container.
>          CreateInterval(iMin - 2, iMax - 2, b - 2, aAppUnitsPerDevPixel,
>                         aMarginRect.TopLeft(), aWM, aContainerSize);

Two things:
 (1) Maybe wrap "iMin, iMax, and b" in parens. Otherwise it sounds like "interval values, iMin, iMax, and b" are 4 distinct things.


 (2) This explanation still feels a little hand-wavy. Could you replace the comment's final 2 lines with the following, to explain the conversions a bit more?
        [...]
        // account for the expanded region edges.  This produces coords that
        // are relative to our margin-rect, so we pass in aMarginRect.TopLeft()
        // to make CreateInterval convert to our container's coordinate space.
Attachment #8959331 - Flags: review?(dholbert) → review+
Attachment #8959330 - Flags: review?(dholbert)
Attachment #8959332 - Flags: review?(dholbert)
Attachment #8968376 - Flags: review?(dholbert)
Attachment #8968698 - Flags: review?(dholbert)
Attachment #8967813 - Flags: review?(dholbert)
Attachment #8968699 - Flags: review?(dholbert)
Attachment #8967814 - Flags: review?(dholbert)
Attachment #8967815 - Flags: review?(dholbert)
Attachment #8967816 - Flags: review?(dholbert)
Attachment #8968700 - Flags: review?(dholbert)
Attachment #8968701 - Flags: review?(dholbert)
Attachment #8968702 - Flags: review?(dholbert)
Attachment #8967817 - Flags: review?(dholbert)
(In reply to Daniel Holbert [:dholbert] (away 4/24 - 5/11) from comment #100)
> Comment on attachment 8959336 [details]
> Bug 1265342 Part 9: Update shape-outside ellipse tests to correct invalid
> use of only one radius parameter.
> 
> 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/

Since MozReview got confused after a bad local merge, review comments from parts 8+ were lost. I just wanted to clarify that this pending issue from the earlier reviews is still to be done (by me).
Comment on attachment 8967813 [details]
Bug 1265342 Part 6a: Implement shape-margin for shape-outside: inset, for some special cases with shape-margin > 0.

https://reviewboard.mozilla.org/r/236488/#review243522


Code analysis found 1 defect in this patch:
 - 1 defect found by clang-tidy

You can run this analysis locally with:
 - `./mach static-analysis check path/to/file.cpp` (C/C++)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: layout/generic/nsFloatManager.cpp:2098
(Diff revision 7)
> +  logicalInsetRect.Inflate(aShapeMargin);
> +  if (physicalRadii[0] == physicalRadii[1] &&
> +      physicalRadii[2] == physicalRadii[3] &&
> +      physicalRadii[4] == physicalRadii[5] &&
> +      physicalRadii[6] == physicalRadii[7]) {
> +    for (int32_t i = 0; i < 8; ++i) {

Warning: Use range-based for loop instead [clang-tidy: modernize-loop-convert]

 0:09.80     for (int32_t i = 0; i < 8; ++i) {
 0:09.80     ^   ~~~~~~~~~~~~~~~~~~~~~~~~~~~
 0:09.80         (int & i : physicalRadii)
Comment on attachment 8967814 [details]
Bug 1265342 Part 7: Implement shape-margin for shape-outside: shape-box.

https://reviewboard.mozilla.org/r/236490/#review243524


Code analysis found 1 defect in this patch:
 - 1 defect found by clang-tidy

You can run this analysis locally with:
 - `./mach static-analysis check path/to/file.cpp` (C/C++)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: layout/generic/nsFloatManager.cpp:2100
(Diff revision 7)
>      return MakeUnique<RoundedBoxShapeInfo>(logicalShapeBoxRect,
>                                             UniquePtr<nscoord[]>());
>    }
>  
> +  // Add aShapeMargin to each of the radii.
> +  for (int32_t i = 0; i < 8; ++i) {

Warning: Use range-based for loop instead [clang-tidy: modernize-loop-convert]

 0:16.57   for (int32_t i = 0; i < 8; ++i) {
 0:16.57   ^   ~~~~~~~~~~~~~~~~~~~~~~~~~~~
 0:16.57       (int & i : physicalRadii)
 0:16.57 Warning: modernize-loop-convert in /cache/sa-central/layout/generic/nsFloatManager.cpp: use range-based for loop instead
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/#review243942

::: layout/generic/nsFloatManager.cpp:1265
(Diff revision 8)
>      case StyleShapeSourceType::Image: {
>        float shapeImageThreshold = mFrame->StyleDisplay()->mShapeImageThreshold;
> +      nscoord shapeMargin = nsLayoutUtils::ResolveToLength<true>(
> +        mFrame->StyleDisplay()->mShapeMargin,

(We should avoid calling StyleDisplay() repeatedly here [2 calls quoted here and 1 more further up]. I spun off a followup bug 1455453 for that, since I don't think it needs to be fixed before this lands.  But if you'd like to fold that tweak into this patch, feel free.)
Attachment #8959330 - Flags: review?(dholbert) → review+
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/#review243948

::: layout/generic/nsFloatManager.cpp:810
(Diff revision 10)
> +
> +nsFloatManager::EllipseShapeInfo::EllipseShapeInfo(const nsPoint& aCenter,
> +                                                   const nsSize& aRadii,
> +                                                   nscoord aShapeMargin,
> +                                                   int32_t aAppUnitsPerDevPixel,
> +                                                   WritingMode aWM,
> +                                                   const nsSize& aContainerSize)
> +  : mCenter(aCenter)
> +  , mRadii(aRadii)
> +  , mShapeMargin(aShapeMargin)
> +{
> +  MOZ_ASSERT(!(RadiiAreRoughlyEqual(aRadii) ||
> +               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.");
> +}

Nit for future reference (no action needed for now):

The fatal MOZ_ASSERT_UNREACHABLE(...) statements here (and in the other APIs) don't seem justified, because AFAIK this line *is* reachable.  There's a MakeUnique call at the very bottom of this patch that calls into this constructor.

It'd be better to either truly make it unreachable, or to use a nonfatal assert like NS_ERROR(...) so that we can proceed gracefully at this point in the patch queue (and so that if someone is hg-bisect'ing a testcase that happens to use shape-margin and they're trying to debug something unrelated, they won't be blocked by this fatal assertion killing their build).

Not a huge problem, since these are just temporary placeholders that get replaced in the next patch, and the 'hg-bisect' scenario is probably unlikely to surface in real life for this case. But worth keeping in mind for future incremental patch stacks.

Anyway, if it's easy, consider swapping in NS_ERROR(...) for these, but also no big deal if you just leave them as-is.  (I know that'd cause bitrot so it might not be worth it.)
Attachment #8959332 - Flags: review?(dholbert) → review+
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/#review243952

::: layout/generic/nsFloatManager.cpp:727
(Diff revision 7)
>    // Construct the float area using rasterization to calculate the shape
>    // boundary. This constructor accounts for the fact that applying
>    // 'shape-margin' to an ellipse produces a shape that is not mathematically
>    // representable as an ellipse.
>    EllipseShapeInfo(const nsPoint& aCenter,
>                     const nsSize& aRadii,
>                     nscoord aShapeMargin,
> -                   int32_t aAppUnitsPerDevPixel,
> +                   int32_t aAppUnitsPerDevPixel);
> -                   WritingMode aWM,
> -                   const nsSize& aContainerSize);

Nit: this change (removing args from the constructor) should be folded in to the previous patch (part 5a), for simplicity & to avoid unnecessary churn.

That's where this constructor is created (as a stub to be filled in later), and that stub shouldn't have these extra params if they're doomed for removal anyway.

::: layout/generic/nsFloatManager.cpp:768
(Diff revision 7)
>    void Translate(nscoord aLineLeft, nscoord aBlockStart) override
>    {
> +    for (nsRect& interval : mIntervals) {
> +      interval.MoveBy(aLineLeft, aBlockStart);
> +    }
> +
>      mCenter.MoveBy(aLineLeft, aBlockStart);
>    }

Please move this chunk to the previous patch (where we introduce the mIntervals member variable).

This chunk is associated with that member-variable introduction, and it's not really involved in what this patch here is ostensibly about (supporting shape-margin > 0)

::: layout/generic/nsFloatManager.cpp:879
(Diff revision 7)
> +  // effectively a 3x3 neighborhood. We need to expand our distance field by
> +  // a further 2 pixels in both axes, on the trailing block edge and the
> +  // leading inline edge. We call this edge area the expanded region.
> +  static const int32_t iExpand = 2;
> +  static const int32_t bExpand = 2;
> +  const int32_t iSize = bounds.width + iExpand;
> +  const int32_t bSize = bounds.height + bExpand;

Could you add a parenthetical at the end of the second-to-last sentence here, to indicate what these "leading"/"trailing" directions actually represent for our ellipse?  (i.e. are we expanding 2px outwards, or 2px inwards?)

Without clarification, the phrase "trailing block edge" is particularly ambiguous right now, because the loop below this does its iteration in *reverse* block direction, and so "trailing" could be quite reasonably interpreted as being higher B-values ("trailing" from previous loop iterations), or as being lower B-values ("trailing" in magnitude of B value).

[Looking at the code, I think the expanded region is "inwards" into the ellipse, but I might be reading it backwards]

::: layout/generic/nsFloatManager.cpp:892
(Diff revision 7)
> +  // First pass setting distance field, in negative block direction, three
> +  // cases:
> +  // 1) Expanded region pixel: set to MAX_MARGIN_5X.
> +  // 2) Pixel within the ellipse: set to 0.
> +  // 3) Other pixel: set to minimum neighborhood distance value, computed
> +  //                 with 5-7-11 chamfer.

Maybe s/First/Single/?   ("First" is confusing, since we only do a single pass here, unlike for <image> where we do 2 passes.)

::: layout/generic/nsFloatManager.cpp:946
(Diff revision 7)
> +        df[index] = std::min<dfType>(df[index - 1] + 5,
> +                    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))));

Could you add a followup patch at the end of this queue (or followup bug) to add assertions to the start of every clause with df[$EXPRESSION], to validate that our largest + smallest $EXPRESSION values are in-bounds for our matrix? (I think there would be 3 places that need this -- here and 2 for shape-image).

This would be just to validate that we're not doing any out-of-bounds array reads here (and to help fuzzers discover if we are). Something like the following (I think the suggested code-comment holds, but please make sure its logic is sound):

        // In any loop-iteration where |index| doesn't satisfy this assert,
        // we'll take the "expanded region" clause above, not this one.
        MOZ_ASSERT(index - 1 >= 0 &&
                   index + (iSize * 2) - 1 < iSize * bSize,
                  "Indices used below should be in-bounds for df matrix");

::: layout/generic/nsFloatManager.cpp:961
(Diff revision 7)
> +    if (!bIsInExpandedRegion) {
> +      MOZ_ASSERT(!(bIsLessThanEllipseBStart && iMax == iIntercept),
> +                 "In the shape-margin region, we should always find a pixel "
> +                 "within the margin for each block row.");
> +      // Origin for this interval is at the minimum value in both axes
> +      // (which is the top center for horizontal-tb writing mode) adjusted
> +      // in the positive block direction by bInAppUnits.
> +      nsPoint origin(aCenter.x,
> +                     aCenter.y - radiiPlusShapeMargin.height + bInAppUnits);
> +      // Size is an inline iMax plus 1 (to account for the whole pixel) dev
> +      // pixels, by 1 block dev pixel. We convert this to app units.
> +      nsSize size((iMax - iExpand + 1) * aAppUnitsPerDevPixel,
> +                  aAppUnitsPerDevPixel);
> +      mIntervals.AppendElement(nsRect(origin, size));

Should we be skipping this create-interval clause if iMax is -1?  (Really, maybe we need "iMax >= 0" in the if-check, here, to cover that & for robustness in case of gigantic CSS "width", which could give us an iMax that overflows past MAX_INT and ends up negative?)

Seems possibly-wise for 2 reasons:
 (a) for consistency with the similar shape-outside:<image> code (which checks iMax against -1 before creating an interval)

 (b) for sanity, because creating a 0-sized interval (or a negative-sized interval) seems bogus.

Or, maybe there's there already good reason to be sure that iMax is never -1 here... If so, maybe document and/or assert that. Your existing assertion might already implicitly cover that but I'm not sure.

::: layout/generic/nsFloatManager.cpp:983
(Diff revision 7)
> -nsFloatManager::EllipseShapeInfo::LineLeft(const nscoord aBStart,
> -                                           const nscoord aBEnd) const
> +nsFloatManager::EllipseShapeInfo::LineEdge(const nscoord aBStart,
> +                                           const nscoord aBEnd,
> +                                           bool aIsLineLeft) const
>  {
> +  // If no mShapeMargin, just compute the edge using math.
>    if (mShapeMargin == 0) {
> -    nscoord lineLeftDiff =
> +    nscoord lineDiff =
>        ComputeEllipseLineInterceptDiff(BStart(), BEnd(),
>                                        mRadii.width, mRadii.height,
>                                        mRadii.width, mRadii.height,
>                                        aBStart, aBEnd);
> -    return mCenter.x - mRadii.width + lineLeftDiff;
> +    return mCenter.x + (aIsLineLeft ? (-mRadii.width + lineDiff) :
> +                                      (mRadii.width - lineDiff));
>    }

(I was hoping to see the nonfunctional parts of this "LineEdge" stuff to be spun out into in its own smaller patch -- see comment 115 -- but I suppose at this point it's not a huge deal for it to be grouped in here, so never mind about that.)
Attachment #8967199 - Flags: review?(dholbert) → review+
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/#review243952

> Could you add a parenthetical at the end of the second-to-last sentence here, to indicate what these "leading"/"trailing" directions actually represent for our ellipse?  (i.e. are we expanding 2px outwards, or 2px inwards?)
> 
> Without clarification, the phrase "trailing block edge" is particularly ambiguous right now, because the loop below this does its iteration in *reverse* block direction, and so "trailing" could be quite reasonably interpreted as being higher B-values ("trailing" from previous loop iterations), or as being lower B-values ("trailing" in magnitude of B value).
> 
> [Looking at the code, I think the expanded region is "inwards" into the ellipse, but I might be reading it backwards]

(Ah, I see the next patch (5c) rewrites this comment a bit, as part of changing iteration order.  Maybe it'd be simplest to address this review nit as part of that patch, so that it doesn't trigger bitrot/rebase pain.)
Comment on attachment 8968376 [details]
Bug 1265342 Part 5c: Change ellipse difference field math to use postive-inline, positive-block quadrant.

https://reviewboard.mozilla.org/r/237064/#review244024

(This seems like it perhaps should've just been folded into 5b, so that we implement this in the intended direction up-front, but I'll go with it. :) I'm guessing you split it up like this for my benefit since I'd already seen 5b - thanks!)

::: commit-message-6578d:1
(Diff revision 4)
> +Bug 1265342 Part 5c: Change ellipse difference field math to use postive-inline, positive-block quadrant.

typo: "postive-inline"

s/post/posit/

::: layout/generic/nsFloatManager.cpp:894
(Diff revision 4)
>    // First pass setting distance field, in negative block direction, three
>    // cases:

s/negative block direction/positive block direction/

(as of this patch)

::: layout/generic/nsFloatManager.cpp:1014
(Diff revision 4)
> -  // whichever two of them are within the upper-right quadrant.
> -  nscoord bLargestWithinIntervals = std::max(
> -    bStartIsAboveOrAtCenter ? aBStart : aBStart - (aBStart - mCenter.y) * 2,
> -    bEndIsBelowOrAtCenter ? aBEnd - (aBEnd - mCenter.y) * 2 : aBEnd);
> +  // whichever two of them are within the lower-right quadrant. When we
> +  // reflect from the upper-right quadrant to the lower-right, we have to
> +  // subtract 1 from the reflection, to account that block values are always
> +  // addressed from the leading block edge.
> +  nscoord bSmallestWithinIntervals = std::min(
> +    bStartIsAboveCenter ? aBStart + (mCenter.y - aBStart) * 2 - 1 : aBStart,
> +    bEndIsBelowOrAtCenter ? aBEnd : aBEnd + (mCenter.y - aBEnd) * 2 - 1);

Please add a bit more explanation for this "-1" business... It's not obvious why its required right now. (And I think it only matters for nscoord values that happen to be at exact pixel boundaries, and I'm not convinced that those are worth worrying about...)

::: layout/generic/nsFloatManager.cpp:1031
(Diff revision 4)
>    // The interval is storing the line right value. If aIsLineLeft is true,
> -  // return the line right value reflected about the center.
> +  // return the line right value reflected about the center. Again, we
> +  // subtract 1 from the reflection to account for leading inline edge.
>    nscoord iLineRight = mIntervals[index].XMost();
> -  return aIsLineLeft ? iLineRight - (iLineRight - mCenter.x) * 2 : iLineRight;
> +  return aIsLineLeft ? iLineRight - (iLineRight - mCenter.x) * 2 - 1
> +                     : iLineRight;

I'm less convinced that the -1 business makes sense here, because here we're not using -1 as an address (where being off will produce a completely different "band"), as we were above for the B axis.  Here, we're just dealing with the resulting extent of the ellipse+margin, and it seems to me that that extent should be reflectable across the center without any need for +/-1 adjustments...
Comment on attachment 8968376 [details]
Bug 1265342 Part 5c: Change ellipse difference field math to use postive-inline, positive-block quadrant.

https://reviewboard.mozilla.org/r/237064/#review244210

::: layout/generic/nsFloatManager.cpp:1014
(Diff revision 4)
> -  // whichever two of them are within the upper-right quadrant.
> -  nscoord bLargestWithinIntervals = std::max(
> -    bStartIsAboveOrAtCenter ? aBStart : aBStart - (aBStart - mCenter.y) * 2,
> -    bEndIsBelowOrAtCenter ? aBEnd - (aBEnd - mCenter.y) * 2 : aBEnd);
> +  // whichever two of them are within the lower-right quadrant. When we
> +  // reflect from the upper-right quadrant to the lower-right, we have to
> +  // subtract 1 from the reflection, to account that block values are always
> +  // addressed from the leading block edge.
> +  nscoord bSmallestWithinIntervals = std::min(
> +    bStartIsAboveCenter ? aBStart + (mCenter.y - aBStart) * 2 - 1 : aBStart,
> +    bEndIsBelowOrAtCenter ? aBEnd : aBEnd + (mCenter.y - aBEnd) * 2 - 1);

Here's how I think about it. If our pixels are addressed by their minimum x,y corner, then whenever we reflect about the center, we have to subtract 1 from both axes to keep the math right. Reflection about the center can be defined subtraction of twice the distance from the center.

Imagine a 2x2 grid of pixels. If we consider the pixel at the top left, its reflection is in the pixel in the bottom right. The pixel at the top left is (0,0). The bottom right pixel is (1,1). Importantly, the center, which is not a pixel but is a mathematical concept, is ALSO (1,1). 

Reflecting the top-left pixel (in either axis): 0 - (-1) * 2 - 1 = 1. If we didn't subtract 1 in this formula, we'd be addressing one pixel too far down/right, which would be pixel 2.

Reflecting the bottom-right pixel (in either axis): 1 - (0) * 2 - 1 = 0. If we didn't subtract 1 in this formula, we'd be addressing one pixel too far down/right, which would be pixel 1.

This reasoning holds for both x and y because we address the pixels from the minimum (x,y) corner.
Per discussion just now: we're really tracking abstract positions, not positions-of-pixels-with-unit-thickness, so I don't think that logic quite applies here.

I think we do need the -1 in the block/y axis, purely as an artifact of the way we "address" our intervals (inclusive of BStart, exclusive of BEnd), and the fact that an "0" position would find itself in the first interval and so we need to be sure "reflected-0" ends up in the final interval (rather than falling barely off the end of our intervals altogether).  But we don't need that sort of fixup in the inline axis, where we really are just reflecting an abstract distance-from-the-central-axis-of-the-ellipse value to produce an abstract position.
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/#review243952

> Could you add a followup patch at the end of this queue (or followup bug) to add assertions to the start of every clause with df[$EXPRESSION], to validate that our largest + smallest $EXPRESSION values are in-bounds for our matrix? (I think there would be 3 places that need this -- here and 2 for shape-image).
> 
> This would be just to validate that we're not doing any out-of-bounds array reads here (and to help fuzzers discover if we are). Something like the following (I think the suggested code-comment holds, but please make sure its logic is sound):
> 
>         // In any loop-iteration where |index| doesn't satisfy this assert,
>         // we'll take the "expanded region" clause above, not this one.
>         MOZ_ASSERT(index - 1 >= 0 &&
>                    index + (iSize * 2) - 1 < iSize * bSize,
>                   "Indices used below should be in-bounds for df matrix");

I'll add as another piece to this bug.

> Should we be skipping this create-interval clause if iMax is -1?  (Really, maybe we need "iMax >= 0" in the if-check, here, to cover that & for robustness in case of gigantic CSS "width", which could give us an iMax that overflows past MAX_INT and ends up negative?)
> 
> Seems possibly-wise for 2 reasons:
>  (a) for consistency with the similar shape-outside:<image> code (which checks iMax against -1 before creating an interval)
> 
>  (b) for sanity, because creating a 0-sized interval (or a negative-sized interval) seems bogus.
> 
> Or, maybe there's there already good reason to be sure that iMax is never -1 here... If so, maybe document and/or assert that. Your existing assertion might already implicitly cover that but I'm not sure.

Good points. For clarity, I've changed the code to set iInterval to nscoord_MIN when there's no intersection with the actual ellipse. The interval creation only triggers now on iMax > nscoord_MIN. I also changed the assert about always creating an interval for each block row into a warning. This should be true, but doesn't cause failures if it's not true.
Comment on attachment 8968376 [details]
Bug 1265342 Part 5c: Change ellipse difference field math to use postive-inline, positive-block quadrant.

https://reviewboard.mozilla.org/r/237064/#review244024

Yes, that's my intent. After I wrote the code in part 5b, I realized it should have this structure, but part 5b already had some review notes. Once I've addressed review notes on 5b and 5c, I will collapse them together.
Attachment #8968376 - Attachment is obsolete: true
Attachment #8968376 - Flags: review?(dholbert)
(In reply to Brad Werth [:bradwerth] from comment #228)
> > Could you add a followup patch at the end of this queue (or followup bug) to add assertions to the start of every clause with df[$EXPRESSION], to validate that our largest + smallest $EXPRESSION values are in-bounds for our matrix? (I think there would be 3 places that need this -- here and 2 for shape-image).
> > 
> > This would be just to validate that we're not doing any out-of-bounds array reads here (and to help fuzzers discover if we are). Something like the following (I think the suggested code-comment holds, but please make sure its logic is sound):
> > 
> >         // In any loop-iteration where |index| doesn't satisfy this assert,
> >         // we'll take the "expanded region" clause above, not this one.
> >         MOZ_ASSERT(index - 1 >= 0 &&
> >                    index + (iSize * 2) - 1 < iSize * bSize,
> >                   "Indices used below should be in-bounds for df matrix");
> 
> I'll add as another piece to this bug.

Since all the clauses use df[index], the correct/safe place to put this assertion would be before any of the clauses. It wouldn't be asserting that another clause should be taken, but that the index calculation is valid. That's what I'll do in the patches.
Comment on attachment 8968698 [details]
Bug 1265342 Part 5d: Move EllipseShapeInfo class definition ahead of RoundedBoxShapeInfo so it can be referenced by rounded boxes.

https://reviewboard.mozilla.org/r/237390/#review244350
Attachment #8968698 - Flags: review?(dholbert) → review+
Comment on attachment 8967813 [details]
Bug 1265342 Part 6a: Implement shape-margin for shape-outside: inset, for some special cases with shape-margin > 0.

https://reviewboard.mozilla.org/r/236488/#review244352

::: layout/generic/nsFloatManager.cpp:1039
(Diff revision 8)
> +  // A shape-margin value extends the boundaries of the float area.
> +  nscoord mShapeMargin;

Optional nit: looks like this new member-var is set in the constructor's init list & never changes. Probably good to mark it as "const" to make that invariant clearer/enforced?

(I guess the same goes for existing members mRect and mRadii... I think those can be "const nsRect" and "const UniquePtr" -- feel free/encouraged to upgrade those here, too, if it's just a one-liner tweak.)

(If you make this change, feel free to do it in a separate followup patch, if that's easier bitrot-wise.)

::: layout/generic/nsFloatManager.cpp:2111
(Diff revision 8)
> +    for (int& i : physicalRadii) {
> +      physicalRadii[i] += aShapeMargin;
> +    }

Hmm, this looks wrong... Inside the loop there, we're misusing the loop variable "i" as if it were an array-index. But in fact it's not an array-index!  In a range-based for loop, the loop variable is an actual entry in the array. (And in this case, the array entries happen to be int-flavored since nscoord is int, or int-ish, so this is an easy mistake to make.)

So unless I'm misunderstanding, I think this should really be:
   for (nscoord& r : physicalRadii) {
     r += aShapeMargin;
   }

(I'm a bit surprised this didn't make any tests explode... please be sure we've got code coverage for this "fast path" with equal border-radius-pairs and a nonzero shape-margin.)
Attachment #8967813 - Flags: review?(dholbert) → review+
Comment on attachment 8968699 [details]
Bug 1265342 Part 6b: Implement shape-margin for shape-outside: inset, for general case of shape-margin > 0.

https://reviewboard.mozilla.org/r/237392/#review244378

(This is probably r+ with issues addressed, but setting r- for now for the logic error noted below in LineLeft/LineRight.  Re-review should be quick & easy once that's fixed, though, I'm guessing.

::: layout/generic/nsFloatManager.cpp:1052
(Diff revision 4)
> +  // If mShapeMargin > 0 and any of our mRadii pairs are unequal, we will
> +  // construct EllipseShapeInfo objects for each corner.
> +  UniquePtr<EllipseShapeInfo> mLogicalTopLeftCorner;
> +  UniquePtr<EllipseShapeInfo> mLogicalTopRightCorner;
> +  UniquePtr<EllipseShapeInfo> mLogicalBottomLeftCorner;
> +  UniquePtr<EllipseShapeInfo> mLogicalBottomRightCorner;

This naming (LogicalTopLeft) feels a bit confusing...  Could you add a comment briefly explaining the naming?  Maybe something like:

"NOTE: mLogicalTopLeft is really the BStart Line-Left corner (etc for the others)."

::: layout/generic/nsFloatManager.cpp:1073
(Diff revision 4)
> +  mLogicalTopLeftCorner = MakeUnique<EllipseShapeInfo>(
> +    nsPoint(mRect.X() + mRadii[eCornerTopLeftX],
> +            mRect.Y() + mRadii[eCornerTopLeftY]),
> +    nsSize(mRadii[eCornerTopLeftX], mRadii[eCornerTopLeftY]),
> +    mShapeMargin, aAppUnitsPerDevPixel);
> +
> +  mLogicalTopRightCorner = MakeUnique<EllipseShapeInfo>(
> +      nsPoint(mRect.XMost() - mRadii[eCornerTopRightX],
> +              mRect.Y() + mRadii[eCornerTopRightY]),
> +      nsSize(mRadii[eCornerTopRightX], mRadii[eCornerTopRightY]),

The indentation is inconsistent here, between the first "mXYZCorner = ..." assignment and the rest. The first one, extra lines are indented 2 spaces, but for the rest of the corners, extra lines are indented 4 spaces.

Please make that consistent (probably the former, i.e. 2 spaces of indentation).

::: layout/generic/nsFloatManager.cpp:1105
(Diff revision 4)
> +  if (mShapeMargin == 0) {
> -  if (!mRadii) {
> +    if (!mRadii) {
> -    return mRect.x;
> +      return mRect.x;
> -  }
> +    }
>  
> -  nscoord lineLeftDiff =
> +    nscoord lineLeftDiff =
> -    ComputeEllipseLineInterceptDiff(
> +      ComputeEllipseLineInterceptDiff(
> -      mRect.y, mRect.YMost(),
> +        mRect.y, mRect.YMost(),
> -      mRadii[eCornerTopLeftX], mRadii[eCornerTopLeftY],
> +        mRadii[eCornerTopLeftX], mRadii[eCornerTopLeftY],
> -      mRadii[eCornerBottomLeftX], mRadii[eCornerBottomLeftY],
> +        mRadii[eCornerBottomLeftX], mRadii[eCornerBottomLeftY],
> -      aBStart, aBEnd);
> +        aBStart, aBEnd);
> -  return mRect.x + lineLeftDiff;
> +    return mRect.x + lineLeftDiff;
> -}
> +  }
>  
> +  // Determine if aBEnd is within our top corner.
> +  if (aBEnd < mLogicalTopLeftCorner->BEnd()) {

In your LineLeft and LineRight changes here, it seems you're assuming that having a nonzero shape-margin will necessarily mean we've got non-null "mLogicalTopLeftCorner" etc.

However, that's not necessarily a valid assumption! If we take one of the special fast paths from 6a (for zero or equal radii), then that assumption does not hold.

Please fix the logic to account for that.  I think maybe you should replace the "mShapeMargin == 0" check with "if (!mLogicalTopLeftCorner)", or something like that. (just picking a corner at random, since they're all or nothing)

And please be sure we've got testcases to exercise this scenario.
Attachment #8968699 - Flags: review?(dholbert) → review-
Comment on attachment 8967814 [details]
Bug 1265342 Part 7: Implement shape-margin for shape-outside: shape-box.

https://reviewboard.mozilla.org/r/236490/#review244382

::: layout/generic/nsFloatManager.cpp:1926
(Diff revision 8)
>      case StyleShapeSourceType::Box: {
> +      nscoord shapeMargin = nsLayoutUtils::ResolveToLength<true>(
> +        mFrame->StyleDisplay()->mShapeMargin,
> +        LogicalSize(aWM, aContainerSize).ISize(aWM));

Looks like we're doing this calculation in every reachable "case" here, except for the default "None" case.

To reduce code duplication, could you pull this variable out into a single local variable declared before the switch statement?

Maybe:

 nscoord shapeMargin = (shapeOutside.GetType() == StyleShapeSourceType::None)
   ? 0
   : nLayoutUtils::ResolveToLength<true>(...);

::: layout/generic/nsFloatManager.cpp:2112
(Diff revision 8)
> +  // Add aShapeMargin to each of the radii.
> +  for (int& i : physicalRadii) {
> +    physicalRadii[i] += aShapeMargin;

As in end of comment 251: I'm pretty sure this is misusing array-entries as if they were array-indices. I think you really want "for (nscoord& r : physicalRadii)" here.

(Please be sure we end up with an automated testcase that exercises this codepath -- I imagine such a testcase would crash or would produce visibly-broken behavior with the current patch here.)
Attachment #8967814 - Flags: review?(dholbert) → review+
Comment on attachment 8967815 [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/236492/#review244386

::: testing/web-platform/tests/css/css-shapes/shape-outside/supported-shapes/inset/shape-outside-inset-010.html:22
(Diff revision 8)
>              font-family: Ahem;
>              font-size: 25px;
> +            line-height: 1;

Optional nit: if you like, these 3 lines could be condensed to this one-liner:

 font: 25px/1 Ahem;

...which happens to be precisely what is recommended at http://web-platform-tests.org/writing-tests/ahem.html

("...the font shorthand should normally be used. As a result, what is typically recommended is:...font: 25px/1 Ahem;")
Attachment #8967815 - Flags: review?(dholbert) → review+
Comment on attachment 8967816 [details]
Bug 1265342 Part 9: Update shape-outside ellipse tests to correct invalid use of only one radius parameter.

https://reviewboard.mozilla.org/r/236494/#review244388
Attachment #8967816 - Flags: review?(dholbert) → review+
Comment on attachment 8968700 [details]
Bug 1265342 Part 10: Update a shape-outside circle test to make it more precise and correct.

https://reviewboard.mozilla.org/r/237394/#review244390

r=me, assuming you've checked that this continues to pass in at least one other browser (i.e. assuming we're not breaking the test from their perspective)

(Did it previously pass in other browsers, without this change? If so: is that an interop concern, and maybe we should few file a bug on them?)
Attachment #8968700 - Flags: review?(dholbert) → review+
Comment on attachment 8968700 [details]
Bug 1265342 Part 10: Update a shape-outside circle test to make it more precise and correct.

https://reviewboard.mozilla.org/r/237394/#review244390

This test, like many of the web platform tests that are touched in this patch, did not pass in Chrome, judging visually. The only ones that passed were the ones that rely on a UA-specified line-height: 1 value. I didn't run the wpt test suite routed through other browsers to confirm that the test suite detected the failure in other browsers. My assumption (also not checked) is that these tests have been manfiested off in other browsers as well. The reasons the tests fail are quite endemic (text can't fit in container even if float area is pushing it, etc.)
Comment on attachment 8968700 [details]
Bug 1265342 Part 10: Update a shape-outside circle test to make it more precise and correct.

https://reviewboard.mozilla.org/r/237394/#review244390

Great, thanks.  Checking Chrome is sufficient, I think, since it looks like the tests come for Adobe who IIRC were working with the Blink implementation.
Comment on attachment 8968701 [details]
Bug 1265342 Part 11: Update a shape-outside: image test to supply a correct shape-image-threshold.

https://reviewboard.mozilla.org/r/237396/#review244392
Attachment #8968701 - Flags: review?(dholbert) → review+
Comment on attachment 8967817 [details]
Bug 1265342 Part 12: Change almost all non-polygon shape-outside web-platform tests to expected pass.

https://reviewboard.mozilla.org/r/236496/#review244404
Attachment #8967817 - Flags: review?(dholbert) → review+
Comment on attachment 8968702 [details]
Bug 1265342 Part 4b: Add some logic and asserts to ensure distance field index values and image index values are in-bounds.

https://reviewboard.mozilla.org/r/237398/#review244412

::: commit-message-86eb8:3
(Diff revision 4)
> +The container in this test was sized to 200px, which is not enough to contain
> +the 100px float area, the 100px width of one character of the floated text,
> +and the blank space in-between the two. This change expands the container to

I'm not sure the whitespace actually impacted the layout here...

Based on this change, you're claiming that whitespace was significant (and affected the rendering) at these positions:

 - between a float and the first character of a line of text.
 - immediately before the end of a block (the block with id="test")

But I'm pretty sure whitespace doesn't render at those positions (per spec, and per browser behavior). Testcase: https://jsfiddle.net/oxa4xxcw/

So unless I'm missing something, I don't think the whitespace changes here are necessary...

::: commit-message-86eb8:8
(Diff revision 4)
> +whitespace. Finally, the visible green image was replaced by a colored div
> +element, to avoid antialiasing artifacts from upscaling the image.

Two issues:
 (1) I don't think it's "antialiasing artifacts from upscaling" that you're addressing here - rather, it's the fact that the image has a column of partially transparent pixels at its right edge (which won't match the solid-green pixels in the reference case, regardless of scaling)


 (2) Disregarding those fuzzy pixels for the moment -- the layout aspects of this change (swapping an image for a block) seem to re-break the test's layout for me, in Chrome.  Your first change here (200px-->240px) makes the test start passing for me in Chrome (ignoring fuzzy pixels), and then this change makes it fail (the second green rect shifts much too far to the left and overlaps the first green rect).  If it doesn't cause similar breakage in Firefox, then that might be an interop difference worth investigating as a Firefox or Chrome bug...  But either way, we should ideally leave this test in a state where it passes interoperably, unless we're extremely confident that we're on the right side of the disagreement & we've filed a bug on Chrome.
Attachment #8968702 - Flags: review?(dholbert) → review-
Comment on attachment 8968702 [details]
Bug 1265342 Part 4b: Add some logic and asserts to ensure distance field index values and image index values are in-bounds.

https://reviewboard.mozilla.org/r/237398/#review244412

> Two issues:
>  (1) I don't think it's "antialiasing artifacts from upscaling" that you're addressing here - rather, it's the fact that the image has a column of partially transparent pixels at its right edge (which won't match the solid-green pixels in the reference case, regardless of scaling)
> 
> 
>  (2) Disregarding those fuzzy pixels for the moment -- the layout aspects of this change (swapping an image for a block) seem to re-break the test's layout for me, in Chrome.  Your first change here (200px-->240px) makes the test start passing for me in Chrome (ignoring fuzzy pixels), and then this change makes it fail (the second green rect shifts much too far to the left and overlaps the first green rect).  If it doesn't cause similar breakage in Firefox, then that might be an interop difference worth investigating as a Firefox or Chrome bug...  But either way, we should ideally leave this test in a state where it passes interoperably, unless we're extremely confident that we're on the right side of the disagreement & we've filed a bug on Chrome.

i.e. if I load our current mozilla-central copy of the test[1] in Chrome, and use devtools to adjust #test to have width:240px (the first part of your patch), then it basically passes (ignoring fuzzy pixels).


In contrast, if I load your fully-patched copy of the test[2] in Chrome, it fails more severely.  (After the image loads, I visually see a single fat green rect, with a blue line going through it and a bunch of red to its right.)


[1] https://hg.mozilla.org/mozilla-central/raw-file/cc0d7de218cb/testing/web-platform/tests/css/css-shapes/shape-outside/shape-image/shape-image-024.html
[2] https://hg.mozilla.org/try/raw-file/d4f616fa5c08/testing/web-platform/tests/css/css-shapes/shape-outside/shape-image/shape-image-024.html
Comment on attachment 8968702 [details]
Bug 1265342 Part 4b: Add some logic and asserts to ensure distance field index values and image index values are in-bounds.

https://reviewboard.mozilla.org/r/237398/#review244412

> i.e. if I load our current mozilla-central copy of the test[1] in Chrome, and use devtools to adjust #test to have width:240px (the first part of your patch), then it basically passes (ignoring fuzzy pixels).
> 
> 
> In contrast, if I load your fully-patched copy of the test[2] in Chrome, it fails more severely.  (After the image loads, I visually see a single fat green rect, with a blue line going through it and a bunch of red to its right.)
> 
> 
> [1] https://hg.mozilla.org/mozilla-central/raw-file/cc0d7de218cb/testing/web-platform/tests/css/css-shapes/shape-outside/shape-image/shape-image-024.html
> [2] https://hg.mozilla.org/try/raw-file/d4f616fa5c08/testing/web-platform/tests/css/css-shapes/shape-outside/shape-image/shape-image-024.html

You're right. The whitespace shouldn't matter, and I think it points to an issue with our float: right behavior that is not specific to this patch. I think I will leave this test disabled (and unchanged, for now) since it doesn't pass on Chrome as-is, and it doesn't pass with my "fix" on Chrome either.
Comment on attachment 8967813 [details]
Bug 1265342 Part 6a: Implement shape-margin for shape-outside: inset, for some special cases with shape-margin > 0.

https://reviewboard.mozilla.org/r/236488/#review244352

> Optional nit: looks like this new member-var is set in the constructor's init list & never changes. Probably good to mark it as "const" to make that invariant clearer/enforced?
> 
> (I guess the same goes for existing members mRect and mRadii... I think those can be "const nsRect" and "const UniquePtr" -- feel free/encouraged to upgrade those here, too, if it's just a one-liner tweak.)
> 
> (If you make this change, feel free to do it in a separate followup patch, if that's easier bitrot-wise.)

mRect changes in the Translate() method, so that has to remain non-const, but I made the other changes.

> Hmm, this looks wrong... Inside the loop there, we're misusing the loop variable "i" as if it were an array-index. But in fact it's not an array-index!  In a range-based for loop, the loop variable is an actual entry in the array. (And in this case, the array entries happen to be int-flavored since nscoord is int, or int-ish, so this is an easy mistake to make.)
> 
> So unless I'm misunderstanding, I think this should really be:
>    for (nscoord& r : physicalRadii) {
>      r += aShapeMargin;
>    }
> 
> (I'm a bit surprised this didn't make any tests explode... please be sure we've got code coverage for this "fast path" with equal border-radius-pairs and a nonzero shape-margin.)

Agreed that no WPT tests cover this. More need to be created and I'll try to do a quick batch of them as a final part of this patch.
Attachment #8970258 - Flags: review?(dholbert)
Comment on attachment 8968699 [details]
Bug 1265342 Part 6b: Implement shape-margin for shape-outside: inset, for general case of shape-margin > 0.

https://reviewboard.mozilla.org/r/237392/#review244378

> In your LineLeft and LineRight changes here, it seems you're assuming that having a nonzero shape-margin will necessarily mean we've got non-null "mLogicalTopLeftCorner" etc.
> 
> However, that's not necessarily a valid assumption! If we take one of the special fast paths from 6a (for zero or equal radii), then that assumption does not hold.
> 
> Please fix the logic to account for that.  I think maybe you should replace the "mShapeMargin == 0" check with "if (!mLogicalTopLeftCorner)", or something like that. (just picking a corner at random, since they're all or nothing)
> 
> And please be sure we've got testcases to exercise this scenario.

mShapeMargin is not a stored value of the shape-margin property; it's a calculated value that's only used when needed. There's no path with mShapeMargin == 0 that also creates corners. The assert in Translate() is explicit about this, and I've added similar asserts in LineLeft() and LineRight(). I've also added a comment in the class definition to make this clear. I've added a new RadiiHaveEllipticalCorners function that is used to route to the second constructor and is asserted there. It's true that there are paths in CreateInset() that will inflate the rect and the radii if it's safe to do so, but those paths also route through the simpler constructor that sets mShapeMargin to 0 and does not create the corner objects.
Comment on attachment 8970258 [details]
Bug 1265342 Part 5c: Add some asserts to ensure ellipse distance field index values are in-bounds.

https://reviewboard.mozilla.org/r/239050/#review244754
Attachment #8970258 - Flags: review?(dholbert) → review+
Comment on attachment 8968699 [details]
Bug 1265342 Part 6b: Implement shape-margin for shape-outside: inset, for general case of shape-margin > 0.

https://reviewboard.mozilla.org/r/237392/#review244762

Maybe rename this to AllCornersHaveBalancedRadii() or something like that? (and flip the polarity, i.e. make it "== && == && ..." and add/remove "!" at callsites)

I find the current naming ("RadiiHaveEllipticalCorners") confusing, because:
 - the goal of this function seems to be to *exclude* circles, but in fact circles *are a form of ellipse*.
 - it's ambiguous whether you're checking for at-least-one-corner vs. all-corners being elliptical.

::: layout/generic/nsFloatManager.cpp:1040
(Diff revision 6)
> +  static bool RadiiHaveEllipticalCorners(const nscoord* aRadii) {
> +    return (aRadii[eCornerTopLeftX] != aRadii[eCornerTopLeftY] ||
> +            aRadii[eCornerTopRightX] != aRadii[eCornerTopRightY] ||
> +            aRadii[eCornerBottomLeftX] != aRadii[eCornerBottomLeftY] ||
> +            aRadii[eCornerBottomRightX] != aRadii[eCornerBottomRightY]);
> +  }
Attachment #8968699 - Flags: review?(dholbert) → review+
Comment on attachment 8968699 [details]
Bug 1265342 Part 6b: Implement shape-margin for shape-outside: inset, for general case of shape-margin > 0.

https://reviewboard.mozilla.org/r/237392/#review244762

> 

Sorry, MozReview seems to have burped and dropped my comment text (I think maybe because I didn't hit "save" before hitting "publish" or something).

Here's what I meant to post:
====================
Could you rename this to AllCornersHaveBalancedRadii(), or something along those lines? (and flip the polarity, i.e. make it "== && == && ..." and add/remove "!" at callsites)

I find the current naming ("RadiiHaveEllipticalCorners") confusing, because:
 - the goal of this function seems to be to *exclude* circles, but in fact circles *are a form of ellipse*.
 - it's ambiguous whether you're checking for at-least-one-corner vs. all-corners being elliptical.

I think my suggested rename is clearer on what's specifically being checked for (and it make it clearer at the callsite why this function informs whether we can directly inflate by shape-margin, IMO).
(Ah, looks like the MozReview burp was that I pasted my review nit back into the wrong MozReview box -- the high-level-overview box rather than the "issue" box -- after pulling it out & editing my wording slightly. :) Sorry for the noise. r=me on part 6b with that one nit addressed, anyway.)
(Bikeshedding myself: Maybe "EachCornerHasBalancedRadii" would be better still, to make it clearer that this is an independent per-corner check, rather than an all-corners-having-the-same-value check)
Attachment #8968702 - Flags: review?(dholbert)
Attachment #8970374 - Flags: review?(dholbert)
Comment on attachment 8970374 [details]
Bug 1265342 Part 13: Add two new WPT tests of shape-outside: inset with shape-margin, and update manifests.

https://reviewboard.mozilla.org/r/239160/#review244826
Attachment #8970374 - Flags: review?(dholbert) → review+
Attachment #8968702 - Flags: review?(dholbert) → review?(jfkthame)
Comment on attachment 8968702 [details]
Bug 1265342 Part 4b: Add some logic and asserts to ensure distance field index values and image index values are in-bounds.

https://reviewboard.mozilla.org/r/237398/#review244888

Stealing review back; I'm not quite gone yet. :)

::: layout/generic/nsFloatManager.cpp:1166
(Diff revision 7)
>      // We call this edge area the "expanded region".
>  
> +    // Our expansion amounts need to be the same, and non-negative for our
> +    // math to work, but we don't want to deal with casting them from
> +    // unsigned ints.
> +    int32_t expand = 2;

Could we make this "static const" so it's more clearly/enforcably a constant? And maybe give it a clearer name?

How about something like:
 static const int32_t kExpansionPerSide = 2;

(The "PerSide" aspect makes it clearer why we multiply it by 2 in some places)

::: layout/generic/nsFloatManager.cpp:1225
(Diff revision 7)
>                     row < (dfOffset.y + h) &&
>                     aAlphaPixels[col - dfOffset.x +
>                                  (row - dfOffset.y) * aStride] > threshold) {
>            // Case 2: Image pixel that is opaque.
> +          MOZ_ASSERT(col - dfOffset.x + (row - dfOffset.y) * aStride >= 0 &&
> +            col - dfOffset.x + (row - dfOffset.y) * aStride < (aStride * h),
> +            "Our aAlphaPixels index should be in-bounds.");

This complex arithmetic is duplicated 3x here (once in the condition, twice in the assert).

Might be worth collapsing the two latter copies into a single DebugOnly<int32_t> helper variable (declared/defined just before the assert, and used in the assertion condition), to partially reduce this duplication.

::: layout/generic/nsFloatManager.cpp:1287
(Diff revision 7)
>      // for the interval we'll create for that block axis value (b).
>  
>      // At the end of each row (or column in vertical writing modes),
>      // if any of the other pixels had a value less than usedMargin5X,
>      // we create an interval.
> -    for (int32_t b = bSize - 3; b >= 2; --b) {
> +    for (int32_t b = bSize - (expand + 1); b >= expand; --b) {

Two things:
 (1) Up to you, but IMO "bSize - expand - 1" is slightly clearer here (rather than parenthesizing expand+1) -- that's a closer fit to the backwards-iteration idiom of "for (int i = $LENGTH - 1; i >= 0; --i).  Here, $LENGTH is bSize - expand.

 (2) Regardless of how this initial b value is spelled arithmetically, a comment would be helpful to hint at what it represents. E.g.:
// Note: "bsize-expand-1" is the index of the final row of
// pixels before the trailing expanded region.

 (3) ^^The above goes for the isize loop inside this one, too.

::: layout/generic/nsFloatManager.cpp:1348
(Diff revision 7)
>        }
>  
>        if (iMax != -1) {
>          // Our interval values, iMin, iMax, and b are all calculated from
>          // the expanded region, which is based on the margin rect. To create
> -        // our interval, we have to subtract 2 from (iMin, iMax, and b) to
> +        // our interval, we have to subtract expand from (iMin, iMax, and b)

This sentence is confusing ("subtract expand from") -- it's non-obvious that "expand" is a variable rather than just a word).

Assuming you rename this variable as suggested above, though (and update this comment with the new name), this will presumably become clearer. Hooray! :)
Attachment #8968702 - Flags: review+
Attachment #8968702 - Flags: review?(jfkthame)
Pushed by bwerth@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/5da0c34c9600
Part 0: Servo changes to add shape-margin. r=xidorn
https://hg.mozilla.org/integration/autoland/rev/802aa95a52d9
Part 1: Add shape-margin to style system (Gecko bindings). r=xidorn
https://hg.mozilla.org/integration/autoland/rev/5ee470e75a53
Part 2a: Move interval binary search method into ShapeInfo. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/ce473fa5f1f4
Part 2b: Refactor interval creation for shape-outside:image. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/a8695981eec0
Part 3: Stub in shape-margin for shape-outside: image, by implementing only for shape-margin: 0. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/332382c708e1
Part 4a: Complete the implementation of shape-margin for shape-outside: image (handling shape-margin: > 0). r=dholbert
https://hg.mozilla.org/integration/autoland/rev/64fc631ef122
Part 4b: Add some logic and asserts to ensure distance field index values and image index values are in-bounds. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/53eb7f14463d
Part 5a: Implement shape-margin for shape-outside: circle and ellipse (ellipse only for shape-margin: 0). r=dholbert
https://hg.mozilla.org/integration/autoland/rev/d16f7ff85677
Part 5b: Complete the implementation of shape-margin for ellipse (handling shape-margin: > 0). r=dholbert
https://hg.mozilla.org/integration/autoland/rev/09da0968333a
Part 5c: Add some asserts to ensure ellipse distance field index values are in-bounds. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/c8e3dbc6c729
Part 5d: Move EllipseShapeInfo class definition ahead of RoundedBoxShapeInfo so it can be referenced by rounded boxes. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/048cce2fb99f
Part 6a: Implement shape-margin for shape-outside: inset, for some special cases with shape-margin > 0. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/7109288f10b3
Part 6b: Implement shape-margin for shape-outside: inset, for general case of shape-margin > 0. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/0791ff3d9194
Part 7: Implement shape-margin for shape-outside: shape-box. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/a90aa063b2fd
Part 8: Update shape-outside inset tests to explicitly set a line-height of 1, and not rely on a UA-specific value. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/9b200122014e
Part 9: Update shape-outside ellipse tests to correct invalid use of only one radius parameter. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/83aee3dafe4b
Part 10: Update a shape-outside circle test to make it more precise and correct. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/fd247f38f8a7
Part 11: Update a shape-outside: image test to supply a correct shape-image-threshold. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/62d571916086
Part 12: Change almost all non-polygon shape-outside web-platform tests to expected pass. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/6c80ec2d0398
Part 13: Add two new WPT tests of shape-outside: inset with shape-margin, and update manifests. r=dholbert
Backed out for mochitest-plain-headless failures on layout/style/test/test_first_letter_restrictions.html.

Push: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=6c80ec2d039813b285559e6b853998f566d1fa35
Log: https://treeherder.mozilla.org/logviewer.html#?job_id=175384938&repo=autoland&lineNumber=3196
Flags: needinfo?(bwerth)
Backout by nbeleuzu@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/723a9f786923
Backed out 20 changesets for mochitest-plain-headless failures on layout/style/test/test_first_letter_restrictions.html. CLOSED TREE
Also:
1. opt mochitest failures on layout/style/test/test_first_letter_restrictions.html 
Log: https://treeherder.mozilla.org/logviewer.html#?job_id=175398117&repo=autoland&lineNumber=2716

2. test-verify-wpt failures on css-shapes/shape-outside/
Log:  https://treeherder.mozilla.org/logviewer.html#?job_id=175383959&repo=autoland&lineNumber=778
(In reply to Narcis Beleuzu [:NarcisB] from comment #372) 
> 2. test-verify-wpt failures on css-shapes/shape-outside/
> Log: 
> https://treeherder.mozilla.org/logviewer.
> html#?job_id=175383959&repo=autoland&lineNumber=778

I'm working on the other issues, and I think I have fixes. I will attempt another landing soon. In the meanwhile I want to note that TVw job is Tier 2 and shouldn't block landing. This patch needs to land despite TVw failure because TVw currently fails on macOS for all WPT reftests that use the Ahem font. The test log shows that the failure is due to the font failing to load on macOS, as expected.
Pushed by bwerth@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/7674c13b14de
Part 0: Servo changes to add shape-margin. r=xidorn
https://hg.mozilla.org/integration/autoland/rev/a4d6eb037963
Part 1: Add shape-margin to style system (Gecko bindings). r=xidorn
https://hg.mozilla.org/integration/autoland/rev/9a192430ce29
Part 2a: Move interval binary search method into ShapeInfo. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/ab95312a3db6
Part 2b: Refactor interval creation for shape-outside:image. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/3b2ffd07d8e6
Part 3: Stub in shape-margin for shape-outside: image, by implementing only for shape-margin: 0. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/e5c0631408b2
Part 4a: Complete the implementation of shape-margin for shape-outside: image (handling shape-margin: > 0). r=dholbert
https://hg.mozilla.org/integration/autoland/rev/546464551e64
Part 4b: Add some logic and asserts to ensure distance field index values and image index values are in-bounds. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/c24818678398
Part 5a: Implement shape-margin for shape-outside: circle and ellipse (ellipse only for shape-margin: 0). r=dholbert
https://hg.mozilla.org/integration/autoland/rev/a672de34a369
Part 5b: Complete the implementation of shape-margin for ellipse (handling shape-margin: > 0). r=dholbert
https://hg.mozilla.org/integration/autoland/rev/29c3c172caa0
Part 5c: Add some asserts to ensure ellipse distance field index values are in-bounds. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/64161554cd20
Part 5d: Move EllipseShapeInfo class definition ahead of RoundedBoxShapeInfo so it can be referenced by rounded boxes. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/8d8361873abe
Part 6a: Implement shape-margin for shape-outside: inset, for some special cases with shape-margin > 0. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/93dacd65e009
Part 6b: Implement shape-margin for shape-outside: inset, for general case of shape-margin > 0. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/0cb6c9f7c6db
Part 7: Implement shape-margin for shape-outside: shape-box. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/581d1a27f4ef
Part 8: Update shape-outside inset tests to explicitly set a line-height of 1, and not rely on a UA-specific value. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/36bf72f6946d
Part 9: Update shape-outside ellipse tests to correct invalid use of only one radius parameter. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/185cc5b7b93f
Part 10: Update a shape-outside circle test to make it more precise and correct. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/26ca0097fff7
Part 11: Update a shape-outside: image test to supply a correct shape-image-threshold. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/d5a9b0478b93
Part 12: Change almost all non-polygon shape-outside web-platform tests to expected pass. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/3032f8adbcf1
Part 13: Add two new WPT tests of shape-outside: inset with shape-margin, and update manifests. r=dholbert
Created web-platform-tests PR https://github.com/w3c/web-platform-tests/pull/10648 for changes under testing/web-platform/tests
Whiteboard: [designer-tools] → [designer-tools][wptsync upstream]
Flags: needinfo?(bwerth)
The shape-margin property has already been documented fairly nicely:

https://developer.mozilla.org/en-US/docs/Web/CSS/shape-margin

I have added an entry into our experimental features page to cover it:

https://developer.mozilla.org/en-US/Firefox/Experimental_features#CSS
You need to log in before you can comment on or make changes to this bug.