Last Comment Bug 613659 - implement box-decoration-break: Left/right part of a box-shadow should only be drawn on the first/last continuation of an inline box
: implement box-decoration-break: Left/right part of a box-shadow should only b...
Status: RESOLVED FIXED
: css3, dev-doc-complete, testcase
Product: Core
Classification: Components
Component: Layout: Block and Inline (show other bugs)
: Trunk
: All All
: -- normal with 4 votes (vote)
: mozilla32
Assigned To: Mats Palmgren (vacation)
:
Mentors:
Depends on: 1061094 998792 1031444 1067088 1178575 1235152
Blocks: css3-background 988653 998535 1006326 1025669
  Show dependency treegraph
 
Reported: 2010-11-19 16:25 PST by Mats Palmgren (vacation)
Modified: 2015-12-25 06:01 PST (History)
20 users (show)
mats: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Testcase #1 (inset) (1.83 KB, text/html)
2010-11-21 15:20 PST, Mats Palmgren (vacation)
no flags Details
screenshot #1 (9.61 KB, image/png)
2010-11-21 15:21 PST, Mats Palmgren (vacation)
no flags Details
screenshot #2 (10.46 KB, image/png)
2010-11-21 15:22 PST, Mats Palmgren (vacation)
no flags Details
Testcase #2 (inset) (3.37 KB, text/html)
2010-11-22 10:46 PST, Mats Palmgren (vacation)
no flags Details
screenshot #3 (suggested rendering) (14.14 KB, image/png)
2010-11-22 10:54 PST, Mats Palmgren (vacation)
no flags Details
Testcase #3 (outset) (3.45 KB, text/html)
2010-11-23 14:28 PST, Mats Palmgren (vacation)
no flags Details
Screenshot #4 (suggested rendering for outset) (25.60 KB, image/png)
2010-11-23 14:30 PST, Mats Palmgren (vacation)
no flags Details
Testcase #4 (inset) (4.20 KB, text/html)
2010-11-30 19:31 PST, Mats Palmgren (vacation)
no flags Details
Testcase #5 (outset) (4.04 KB, text/html)
2010-11-30 19:31 PST, Mats Palmgren (vacation)
no flags Details
screenshot #5 (suggested rendering for inset) (16.76 KB, image/png)
2010-11-30 19:34 PST, Mats Palmgren (vacation)
no flags Details
screenshot #6 (suggested rendering for outset) (28.91 KB, image/png)
2010-11-30 19:34 PST, Mats Palmgren (vacation)
no flags Details
Part 1 v1, style system support (21.48 KB, patch)
2010-11-30 19:40 PST, Mats Palmgren (vacation)
no flags Details | Diff | Splinter Review
Part 2 v1, add box-shadow rendering for 'split' (5.82 KB, patch)
2010-11-30 19:45 PST, Mats Palmgren (vacation)
no flags Details | Diff | Splinter Review
Part 3 v1, replace -moz-background-inline-policy (28.13 KB, patch)
2010-12-07 17:14 PST, Mats Palmgren (vacation)
no flags Details | Diff | Splinter Review
Part 4 v1, remove -moz-background-inline-policy (18.99 KB, patch)
2010-12-07 17:15 PST, Mats Palmgren (vacation)
no flags Details | Diff | Splinter Review
part 1. Implement box-decoration-break in the style system. (15.00 KB, patch)
2014-01-22 04:18 PST, Mats Palmgren (vacation)
no flags Details | Diff | Splinter Review
part 2. Implement box-decoration-break layout for border/paddding/margin/box-shadow (23.24 KB, patch)
2014-01-22 04:19 PST, Mats Palmgren (vacation)
no flags Details | Diff | Splinter Review
part 3. Implement box-decoration-break layout for backgrounds. (32.27 KB, patch)
2014-01-22 04:20 PST, Mats Palmgren (vacation)
no flags Details | Diff | Splinter Review
part 4. Remove remaining vestiges of -moz-background-inline-policy. (18.57 KB, patch)
2014-01-22 04:21 PST, Mats Palmgren (vacation)
no flags Details | Diff | Splinter Review
part 5. Reftests for box-decoration-break. (23.23 KB, patch)
2014-01-22 04:22 PST, Mats Palmgren (vacation)
no flags Details | Diff | Splinter Review
simple test (356 bytes, text/html)
2014-01-22 16:07 PST, Cameron McCormack (:heycam)
no flags Details
simple test (heycam's expected rendering) (1.42 KB, image/png)
2014-01-22 16:08 PST, Cameron McCormack (:heycam)
no flags Details
part 1. Implement box-decoration-break in the style system. (13.64 KB, patch)
2014-01-26 10:20 PST, Mats Palmgren (vacation)
cam: review+
Details | Diff | Splinter Review
part 2. Add more detailed GetBorderRadii() method (9.53 KB, patch)
2014-01-26 10:22 PST, Mats Palmgren (vacation)
cam: review+
Details | Diff | Splinter Review
part 3. Implement box-decoration-break layout for border/paddding/margin/box-shadow. (41.22 KB, patch)
2014-01-26 10:23 PST, Mats Palmgren (vacation)
no flags Details | Diff | Splinter Review
part 4. Implement box-decoration-break layout for backgrounds. (39.28 KB, patch)
2014-01-26 10:24 PST, Mats Palmgren (vacation)
cam: review+
Details | Diff | Splinter Review
part 5. Remove remaining vestiges of -moz-background-inline-policy. (18.57 KB, patch)
2014-01-26 10:25 PST, Mats Palmgren (vacation)
no flags Details | Diff | Splinter Review
part 6. Reftests for box-decoration-break. (32.62 KB, patch)
2014-01-26 10:27 PST, Mats Palmgren (vacation)
cam: review+
Details | Diff | Splinter Review
screenshot (121.29 KB, image/png)
2014-01-26 10:41 PST, Mats Palmgren (vacation)
no flags Details
part 5b. Make sure we Save/Restore the gfx context when using Clip. (4.11 KB, patch)
2014-02-03 15:01 PST, Mats Palmgren (vacation)
cam: review+
Details | Diff | Splinter Review
part 2. Add more detailed GetBorderRadii() method (9.64 KB, patch)
2014-02-16 05:41 PST, Mats Palmgren (vacation)
mats: review+
Details | Diff | Splinter Review
part 3. Implement box-decoration-break layout for border/paddding/margin/box-shadow. (43.12 KB, patch)
2014-02-16 05:50 PST, Mats Palmgren (vacation)
no flags Details | Diff | Splinter Review
part 7. Change mode-lines and indentation to 2-space indent. (402.47 KB, patch)
2014-02-16 05:55 PST, Mats Palmgren (vacation)
no flags Details | Diff | Splinter Review
part 3. Implement box-decoration-break layout for border/paddding/margin/box-shadow. (50.35 KB, patch)
2014-03-26 09:14 PDT, Mats Palmgren (vacation)
cam: review+
Details | Diff | Splinter Review
part 4. Implement box-decoration-break layout for backgrounds. (47.20 KB, patch)
2014-03-26 09:25 PDT, Mats Palmgren (vacation)
cam: review+
jmuizelaar: review+
Details | Diff | Splinter Review
part 5. Remove remaining vestiges of -moz-background-inline-policy. (18.40 KB, patch)
2014-03-26 09:27 PDT, Mats Palmgren (vacation)
cam: review+
Details | Diff | Splinter Review
part 6. Reftests for box-decoration-break. (38.89 KB, patch)
2014-03-26 09:31 PDT, Mats Palmgren (vacation)
cam: review+
Details | Diff | Splinter Review
part 7. Change mode-lines and indentation to 2-space indent. (404.11 KB, patch)
2014-03-26 09:34 PDT, Mats Palmgren (vacation)
mats: review+
Details | Diff | Splinter Review
part 7b. Some additional manual changes to property_database.js. (2.61 KB, patch)
2014-03-26 09:36 PDT, Mats Palmgren (vacation)
cam: review+
Details | Diff | Splinter Review
Testcase #8 (1.33 KB, text/html)
2014-04-14 17:30 PDT, Mats Palmgren (vacation)
no flags Details
part 3. Implement box-decoration-break layout for border/paddding/margin/box-shadow. (50.57 KB, patch)
2014-04-14 17:35 PDT, Mats Palmgren (vacation)
mats: review+
Details | Diff | Splinter Review
fix b2g build error (957 bytes, patch)
2014-04-17 06:50 PDT, Mats Palmgren (vacation)
no flags Details | Diff | Splinter Review
part 8, build fix for Windows (2.28 KB, patch)
2014-04-17 16:34 PDT, Mats Palmgren (vacation)
dholbert: review+
Details | Diff | Splinter Review
part 9, bug fix for regression bug 998792 (6.69 KB, patch)
2014-04-21 07:55 PDT, Mats Palmgren (vacation)
roc: review+
Details | Diff | Splinter Review

Description Mats Palmgren (vacation) 2010-11-19 16:25:53 PST
Left/right part of a box-shadow should only be drawn on the first/last continuation of an inline box.

Follow-up from bug 611574.

The 'box-shadow' spec [1] says "If an element has multiple boxes, all of them get drop shadows, but shadows are only drawn where borders would also be drawn; see ‘box-decoration-break’.".

The 'box-decoration-break' spec [2] defines 'slice' as the initial value and
for that value "No box-shadow is drawn at the broken edge"

[1] http://www.w3.org/TR/css3-background/#the-box-shadow
[2] http://www.w3.org/TR/css3-background/#the-box-decoration-break
Comment 1 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2010-11-21 14:26:02 PST
I'm not quite sure what that means. We have to slice up the shadow, or what?
Comment 2 Mats Palmgren (vacation) 2010-11-21 15:20:46 PST
Created attachment 492224 [details]
Testcase #1 (inset)
Comment 3 Mats Palmgren (vacation) 2010-11-21 15:21:36 PST
Created attachment 492225 [details]
screenshot #1
Comment 4 Mats Palmgren (vacation) 2010-11-21 15:22:03 PST
Created attachment 492226 [details]
screenshot #2
Comment 5 Mats Palmgren (vacation) 2010-11-21 15:28:46 PST
(In reply to comment #1)
> I'm not quite sure what that means. We have to slice up the shadow, or what?

Yes, I think "6.1. The ‘box-decoration-break’ property" is quite clear.
Our current rendering of box shadows is what 'clone' should do.
I've attached a couple of screenshots of possible renderings for 'slice'.
Comment 6 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2010-11-21 16:51:32 PST
Note that we implement an old version of box-decoration-break as -moz-background-inline-policy.
Comment 7 Mats Palmgren (vacation) 2010-11-22 10:46:21 PST
Created attachment 492374 [details]
Testcase #2 (inset)
Comment 8 Mats Palmgren (vacation) 2010-11-22 10:54:04 PST
Created attachment 492376 [details]
screenshot #3 (suggested rendering)

This is the correct rendering for the testcase.  In the earlier screenshots
the blurring was affected by the skipped side.
Comment 9 Mats Palmgren (vacation) 2010-11-23 14:28:49 PST
Created attachment 492797 [details]
Testcase #3 (outset)
Comment 10 Mats Palmgren (vacation) 2010-11-23 14:30:01 PST
Created attachment 492798 [details]
Screenshot #4 (suggested rendering for outset)
Comment 11 Mats Palmgren (vacation) 2010-11-30 19:31:04 PST
Created attachment 494286 [details]
Testcase #4 (inset)
Comment 12 Mats Palmgren (vacation) 2010-11-30 19:31:46 PST
Created attachment 494288 [details]
Testcase #5 (outset)
Comment 13 Mats Palmgren (vacation) 2010-11-30 19:34:16 PST
Created attachment 494289 [details]
screenshot #5 (suggested rendering for inset)
Comment 14 Mats Palmgren (vacation) 2010-11-30 19:34:57 PST
Created attachment 494290 [details]
screenshot #6 (suggested rendering for outset)
Comment 15 Mats Palmgren (vacation) 2010-11-30 19:40:08 PST
Created attachment 494291 [details] [diff] [review]
Part 1 v1, style system support

This part style system support for the 'box-decoration-break' CSS property
with values 'split' (initial) and 'clone'.
Comment 16 Mats Palmgren (vacation) 2010-11-30 19:45:01 PST
Created attachment 494293 [details] [diff] [review]
Part 2 v1, add box-shadow rendering for 'split'

This part makes the current box-shadow rendering be that of 'clone'
and implements the new rendering for 'split' (see screenshots).
Comment 17 Mats Palmgren (vacation) 2010-11-30 20:26:22 PST
Fwiw, all the major UAs [1] does the 'clone' rendering for box-shadow
as the default, contrary to what the spec says.
http://www.w3.org/TR/css3-background/#the-box-shadow
http://www.w3.org/TR/css3-background/#the-box-decoration-break

[1] Opera 11 beta 1, IE9 beta, nightly builds of Chrome and Safari,
    and also Firefox so far
Comment 18 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2010-11-30 20:43:55 PST
(In reply to comment #17)
> Fwiw, all the major UAs [1] does the 'clone' rendering for box-shadow
> as the default, contrary to what the spec says.
> http://www.w3.org/TR/css3-background/#the-box-shadow
> http://www.w3.org/TR/css3-background/#the-box-decoration-break
> 
> [1] Opera 11 beta 1, IE9 beta, nightly builds of Chrome and Safari,
>     and also Firefox so far

I don't think we need to worry about compat here; box-shadow is pretty new and probably rarely used on inlines.

However, we should be removing -moz-background-inline-policy and changing the code that looks at it to look at box-decoration-break.  (See comment 6.)  That's probably best done as additional patches on top (in the reverse order:  change the code to look at the new property, and then remove the old one.)
Comment 19 Mats Palmgren (vacation) 2010-12-04 21:53:45 PST
Ok, so 'continuous' corresponds to 'split' and 'each-box' to 'clone',
but what should I do with 'bounding-box'?
https://developer.mozilla.org/en/CSS/-moz-background-inline-policy
Comment 20 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2010-12-05 12:13:03 PST
Create a -moz-bounding-box value for box-decoration-break, I guess.
Comment 21 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2010-12-05 12:22:06 PST
I think just remove the code for it, actually.
Comment 22 Mats Palmgren (vacation) 2010-12-07 17:14:14 PST
Created attachment 496030 [details] [diff] [review]
Part 3 v1, replace -moz-background-inline-policy

Switch from -moz-background-inline-policy to box-decoration-break and
remove uses of -moz-background-inline-policy:bounding-box
Comment 23 Mats Palmgren (vacation) 2010-12-07 17:15:23 PST
Created attachment 496031 [details] [diff] [review]
Part 4 v1, remove -moz-background-inline-policy

Remove '-moz-background-inline-policy' property, 'bounding-box' and
'each-box' keywords ('continuous' is still in use by 'speak-numeral').
Comment 24 Lea Verou 2011-04-13 07:10:53 PDT
What happened with this? Neither box-decoration-break, nor -moz-box-decoration-break are in 4.2pre1
Comment 25 Jet Villegas (:jet) 2011-11-17 18:31:38 PST
Please update the patches to reflect the latest code.
Comment 26 Tim Nguyen :ntim (use needinfo?) 2013-12-30 04:36:43 PST
Please rebase your patches then ask for review so Firefox can get this implemented.
Comment 27 Mats Palmgren (vacation) 2014-01-22 04:18:53 PST
Created attachment 8363601 [details] [diff] [review]
part 1. Implement box-decoration-break in the style system.
Comment 28 Mats Palmgren (vacation) 2014-01-22 04:19:53 PST
Created attachment 8363602 [details] [diff] [review]
part 2. Implement box-decoration-break layout for border/paddding/margin/box-shadow
Comment 29 Mats Palmgren (vacation) 2014-01-22 04:20:45 PST
Created attachment 8363604 [details] [diff] [review]
part 3. Implement box-decoration-break layout for backgrounds.
Comment 30 Mats Palmgren (vacation) 2014-01-22 04:21:33 PST
Created attachment 8363605 [details] [diff] [review]
part 4. Remove remaining vestiges of -moz-background-inline-policy.
Comment 31 Mats Palmgren (vacation) 2014-01-22 04:22:59 PST
Created attachment 8363606 [details] [diff] [review]
part 5. Reftests for box-decoration-break.

https://tbpl.mozilla.org/?tree=Try&rev=827eab9c6f4c
https://tbpl.mozilla.org/?tree=Try&rev=833a6ff1912e
Comment 32 Cameron McCormack (:heycam) 2014-01-22 16:05:34 PST
Mats, I think some of your renderings aren't what I'd expect from reading the spec text.  In http://dev.w3.org/csswg/css-break/#break-decoration it says that for 'slice':

  The effect is as though the element were rendered with no breaks present, and then sliced by the
  breaks afterward.
Comment 33 Cameron McCormack (:heycam) 2014-01-22 16:07:11 PST
Created attachment 8364041 [details]
simple test

So for this test case...
Comment 34 Cameron McCormack (:heycam) 2014-01-22 16:08:11 PST
Created attachment 8364042 [details]
simple test (heycam's expected rendering)

...I would expect it to render like this.

Can you explain from the spec how you get your slicing behaviour?
Comment 35 Mats Palmgren (vacation) 2014-01-23 01:57:00 PST
I think you're right.  I must have misunderstood that part somehow, but now that I read it
it seems quite clear.
Comment 36 Mats Palmgren (vacation) 2014-01-26 10:20:58 PST
Created attachment 8365688 [details] [diff] [review]
part 1. Implement box-decoration-break in the style system.
Comment 37 Mats Palmgren (vacation) 2014-01-26 10:22:46 PST
Created attachment 8365690 [details] [diff] [review]
part 2. Add more detailed GetBorderRadii() method
Comment 38 Mats Palmgren (vacation) 2014-01-26 10:23:36 PST
Created attachment 8365691 [details] [diff] [review]
part 3. Implement box-decoration-break layout for border/paddding/margin/box-shadow.
Comment 39 Mats Palmgren (vacation) 2014-01-26 10:24:42 PST
Created attachment 8365692 [details] [diff] [review]
part 4. Implement box-decoration-break layout for backgrounds.
Comment 40 Mats Palmgren (vacation) 2014-01-26 10:25:24 PST
Created attachment 8365693 [details] [diff] [review]
part 5. Remove remaining vestiges of -moz-background-inline-policy.
Comment 41 Mats Palmgren (vacation) 2014-01-26 10:27:33 PST
Created attachment 8365695 [details] [diff] [review]
part 6. Reftests for box-decoration-break.

https://tbpl.mozilla.org/?tree=Try&rev=2f2a0d6734d0
Comment 42 Mats Palmgren (vacation) 2014-01-26 10:41:24 PST
Created attachment 8365698 [details]
screenshot

So, just to clarify, I have now changed our border/background/box-shadow
rendering from the old "skip sides" method to the "visual clipping"
algorithm that is specified by the spec.  Here's a screenshot to illustrate.
Note the curved borders on the middle continuations in the bottom four
tests.  And the sliced inline "iM" in the upper right.
Comment 43 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2014-01-27 10:24:57 PST
FWIW, how box-decoration-break interacts with some other properties was discussed in today's CSS WG meeting:

[2014-01-27 10:23:06 -0800] <TabAtkins> krit: clip-path/clip/mask same as background wrt box-decoration-break
[2014-01-27 10:23:19 -0800] <TabAtkins> krit: mask-box same behavior as border-image wrt box-decoration-break
[2014-01-27 10:23:52 -0800] <TabAtkins> fantasai: So b-d-b controls masking/clipping as well as backgrounds.
[2014-01-27 10:24:18 -0800] <TabAtkins> smfr: Are there any cases where you'd want to use different b-d-b behaviors between the properties?
[2014-01-27 10:24:21 -0800] <TabAtkins> krit: I think not.
[2014-01-27 10:24:45 -0800] <TabAtkins> RESOLVED: clipping/masking properties respond to box-decoration-break as proposed above.
Comment 44 Mats Palmgren (vacation) 2014-02-03 05:46:16 PST
Cameron, I've found a minor bug in part 4 which I'm working on...
I misread this line:
http://hg.mozilla.org/mozilla-central/annotate/01a67765dbc7/layout/base/nsCSSRendering.cpp#l2492
as actually Save()-ing the gfx context at that point, but it's not, so my
added Clip() later in the same method lingered on and cause a reftest
failure on Android.

It's a minor tweak to get it right and shouldn't affect the review much.
Comment 45 Mats Palmgren (vacation) 2014-02-03 15:01:03 PST
Created attachment 8369701 [details] [diff] [review]
part 5b.  Make sure we Save/Restore the gfx context when using Clip.

This goes on top of part 5.  I'll fold it before landing but I figured
it might be easier to review it as a separate patch.  There's only one
use of Reset() in the tree and that API doesn't really fit this code.
It makes more sense to have a EnsureSaved() method that you can call
to do the "delayed Save" in code paths that need it.
Comment 46 Cameron McCormack (:heycam) 2014-02-05 20:28:54 PST
Comment on attachment 8365688 [details] [diff] [review]
part 1. Implement box-decoration-break in the style system.

Review of attachment 8365688 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/style/test/property_database.js
@@ +4892,5 @@
> +		initial_values: [ "slice" ],
> +		other_values: [ "clone" ],
> +		invalid_values: [ "auto",  "none",  "1px" ]
> +	};
> +}

Use two spaces instead of tabs.
Comment 47 Cameron McCormack (:heycam) 2014-02-05 20:43:05 PST
Comment on attachment 8365690 [details] [diff] [review]
part 2. Add more detailed GetBorderRadii() method

Review of attachment 8365690 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/generic/nsGfxScrollFrame.cpp
@@ +4155,2 @@
>  {
> +  if (!mOuter->nsContainerFrame::GetBorderRadii(aRadii, aSkipSides, aFrameSize, aBorderArea))

Wrap to fit 80 columns.
Comment 48 Cameron McCormack (:heycam) 2014-02-05 23:27:47 PST
Comment on attachment 8365691 [details] [diff] [review]
part 3. Implement box-decoration-break layout for border/paddding/margin/box-shadow.

Review of attachment 8365691 [details] [diff] [review]:
-----------------------------------------------------------------

Removing r? as there are some questions I have below.

::: layout/base/nsCSSRendering.cpp
@@ +60,5 @@
>  
>  static int gFrameTreeLockCount = 0;
>  
> +static void
> +ApplySkipSides(int aSkipSides, nsMargin* aMargin)

To be consistent with nsIFrame::ApplySkipSides, make it take (nsMargin& aMargin, int aSkipSides)?

@@ +79,5 @@
> +                      bool* aHaveRoundedCorners)
> +{
> +  nsRect borderArea = aBorderArea;
> +  nscoord radii[8];
> +  if (aForFrame->StyleBorder()->mBoxDecorationBreak == NS_STYLE_BOX_DECORATION_BREAK_SLICE) {

Wrap line to avoid going over 80 columns.

@@ +84,5 @@
> +    nsLayoutUtils::JoinBoxesForSlice(borderArea, aForFrame);
> +  }
> +  nsSize sz = borderArea.Size();
> +  nsSize frameSize = aForFrame->GetSize();
> +  if (&aBorder == aForFrame->StyleBorder() &&

I know this is in the old code, but can you explain why we're comparing the style struct here, and why we do different things?

@@ +85,5 @@
> +  }
> +  nsSize sz = borderArea.Size();
> +  nsSize frameSize = aForFrame->GetSize();
> +  if (&aBorder == aForFrame->StyleBorder() &&
> +      frameSize == aBorderArea.Size()) {

Use sz instead of aBorderArea.Size() since we just got it above.

@@ +92,5 @@
> +    *aHaveRoundedCorners =
> +      nsIFrame::ComputeBorderRadii(aBorder.mBorderRadius, frameSize, sz, 0, radii);
> +  }
> +  if (*aHaveRoundedCorners) {
> +    auto a2d = aForFrame->PresContext()->AppUnitsPerDevPixel();

"a2d" sounds like it's the number to multiply by to convert an app unit value to a dev pixel, but it's really the other way around.

@@ +95,5 @@
> +  if (*aHaveRoundedCorners) {
> +    auto a2d = aForFrame->PresContext()->AppUnitsPerDevPixel();
> +    nsCSSRendering::ComputePixelRadii(radii, a2d, aBgRadii);
> +  } else {
> +    return aBorderArea;

Why do we return the un-joined border area if there was no border radius?  Is this function only called when working out how to draw the radii, and nothing else (like background slicing)?

@@ +97,5 @@
> +    nsCSSRendering::ComputePixelRadii(radii, a2d, aBgRadii);
> +  } else {
> +    return aBorderArea;
> +  }
> +  return borderArea;

Move this into the |if (*aHaveRoundedCorners)| block, then move the |return aBorderArea| outside the else.

@@ +538,5 @@
> +  // Compute the outermost boundary of the area that might be painted.
> +  // Same coordinate space as aBorderArea & aBGClipRect.
> +  gfxCornerSizes bgRadii;
> +  bool haveRoundedCorners;
> +  nsRect borderArea = ::GetBorderAreaAndRadii(aForFrame, aStyleBorder, aBorderArea,

Wrap line to avoid going over 80 columns, and no need for the "::".

@@ +541,5 @@
> +  bool haveRoundedCorners;
> +  nsRect borderArea = ::GetBorderAreaAndRadii(aForFrame, aStyleBorder, aBorderArea,
> +                                              &bgRadii, &haveRoundedCorners);
> +  if (!haveRoundedCorners) {
> +    ::ApplySkipSides(aSkipSides, &border);

Why do we do this now only if we don't have border radius?

@@ +551,5 @@
>  
>    // Get our conversion values
>    nscoord twipsPerPixel = aPresContext->DevPixelsToAppUnits(1);
>  
>    // convert outer and inner rects

Presumably a comment left over from a while ago.  (Did the inner rect used to be the content box or something?)  Anyway, please update the comment.  Also can you give oRect a better name?

@@ +594,5 @@
>  
> +  //SF ("bgRadii: %f %f %f %f\n", bgRadii[0], bgRadii[1], bgRadii[2], bgRadii[3]);
> +
> +  if (haveRoundedCorners &&
> +      aForFrame->StyleBorder()->mBoxDecorationBreak == NS_STYLE_BOX_DECORATION_BREAK_SLICE) {

wrap line

@@ +1073,5 @@
> +    hasBorderRadius = true; // we'll update this below
> +  }
> +
> +  nsRect frameRect =
> +    nativeTheme ? aForFrame->GetVisualOverflowRectRelativeToSelf() + aFrameArea.TopLeft() : aFrameArea;

Wrap this line while you're moving it (and the one below it).

@@ +1086,5 @@
>      nscoord twipsRadii[8];
>      NS_ASSERTION(aFrameArea.Size() == aForFrame->VisualBorderRectRelativeToSelf().Size(),
>                   "unexpected size");
> +    nsSize sz = frameRect.Size();
> +    hasBorderRadius = aForFrame->GetBorderRadii(twipsRadii, 0, sz, sz);

Can you explain why we don't do like the if statement in GetBorderAreaAndRadii and call ComputeBorderRadii instead of GetBorderRadii sometimes?

@@ +1291,5 @@
> +  NS_ASSERTION(aForFrame->GetType() == nsGkAtoms::fieldSetFrame ||
> +               aFrameArea.Size() == aForFrame->GetSize(), "unexpected size");
> +
> +  nsRect frameRect = aFrameArea;
> +  if (aForFrame->StyleBorder()->mBoxDecorationBreak == NS_STYLE_BOX_DECORATION_BREAK_SLICE) {

wrap line

::: layout/base/nsCSSRenderingBorders.cpp
@@ -119,5 @@
>                                           const gfxFloat* aBorderWidths,
>                                           gfxCornerSizes& aBorderRadii,
>                                           const nscolor* aBorderColors,
>                                           nsBorderColors* const* aCompositeColors,
> -                                         int aSkipSides,

Would have preferred a separate patch to remove the currently unused aSkipSides, but don't worry about it now.

::: layout/base/nsDisplayList.cpp
@@ -2673,5 @@
>  
>    PROFILER_LABEL("nsDisplayBoxShadowOuter", "Paint");
>    for (uint32_t i = 0; i < rects.Length(); ++i) {
>      aCtx->PushState();
> -    aCtx->IntersectClip(rects[i]);

I guess this is no longer necessary since we now clip in PaintBoxShadowOuter?

::: layout/base/nsLayoutUtils.cpp
@@ +4051,5 @@
>    return contentBottom;
>  }
>  
> +/* static */ void
> +nsLayoutUtils::JoinBoxesForSlice(nsRect& aFrameArea, nsIFrame* aFrame)

Wrap to avoid going over 80 columns in a few different places in this function.

::: layout/generic/nsContainerFrame.cpp
@@ +868,5 @@
> +      clonePBM = startPBM;
> +    }
> +  }
> +
> +  nscoord endPBM = 

trailing white space

@@ +873,5 @@
> +    // clamp negative calc() to 0
> +    std::max(GetCoord(stylePadding->mPadding.Get(endSide), 0), 0) +
> +    styleBorder->GetComputedBorderWidth(endSide) +
> +    GetCoord(styleMargin->mMargin.Get(endSide), 0);
> +  if (MOZ_UNLIKELY(!sliceBreak)) {

Make this MOZ_LIKELY(sliceBreak) to match the earlier one (and to remove the double negative)?

@@ +883,5 @@
>  
>    nsContainerFrame *lastInFlow;
>    for (nsContainerFrame *nif = this; nif;
>         nif = static_cast<nsContainerFrame*>(nif->GetNextInFlow())) {
> +    if (aData->currentLine == 0) {

As I'm not super familiar with this code: where does aData->currentLine get reset to zero when we reach a next in flow that is on the next line?

Also why don't we add endPBM on to each of these continuations in the box-decoration-break:clone case?

Can you add some (or add to the existing) comments in this function explaining how we want to include the PBM on each fragment when in clone mode.

@@ +914,5 @@
>    // last line.
>    // We reached the last-in-flow, but it might have a next bidi
>    // continuation, in which case that continuation should handle
>    // the endSide border.
> +  if (MOZ_LIKELY(!lastInFlow->GetNextContinuation() && sliceBreak)) {

Should the |!lastInFlow->GetNextContinuation()| bit really be in the MOZ_LIKELY?

::: layout/generic/nsImageFrame.cpp
@@ +1778,5 @@
>  int
>  nsImageFrame::GetSkipSides(const nsHTMLReflowState* aReflowState) const
>  {
> +  if (MOZ_UNLIKELY(StyleBorder()->mBoxDecorationBreak ==
> +                     NS_STYLE_BOX_DECORATION_BREAK_CLONE)) {

Make this |LIKELY ... == NS_STYLE_BOX_DECORATION_BREAK_SLICE| to match the previous MOZ_LIKELY calls in this patch.  Same for the remaining MOZ_UNLIKELYs.

::: layout/generic/nsInlineFrame.cpp
@@ +116,5 @@
>      !IsMarginZero(margin->mMargin.GetLeft());
>    if (haveLeft || haveRight) {
> +    if ((GetStateBits() & NS_FRAME_IS_SPECIAL) &&
> +        StyleBorder()->mBoxDecorationBreak ==
> +          NS_STYLE_BOX_DECORATION_BREAK_SLICE) {

Can you explain this bit?  Does this just make sure in clone mode we take into account the PBM that would be at the start/end of an inline that was split by a block?  A comment would be good.

@@ +496,5 @@
>    // Don't offset by our start borderpadding if we have a prev continuation or
>    // if we're in a part of an {ib} split other than the first one.
> +  if ((!GetPrevContinuation() && !FrameIsNonFirstInIBSplit()) ||
> +      MOZ_UNLIKELY(StyleBorder()->mBoxDecorationBreak ==
> +                     NS_STYLE_BOX_DECORATION_BREAK_CLONE)) {

Please update the comment to state how box-decoration-break affects the "Don't offset by ...".

@@ +660,5 @@
>    // continuation or if we're in a part of an {ib} split other than the first
>    // one.
> +  if ((!GetPrevContinuation() && !FrameIsNonFirstInIBSplit()) ||
> +      MOZ_UNLIKELY(StyleBorder()->mBoxDecorationBreak ==
> +                     NS_STYLE_BOX_DECORATION_BREAK_CLONE)) {

Again update the comment please.

@@ +675,5 @@
>     */
> +  if ((NS_FRAME_IS_COMPLETE(aStatus) && !LastInFlow()->GetNextContinuation() &&
> +       !FrameIsNonLastInIBSplit()) ||
> +      MOZ_UNLIKELY(StyleBorder()->mBoxDecorationBreak ==
> +                     NS_STYLE_BOX_DECORATION_BREAK_CLONE)) {

Given you check for NS_STYLE_BOX_DECORATION_BREAK_CLONE a few times in this function, maybe store it in a local variable to avoid cluttering up these conditions.

::: layout/generic/nsLineLayout.cpp
@@ +1074,5 @@
>    // the start margin for later continuations anyway.
> +  if ((pfd->mFrame->GetPrevContinuation() ||
> +       pfd->mFrame->FrameIsNonFirstInIBSplit()) &&
> +      aReflowState.mStyleBorder->mBoxDecorationBreak ==
> +        NS_STYLE_BOX_DECORATION_BREAK_SLICE) {

Please update the comment.

@@ +1157,5 @@
>           pfd->mFrame->LastInFlow()->GetNextContinuation() ||
>           pfd->mFrame->FrameIsNonLastInIBSplit())
> +        && !pfd->GetFlag(PFD_ISLETTERFRAME)
> +        && pfd->mFrame->StyleBorder()->mBoxDecorationBreak ==
> +             NS_STYLE_BOX_DECORATION_BREAK_SLICE) {

Update the comment above.
Comment 49 Mats Palmgren (vacation) 2014-02-15 10:05:36 PST
(In reply to Cameron McCormack (:heycam) from comment #46)
> ::: layout/style/test/property_database.js
> Use two spaces instead of tabs.

This file actually requests TAB-indentation for some reason:
http://mxr.mozilla.org/mozilla-central/source/layout/style/test/property_database.js#1
Should I change that and re-indent the entire file?
Comment 50 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2014-02-15 14:00:51 PST
Yes, but in a separate, whitespace-only (plus modeline) changeset.
Comment 51 Mats Palmgren (vacation) 2014-02-15 14:31:25 PST
(In reply to Cameron McCormack (:heycam) from comment #48)
> ::: layout/base/nsCSSRendering.cpp
> > +  if (&aBorder == aForFrame->StyleBorder() &&
> 
> I know this is in the old code, but can you explain why we're comparing the
> style struct here, and why we do different things?

I don't know, it's pretty cryptic to me too.
It looks like dbaron added this in bug 459144, maybe he knows?
http://hg.mozilla.org/mozilla-central/rev/a118b42abad1
Comment 52 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2014-02-15 14:54:48 PST
So that we don't use the nsIFrame method in cases where the caller is passing in an alternative nsStyleBorder*, which we do for internal table parts and for painting the canvas background.  (Note that bug 921341 is likely to change that code soon as well.)

(It would probably make more sense to refactor that so all the callers that don't want to override pass nullptr.)
Comment 53 Mats Palmgren (vacation) 2014-02-16 05:41:15 PST
Created attachment 8376817 [details] [diff] [review]
part 2. Add more detailed GetBorderRadii() method

I just moved the aSkipSides param for GetBorderRadii like so:

  virtual bool GetBorderRadii(const nsSize& aFrameSize,
                              const nsSize& aBorderArea,
                              int aSkipSides,
                              nscoord aRadii[8]) const;

so that it takes those params in the same order as
the existing ComputeBorderRadii.

No need for another review, unless you wish.
Comment 54 Mats Palmgren (vacation) 2014-02-16 05:50:37 PST
Created attachment 8376819 [details] [diff] [review]
part 3. Implement box-decoration-break layout for border/paddding/margin/box-shadow.

(In reply to Cameron McCormack (:heycam) from comment #48)
> > +static void
> > +ApplySkipSides(int aSkipSides, nsMargin* aMargin)
> 
> To be consistent with nsIFrame::ApplySkipSides, make it take (nsMargin&
> aMargin, int aSkipSides)?

I think the convention in Layout code is to use T* for [in-]out-params and
usually they go last.  Maybe you meant nsIFrame::GetSkipSides that have
a aSkipSides param? -- I've changed that instead so that the params are
now in the same order as nsIFrame::ComputeBorderRadii, so the out-param
aRadii comes last. (in part 2)

nsIFrame::ApplySkipSides has this signature:
  void ApplySkipSides(nsMargin& aMargin,
                      const nsHTMLReflowState* aReflowState = nullptr) const;
I suppose we should change that to nsMargin* as well.
That seems like a separate bug though.

> Wrap line to avoid going over 80 columns.

I thought the consensus on the recent code style thread was that we
could use 99 if it would hurt readability to wrap it to 80.
Perhaps it was wishful thinking on my part... :-)

That said, I have wrapped the lines you requested.

> > +  nsSize sz = borderArea.Size();
> > +  nsSize frameSize = aForFrame->GetSize();
> > +  if (&aBorder == aForFrame->StyleBorder() &&
> > +      frameSize == aBorderArea.Size()) {
> 
> Use sz instead of aBorderArea.Size() since we just got it above.

Can't do that since borderArea != aBorderArea at that point.
So I take that as evidence that the JoinBoxesForSlice signature is not
clear enough that it modifies the nsRect& it takes.  Mea culpa, I should
have thought of the rule I just mentioned above.  So I've made it take
an nsRect* instead.

> "a2d" sounds like it's the number to multiply by to convert an app unit
> value to a dev pixel, but it's really the other way around.

Indeed, I made it "d2a" instead.

> @@ +95,5 @@
> > +  if (*aHaveRoundedCorners) {
> > +    auto a2d = aForFrame->PresContext()->AppUnitsPerDevPixel();
> > +    nsCSSRendering::ComputePixelRadii(radii, a2d, aBgRadii);
> > +  } else {
> > +    return aBorderArea;
> 
> Why do we return the un-joined border area if there was no border radius?

I figured I should keep the existing ApplySkipSides drawing path in that
(common) case, since it should be faster than drawing the whole area with
a clip.


> Move this into the |if (*aHaveRoundedCorners)| block, then move the |return
> aBorderArea| outside the else.

Fixed.

> > +  nsRect borderArea = ::GetBorderAreaAndRadii(
> 
> Wrap line to avoid going over 80 columns, and no need for the "::".

I use "::" here as a hint to the reader that this is not a method call
but rather a global function, and usually a "static" in the same file.
Is that not a useful hint?

> > +  if (!haveRoundedCorners) {
> > +    ::ApplySkipSides(aSkipSides, &border);
> 
> Why do we do this now only if we don't have border radius?

See earlier answer.

> Presumably a comment left over from a while ago.  (Did the inner rect used
> to be the content box or something?)  Anyway, please update the comment. 
> Also can you give oRect a better name?

I cleaned up the code a bit here, and renamed oRect -> borderAreaPx.

> ::: layout/base/nsDisplayList.cpp
> > -    aCtx->IntersectClip(rects[i]);
> 
> I guess this is no longer necessary since we now clip in PaintBoxShadowOuter?

Correct.


> ::: layout/generic/nsContainerFrame.cpp
> > +  nscoord endPBM = 
> 
> trailing white space

Fixed.

> > +  if (MOZ_UNLIKELY(!sliceBreak)) {
> 
> Make this MOZ_LIKELY(sliceBreak) to match the earlier one (and to remove the
> double negative)?

Can't do that since it would change the logic.  MOZ_UNLIKELY is just a hint
to the compiler, it doesn't affect the expression.

> > +    if (aData->currentLine == 0) {
> 
> As I'm not super familiar with this code: where does aData->currentLine get
> reset to zero when we reach a next in flow that is on the next line?

grep -r 'ForceBreak(' layout/
should give you a good overview I think.

> Also why don't we add endPBM on to each of these continuations in the
> box-decoration-break:clone case?

I do that actually, that's what 'clonePBM' is about, its value is
startPBM + endPBM for box-decoration-break:clone, and it's added to
each line.  For :slice it's zero.

> Can you add some (or add to the existing) comments in this function
> explaining how we want to include the PBM on each fragment when in clone
> mode.

I added:
  // For box-decoration-break:clone we setup clonePBM = startPBM + endPBM and
  // add that to each line.  For box-decoration-break:slice clonePBM is zero.

> > +  if (MOZ_LIKELY(!lastInFlow->GetNextContinuation() && sliceBreak)) {
> 
> Should the |!lastInFlow->GetNextContinuation()| bit really be in the
> MOZ_LIKELY?

Yes, it's very unlikely that the last-in-flow have a next-continuation.
It only happens in some rare bidi-cases AFAIK.


> ::: layout/generic/nsImageFrame.cpp
> >  nsImageFrame::GetSkipSides(const nsHTMLReflowState* aReflowState) const
> >  {
> > +  if (MOZ_UNLIKELY(StyleBorder()->mBoxDecorationBreak ==
> > +                     NS_STYLE_BOX_DECORATION_BREAK_CLONE)) {
> 
> Make this |LIKELY ... == NS_STYLE_BOX_DECORATION_BREAK_SLICE| to match the
> previous MOZ_LIKELY calls in this patch.  Same for the remaining
> MOZ_UNLIKELYs.

Again, that would change the logic.  I think the code is clearer as written,
i.e. with the CLONE case as an early return.

> ::: layout/generic/nsInlineFrame.cpp
> >    if (haveLeft || haveRight) {
> > +    if ((GetStateBits() & NS_FRAME_IS_SPECIAL) &&
> > +        StyleBorder()->mBoxDecorationBreak ==
> > +          NS_STYLE_BOX_DECORATION_BREAK_SLICE) {
> 
> Can you explain this bit?  Does this just make sure in clone mode we take
> into account the PBM that would be at the start/end of an inline that was
> split by a block?  A comment would be good.

This is the IsSelfEmpty() method.  Since "haveLeft || haveRight" is true
and all continuations have that border/padding/margin in the b-d-b:clone case
it follows that this frame isn't empty, so we should skip this if-block and
just return false.  I've added a code comment to explain it.

> Please update the comment to state how box-decoration-break affects the
> "Don't offset by ...".

Fixed.

> Again update the comment please.

Fixed.

> Given you check for NS_STYLE_BOX_DECORATION_BREAK_CLONE a few times in this
> function, maybe store it in a local variable to avoid cluttering up these
> conditions.

Fixed.

> ::: layout/generic/nsLineLayout.cpp
> Please update the comment.

Fixed.

> Update the comment above.

Fixed.
Comment 55 Mats Palmgren (vacation) 2014-02-16 05:55:15 PST
Created attachment 8376820 [details] [diff] [review]
part 7. Change mode-lines and indentation to 2-space indent.

Note: there is one TAB in one of the test values for 'stroke-dasharray'.
I kept that since I think it's part of the test.
Comment 56 Mats Palmgren (vacation) 2014-02-16 05:56:27 PST
Test builds if you need them:
https://tbpl.mozilla.org/?tree=Try&rev=865a7c888639
Comment 57 Cameron McCormack (:heycam) 2014-02-17 14:30:17 PST
(In reply to Mats Palmgren (:mats) from comment #54)
> (In reply to Cameron McCormack (:heycam) from comment #48)
> > > +static void
> > > +ApplySkipSides(int aSkipSides, nsMargin* aMargin)
> > 
> > To be consistent with nsIFrame::ApplySkipSides, make it take (nsMargin&
> > aMargin, int aSkipSides)?
> 
> I think the convention in Layout code is to use T* for [in-]out-params and
> usually they go last.  Maybe you meant nsIFrame::GetSkipSides that have
> a aSkipSides param? -- I've changed that instead so that the params are
> now in the same order as nsIFrame::ComputeBorderRadii, so the out-param
> aRadii comes last. (in part 2)
> 
> nsIFrame::ApplySkipSides has this signature:
>   void ApplySkipSides(nsMargin& aMargin,
>                       const nsHTMLReflowState* aReflowState = nullptr) const;
> I suppose we should change that to nsMargin* as well.
> That seems like a separate bug though.

I did mean nsIFrame::ApplySkipSides I think, as it has its nsMargin out-param first.  But you are right, better to have the out-params last.  (The optional argument in nsIFrame::ApplySkipSides makes it hard to move the out-param to the end I guess.)

> > Wrap line to avoid going over 80 columns.
> 
> I thought the consensus on the recent code style thread was that we
> could use 99 if it would hurt readability to wrap it to 80.
> Perhaps it was wishful thinking on my part... :-)
> 
> That said, I have wrapped the lines you requested.

It is my wish too, still waiting for consensus / style guide changes. :-)

> > > +  nsSize sz = borderArea.Size();
> > > +  nsSize frameSize = aForFrame->GetSize();
> > > +  if (&aBorder == aForFrame->StyleBorder() &&
> > > +      frameSize == aBorderArea.Size()) {
> > 
> > Use sz instead of aBorderArea.Size() since we just got it above.
> 
> Can't do that since borderArea != aBorderArea at that point.
> So I take that as evidence that the JoinBoxesForSlice signature is not
> clear enough that it modifies the nsRect& it takes.  Mea culpa, I should
> have thought of the rule I just mentioned above.  So I've made it take
> an nsRect* instead.

Ah I didn't notice that while reading GetBorderAreaAndRadii.  (Generally I am happy with reference arguments for things which must not be null, but you are right that making it a pointer probably would have made it more obvious that it changes the argument.)

> > @@ +95,5 @@
> > > +  if (*aHaveRoundedCorners) {
> > > +    auto a2d = aForFrame->PresContext()->AppUnitsPerDevPixel();
> > > +    nsCSSRendering::ComputePixelRadii(radii, a2d, aBgRadii);
> > > +  } else {
> > > +    return aBorderArea;
> > 
> > Why do we return the un-joined border area if there was no border radius?
> 
> I figured I should keep the existing ApplySkipSides drawing path in that
> (common) case, since it should be faster than drawing the whole area with
> a clip.

I am just confused why we might want to return different border area values.  If you change the |return aBorderArea;| to |return borderArea;|, will the painted effect be the same?  I can't see anything that applies a clip or not based on the specific area that is returned.  In PaintBorderWithBorderStyle, it  looks at haveRoundedCorners , which we know will be false.

> > > +  nsRect borderArea = ::GetBorderAreaAndRadii(
> > 
> > Wrap line to avoid going over 80 columns, and no need for the "::".
> 
> I use "::" here as a hint to the reader that this is not a method call
> but rather a global function, and usually a "static" in the same file.
> Is that not a useful hint?

I guess I have never used it as a hint, but only when required to disambiguate it from a method call.  If I wanted to know what GetBorderAreaAndRadii was I'd search and notice that it was a static function.  But feel free to keep it if you feel it is a useful hint.

> > > +  if (!haveRoundedCorners) {
> > > +    ::ApplySkipSides(aSkipSides, &border);
> > 
> > Why do we do this now only if we don't have border radius?
> 
> See earlier answer.

Sorry which earlier answer was that?  Is this just an optimization?

> > > +  if (MOZ_UNLIKELY(!sliceBreak)) {
> > 
> > Make this MOZ_LIKELY(sliceBreak) to match the earlier one (and to remove the
> > double negative)?
> 
> Can't do that since it would change the logic.  MOZ_UNLIKELY is just a hint
> to the compiler, it doesn't affect the expression.

My bad.

> > > +    if (aData->currentLine == 0) {
> > 
> > As I'm not super familiar with this code: where does aData->currentLine get
> > reset to zero when we reach a next in flow that is on the next line?
> 
> grep -r 'ForceBreak(' layout/
> should give you a good overview I think.

Got it.

> > Also why don't we add endPBM on to each of these continuations in the
> > box-decoration-break:clone case?
> 
> I do that actually, that's what 'clonePBM' is about, its value is
> startPBM + endPBM for box-decoration-break:clone, and it's added to
> each line.  For :slice it's zero.

Ah I see.

> > Can you add some (or add to the existing) comments in this function
> > explaining how we want to include the PBM on each fragment when in clone
> > mode.
> 
> I added:
>   // For box-decoration-break:clone we setup clonePBM = startPBM + endPBM and
>   // add that to each line.  For box-decoration-break:slice clonePBM is zero.

Great. :-)

> > > +  if (MOZ_LIKELY(!lastInFlow->GetNextContinuation() && sliceBreak)) {
> > 
> > Should the |!lastInFlow->GetNextContinuation()| bit really be in the
> > MOZ_LIKELY?
> 
> Yes, it's very unlikely that the last-in-flow have a next-continuation.
> It only happens in some rare bidi-cases AFAIK.

OK.

> > ::: layout/generic/nsImageFrame.cpp
> > >  nsImageFrame::GetSkipSides(const nsHTMLReflowState* aReflowState) const
> > >  {
> > > +  if (MOZ_UNLIKELY(StyleBorder()->mBoxDecorationBreak ==
> > > +                     NS_STYLE_BOX_DECORATION_BREAK_CLONE)) {
> > 
> > Make this |LIKELY ... == NS_STYLE_BOX_DECORATION_BREAK_SLICE| to match the
> > previous MOZ_LIKELY calls in this patch.  Same for the remaining
> > MOZ_UNLIKELYs.
> 
> Again, that would change the logic.  I think the code is clearer as written,
> i.e. with the CLONE case as an early return.

OK.

> > ::: layout/generic/nsInlineFrame.cpp
> > >    if (haveLeft || haveRight) {
> > > +    if ((GetStateBits() & NS_FRAME_IS_SPECIAL) &&
> > > +        StyleBorder()->mBoxDecorationBreak ==
> > > +          NS_STYLE_BOX_DECORATION_BREAK_SLICE) {
> > 
> > Can you explain this bit?  Does this just make sure in clone mode we take
> > into account the PBM that would be at the start/end of an inline that was
> > split by a block?  A comment would be good.
> 
> This is the IsSelfEmpty() method.  Since "haveLeft || haveRight" is true
> and all continuations have that border/padding/margin in the b-d-b:clone case
> it follows that this frame isn't empty, so we should skip this if-block and
> just return false.  I've added a code comment to explain it.

OK makes sense.
Comment 58 Cameron McCormack (:heycam) 2014-02-17 15:00:10 PST
Comment on attachment 8376819 [details] [diff] [review]
part 3. Implement box-decoration-break layout for border/paddding/margin/box-shadow.

Review of attachment 8376819 [details] [diff] [review]:
-----------------------------------------------------------------

Where did the pref definition in modules/libpref/src/init/all.js go?
Comment 59 Cameron McCormack (:heycam) 2014-02-17 15:50:31 PST
Comment on attachment 8365692 [details] [diff] [review]
part 4. Implement box-decoration-break layout for backgrounds.

Review of attachment 8365692 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/base/nsCSSRendering.cpp
@@ +2623,5 @@
>            SetupBackgroundClip(clipState, ctx, haveRoundedCorners,
>                                appUnitsPerPixel, &autoSR);
>            clipSet = true;
> +          if (haveRoundedCorners &&
> +              aForFrame->StyleBorder()->mBoxDecorationBreak == NS_STYLE_BOX_DECORATION_BREAK_SLICE) {

wrap line

@@ +2627,5 @@
> +              aForFrame->StyleBorder()->mBoxDecorationBreak == NS_STYLE_BOX_DECORATION_BREAK_SLICE) {
> +            // We're drawing the background for the joined continuation boxes
> +            // so we need to clip that to the slice that we want for this frame.
> +            ctx->NewPath();
> +            ctx->Rectangle(nsLayoutUtils::RectToGfxRect(aBorderArea, appUnitsPerPixel));

wrap line

::: layout/reftests/bugs/368020-3-ref.html
@@ +4,5 @@
>  <title>Testcase, bug 368020</title>
>  </head>
>  <body style="line-height: 3; width: 500px; height: 250px;">
>  
> +    <span style="background: url(repeatable-diagonal-gradient.png); background-clip: border-box; background-clip: border; background-origin: padding-box; background-origin: padding; margin: 7px 4px 2px 18px; border: 6px transparent solid; border-width: 2px 10px 15px 2px;">

May as well get rid of the incorrect 'background-origin: padding'.

::: layout/reftests/bugs/368020-3.html
@@ +4,5 @@
>  <title>Testcase, bug 368020</title>
>  </head>
>  <body style="line-height: 3; width: 500px; height: 250px;">
>  
> +    <span style="background: url(repeatable-diagonal-gradient.png); background-clip: padding-box; background-clip: padding; background-origin: content-box; background-origin: content; border: medium transparent solid; border-width: 7px 4px 2px 18px; padding: 2px 2% 3% 2px;">

And here.

::: layout/reftests/bugs/368020-5-ref.html
@@ +4,5 @@
>  <title>Testcase, bug 368020</title>
>  </head>
>  <body style="line-height: 3; width: 500px; height: 250px;">
>  
> +    <span style="background: url(repeatable-diagonal-gradient.png); background-clip: border-box; background-clip: border; background-origin: padding-box; background-origin: padding; margin: 7px 4px 2px 18px; border: 6px transparent solid; border-width: 2px 10px 15px 2px; box-decoration-break: clone;">

And here.

::: layout/reftests/bugs/368020-5.html
@@ +4,5 @@
>  <title>Testcase, bug 368020</title>
>  </head>
>  <body style="line-height: 3; width: 500px; height: 250px;">
>  
> +    <span style="background: url(repeatable-diagonal-gradient.png); background-clip: padding-box; background-clip: padding; background-origin: content-box; background-origin: content; border: medium transparent solid; border-width: 7px 4px 2px 18px; padding: 2px 10px 15px 2px; box-decoration-break: clone;">

And here.
Comment 60 Cameron McCormack (:heycam) 2014-02-17 15:57:08 PST
Comment on attachment 8365693 [details] [diff] [review]
part 5. Remove remaining vestiges of -moz-background-inline-policy.

Review of attachment 8365693 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/base/nsDisplayList.cpp
@@ -2037,5 @@
> -  // this could easily lead to O(N^2) behavior inside InlineBackgroundData,
> -  // which expects frames to be sent to it in content order, not reverse
> -  // content order which we'll produce here.
> -  // Of course, if there's only one frame in the flow, it doesn't matter.
> -  if (mBackgroundStyle->mBackgroundInlinePolicy == NS_STYLE_BG_INLINE_POLICY_EACH_BOX ||

Why don't we replace this with something like |mBorderStyle->mBoxDecorationBreak == NS_STYLE_NS_STYLE_BOX_DECORATION_BREAK_CLONE|?

::: layout/style/nsCSSKeywordList.h
@@ -187,5 @@
>  CSS_KEY(border-box, border_box)
>  CSS_KEY(both, both)
>  CSS_KEY(bottom, bottom)
>  CSS_KEY(bottom-outside, bottom_outside)
> -CSS_KEY(bounding-box, bounding_box)

This keyword will return soon in bug 945187. :-)
Comment 61 Cameron McCormack (:heycam) 2014-02-17 16:08:59 PST
Comment on attachment 8369701 [details] [diff] [review]
part 5b.  Make sure we Save/Restore the gfx context when using Clip.

Review of attachment 8369701 [details] [diff] [review]:
-----------------------------------------------------------------

r=me for the nsCSSRendering.cpp bits.

::: gfx/thebes/gfxContext.h
@@ +831,5 @@
> +  void EnsureSaved(gfxContext *aContext) {
> +    MOZ_ASSERT(!mContext || mContext == aContext, "wrong context");
> +    if (!mContext) {
> +        mContext = aContext;
> +        mContext->Save();

A gfx peer should review this bit.  (Sad face at "c-basic-offset: 4" modeline and the preponderance of 2ch indents in half the file.)

::: layout/base/nsCSSRendering.cpp
@@ +1698,5 @@
>      aCtx->Clip();
>    }
>  
>    if (aHaveRoundedCorners) {
> +    aAutoSR->EnsureSaved(aCtx); // XXXmats move this before NewPath?

Looks good to move.
Comment 62 Cameron McCormack (:heycam) 2014-02-17 16:33:07 PST
Comment on attachment 8365695 [details] [diff] [review]
part 6. Reftests for box-decoration-break.

Review of attachment 8365695 [details] [diff] [review]:
-----------------------------------------------------------------

Do we already have tests for the default box-decoration-break mode with inset/outset box shadows?  If not we should have some.

::: layout/reftests/css-break/box-decoration-break-1-ref.html
@@ +8,5 @@
> +  <title>CSS Test: Testing box-decoration-break:clone</title>
> +  <link rel="author" title="Mats Palmgren" href="https://bugzilla.mozilla.org/show_bug.cgi?id=613659">
> +  <meta charset="utf-8">
> +<style>
> +span,button {  line-height:4em; }

Remove this declaration?

@@ +11,5 @@
> +<style>
> +span,button {  line-height:4em; }
> +
> +span {
> +  line-height:4em; 

Trailing space.

::: layout/reftests/css-break/box-decoration-break-1.html
@@ +14,5 @@
> +
> +span {
> +  box-decoration-break:clone;
> +
> +  line-height:4em; 

Trailing space.

@@ +31,5 @@
> +.o10 {
> +  border-radius: 17px;
> +  margin-left:0;
> +}
> +.o10x {

I do not understand these class names at all. :-)

::: layout/reftests/css-break/box-decoration-break-with-inset-box-shadow-1-ref.html
@@ +1,5 @@
> +<!DOCTYPE HTML>
> +<html>
> +<head>
> +<style>
> +span,button {  border:3px dashed pink; margin:0 1em; line-height:4em; }

Remove ",button".

@@ +5,5 @@
> +span,button {  border:3px dashed pink; margin:0 1em; line-height:4em; }
> +
> +span {
> +  font-family:monospace;
> +  padding:1em 1em; 

Trailing space.

::: layout/reftests/css-break/box-decoration-break-with-inset-box-shadow-1.html
@@ +1,3 @@
> +<!DOCTYPE HTML>
> +<html>
> +<head>

Add some metadata/title in here like in the other tests.

@@ +1,5 @@
> +<!DOCTYPE HTML>
> +<html>
> +<head>
> +<style>
> +span,button {  border:3px dashed pink; margin:0 1em; line-height:4em; box-decoration-break:clone; }

Remove ",button".

@@ +5,5 @@
> +span,button {  border:3px dashed pink; margin:0 1em; line-height:4em; box-decoration-break:clone; }
> +
> +span {
> +  font-family:monospace;
> +  padding:1em 1em; 

Trailing space.

::: layout/reftests/css-break/box-decoration-break-with-outset-box-shadow-1-ref.html
@@ +5,5 @@
> +span {  border:3px dashed pink; line-height:80px;}
> +
> +span {
> +  font-family:monospace;
> +  padding:1em 1em; 

Trailing space.

::: layout/reftests/css-break/box-decoration-break-with-outset-box-shadow-1.html
@@ +5,5 @@
> +span {  border:3px dashed pink; line-height:80px; box-decoration-break:clone; }
> +
> +span {
> +  font-family:monospace;
> +  padding:1em 1em; 

Trailing space.

::: layout/reftests/css-break/reftest.list
@@ +1,1 @@
> +pref(layout.css.box-decoration-break.enabled,true) == box-decoration-break-1.html box-decoration-break-1-ref.html

You can use:

  default-preferences pref(layout.css.box-decoration-break.enabled,true)

to avoid repeating it on each line in the file if you want.

@@ +1,3 @@
> +pref(layout.css.box-decoration-break.enabled,true) == box-decoration-break-1.html box-decoration-break-1-ref.html
> +
> +skip-if(Android||B2G) fuzzy(1,20) pref(layout.css.box-decoration-break.enabled,true) == box-decoration-break-with-inset-box-shadow-1.html box-decoration-break-with-inset-box-shadow-1-ref.html

Do you need these skip-ifs if you have them in the parent reftest.list?  Or vice versa maybe, if box-decoration-break-1.html works fine on these platforms.

::: layout/reftests/reftest.list
@@ +54,5 @@
>  # blending/
>  skip-if(B2G||Android) include css-blending/reftest.list
>  
> +# Tests for the css-break spec
> +skip-if(Android||B2G) include css-break/reftest.list

OOC, what doesn't work on these platforms?
Comment 63 Cameron McCormack (:heycam) 2014-02-17 16:33:25 PST
Comment on attachment 8376820 [details] [diff] [review]
part 7. Change mode-lines and indentation to 2-space indent.

Review of attachment 8376820 [details] [diff] [review]:
-----------------------------------------------------------------

If you're changing the modeline to a 2 space indent, do you want to reindent the body of the file that mostly uses 4?
Comment 64 Mats Palmgren (vacation) 2014-02-18 11:21:02 PST
(In reply to Cameron McCormack (:heycam) from comment #57)
> If you change the |return aBorderArea;| to |return borderArea;|, will the
> painted effect be the same?  

No, it draws borders to far when the frame has continuations.

> I can't see anything that applies a clip or not
> based on the specific area that is returned.  In PaintBorderWithBorderStyle,
> it  looks at haveRoundedCorners , which we know will be false.

Right, we will either do this:
  if (!haveRoundedCorners) {
    ::ApplySkipSides(aSkipSides, &border);
  }

Or, a bit later:
  if (haveRoundedCorners && aForFrame->StyleBorder()->mBoxDecorationBreak ==
                              NS_STYLE_BOX_DECORATION_BREAK_SLICE) {
    // We're drawing borders around the joined continuation boxes so we need
    // to clip that to the slice that we want for this frame.
    aRenderingContext.IntersectClip(aBorderArea);
  }

So there are two paths.  Either we draw borders using the larger
"JoinBoxesForSlice" area, and apply a clip to get the part for the
frame at hand.  Or, we use the area of the current frame and do
ApplySkipSides to exclude the sides that don't apply for this frame.

But you're right that the code is a bit hard to follow...  I'll fix
that somehow.  Would be clearer if GetBorderAreaAndRadii always
returned 'borderArea' and then separate the paths in
PaintBorderWithBorderStyle? something like this:

  if (borderArea.IsEqualEdges(aBorderArea)) {
    ::ApplySkipSides(aSkipSides, &border);
  } else {
    aRenderingContext.IntersectClip(aBorderArea);
  }

and also rename 'borderArea' to 'joinedBorderArea' and add some
code comments?

(I may have to give up sharing GetBorderAreaAndRadii for backgrounds
though, I haven't checked yet.)
Comment 65 Mats Palmgren (vacation) 2014-02-18 11:23:34 PST
(In reply to Cameron McCormack (:heycam) from comment #58)
> Where did the pref definition in modules/libpref/src/init/all.js go?

Not sure what you mean - it's in part 1, as always.
Comment 66 Cameron McCormack (:heycam) 2014-02-18 12:41:10 PST
(In reply to Mats Palmgren (:mats) from comment #65)
> Not sure what you mean - it's in part 1, as always.

Sorry, I was looking at the interdiff between the old/new part 3 patch and saw it disappear from there and forgot that I already saw it in part 1.
Comment 67 Cameron McCormack (:heycam) 2014-02-20 18:07:03 PST
(In reply to Mats Palmgren (:mats) from comment #64)
> So there are two paths.  Either we draw borders using the larger
> "JoinBoxesForSlice" area, and apply a clip to get the part for the
> frame at hand.  Or, we use the area of the current frame and do
> ApplySkipSides to exclude the sides that don't apply for this frame.

Why do we call ApplySkipSides in the !haveRoundedCorners case?

> But you're right that the code is a bit hard to follow...  I'll fix
> that somehow.  Would be clearer if GetBorderAreaAndRadii always
> returned 'borderArea' and then separate the paths in
> PaintBorderWithBorderStyle? something like this:
> 
>   if (borderArea.IsEqualEdges(aBorderArea)) {
>     ::ApplySkipSides(aSkipSides, &border);
>   } else {
>     aRenderingContext.IntersectClip(aBorderArea);
>   }
> 
> and also rename 'borderArea' to 'joinedBorderArea' and add some
> code comments?

I think that would be clearer, although it would be good if we didn't actually have to do a comparison of the border edge positions and just got a boolean out of GetBorderAreaAndRadii that we could use instead.
Comment 68 Mats Palmgren (vacation) 2014-03-26 09:14:25 PDT
Created attachment 8397142 [details] [diff] [review]
part 3. Implement box-decoration-break layout for border/paddding/margin/box-shadow.

(In reply to Cameron McCormack (:heycam) from comment #67)
> Why do we call ApplySkipSides in the !haveRoundedCorners case?

Using ApplySkipSides and the frame's are (not the joined area) was
just an optimization since continuations shouldn't affect other
continuations' borders unless they are rounded.  But I now think
that's not entirely correct since it affects how dashed/dotted
borders start, and maybe other things.
So I removed that bit, and added comments in PaintBorderWithStyleBorder
for the different b-d-b:slice cases for drawing borders.  Drawing
non-rounded borders using the joined area showed a bug in the way
I calculated the area for inlines (BIDI, ib-split).  Fortunately there
is code in InlineBackgroundData::GetContinuousRect that I could reuse.
Note that the joined area for backgrounds is NOT the same as for
borders, but it's easy to "rotate" the area to get the left-border
continuation as the origin instead.
Comment 69 Mats Palmgren (vacation) 2014-03-26 09:25:34 PDT
Created attachment 8397152 [details] [diff] [review]
part 4. Implement box-decoration-break layout for backgrounds.

(In reply to Cameron McCormack (:heycam) from comment #59)
> part 4. Implement box-decoration-break layout for backgrounds.

Fixed all the nits.  And some minor tweaks.

Please ignore the gfx/thebes/gfxContext.h bit -
I'll ask a GFX peer to review that bit.
Comment 70 Mats Palmgren (vacation) 2014-03-26 09:27:18 PDT
Created attachment 8397153 [details] [diff] [review]
part 5. Remove remaining vestiges of -moz-background-inline-policy.

(In reply to Cameron McCormack (:heycam) from comment #60)
> part 5. Remove remaining vestiges of -moz-background-inline-policy.
> ::: layout/base/nsDisplayList.cpp
> @@ -2037,5 @@
> > -  // this could easily lead to O(N^2) behavior inside InlineBackgroundData,
> > -  // which expects frames to be sent to it in content order, not reverse
> > -  // content order which we'll produce here.
> > -  // Of course, if there's only one frame in the flow, it doesn't matter.
> > -  if (mBackgroundStyle->mBackgroundInlinePolicy == NS_STYLE_BG_INLINE_POLICY_EACH_BOX ||
> 
> Why don't we replace this with something like
> |mBorderStyle->mBoxDecorationBreak ==
> NS_STYLE_NS_STYLE_BOX_DECORATION_BREAK_CLONE|?

Yeah, good point, did so.

> ::: layout/style/nsCSSKeywordList.h
> > -CSS_KEY(bounding-box, bounding_box)
> 
> This keyword will return soon in bug 945187. :-)

OK, I'll still remove it here though so that when it's re-added the line
will point to the right changeset/bug.
Comment 71 Mats Palmgren (vacation) 2014-03-26 09:31:39 PDT
Created attachment 8397157 [details] [diff] [review]
part 6. Reftests for box-decoration-break.

(In reply to Cameron McCormack (:heycam) from comment #62)
> part 6. Reftests for box-decoration-break.
> 
> Do we already have tests for the default box-decoration-break mode with
> inset/outset box shadows?  If not we should have some.

I'm pretty sure we don't, since the current box shadow rendering is more
clone-like than slice, and I didn't have to update any box shadow tests.
I'll try to write some.

I fixed the other nits on the test files.

> ::: layout/reftests/css-break/reftest.list
> Do you need these skip-ifs if you have them in the parent reftest.list?  Or
> vice versa maybe, if box-decoration-break-1.html works fine on these
> platforms.

Yeah, I'm confused about that as well.  I would have thought that
"skip-if(B2G||Android)" on the 'include' would skip these tests on those
platforms but apparently not since I had to add it here too.
Anyway...

> OOC, what doesn't work on these platforms?

IIRC, there were some fuzzy errors in the shadow pixels that I concluded
must be some GFX platform weirdness.  But now that I tried it again the
tests passed!  So, let's try having them enabled for the initial landing,
we can disable them later in case they have intermittent failures.
Comment 72 Mats Palmgren (vacation) 2014-03-26 09:34:36 PDT
Created attachment 8397160 [details] [diff] [review]
part 7. Change mode-lines and indentation to 2-space indent.

(In reply to Cameron McCormack (:heycam) from comment #63)
> part 7. Change mode-lines and indentation to 2-space indent.
> If you're changing the modeline to a 2 space indent, do you want to reindent
> the body of the file that mostly uses 4?

Fixed.  (This part was scripted.)
Comment 73 Mats Palmgren (vacation) 2014-03-26 09:36:59 PDT
Created attachment 8397166 [details] [diff] [review]
part 7b. Some additional manual changes to property_database.js.

Some additional manual changes to property_database.js.
Comment 75 Cameron McCormack (:heycam) 2014-03-30 23:45:41 PDT
Comment on attachment 8397142 [details] [diff] [review]
part 3. Implement box-decoration-break layout for border/paddding/margin/box-shadow.

Review of attachment 8397142 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/base/nsCSSRendering.cpp
@@ +98,4 @@
>    }
>  
> +  /**
> +   * Return a continuous rect for (an inline) aFrame relative the continuation

"relative to"

@@ +158,5 @@
>      return nsRect(-x, 0, mUnbrokenWidth, mFrame->GetSize().height);
>    }
>  
> +  /**
> +   * Return a continuous rect for (an inline) aFrame relative the continuation

"relative to"

@@ +170,5 @@
> +  {
> +    // Calling GetContinuousRect(aFrame) here may lead to Reset/Init which
> +    // resets our mLeftBorderData so we save it ...
> +    BorderLeftData saved(mLeftBorderData);
> +    nsRect borderArea = GetContinuousRect(aFrame);

I find it a bit confusing that we have aBorderArea and borderArea, but one of them is the big continuous rectangle, and the other is for the single frame aFrame.  Can you change the name of one of them?

@@ +179,5 @@
> +        mLeftBorderData.SetX(GetContinuousRect(mLeftBorderData.mFrame).x);
> +      }
> +    } else {
> +      // ... and restore it when possible.
> +      mLeftBorderData.SetX(saved.mX);

We know saved.mIsValid is true here, so you could just assign to mX instead of calling SetX.

@@ +182,5 @@
> +      // ... and restore it when possible.
> +      mLeftBorderData.SetX(saved.mX);
> +    }
> +    if (borderArea.x > mLeftBorderData.mX) {
> +      borderArea.x = -(mUnbrokenWidth + borderArea.x - aBorderArea.width);

I'm a bit slow here; can you explain this, maybe with a small example?  (I'm not sure "rotate" in the function's comment is the right word to describe what's happening here?)

@@ +209,5 @@
>  protected:
> +  struct BorderLeftData {
> +    nsIFrame* mFrame;
> +    nscoord   mX;
> +    bool      mIsValid;

Can you please document these -- at least mFrame and mX?

@@ +220,5 @@
> +  nsRect         mBoundingBox;
> +  nscoord        mContinuationPoint;
> +  nscoord        mUnbrokenWidth;
> +  nscoord        mLineContinuationPoint;
> +  BorderLeftData mLeftBorderData;

Change the type to LeftBorderData or the member to mBorderLeftData.

@@ +487,5 @@
> +  return borderArea;
> +}
> +
> +/**
> + * Extend aBorderArea relative aFrame's origin to calculate a hypothetical

s/relative aFrame's origin/, which is relative to aFrame's origin,/

@@ +631,5 @@
>    }
>  
> +  // Compute the outermost boundary of the area that might be painted.
> +  // Same coordinate space as aBorderArea & aBGClipRect.
> +  nsRect borderArea =

Did you want to rename this to joinedBorderArea per comment 67?

@@ +643,5 @@
> +  // start drawing
> +  gfxContext* ctx = aRenderingContext.ThebesContext();
> +  ctx->Save();
> +
> +  // we can assume that we're already clipped to aDirtyRect -- I think? (!?)

As we've had this comment/code for five years I think this must be true all the time. :-)  Feel free to remove it, especially since you mention clipping in the if statements below.

@@ +692,5 @@
>    }
>  
>    SF(" borderStyles: %d %d %d %d\n", borderStyles[0], borderStyles[1], borderStyles[2], borderStyles[3]);
> +  //SF ("bgRadii: %f %f %f %f\n", bgRadii[0], bgRadii[1], bgRadii[2], bgRadii[3]);
> +#if 0

Put a blank line before this one.

@@ +1158,5 @@
> +    aForFrame->GetVisualOverflowRectRelativeToSelf() + aFrameArea.TopLeft() :
> +    aFrameArea;
> +  frameRect = ::BoxDecorationRectForBorder(aForFrame, frameRect);
> +
> +  // Get any border radius, since box-shadow must also have rounded corners if the frame does.

Mind the 80 column limit.

::: layout/generic/nsFrame.cpp
@@ +946,5 @@
>  
>  int
>  nsIFrame::GetSkipSides(const nsHTMLReflowState* aReflowState) const
>  {
> +  if (MOZ_UNLIKELY(StyleBorder()->mBoxDecorationBreak ==

Unsure whether it's worth using a MOZ_UNLIKELY for a perfectly valid use of box-decoration-break.

::: layout/generic/nsInlineFrame.cpp
@@ +677,3 @@
>     */
> +  if ((NS_FRAME_IS_COMPLETE(aStatus) && !LastInFlow()->GetNextContinuation() &&
> +       !FrameIsNonLastInIBSplit()) || boxDecorationBreakClone) {

I think it might be clearer with the original line breaks after each ANDed term.

::: layout/generic/nsSplittableFrame.cpp
@@ +252,5 @@
>    if (IS_TRUE_OVERFLOW_CONTAINER(this)) {
>      return LOGICAL_SIDES_B_BOTH;
>    }
>  
> +  if (MOZ_UNLIKELY(StyleBorder()->mBoxDecorationBreak ==

Maybe put this at the top of the function, for symmetry with the other GetLogicalSkipSides changes in this patch?  I'm assuming the order doesn't matter here.
Comment 76 Cameron McCormack (:heycam) 2014-03-31 00:08:34 PDT
Comment on attachment 8397166 [details] [diff] [review]
part 7b. Some additional manual changes to property_database.js.

Review of attachment 8397166 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/style/test/property_database.js
@@ +3677,5 @@
>      inherited: true,
>      type: CSS_TYPE_LONGHAND,
>      initial_values: [ "none", "context-value" ],
> +    // This TAB is intentional to test parsing --------------v
> +    other_values: [ "5px,3px,2px", "5px 3px 2px", "  5px ,3px	, 2px ", "1px", "5%", "3em" ],

Just use \t in the string rather than point out a maybe-invisible tab character.
Comment 77 Mats Palmgren (vacation) 2014-04-14 17:30:01 PDT
Created attachment 8406519 [details]
Testcase #8

(In reply to Cameron McCormack (:heycam) from comment #75)
> I'm a bit slow here; can you explain this, maybe with a small example?  (I'm
> not sure "rotate" in the function's comment is the right word to describe
> what's happening here?)

In the attached example, the GetContinuousRect rect is in the order
1,2,3 (background order).  For border rendering we want x-coordintes
as if the order was 1,2,3 (top), 3,2,1 (bottom).
The code that does that:

    if (joinedBorderArea.x > mLeftBorderData.mX) {
      joinedBorderArea.x =
        -(mUnbrokenWidth + joinedBorderArea.x - aBorderArea.width);
    } else {
      joinedBorderArea.x -= mLeftBorderData.mX;
    }

So, given the x-offset of "1" relative aFrame (joinedBorderArea.x) and
the x-offset of "1" relative the left-border-frame (mLeftBorderData.mX)
in the GetContinuousRect rect, this calculates the x-offset relative
the left-border-frame instead of "1" and reorders the frames that are
left of the left-border (1 and 2 in the RTL case at the bottom).

For a typical LTR case (top) the order and offsets do not change.
mLeftBorderData.mX is zero and we fall into the else-part since
the 1 has negative relative offset to 2 and 3.

For RTL (bottom), the left-border-frame has the most negative
x-offset (to 1) so the if-condition is true for 1 and 2.
For 3 we hit the else-part and x becomes zero.

Yeah, "rotate" isn't the right word.  I'm not sure what is.
Comment 78 Mats Palmgren (vacation) 2014-04-14 17:35:35 PDT
Created attachment 8406520 [details] [diff] [review]
part 3. Implement box-decoration-break layout for border/paddding/margin/box-shadow.

(In reply to Cameron McCormack (:heycam) from comment #75)
> Comment on attachment 8397142 [details] [diff] [review]
> part 3. Implement box-decoration-break layout for
> border/paddding/margin/box-shadow.
> > + * Extend aBorderArea relative aFrame's origin to calculate a hypothetical
> 
> s/relative aFrame's origin/, which is relative to aFrame's origin,/

Done.  I also changed "Extend" to "Inflate" to make it clearer that
the result has the same origin, it just extends outward.

> > +  if (MOZ_UNLIKELY(StyleBorder()->mBoxDecorationBreak ==
> 
> Unsure whether it's worth using a MOZ_UNLIKELY for a perfectly valid use of
> box-decoration-break.

I suspect that b-d-b:clone will be very rarely used, so a compiler hint
seems worth it.

> ::: layout/generic/nsSplittableFrame.cpp
> >    if (IS_TRUE_OVERFLOW_CONTAINER(this)) {
> >      return LOGICAL_SIDES_B_BOTH;
> >    }
> >  
> > +  if (MOZ_UNLIKELY(StyleBorder()->mBoxDecorationBreak ==
> 
> Maybe put this at the top of the function, for symmetry with the other
> GetLogicalSkipSides changes in this patch?  I'm assuming the order doesn't
> matter here.

The order does matter.  We shouldn't render anything for a true overflow
container itself, just its children, so b-d-b:clone doesn't apply.
(I think s/LOGICAL_SIDES_B_BOTH/LOGICAL_SIDES_ALL/ and a comment would
make that clearer. I'll file that as a separate bug.)
(layout/reftests/pagination/border-breaking-002-cols.xhtml has an
example if you want test this with "return 0" there)

I fixed the other nits as requested.
Comment 79 Mats Palmgren (vacation) 2014-04-14 17:54:47 PDT
Comment on attachment 8397152 [details] [diff] [review]
part 4. Implement box-decoration-break layout for backgrounds.

Jeff, can you review this small change to gfxContextAutoSaveRestore
in gfx/thebes/gfxContext.h please? (ignore the rest of the patch)

The change makes it easier to do a Save() only when needed, typically
when we get into a branch that needs to add a clip. The Reset() method
became unused in the process so I removed it.
Comment 80 Cameron McCormack (:heycam) 2014-04-16 16:03:26 PDT
(In reply to Mats Palmgren (:mats) from comment #77)
> In the attached example, the GetContinuousRect rect is in the order
> 1,2,3 (background order).  For border rendering we want x-coordintes
> as if the order was 1,2,3 (top), 3,2,1 (bottom).

Thanks, I understand now.  Is it possible to describe this in a comment somehow, that we're doing this to just reverse the positions of each frame its container is RTL?

> Yeah, "rotate" isn't the right word.  I'm not sure what is.

Is it really "reverse"?  I'm OK with leaving it as "rotate" if there's no better word, especially if you add the comment, as it should be clearer then.
Comment 82 Mats Palmgren (vacation) 2014-04-17 06:50:25 PDT
Created attachment 8408230 [details] [diff] [review]
fix b2g build error
Comment 83 Mats Palmgren (vacation) 2014-04-17 06:57:24 PDT
Looks like unified builds hid this problem on other platforms,
I wonder why it didn't show up on Try though?
https://hg.mozilla.org/integration/mozilla-inbound/rev/21043f348d65
Comment 84 Mats Palmgren (vacation) 2014-04-17 07:04:37 PDT
Oh, "B2G Desktop Windows Opt" is the one B2G platform I didn't test.
I'm guessing this is a relatively new platform so it's missing in my Try template patch.
What are the odds? :-)
Comment 85 Mats Palmgren (vacation) 2014-04-17 08:14:20 PDT
Backed out for build failure on B2G Windows.
https://hg.mozilla.org/integration/mozilla-inbound/rev/70862f5149c6
Comment 86 Mats Palmgren (vacation) 2014-04-17 16:34:01 PDT
Created attachment 8408642 [details] [diff] [review]
part 8, build fix for Windows

I tried various tricks to make the Windows compiler accept an inlined
method here but without success. :(
Comment 87 Daniel Holbert [:dholbert] (mostly OOTO until Aug 9th) 2014-04-18 15:24:40 PDT
Comment on attachment 8408642 [details] [diff] [review]
part 8, build fix for Windows

[stealing review on part 8 per IRC]

r=me
Comment 90 Mats Palmgren (vacation) 2014-04-20 12:43:17 PDT
Backed out on m-c:
remote:   https://hg.mozilla.org/mozilla-central/rev/4bc69199b7e1
remote:   https://hg.mozilla.org/mozilla-central/rev/28dc0100f9b0
Comment 91 Mats Palmgren (vacation) 2014-04-21 07:55:16 PDT
Created attachment 8409675 [details] [diff] [review]
part 9, bug fix for regression bug 998792

Sorry to bother you again, I need a method to explicitly Restore the
gfxContext if any were Saved.  (I somehow got the crazy idea that
~gfxContextAutoSaveRestore would run when assigning 'autoSR')

(this is to fix bug 998792)

https://tbpl.mozilla.org/?tree=Try&rev=adc91681d837
https://tbpl.mozilla.org/?tree=Try&rev=5066da0cbb86
Comment 92 Mats Palmgren (vacation) 2014-04-30 13:02:44 PDT
Comment on attachment 8409675 [details] [diff] [review]
part 9, bug fix for regression bug 998792

roc, this is a trivial 7-line patch to fix a bug I introduced in the
earlier set of patches.  Can you review it please?

It adds a new method to gfxContextAutoSaveRestore to call Restore()
on any saved mContext and then null it out.
Comment 93 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2014-05-04 23:50:33 PDT
Comment on attachment 8409675 [details] [diff] [review]
part 9, bug fix for regression bug 998792

Review of attachment 8409675 [details] [diff] [review]:
-----------------------------------------------------------------

r+ either way

::: gfx/thebes/gfxContext.h
@@ +819,5 @@
>      mContext->Save();
>    }
>  
>    ~gfxContextAutoSaveRestore() {
> +    Clear();

Why don't we call this Restore()?
Comment 96 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2014-05-05 22:08:44 PDT
Is there a bug on turning on the pref?

(It seems less than ideal to remove -moz-background-inline-policy without enabling box-decoration-break, although I suppose it's unlikely to be used much.)
Comment 97 Mats Palmgren (vacation) 2014-05-06 00:43:20 PDT
(In reply to David Baron [:dbaron] (needinfo? me) (UTC-7) from comment #96)
> Is there a bug on turning on the pref?

I filed bug 1006326 on enabling it by default in Nightly.

> (It seems less than ideal to remove -moz-background-inline-policy without
> enabling box-decoration-break, although I suppose it's unlikely to be used
> much.)

Yeah, the code is rather entangled so it seems hard maintaining two code paths.
Comment 98 Jean-Yves Perrier [:teoli] 2014-06-13 02:51:11 PDT
So, if I understand correctly, as box-decoration-break is behind a pref that is off by default even in Nightly, the only user facing feature here is to remove -moz-background-inline-policy.

Mats, am I correct?
Comment 99 Mats Palmgren (vacation) 2014-07-11 17:35:54 PDT
This is now preffed on by default for Firefox 32 (bug 1006326) and I've updated the MDN pages.
https://developer.mozilla.org/en-US/docs/Web/CSS/box-decoration-break
https://developer.mozilla.org/en-US/docs/Web/CSS/-moz-background-inline-policy

Note You need to log in before you can comment on or make changes to this bug.