Closed Bug 686281 Opened 13 years ago Closed 8 years ago

Implement CSS Mask Image properties (mask, mask-image, etc.)

Categories

(Core :: Layout, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla47
Tracking Status
firefox47 --- fixed

People

(Reporter: amfibia95, Assigned: u459114)

References

(Blocks 3 open bugs, )

Details

(Keywords: dev-doc-complete, Whiteboard: p=0, [DocArea=CSS])

Attachments

(13 files, 41 obsolete files)

84.13 KB, patch
u459114
: review+
Details | Diff | Splinter Review
81.44 KB, patch
u459114
: review+
Details | Diff | Splinter Review
30.28 KB, patch
u459114
: review+
Details | Diff | Splinter Review
58.44 KB, patch
u459114
: review+
Details | Diff | Splinter Review
14.89 KB, patch
u459114
: review+
Details | Diff | Splinter Review
9.95 KB, patch
u459114
: review+
Details | Diff | Splinter Review
1.72 KB, patch
u459114
: review+
Details | Diff | Splinter Review
15.37 KB, patch
u459114
: review+
Details | Diff | Splinter Review
2.56 KB, patch
u459114
: review+
Details | Diff | Splinter Review
864 bytes, patch
u459114
: review+
Details | Diff | Splinter Review
10.72 KB, patch
u459114
: review+
Details | Diff | Splinter Review
14.25 KB, patch
u459114
: review+
Details | Diff | Splinter Review
42.45 KB, patch
u459114
: review+
Details | Diff | Splinter Review
User Agent: Mozilla/5.0 (Windows NT 6.1; rv:7.0) Gecko/20100101 Firefox/7.0 Firefox/7.0
Build ID: 20110908135051

Steps to reproduce:

I suggest adding a new CSS property-moz-mask.
Make analog: http://www.webkit.org/blog/181/css-masks/
Version: 7 Branch → unspecified
Severity: normal → enhancement
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Windows 7 → All
Hardware: x86 → All
Version: unspecified → Trunk
Summary: CSS Mask Image (-moz-mask) proporse → Implement CSS Mask Image (-moz-mask) proposal
Component: General → Layout
Product: Firefox → Core
QA Contact: general → layout
We already support this, using SVG masks, no?
Two different approaches. SVG is probably more powerful, but using an image is easier.
Supporting the image syntax as well as SVG makes sense.
Да лучше бы реализовали CSS Content, а то JNG приходится чинить за счет места в src. А из-за этого сохранить JNG нельзя!
Will mask-image accept the -moz-element() image type? I'd love to use text as a mask for an image (=text with a pattern).
SVG masks already let you use text as a mask.
Han any on had experience in animating or using transition on a svg mask?
I was first hoping I would be able to use a CSS gradient for a mask. When I found I couldn't do that I was then hoping I could use an image, preferably a data uri I could embed right into the CSS, but alas it currently requires an external SVG file, a file type I would rather not have to deal with.
See also the related W3C Working Draft at http://www.w3.org/TR/css-masking/
I just voted for this bug, so let me explain:
SVG masks are theoretically more powerful and versatile. It's only when you use it in practice that you notice its main issue: performance. When you try to use it for browser-chrome (applied to XUL), you notice that hardware-accelerated rendering is problematic and slow.
I have no benchmarks backing this statement at the moment, but I believe when you ask the UX or fx-desktop team members, you'll get a confirmation nonetheless.

Hardware-accelerated rendering of a raster of pixels (with and without alpha-blending), however, is a solved problem! If we would have had this feature right now, implementing customizable curvy tabs, for example, would've been jet-boosted.
But let's not stop at curvy tabs! Any custom-shaped panel, overlays, toolbars, multi-layer buttons (do I hear mobile?), etc.

To make things scale properly across multiple resolutions and/ or pixel densities, developers will have to use @media queries in this case, though, which is less flexible than what SVG offers out of the box. But this is a trade-off that developers can reason about, which they have to do anyway nowadays when they prepare content for various form factors.

So, having this feature implemented would be a BIG step forward imho!
We will do this since there's a spec for it now, but we should also accelerate rendering of SVG masks.
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #11)
> We will do this since there's a spec for it now, but we should also
> accelerate rendering of SVG masks.

Epic, on both counts! I guess this won't just happen overnight, but good to hear nonetheless.
Assignee: nobody → ncameron
Blocks: 877294
Depends on: 700926
Whiteboard: [Australis:P?][Australis:M?]
Whiteboard: [Australis:P?][Australis:M?]
Summary: Implement CSS Mask Image (-moz-mask) proposal → Implement CSS Mask Image properties (mask, mask-image, etc.)
The spec (http://www.w3.org/TR/css-masking/) is in last call, the editor's version has been updated since then, though.
Whiteboard: p=0
No longer blocks: fxdesktopbacklog
Flags: firefox-backlog+
Is mask-size in the spec and proposition?
(In reply to adrian from comment #14)
> Is mask-size in the spec and proposition?

mask-size and mask-image, mask-type property, mask-repeat, mask-position, mask-clip, mask-origin, mask-composite. See http://dev.w3.org/fxtf/css-masking-1/
Picking this up until I can find a proper assignee.
Assignee: ncameron → bugs
This issue should be blocking bug 913153.

Sebastian
Blocks: css3test
Assignee: bugs → cku
Attached patch WIP - mask-image + mask-repeat (obsolete) — Splinter Review
Attachment #8667692 - Attachment is obsolete: true
Attachment #8668867 - Attachment is obsolete: true
TBD
1. (CSS parsing) Integrate ParseMaskXXX & ParseBackgroundXXX
2. (CSS computing) Create nsStyleSVGStyle::Layers, instead of using nsStyleBackgroundLayer, since attributes of them is not the same.
3. (CSS rendering) Mask painting in nsSVGIntegrationUtils::PaintFramesWithEffects is only a simple prototype.
TBD:
1. integration between css-background and css-mask.
   Apparently, we can use most background-image functions and data structure for mask.
2. mask rendering position.
Attachment #8669592 - Attachment is obsolete: true
Attachment #8671293 - Attachment is obsolete: true
Attached patch WIP 5 (obsolete) — Splinter Review
Attachment #8674131 - Attachment is obsolete: true
TBD:
1. Most requirements are implemented, except mask-mode(luminance) and short-hand.
2. Change PaintBackground semantic to PaintCSSLayer.
Attachment #8674796 - Attachment is obsolete: true
Rebase
Attachment #8677956 - Attachment is obsolete: true
Attachment #8678121 - Attachment is obsolete: true
Attachment #8681281 - Attachment is obsolete: true
Comment on attachment 8681973 [details] [diff] [review]
WIP 9 - merge mask & background properties into layer property

Why are you trying to use the same layer structure for both backgrounds and masks?  (I could understand wanting a common base class for the things they share, but it seems wasteful and probably also confusing to use the same struct.)  Or does it save space (because of packing) or code to have it all together?  If so, it would be good to have something to prevent it from being confusing about what's actually used.  I also wouldn't use nsCSSLayers as the name; we tend to use nsCSS* for specified style data and nsStyle* for computed style data.

Also I'm not inclined to think we want a separate style struct.  That would perhaps be desirable for if the mask properties were widely used, but I don't see them being as widely used as the background properties.  (It is sometimes nice for rule tree optimizations if a single shorthand sets all of the properties in a struct, although it's less important for non-inherited properties like these.)  I'd probably suggest just adding to nsStyleSVGReset for now.
It's also going to be a *lot* easier to review this if it is done as a stack of patches rather than a single patch.
(In reply to David Baron [:dbaron] ⌚UTC+8 from comment #31)
> Comment on attachment 8681973 [details] [diff] [review]
> WIP 9 - merge mask & background properties into layer property
> 
> Why are you trying to use the same layer structure for both backgrounds and
> masks?  (I could understand wanting a common base class for the things they
> share, but it seems wasteful and probably also confusing to use the same
> struct.)  Or does it save space (because of packing) or code to have it all
> together?  If so, it would be good to have something to prevent it from
> being confusing about what's actually used.  I also wouldn't use nsCSSLayers
> as the name; we tend to use nsCSS* for specified style data and nsStyle* for
> computed style data.
Have a single base class does not really helpful in CSS parser side, but I can see benefit in CSS render side.
I use nsCSSRendering::PaintBackgroundWithSC, with minor code change, for both background and mask rendering because of this new base class. Without this base class, I need either to create another nsCSSRendering::PaintMaskWithSC function for mask rendering, or change nsCSSRendering::PaintBackgroundWithSC and some functions called by it(PrepareBackgroundLayer for exp) to template functions, so that it can process both background style data and mask style data.

nsCSSLayers was designed to be a member data of nsStyleBackground and nsStyleMask(or nsStyleSVGReset as your suggestion), so named it as nsStyleLayer is not that fit. nsLayer is to common and easy to confuse with gfx layer. Like you said, nsCSS* is not suitable as well.... I will pick up another name.
> Also I'm not inclined to think we want a separate style struct.  That would
> perhaps be desirable for if the mask properties were widely used, but I
> don't see them being as widely used as the background properties.  (It is
> sometimes nice for rule tree optimizations if a single shorthand sets all of
> the properties in a struct, although it's less important for non-inherited
> properties like these.)  I'd probably suggest just adding to nsStyleSVGReset
> for now.
OK
(In reply to David Baron [:dbaron] ⌚UTC+8 from comment #32)
> It's also going to be a *lot* easier to review this if it is done as a stack
> of patches rather than a single patch.

OK, will do soon.
(In reply to C.J. Ku[:cjku] from comment #33)
> nsCSSLayers was designed to be a member data of nsStyleBackground and
> nsStyleMask(or nsStyleSVGReset as your suggestion), so named it as
> nsStyleLayer is not that fit. nsLayer is to common and easy to confuse with
> gfx layer. Like you said, nsCSS* is not suitable as well.... I will pick up
> another name.

I think nsStyleLayers is probably fine; it's similar to the existing nsStyleGradient or nsStyleImage.

But given how many properties are common, having a single class (without a hierarchy) seems fine, as long as there are comments explaining which members are/aren't used in which situations.
Attached patch 1. Mask CSS property (obsolete) — Splinter Review
Attachment #8681973 - Attachment is obsolete: true
Attached patch 2. Mask Style (obsolete) — Splinter Review
Attached patch 3. Mask Rendering (obsolete) — Splinter Review
Attached file Git hub working branch (obsolete) —
Things not ready
1. mask-mode: luminance mask is not supported yet.
2. mask-size & mask-position animation are not supported yet.
3. svg-mask: mask properties, such as mask-repeat/origin/clip, are not apply to a mask-image if that image is a SVG#mask element.

I intend to do all these three items in the following up bugs, any thought?
(In reply to C.J. Ku[:cjku] from comment #40)
> I intend to do all these three items in the following up bugs, any thought?

It's nearly always fine to separate work like that into followup bugs when the plan is to ship the feature after the followup bugs land (that is, until they're done, have it behind a pref that's either disabled or enabled only for nightly and aurora).  If what you're asking is whether it's ok to ship the feature without those, then I'd have to actually think about it...
Bug 686281 - CSS Mask prop.
Bug 686281 - CSS Mask rendering.
Attachment #8682995 - Attachment is obsolete: true
Attachment #8682996 - Attachment is obsolete: true
Attachment #8682999 - Attachment is obsolete: true
Comment on attachment 8683556 [details]
MozReview Request: Bug 686281 - Implement nsStyleImageLayers; r=dbaron

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24369/diff/1-2/
Comment on attachment 8683557 [details]
MozReview Request: Bug 686281 - Rename *background* to *imagelayer*; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24371/diff/1-2/
Comment on attachment 8683558 [details]
MozReview Request: Bug 686281 - Implement CSS mask style; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24373/diff/1-2/
Did you intend to request review?
Flags: needinfo?(cku)
Comment on attachment 8683556 [details]
MozReview Request: Bug 686281 - Implement nsStyleImageLayers; r=dbaron

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24369/diff/2-3/
Comment on attachment 8683557 [details]
MozReview Request: Bug 686281 - Rename *background* to *imagelayer*; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24371/diff/2-3/
Comment on attachment 8683558 [details]
MozReview Request: Bug 686281 - Implement CSS mask style; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24373/diff/2-3/
Bug 686281 - CSS Mask animation.
Attachment #8683556 - Attachment description: MozReview Request: Bug 686281 - CSS Mask prop. → MozReview Request: Bug 686281- Implement CSS mask properties. r=:dbaron
Attachment #8683556 - Flags: review?(dbaron)
Comment on attachment 8683556 [details]
MozReview Request: Bug 686281 - Implement nsStyleImageLayers; r=dbaron

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24369/diff/3-4/
Attachment #8683557 - Attachment description: MozReview Request: Bug 686281 - CSS Mask Style → MozReview Request: Bug 686281- Implement CSS mask style. r=:dbaron
Attachment #8683557 - Flags: review?(dbaron)
Comment on attachment 8683557 [details]
MozReview Request: Bug 686281 - Rename *background* to *imagelayer*; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24371/diff/3-4/
Attachment #8683558 - Attachment description: MozReview Request: Bug 686281 - CSS Mask rendering. → MozReview Request: Bug 686281- Implement CSS mask rendering. r=:dbaron
Attachment #8683558 - Flags: review?(dbaron)
Comment on attachment 8683558 [details]
MozReview Request: Bug 686281 - Implement CSS mask style; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24373/diff/3-4/
Attachment #8683706 - Attachment description: MozReview Request: Bug 686281 - CSS Mask animation. → MozReview Request: Bug 686281- Implement CSS mask animation. r=:dbaron
Attachment #8683706 - Flags: review?(dbaron)
Comment on attachment 8683706 [details]
MozReview Request: Bug 686281 - Mask CSS parsing and Mask DOM API. r=dbaron

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24415/diff/1-2/
(In reply to David Baron [:dbaron] ⌚UTC+8 from comment #48)
> Did you intend to request review?
I am still working on test case... but yes, if you have time, please help to review mask implementation.
Flags: needinfo?(cku)
Comment on attachment 8683556 [details]
MozReview Request: Bug 686281 - Implement nsStyleImageLayers; r=dbaron

https://reviewboard.mozilla.org/r/24369/#review21991

::: layout/style/Declaration.cpp:396
(Diff revision 4)
> +    case eCSSProperty_mask: {

Is it possible for this (in Declaration.cpp) to share code with background?

::: layout/style/nsCSSParser.cpp:11307
(Diff revision 4)
> +CSSParserImpl::ParseMaskItem(CSSParserImpl::StyleLayersParseState& aState)

Is it possible for this to share code with background?

::: layout/style/nsCSSPropList.h:4014
(Diff revision 4)
> +    "",

Should the new properties be behind a pref for now, or do you think they'll be ready to ship once this lands?  Could you send either an intent to implement, or an intent to implement and ship, to dev-platform, per the policy at https://wiki.mozilla.org/WebAPI/ExposureGuidelines

::: layout/style/nsCSSPropList.h:4045
(Diff revision 4)
> +// TBD
> +// Copy from background-position, which animation type is eStyleAnimType_Custom
> +// Need to do something in StyleAnimationValue::ExtractComputedValue.

Do you need to deal with this TBD?

::: layout/style/nsCSSProps.cpp:865
(Diff revision 4)
> -const KTableValue nsCSSProps::kBackgroundAttachmentKTable[] = {
> -  eCSSKeyword_fixed, NS_STYLE_BG_ATTACHMENT_FIXED,
> -  eCSSKeyword_scroll, NS_STYLE_BG_ATTACHMENT_SCROLL,
> -  eCSSKeyword_local, NS_STYLE_BG_ATTACHMENT_LOCAL,
> +const KTableValue nsCSSProps::kLayerAttachmentKTable[] = {
> +  eCSSKeyword_fixed, NS_STYLE_LAYER_ATTACHMENT_FIXED,
> +  eCSSKeyword_scroll, NS_STYLE_LAYER_ATTACHMENT_SCROLL,
> +  eCSSKeyword_local, NS_STYLE_LAYER_ATTACHMENT_LOCAL,

I think you also need to include the nsStyleConsts.h changes that change the names of these constants in this initial patch; otherwise the patch won't compile on its own.

That said, I'm not sure about the name changes.  I think the current names for both the constants and the tables might be clearer:  I think people have a good idea what background-\* properties are, but less of an idea what yet another meaning for "layer" means here.  I'd be open to a better name, but I tend to think that the current names (NS_STYLE_BG\_\* and kBackground\*) are better than NS_STYLE_LAYER\_\* and kLayer\*.

::: layout/style/nsComputedDOMStylePropertyList.h:65
(Diff revision 4)
> +///// COMPUTED_STYLE_PROP(mask,                    Mask)
> +COMPUTED_STYLE_PROP(mask_clip,                     MaskClip)
> +COMPUTED_STYLE_PROP(mask_image,                    MaskImage)
> +COMPUTED_STYLE_PROP(mask_origin,                   MaskOrigin)
> +COMPUTED_STYLE_PROP(mask_position,                 MaskPosition)
> +COMPUTED_STYLE_PROP(mask_repeat,                   MaskRepeat)
> +COMPUTED_STYLE_PROP(mask_size,                     MaskSize)
> +COMPUTED_STYLE_PROP(mask_mode,                     MaskMode)
> +COMPUTED_STYLE_PROP(mask_composite,                MaskComposite)

I believe this will fail tests because it's out of order.  Have you run this patch through try yet?
https://reviewboard.mozilla.org/r/24369/#review21993

::: layout/style/nsCSSPropList.h:3990
(Diff revision 4)
> +CSS_PROP_SVGRESET(

Also, addition of new properties should have corresponding changes in layout/style/test/property_database.js -- and then the tests should pass with those changes.
Comment on attachment 8683557 [details]
MozReview Request: Bug 686281 - Rename *background* to *imagelayer*; r=dbaron.

https://reviewboard.mozilla.org/r/24371/#review21995

::: layout/base/nsCSSRendering.h:178
(Diff revision 4)
> -  static void ComputeObjectAnchorPoint(const nsStyleBackground::Position& aPos,
> +  static void ComputeObjectAnchorPoint(const nsStyleLayers::Position& aPos,

I don't think the separation between what's in the previous patch and what's in this patch makes any sense; things that are a single logical change that could be in its own patch (e.g., renaming a constant, pulling nsStyleBackground::Layer out into a class that's not within nsStyleBackground) are mixed between the previous patch and this one, thus preventing either from being compiled on their own or understood as a simple change.

This should be merged with the previous patch, but probably some of the initial refactoring steps should then be pulled out into separate patches (such as renaming the constants if you want to keep that renaming, and definitely the refactoring of nsStyleBackground::Layer into something that's reusable outside of nsStyleBackground).
Comment on attachment 8683558 [details]
MozReview Request: Bug 686281 - Implement CSS mask style; r=dbaron.

https://reviewboard.mozilla.org/r/24373/#review21997

::: layout/base/nsCSSRendering.cpp:3081
(Diff revision 4)
> -                   NS_STYLE_BG_ATTACHMENT_LOCAL == aLayer.mAttachment)) {
> +                   NS_STYLE_LAYER_ATTACHMENT_LOCAL == aLayer.mAttachment)) {

This again has tons of changes that should have been in an earlier patch.

I think the important aspects of keeping parts of work separate to make things easy to review are that:
 (1) changes that are big and boring should be separate from changes that are small and interesting
 (2) unless it is hard to do without violating (1), each patch should compile on its own, since that helps regression-finding
Comment on attachment 8683706 [details]
MozReview Request: Bug 686281 - Mask CSS parsing and Mask DOM API. r=dbaron

https://reviewboard.mozilla.org/r/24415/#review21999

This does seem reasonable to have as a separate patch, although I *think* to make it work sensibly on its own, you'd need to have an nsCSSPropList.h change in this patch changing the properties from a temporary eStyleAnimType_None to the eStyleAnimType_Custom that you currently have in the first patch.

::: layout/style/StyleAnimationValue.h:255
(Diff revision 2)
> +    eUnit_MaskPosition, // nsCSSValueList* (never null)

Why does this need to be a separate unit from eUnit_BackgroundPosition?
Attachment #8683706 - Flags: review?(dbaron)
Attachment #8683556 - Attachment description: MozReview Request: Bug 686281- Implement CSS mask properties. r=:dbaron → MozReview Request: Bug 686281- Implement nsStyleLayers and modify nsStyleBackground. r=:dbaron.
Attachment #8683556 - Flags: review?(dbaron)
Comment on attachment 8683556 [details]
MozReview Request: Bug 686281 - Implement nsStyleImageLayers; r=dbaron

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24369/diff/4-5/
Attachment #8683557 - Attachment description: MozReview Request: Bug 686281- Implement CSS mask style. r=:dbaron → MozReview Request: Bug 686281 - Rename *background* to *layer*. r=:dbaron
Attachment #8683557 - Flags: review?(dbaron)
Comment on attachment 8683557 [details]
MozReview Request: Bug 686281 - Rename *background* to *imagelayer*; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24371/diff/4-5/
Attachment #8683558 - Attachment description: MozReview Request: Bug 686281- Implement CSS mask rendering. r=:dbaron → MozReview Request: Bug 686281- Implement CSS mask properties. r=:dbaron
Attachment #8683558 - Flags: review?(dbaron)
Comment on attachment 8683558 [details]
MozReview Request: Bug 686281 - Implement CSS mask style; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24373/diff/4-5/
Attachment #8683706 - Attachment description: MozReview Request: Bug 686281- Implement CSS mask animation. r=:dbaron → MozReview Request: Bug 686281- Implement CSS mask rendering. r=:dbaron
Attachment #8683706 - Flags: review?(dbaron)
Comment on attachment 8683706 [details]
MozReview Request: Bug 686281 - Mask CSS parsing and Mask DOM API. r=dbaron

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24415/diff/2-3/
Bug 686281- Implement CSS mask animation. r=:dbaron
Attachment #8684292 - Flags: review?(dbaron)
Comment on attachment 8683556 [details]
MozReview Request: Bug 686281 - Implement nsStyleImageLayers; r=dbaron

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24369/diff/5-6/
Comment on attachment 8683557 [details]
MozReview Request: Bug 686281 - Rename *background* to *imagelayer*; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24371/diff/5-6/
Comment on attachment 8683558 [details]
MozReview Request: Bug 686281 - Implement CSS mask style; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24373/diff/5-6/
Comment on attachment 8683706 [details]
MozReview Request: Bug 686281 - Mask CSS parsing and Mask DOM API. r=dbaron

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24415/diff/3-4/
Comment on attachment 8684292 [details]
MozReview Request: Bug 686281 - Mask CSS rendering; r=mstange

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24531/diff/1-2/
https://reviewboard.mozilla.org/r/24415/#review21999

> Why does this need to be a separate unit from eUnit_BackgroundPosition?

We don't need it. my bad.
What we need to to change eUnit_BackgroundPosition to eUnit_StyleLayerPosition, but since the name is still under discussion, keep it untouch
Comment on attachment 8683556 [details]
MozReview Request: Bug 686281 - Implement nsStyleImageLayers; r=dbaron

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24369/diff/6-7/
Comment on attachment 8683557 [details]
MozReview Request: Bug 686281 - Rename *background* to *imagelayer*; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24371/diff/6-7/
Attachment #8683557 - Attachment description: MozReview Request: Bug 686281 - Rename *background* to *layer*. r=:dbaron → MozReview Request: Bug 686281- Rename *background* to *layer*. r=:dbaron
Comment on attachment 8683558 [details]
MozReview Request: Bug 686281 - Implement CSS mask style; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24373/diff/6-7/
Comment on attachment 8683706 [details]
MozReview Request: Bug 686281 - Mask CSS parsing and Mask DOM API. r=dbaron

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24415/diff/4-5/
Comment on attachment 8684292 [details]
MozReview Request: Bug 686281 - Mask CSS rendering; r=mstange

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24531/diff/2-3/
https://reviewboard.mozilla.org/r/24371/#review22087

::: layout/base/nsDisplayList.cpp:2372
(Diff revision 7)
> -  if (mBackgroundStyle->mLayers.mLayers.Length() != 1)
> +  if (mBackgroundStyle->Layers().Length() != 1)

Move back to the first patch
https://reviewboard.mozilla.org/r/24373/#review22089

::: layout/style/nsStyleStruct.cpp:1292
(Diff revision 7)
> +  mLayers.mImageCount = 0;

Fix this TODO
Comment on attachment 8683556 [details]
MozReview Request: Bug 686281 - Implement nsStyleImageLayers; r=dbaron

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24369/diff/7-8/
Comment on attachment 8683557 [details]
MozReview Request: Bug 686281 - Rename *background* to *imagelayer*; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24371/diff/7-8/
Comment on attachment 8683558 [details]
MozReview Request: Bug 686281 - Implement CSS mask style; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24373/diff/7-8/
Comment on attachment 8683706 [details]
MozReview Request: Bug 686281 - Mask CSS parsing and Mask DOM API. r=dbaron

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24415/diff/5-6/
Comment on attachment 8684292 [details]
MozReview Request: Bug 686281 - Mask CSS rendering; r=mstange

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24531/diff/3-4/
https://reviewboard.mozilla.org/r/24371/#review21995

> I don't think the separation between what's in the previous patch and what's in this patch makes any sense; things that are a single logical change that could be in its own patch (e.g., renaming a constant, pulling nsStyleBackground::Layer out into a class that's not within nsStyleBackground) are mixed between the previous patch and this one, thus preventing either from being compiled on their own or understood as a simple change.
> 
> This should be merged with the previous patch, but probably some of the initial refactoring steps should then be pulled out into separate patches (such as renaming the constants if you want to keep that renaming, and definitely the refactoring of nsStyleBackground::Layer into something that's reusable outside of nsStyleBackground).

1st patch: Pull out background layer property from nsStyleBackground and put them into nsStyleLayer. Include all necessary change because of this new class.
2st patch: rename work.
https://reviewboard.mozilla.org/r/24371/#review22105

::: layout/style/nsCSSParser.cpp:10521
(Diff revision 8)
> -    return ParseBackgroundRepeat();
> +    return ParseLayerRepeat(eCSSProperty_background_repeat);

Move function parameter change back to the first patch
Comment on attachment 8683556 [details]
MozReview Request: Bug 686281 - Implement nsStyleImageLayers; r=dbaron

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24369/diff/8-9/
Attachment #8683556 - Attachment description: MozReview Request: Bug 686281- Implement nsStyleLayers and modify nsStyleBackground. r=:dbaron. → MozReview Request: Bug 686281 - Implement nsStyleLayers and modify nsStyleBackground. r?:dbaron.
Comment on attachment 8683557 [details]
MozReview Request: Bug 686281 - Rename *background* to *imagelayer*; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24371/diff/8-9/
Attachment #8683557 - Attachment description: MozReview Request: Bug 686281- Rename *background* to *layer*. r=:dbaron → MozReview Request: Bug 686281 - Rename *background* to *layer*. r?:dbaron
Comment on attachment 8683558 [details]
MozReview Request: Bug 686281 - Implement CSS mask style; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24373/diff/8-9/
Attachment #8683558 - Attachment description: MozReview Request: Bug 686281- Implement CSS mask properties. r=:dbaron → MozReview Request: Bug 686281 - Implement CSS mask properties. r?:dbaron
Comment on attachment 8683706 [details]
MozReview Request: Bug 686281 - Mask CSS parsing and Mask DOM API. r=dbaron

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24415/diff/6-7/
Attachment #8683706 - Attachment description: MozReview Request: Bug 686281- Implement CSS mask rendering. r=:dbaron → MozReview Request: Bug 686281 - Implement CSS mask rendering. r?:dbaron
Comment on attachment 8684292 [details]
MozReview Request: Bug 686281 - Mask CSS rendering; r=mstange

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24531/diff/4-5/
Attachment #8684292 - Attachment description: MozReview Request: Bug 686281- Implement CSS mask animation. r=:dbaron → MozReview Request: Bug 686281 - Implement CSS mask animation. r?:dbaron
Comment on attachment 8683556 [details]
MozReview Request: Bug 686281 - Implement nsStyleImageLayers; r=dbaron

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24369/diff/9-10/
Comment on attachment 8683557 [details]
MozReview Request: Bug 686281 - Rename *background* to *imagelayer*; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24371/diff/9-10/
Comment on attachment 8683558 [details]
MozReview Request: Bug 686281 - Implement CSS mask style; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24373/diff/9-10/
Comment on attachment 8683706 [details]
MozReview Request: Bug 686281 - Mask CSS parsing and Mask DOM API. r=dbaron

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24415/diff/7-8/
Comment on attachment 8684292 [details]
MozReview Request: Bug 686281 - Mask CSS rendering; r=mstange

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24531/diff/5-6/
https://reviewboard.mozilla.org/r/24369/#review21991

> Is it possible for this (in Declaration.cpp) to share code with background?

Fixed in the third patch(Bug 686281 - Implement CSS mask properties)

> Is it possible for this to share code with background?

Fixed in the third patch(Bug 686281 - Implement CSS mask properties)

> I think you also need to include the nsStyleConsts.h changes that change the names of these constants in this initial patch; otherwise the patch won't compile on its own.
> 
> That said, I'm not sure about the name changes.  I think the current names for both the constants and the tables might be clearer:  I think people have a good idea what background-\* properties are, but less of an idea what yet another meaning for "layer" means here.  I'd be open to a better name, but I tend to think that the current names (NS_STYLE_BG\_\* and kBackground\*) are better than NS_STYLE_LAYER\_\* and kLayer\*.

How about use StyleLayer to replace Background, SL to replace BG?
And have some place explain what is style layer
https://reviewboard.mozilla.org/r/24369/#review21991

> Should the new properties be behind a pref for now, or do you think they'll be ready to ship once this lands?  Could you send either an intent to implement, or an intent to implement and ship, to dev-platform, per the policy at https://wiki.mozilla.org/WebAPI/ExposureGuidelines

Land this feature with a pref is safer. Will do.
Comment on attachment 8683556 [details]
MozReview Request: Bug 686281 - Implement nsStyleImageLayers; r=dbaron

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24369/diff/10-11/
Comment on attachment 8683557 [details]
MozReview Request: Bug 686281 - Rename *background* to *imagelayer*; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24371/diff/10-11/
Comment on attachment 8683558 [details]
MozReview Request: Bug 686281 - Implement CSS mask style; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24373/diff/10-11/
Comment on attachment 8683706 [details]
MozReview Request: Bug 686281 - Mask CSS parsing and Mask DOM API. r=dbaron

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24415/diff/8-9/
Comment on attachment 8684292 [details]
MozReview Request: Bug 686281 - Mask CSS rendering; r=mstange

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24531/diff/6-7/
Bug 686281 - Implement CSS mask alias. r?:dbaron
Attachment #8684679 - Flags: review?(dbaron)
Bug 686281 - Implement CSS mask test. (WIP)
https://reviewboard.mozilla.org/r/24369/#review21993

> Also, addition of new properties should have corresponding changes in layout/style/test/property_database.js -- and then the tests should pass with those changes.

Will do it in the last patch(Bug 686281 - Implement CSS mask test)
Comment on attachment 8683556 [details]
MozReview Request: Bug 686281 - Implement nsStyleImageLayers; r=dbaron

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24369/diff/11-12/
Comment on attachment 8683557 [details]
MozReview Request: Bug 686281 - Rename *background* to *imagelayer*; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24371/diff/11-12/
Comment on attachment 8683558 [details]
MozReview Request: Bug 686281 - Implement CSS mask style; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24373/diff/11-12/
Comment on attachment 8683706 [details]
MozReview Request: Bug 686281 - Mask CSS parsing and Mask DOM API. r=dbaron

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24415/diff/9-10/
Comment on attachment 8684292 [details]
MozReview Request: Bug 686281 - Mask CSS rendering; r=mstange

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24531/diff/7-8/
Comment on attachment 8684679 [details]
MozReview Request: Bug 686281 - Mask CSS animation; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24617/diff/1-2/
Comment on attachment 8684680 [details]
MozReview Request: Bug 686281 - Mask CSS webkit-alias; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24619/diff/1-2/
Comment on attachment 8683556 [details]
MozReview Request: Bug 686281 - Implement nsStyleImageLayers; r=dbaron

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24369/diff/12-13/
Comment on attachment 8683557 [details]
MozReview Request: Bug 686281 - Rename *background* to *imagelayer*; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24371/diff/12-13/
Comment on attachment 8683558 [details]
MozReview Request: Bug 686281 - Implement CSS mask style; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24373/diff/12-13/
Comment on attachment 8683706 [details]
MozReview Request: Bug 686281 - Mask CSS parsing and Mask DOM API. r=dbaron

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24415/diff/10-11/
Comment on attachment 8684292 [details]
MozReview Request: Bug 686281 - Mask CSS rendering; r=mstange

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24531/diff/8-9/
Comment on attachment 8684679 [details]
MozReview Request: Bug 686281 - Mask CSS animation; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24617/diff/2-3/
Comment on attachment 8684680 [details]
MozReview Request: Bug 686281 - Mask CSS webkit-alias; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24619/diff/2-3/
Bug 686281 - (Bug Fix: will-change-stacking-context-mask-1.html fail) Expands shorthad to longhand. r?dbaron
Attachment #8684815 - Flags: review?(dbaron)
Comment on attachment 8683556 [details]
MozReview Request: Bug 686281 - Implement nsStyleImageLayers; r=dbaron

https://reviewboard.mozilla.org/r/24369/#review22177

I've only been partway through this patch so far, but here are my comments so far.

From what I've seen, I think this should have been at least 4 separate patches:
 (1) refactor part of nsStyleBackground into nsStyleLayers (or whatever we choose to call it
 (2) add mask properties to the layer parsing code in nsCSSParser
 (3) add mask properties to nsStyleLayers
 (4) the assertion about rule node bits
 
There might even be more than that, since I haven't been through the whole thing yet.

Given that I expect this will go through a few cycles of review, and I think it would make re-reviews a lot easier if things are more separated, I think it's at least worth splitting (2) and (4) into separate patches.  IT's probably ok for (1) and (3) to stay together since they're harder to separate.


I'd note that there are some comments about naming here; we might want to get to an agreement about what names to use before you go through the work of making a bunch more substitutions in the patch.

::: layout/base/nsCSSRendering.cpp:2900
(Diff revision 13)
> +  const nsStyleLayers &layers = aBackgroundSC->StyleBackground()->mLayers;

Current style rules prefer & and * before the space rather than after.  (Yes, there's plenty of old code around that doesn't match, but when you're introducing new code you should follow the rule.)

::: layout/base/nsCSSRendering.cpp:2962
(Diff revision 13)
> -    // Return if there are no background layers, all work from this point
> +    // Return if there are no style layers, all work from this point

The comment should stay as it is; it's specific to backgrounds.

::: layout/style/nsStyleStruct.h:98
(Diff revision 13)
> +static_assert(NS_RULE_NODE_IS_ANIMATION_RULE ==
> +  (1 << (nsStyleStructID_Inherited_Count + nsStyleStructID_Reset_Count)),
> +  "NS_RULE_NODE_IS_ANIMATION_RULE cannot fit the number of style struct.");
> +

I'm not sure why this is in this patch.  (I guess you were initially adding a new style struct.)

But if you put it in a separate patch, r=dbaron on that separate patch if you change (nsStyleStructID_Inherited_Count + nsStyleStructID_Reset_Count) to just be nsStyleStructID_Length.

::: layout/style/nsStyleStruct.h:390
(Diff revision 13)
> -struct nsStyleBackground {
> +struct nsStyleLayers {

As I said in comment 58, I'm not happy about this name, especially given the other things we have called layers.

I think nsStyleImageLayers (roughly heycam's suggestion) would be better than nsStyleLayers.  But we might want to agree on new names before you make big changes to the patches and then have to do them again.  (Although depending on what you're using for the patches, name search-and-replace might be very easy; it is with mq.)

::: layout/style/nsStyleStruct.h:507
(Diff revision 13)
> -    uint8_t mAttachment;                // [reset] See nsStyleConsts.h
> -    uint8_t mClip;                      // [reset] See nsStyleConsts.h
> -    uint8_t mOrigin;                    // [reset] See nsStyleConsts.h
> -    uint8_t mBlendMode;                 // [reset] See nsStyleConsts.h
> -    Repeat mRepeat;                     // [reset] See nsStyleConsts.h
> -    Position mPosition;                 // [reset]
> -    nsStyleImage mImage;                // [reset]
> -    Size mSize;                         // [reset]
> +    nsStyleImage  mImage;         // [reset]
> +    uint8_t       mClip;          // [reset] See nsStyleConsts.h
> +    uint8_t       mOrigin;        // [reset] See nsStyleConsts.h
> +    Repeat        mRepeat;        // [reset] See nsStyleConsts.h
> +    Position      mPosition;      // [reset] See nsStyleConsts.h
> +    Size          mSize;          // [reset]
> +    uint8_t       mAttachment;    // [reset] See nsStyleConsts.h
> +                                  // background layer property
> +    uint8_t       mBlendMode;     // [reset] See nsStyleConsts.h
> +                                  // background layer property
> +    uint8_t       mComposite;     // [reset] See nsStyleConsts.h
> +                                  // mask layer property
> +    uint8_t       mMaskMode;      // [reset] See nsStyleConsts.h
> +                                  // mask layer property

It would have been clearer to do the refactoring into its own class in one patch, and the addition of new properties for masks in a separate patch.

I guess I can deal with them being in the same patch, at least so far.

However, you should still pack all of the uint8_t values together, since the struct will pack better that way.  Repeat is a pair of uint8_t values, so it should be next to them.  Position and Size are both pairs of nsStyleCoord, so they have pointer size and alignment, and should be next to each other.  nsStyleImage also has pointer alignment.  So mImage, mPosition, and mSize should be next to each other either at the start or the end, and the other values should all be next to each other.  Otherwise you'll end up with holes in the struct.

::: layout/style/nsStyleStruct.h:513
(Diff revision 13)
> -    nsStyleImage mImage;                // [reset]
> -    Size mSize;                         // [reset]
> +    uint8_t       mAttachment;    // [reset] See nsStyleConsts.h
> +                                  // background layer property
> +    uint8_t       mBlendMode;     // [reset] See nsStyleConsts.h
> +                                  // background layer property
> +    uint8_t       mComposite;     // [reset] See nsStyleConsts.h
> +                                  // mask layer property
> +    uint8_t       mMaskMode;      // [reset] See nsStyleConsts.h
> +                                  // mask layer property

These comments that say "background layer property" and "mask layer property" should explain substantially more clearly what that means, and what rules the variables obey in the case when they're not used.

::: layout/style/nsStyleStruct.h:600
(Diff revision 13)
> +  #define NS_FOR_VISIBLE_STYLELAYER_BACK_TO_FRONT(var_, layers_) \
> +    for (uint32_t var_ = layers_.mImageCount; var_-- != 0; )
> +  #define NS_FOR_VISIBLE_STYLELAYER_BACK_TO_FRONT_WITH_RANGE(var_, layers_, start_, count_) \
> +    NS_ASSERTION((int32_t)(start_) >= 0 && (uint32_t)(start_) < (layers_.mImageCount), "Invalid layer start!"); \

These should probably be called NS_FOR_VISIBLE_STYLEIMAGELAYER..., if we want to use nsStyleImageLayers as the name.

I'm not really happy about that name, though, and would like to find something better.
https://reviewboard.mozilla.org/r/24369/#review22177

> I'm not sure why this is in this patch.  (I guess you were initially adding a new style struct.)
> 
> But if you put it in a separate patch, r=dbaron on that separate patch if you change (nsStyleStructID_Inherited_Count + nsStyleStructID_Reset_Count) to just be nsStyleStructID_Length.

Will do. When I added a new style(nsStyleMask), to fix an assertion crash at [1], I spend almost two days to figue out need to left shift NS_RULE_NODE_IS_ANIMATION_RULE one bit. So, I think it's better to have a static assertion here.

[1] https://dxr.mozilla.org/mozilla-central/source/layout/style/nsRuleNode.h#863
https://reviewboard.mozilla.org/r/24369/#review22177

> The comment should stay as it is; it's specific to backgrounds.

In fact, I use this function nsCSSRendering::PaintBackgroundWithSC to generate mask image as well. I will rollback all this name change before we make the decision of the name of this new concept.
(In reply to C.J. Ku[:cjku] from comment #126)
> I will rollback all this name change before we
> make the decision of the name of this new concept.

I'd recommend not doing that; it should be easy to substitute one new name for another in patches, but it's a little harder to regenerate a list of all the correct places to substitute (at least if there's a risk of false matches).
https://reviewboard.mozilla.org/r/24369/#review22281

::: layout/base/nsCSSRendering.cpp:1749
(Diff revision 13)
> -nsCSSRendering::GetBackgroundClip(const nsStyleBackground::Layer& aLayer,
> +nsCSSRendering::GetBackgroundClip(const nsStyleLayers::Layer& aLayer,
> -                                  nsIFrame* aForFrame, const nsStyleBorder& aBorder,
> +                             nsIFrame* aForFrame, const nsStyleBorder& aBorder,
> -                                  const nsRect& aBorderArea, const nsRect& aCallerDirtyRect,
> +                             const nsRect& aBorderArea, const nsRect& aCallerDirtyRect,
> -                                  bool aWillPaintBorder, nscoord aAppUnitsPerPixel,
> +                             bool aWillPaintBorder, nscoord aAppUnitsPerPixel,
> -                                  /* out */ BackgroundClipState* aClipState)
> +                             /* out */ BackgroundClipState* aClipState)

This should not be changing indentation (although maybe a later patch should be).

::: layout/base/nsCSSRendering.cpp:4840
(Diff revision 13)
> -      //     Make sure to change nsStyleBackground::Size::DependsOnFrameSize
> +      //     Make sure to change nsStyleLayer::Size::DependsOnFrameSize

Missed the "s" here in nsStyleLayers

::: layout/style/nsCSSParser.cpp:824
(Diff revision 13)
> +    nsCSSValueList* mComposite;
> +    nsCSSValueList* mMode;

This should have comments pointing out properties that are only used for background or for mask.

A later (probably separate) patch should also rename BackgroundParseState, perhaps to ImageLayersShorthandParseState.

::: layout/style/nsRuleNode.cpp:6698
(Diff revision 13)
> -  for (uint32_t i = 0; i < bg->mImageCount; ++i)
> +  for (uint32_t i = 0; i < bg->mLayers.mImageCount; ++i)
>      bg->mLayers[i].TrackImages(aContext->PresContext());

This should use nsStyleLayers::TrackImages, rather than calling TrackImages on each layer directly.

::: layout/style/nsStyleConsts.h:311
(Diff revision 13)
> +// See nsStyleLayers
> +#define NS_STYLE_LAYER_MODE_ALPHA               0
> +#define NS_STYLE_LAYER_MODE_LUMINANCE           1
> +#define NS_STYLE_LAYER_MODE_AUTO                2

Probably best to rename LAYER_MODE -> MASK_MODE, since that's what they're used for.

::: layout/style/nsStyleConsts.h:1120
(Diff revision 13)
> +// composite
> +#define NS_STYLE_COMPOSITE_ADD                      0
> +#define NS_STYLE_COMPOSITE_SUBTRACT                 1
> +#define NS_STYLE_COMPOSITE_INTERSECT                2
> +#define NS_STYLE_COMPOSITE_EXCLUDE                  3

Probably COMPOSITE -> COMPOSITE_MODE

::: layout/style/nsStyleStruct.h:592
(Diff revision 13)
> +  const Layer &operator [](uint32_t aIndex) const {
> +    return mLayers[aIndex];
> +  }
> +
> +  Layer &operator [](uint32_t aIndex) {
> +    return mLayers[aIndex];
> +  }

Does anything use these?  If not, I think it's better not to add them, just because it's confusing to have too many ways to do the same thing.

::: layout/style/nsStyleStruct.h:600
(Diff revision 13)
> +  #define NS_FOR_VISIBLE_STYLELAYER_BACK_TO_FRONT(var_, layers_) \
> +    for (uint32_t var_ = layers_.mImageCount; var_-- != 0; )
> +  #define NS_FOR_VISIBLE_STYLELAYER_BACK_TO_FRONT_WITH_RANGE(var_, layers_, start_, count_) \
> +    NS_ASSERTION((int32_t)(start_) >= 0 && (uint32_t)(start_) < (layers_.mImageCount), "Invalid layer start!"); \

These should be renamed to NS_FOR_VISIBLE_IMAGE_LAYERS_... (with an S)

::: layout/style/nsStyleStruct.h:643
(Diff revision 13)
> +  const nsAutoTArray<nsStyleLayers::Layer, 1>& Layers() const {
> +    return mLayers.mLayers;
> +  }
> +
> +  nsAutoTArray<nsStyleLayers::Layer, 1>& Layers()  {
> +    return mLayers.mLayers;
> +  }

I find it confusing to have a function called Layers() that returns something different from mLayers (in particular, it returns mLayers.mLayers).  I think it's probably better to explicitly write mLayers.mLayers than to write Layers(), so I'd prefer to remove these, although I'm ok with it if it's hard to clean up.

::: layout/style/nsStyleStruct.cpp:2316
(Diff revision 13)
> -  NS_ASSERTION(onlyLayer, "auto array must have room for 1 element");
> -  onlyLayer->SetInitialValues();
> +  // We always have at least one background layer.
> +  mLayers.mImageCount = 1;

This is true for masks too; you should just leave the initialization of mImageCount to 1 in nsStyleImageLayer's constructor, and move the comment there.

::: layout/style/nsStyleStruct.cpp:2346
(Diff revision 13)
>    const nsStyleBackground* moreLayers =
> -    mImageCount > aOther.mImageCount ? this : &aOther;
> +    mLayers.mImageCount > aOther.mLayers.mImageCount ? this : &aOther;
>    const nsStyleBackground* lessLayers =
> -    mImageCount > aOther.mImageCount ? &aOther : this;
> +    mLayers.mImageCount > aOther.mLayers.mImageCount ? &aOther : this;

It would be better to make these const nsStyleImageLayers\* instead of const nsStyleBackground\*; then you won't need changes to the code below (or use of the operator[] above that I asked you to remove).

::: layout/style/nsStyleStruct.cpp:2410
(Diff revision 13)
> +// --------------------
> +// nsStyleLayers
> +//

It would be better to have this code before nsStyleBackground, to match the order in the .h file.

::: layout/style/nsStyleStruct.cpp:2420
(Diff revision 13)
> +  , mImageCount(0)

Why are you now initializing mImageCount to 0?  That seems wrong.  There's always at least one item in the computed value of the background-image or mask-image property.

::: layout/style/nsStyleStruct.cpp:2637
(Diff revision 13)
>  nsChangeHint
> -nsStyleBackground::Layer::CalcDifference(const Layer& aOther) const
> +nsStyleLayers::Layer::CalcDifference(const nsStyleLayers::Layer& aOther) const

You need to adjust CalcDifference to produce an appropriate change hint for mMaskMode and mComposite.  (It's ok if that's in another patch, but it better be somewhere.)

I have now finished going through this patch, so hopefully that's all the comments I have and I won't notice anything more when you post a revised version.
Oh, and for naming, I think we should do:
  nsStyleLayers -> nsStyleImageLayers
  NS_FOR_VISIBLE_STYLELAYER... -> NS_FOR_VISIBLE_IMAGE_LAYERS... (note LAYERS not LAYER)

I think it's probably best to just go with that for now; if somebody comes up with a better idea later, we can rename it.
(In reply to David Baron [:dbaron] ⌚UTC+8 from comment #129)
> Oh, and for naming, I think we should do:
>   nsStyleLayers -> nsStyleImageLayers
>   NS_FOR_VISIBLE_STYLELAYER... -> NS_FOR_VISIBLE_IMAGE_LAYERS... (note
> LAYERS not LAYER)
> 
> I think it's probably best to just go with that for now; if somebody comes
> up with a better idea later, we can rename it.

Though I just replied to the dev-platform thread, so maybe somebody will have a better idea.
Comment on attachment 8683558 [details]
MozReview Request: Bug 686281 - Implement CSS mask style; r=dbaron.

https://reviewboard.mozilla.org/r/24373/#review22283

::: layout/generic/nsFrame.cpp:812
(Diff revision 13)
> +  const nsStyleSVGReset *oldSVGReset = aOldStyleContext ?
> +                                   aOldStyleContext->StyleSVGReset() :
> +                                   nullptr;
> +  const nsStyleSVGReset *newSVGReset = StyleSVGReset();

fix indentation of 2nd and 3rd lines, and also use "\* " rather than " \*"

::: layout/generic/nsFrame.cpp:834
(Diff revision 13)
> +    if (!oldBG || i >= oldBG->mLayers.mImageCount ||
> +        !newSVGReset->mLayers[i].mImage.ImageDataEquals(oldBG->mLayers[i].mImage)) {

These three references to oldBG should be oldSVGReset!  (Currently this code will produce pretty bizarre results.)

::: layout/style/Declaration.h:352
(Diff revision 13)
> +  template <typename T>
> +  void GetStyleLayerValue(nsCSSCompressedDataBlock *data,
> +                          nsAString& aValue,
> +                          nsCSSValue::Serialization aSerialization) const;

This would be better done with an array of nsCSSProperty (or, better, a struct of nsCSSProperty) that is static const data than with templates.  You don't actually need the code to be generated twice; you just need a list of 10 enum values.

::: layout/style/Declaration.cpp:223
(Diff revision 13)
> +    image->mValue.AppendToString(eCSSProperty_mask_image, aValue,
> +                                 aSerialization);
> +
> +    aValue.Append(char16_t(' '));
> +    repeat->mXValue.AppendToString(eCSSProperty_mask_repeat, aValue,
> +                                   aSerialization);
> +    if (repeat->mYValue.GetUnit() != eCSSUnit_Null) {
> +      repeat->mYValue.AppendToString(eCSSProperty_mask_repeat, aValue,
> +                                     aSerialization);

For these common properties (and background and size and clip and origin below), I think it's better to use the eCSSProperty_background\* constants, since those are better-known.

This will also make this code easier to review since the "moved" chunks will be more consistent.

::: layout/style/Declaration.cpp:308
(Diff revision 13)
> +    if (!image) {
> +      if (repeat || position || clip || origin || size) {
> +        // Uneven length lists, so can't be serialized as shorthand.
> +        aValue.Truncate();
> +        return;
> +      }
> +      break;
> +    }
> +
> +    if (!repeat || !position || !clip || !origin || !size) {
> +      // Uneven length lists, so can't be serialized as shorthand.
> +      aValue.Truncate();
> +      return;
> +    }

These conditions all need to test all of the values allowed for the relevant property.  So you need to not remove the attachment test for backgrounds, and you need to test composite and mode for mask.

::: layout/style/Declaration.cpp:595
(Diff revision 13)
> -    case eCSSProperty_background: {
> -      // We know from above that all subproperties were specified.
> +    case eCSSProperty_mask: {
> +      GetStyleLayerValue<nsStyleSVGReset>(data, aValue, aSerialization);

Please put mask after background rather than before, since it's newer and less well known.

::: layout/style/Declaration.cpp:596
(Diff revision 13)
> -      // We know from above that all subproperties were specified.
> +      GetStyleLayerValue<nsStyleSVGReset>(data, aValue, aSerialization);
> -      // However, we still can't represent that in the shorthand unless
> -      // they're all lists of the same length.  So if they're different
> -      // lengths, we need to bail out.
> -      // We also need to bail out if an item has background-clip and
> -      // background-origin that are different and not the default
> -      // values.  (We omit them if they're both default.)

Please move this comment rather than deleting it.

::: layout/style/nsCSSParser.cpp:813
(Diff revision 13)
> -  bool ParseBackground();
> +  template <typename T>
> +  bool ParseStyleLayer();

Again, this should be doable using an array or struct of data rather than templates, which will lead to substantially smaller code.

::: layout/style/nsCSSParser.cpp:10962
(Diff revision 13)
>    // Check first for inherit/initial/unset.
> +  if (StyleLayerPropTrait<T>::color != eCSSProperty_UNKNOWN) {

Why this test?  You should accept inherit/initial/unset for 'mask' just like for 'background'.

::: layout/style/nsCSSParser.cpp:10967
(Diff revision 13)
> -           nsCSSProps::SubpropertyEntryFor(eCSSProperty_background);
> +             nsCSSProps::SubpropertyEntryFor(StyleLayerPropTrait<T>::shortHand);

Write "shorthand" with a lowercase h instead of capital H.

(In the struct instead of the template, too.)

::: layout/style/nsCSSParser.cpp:10988
(Diff revision 13)
> +    if (StyleLayerPropTrait<T>::color != eCSSProperty_UNKNOWN) {
> -    // If we saw a color, this must be the last item.
> +      // If we saw a color, this must be the last item.
> -    if (color.GetUnit() != eCSSUnit_Null) {
> +      if (color.GetUnit() != eCSSUnit_Null) {

You should be able to assert this rather than test it, since ParseStyleLayerItem shouldn't produce a color when parsing masks.

::: layout/style/nsCSSParser.cpp:11000
(Diff revision 13)
> +#define APPENDNEXT(_propID, _prop, _propType) \

\_ at the end rather than the beginning of macro parameters, please.  (\_ at the beginning of identifiers is reserved for the implementation.)

Also probably rename prop\_ to propMember\_ or similar.

::: layout/style/nsCSSParser.cpp:11025
(Diff revision 13)
> +#define APPENDVALUE(_propID, _propValue) \

again, _ at the end

::: layout/style/nsCSSParser.cpp:11076
(Diff revision 13)
> +template <typename T>
>  bool
> -CSSParserImpl::ParseBackgroundItem(CSSParserImpl::StyleLayersParseState& aState)
> +CSSParserImpl::ParseStyleLayerItem(CSSParserImpl::StyleLayersParseState& aState)

again, no need for templatization

::: layout/style/nsCSSParser.cpp:11232
(Diff revision 13)
> +      } else if (StyleLayerPropTrait<T>::mode!= eCSSProperty_UNKNOWN &&

space before !=

::: layout/style/nsCSSParser.cpp:11384
(Diff revision 13)
> -// This function is very similar to ParseScrollSnapCoordinate,
> -// ParseBackgroundList, and ParseBackgroundSize.
> +// This function is very similar to ParseScrollSnapCoordinate
> +// and ParseStyleLayerSize.

I think it's still similar, right?  Shouldn't this just substitute the new name?

::: layout/style/nsCSSParser.cpp:11702
(Diff revision 13)
>  // This function is very similar to ParseScrollSnapCoordinate,
> -// ParseBackgroundList, and ParseBackgroundPosition.
> +// and ParseLayerPosition.

same here

::: layout/style/nsCSSProps.cpp:915
(Diff revision 13)
> +const KTableValue nsCSSProps::kLayerModeKTable[] = {
> +  eCSSKeyword_alpha, NS_STYLE_LAYER_MODE_ALPHA,
> +  eCSSKeyword_luminance, NS_STYLE_LAYER_MODE_LUMINANCE,
> +  eCSSKeyword_auto, NS_STYLE_LAYER_MODE_AUTO,
> +  eCSSKeyword_UNKNOWN,-1
> +};

It's not acceptable to have a value called 'auto' here, since that makes parsing the shorthand ambiguous.  This is a spec bug, which I reported at https://lists.w3.org/Archives/Public/public-fx/2015OctDec/0021.html

Could you:
 (1) file a bug on shipping CSS masks, with this bug blocking the new bug
 (2) file a bug on renaming 'auto', marked as blocking the bug from (1), and
 (3) add a FIXME comment here pointing to the bug in (2)

I'm only partly done with this review; I need to continue starting at nsCSSPropList.h.
Attachment #8683558 - Flags: review?(dbaron)
Comment on attachment 8683556 [details]
MozReview Request: Bug 686281 - Implement nsStyleImageLayers; r=dbaron

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24369/diff/13-14/
Attachment #8683556 - Attachment description: MozReview Request: Bug 686281 - Implement nsStyleLayers and modify nsStyleBackground. r?:dbaron. → MozReview Request: Bug 686281 - Create nsStyleImageLayers. r?:dbaron
Attachment #8683556 - Flags: review?(dbaron)
Comment on attachment 8683557 [details]
MozReview Request: Bug 686281 - Rename *background* to *imagelayer*; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24371/diff/13-14/
Attachment #8683557 - Attachment description: MozReview Request: Bug 686281 - Rename *background* to *layer*. r?:dbaron → MozReview Request: Bug 686281 - Integrate nsStyleImageLayers with nsStyleBackground. r?:dbaron.
Comment on attachment 8683558 [details]
MozReview Request: Bug 686281 - Implement CSS mask style; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24373/diff/13-14/
Attachment #8683558 - Attachment description: MozReview Request: Bug 686281 - Implement CSS mask properties. r?:dbaron → MozReview Request: Bug 686281 - Rename *background* to *imagelayer*. r?:dbaron
Attachment #8683558 - Flags: review?(dbaron)
Comment on attachment 8683706 [details]
MozReview Request: Bug 686281 - Mask CSS parsing and Mask DOM API. r=dbaron

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24415/diff/11-12/
Attachment #8683706 - Attachment description: MozReview Request: Bug 686281 - Implement CSS mask rendering. r?:dbaron → MozReview Request: Bug 686281 - Implement CSS mask prop. r?:dbaron
Comment on attachment 8684292 [details]
MozReview Request: Bug 686281 - Mask CSS rendering; r=mstange

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24531/diff/9-10/
Attachment #8684292 - Attachment description: MozReview Request: Bug 686281 - Implement CSS mask animation. r?:dbaron → MozReview Request: Bug 686281 - Implement CSS mask rendering. r?:dbaron
Comment on attachment 8684679 [details]
MozReview Request: Bug 686281 - Mask CSS animation; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24617/diff/3-4/
Attachment #8684679 - Attachment description: MozReview Request: Bug 686281 - Implement CSS mask alias. r?:dbaron → MozReview Request: Bug 686281 - Implement CSS mask animation. r?:dbaron
Comment on attachment 8684680 [details]
MozReview Request: Bug 686281 - Mask CSS webkit-alias; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24619/diff/3-4/
Attachment #8684680 - Attachment description: MozReview Request: Bug 686281 - Implement CSS mask test. (WIP) → MozReview Request: Bug 686281 - Implement CSS mask alias. r?:dbaron
Comment on attachment 8684815 [details]
MozReview Request: Bug 686281 - Mask mochitest; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24647/diff/1-2/
Attachment #8684815 - Attachment description: MozReview Request: Bug 686281 - (Bug Fix: will-change-stacking-context-mask-1.html fail) Expands shorthad to longhand. r?dbaron → MozReview Request: Bug 686281 - Implement CSS mask test. (WIP)
Bug 686281 - (Bug Fix: will-change-stacking-context-mask-1.html fail) Expands shorthand prop to longhands. r?dbaron
Attachment #8685498 - Flags: review?(dbaron)
Bug 686281 - A static assertion to keep the rightness of NS_RULE_NODE_IS_ANIMATION_RULE. r=:dbaron.
Attachment #8683556 - Attachment description: MozReview Request: Bug 686281 - Create nsStyleImageLayers. r?:dbaron → MozReview Request: Bug 686281 - Create nsStyleImageLayers. r?:dbaron.
Comment on attachment 8683556 [details]
MozReview Request: Bug 686281 - Implement nsStyleImageLayers; r=dbaron

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24369/diff/14-15/
Comment on attachment 8683557 [details]
MozReview Request: Bug 686281 - Rename *background* to *imagelayer*; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24371/diff/14-15/
Comment on attachment 8683558 [details]
MozReview Request: Bug 686281 - Implement CSS mask style; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24373/diff/14-15/
Comment on attachment 8683706 [details]
MozReview Request: Bug 686281 - Mask CSS parsing and Mask DOM API. r=dbaron

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24415/diff/12-13/
Comment on attachment 8684292 [details]
MozReview Request: Bug 686281 - Mask CSS rendering; r=mstange

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24531/diff/10-11/
Comment on attachment 8684679 [details]
MozReview Request: Bug 686281 - Mask CSS animation; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24617/diff/4-5/
Comment on attachment 8684680 [details]
MozReview Request: Bug 686281 - Mask CSS webkit-alias; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24619/diff/4-5/
Comment on attachment 8684815 [details]
MozReview Request: Bug 686281 - Mask mochitest; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24647/diff/2-3/
Comment on attachment 8685498 [details]
MozReview Request: Bug 686281 - Expands will-change of a shorthand prop to longhand ones; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24803/diff/1-2/
Comment on attachment 8685499 [details]
MozReview Request: Bug 686281 - A static assertion to keep value correctness of NS_RULE_NODE_IS_ANIMATION_RULE; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24805/diff/1-2/
Comment on attachment 8683556 [details]
MozReview Request: Bug 686281 - Implement nsStyleImageLayers; r=dbaron

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24369/diff/15-16/
Comment on attachment 8683557 [details]
MozReview Request: Bug 686281 - Rename *background* to *imagelayer*; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24371/diff/15-16/
Comment on attachment 8683558 [details]
MozReview Request: Bug 686281 - Implement CSS mask style; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24373/diff/15-16/
Attachment #8683558 - Attachment description: MozReview Request: Bug 686281 - Rename *background* to *imagelayer*. r?:dbaron → MozReview Request: Bug 686281 - Rename *background* to *imagelayer*. r?:dbaron.
Attachment #8683706 - Attachment description: MozReview Request: Bug 686281 - Implement CSS mask prop. r?:dbaron → MozReview Request: Bug 686281 - Implement CSS mask prop. r?:dbaron.
Comment on attachment 8683706 [details]
MozReview Request: Bug 686281 - Mask CSS parsing and Mask DOM API. r=dbaron

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24415/diff/13-14/
Comment on attachment 8684292 [details]
MozReview Request: Bug 686281 - Mask CSS rendering; r=mstange

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24531/diff/11-12/
Attachment #8684292 - Attachment description: MozReview Request: Bug 686281 - Implement CSS mask rendering. r?:dbaron → MozReview Request: Bug 686281 - Implement CSS mask rendering. r?:dbaron.
Comment on attachment 8684679 [details]
MozReview Request: Bug 686281 - Mask CSS animation; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24617/diff/5-6/
Comment on attachment 8684680 [details]
MozReview Request: Bug 686281 - Mask CSS webkit-alias; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24619/diff/5-6/
Comment on attachment 8684815 [details]
MozReview Request: Bug 686281 - Mask mochitest; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24647/diff/3-4/
Comment on attachment 8685498 [details]
MozReview Request: Bug 686281 - Expands will-change of a shorthand prop to longhand ones; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24803/diff/2-3/
Comment on attachment 8685499 [details]
MozReview Request: Bug 686281 - A static assertion to keep value correctness of NS_RULE_NODE_IS_ANIMATION_RULE; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24805/diff/2-3/
Comment on attachment 8683706 [details]
MozReview Request: Bug 686281 - Mask CSS parsing and Mask DOM API. r=dbaron

https://reviewboard.mozilla.org/r/24415/#review22425

This is continued from comment 131, though my comments on the first half of the patch are now associated with a different patch in reviewboard, since reviewboard was confused by the patch addition.

::: layout/style/nsCSSPropList.h:3984
(Diff revision 13)
> -CSS_PROP_SVGRESET(
> +CSS_PROP_SHORTHAND(
>      mask,
>      mask,
>      Mask,
> -    CSS_PROPERTY_PARSE_VALUE |
> -        CSS_PROPERTY_CREATES_STACKING_CONTEXT,
> +    CSS_PROPERTY_PARSE_FUNCTION,
> +    "layout.css.masking.enabled")

Making the shorthand conditional on the pref doesn't work, because that means you're disabling our existing support for 'mask'.

If you want a pref, it's going to be extra work to make the existing stuff still work, but not enable the new stuff.

The other option is to just try not to land until you're reasonably confident it will be ready to ship in the same release.

::: layout/style/nsCSSPropList.h:3996
(Diff revision 13)
> +        CSS_PROPERTY_APPLIES_TO_FIRST_LETTER_AND_FIRST_LINE |
> +        CSS_PROPERTY_APPLIES_TO_PLACEHOLDER |

I think all of these properties should not apply to ::first-line, ::first-letter, and ::placeholder.  Relevant specs are:
https://drafts.csswg.org/css-pseudo/#first-line-styling
https://drafts.csswg.org/css-pseudo/#first-letter-styling
https://drafts.fxtf.org/css-masking-1/ (which doesn't mention first-line or first-letter)

::: layout/style/nsCSSPropList.h:3999
(Diff revision 13)
> +        CSS_PROPERTY_IGNORED_WHEN_COLORS_DISABLED |

I don't think you want CSS_PROPERTY_IGNORED_WHEN_COLORS_DISABLED for any mask properties.

::: layout/style/nsCSSPropList.h:4002
(Diff revision 13)
> +    VARIANT_IMAGE,

Please keep the "// used by list parsing" comment here.

::: layout/style/nsCSSPropList.h:4006
(Diff revision 13)
>  CSS_PROP_SVGRESET(

please list these properties in alphabetical order in nsCSSPropList.h

::: layout/style/nsCSSPropList.h:4053
(Diff revision 13)
> +        CSS_PROPERTY_UNITLESS_LENGTH_QUIRK |

The unitless length quirk should not apply to any new properties.

::: layout/style/nsComputedDOMStyle.h
(Diff revision 13)
> -  mozilla::dom::CSSValue* DoGetMask();

I think you need to keep the mask getter, since it used to be a longhand, but it should return null when the shorthand can't express the values.  See DoGetOverflow for an example.  (You need to check if any of the properties other than mask-image have non-initial values, or if mask-image has a non-1 length.)

::: layout/style/nsComputedDOMStyle.cpp:2148
(Diff revision 13)
> +nsComputedDOMStyle::DoGetMaskImage()
> +{
> +  const nsStyleSVGReset* svgReset = StyleSVGReset();
> +
> +  nsDOMCSSValueList *valueList = GetROCSSValueList(true);
> +
> +  for (uint32_t i = 0, i_end = svgReset->mLayers.mImageCount; i < i_end; ++i) {
> +    nsROCSSPrimitiveValue *val = new nsROCSSPrimitiveValue;
> +    valueList->AppendCSSValue(val);
> +
> +    const nsStyleImage& image = svgReset->mLayers.mLayers[i].mImage;
> +    SetValueToStyleImage(image, val);
> +  }
> +
> +  return valueList;

This should share code with DoGetBackgroundImage.

::: layout/style/nsComputedDOMStyle.cpp:2256
(Diff revision 13)
>  CSSValue*
> +nsComputedDOMStyle::DoGetMaskPosition()
> +{
> +  const nsStyleSVGReset* svgReset = StyleSVGReset();
> +
> +  nsDOMCSSValueList *valueList = GetROCSSValueList(true);
> +
> +  for (uint32_t i = 0, i_end = svgReset->mLayers.mPositionCount; i < i_end; ++i) {
> +    nsDOMCSSValueList *itemList = GetROCSSValueList(false);
> +    valueList->AppendCSSValue(itemList);
> +
> +    SetValueToPosition(svgReset->mLayers.mLayers[i].mPosition, itemList);
> +  }
> +
> +  return valueList;
> +}

This should share code with DoGetBackgroundPosition.

::: layout/style/nsComputedDOMStyle.cpp:2320
(Diff revision 13)
> +
> +CSSValue*
> +nsComputedDOMStyle::DoGetMaskRepeat()

This should share code with DoGetBackgroundRepeat.

::: layout/style/nsComputedDOMStyle.cpp:2451
(Diff revision 13)
>  CSSValue*
> +nsComputedDOMStyle::DoGetMaskSize()
> +{

This should share code with DoGetBackgroundSize.

::: layout/style/nsComputedDOMStylePropertyList.h:314
(Diff revision 13)
> -COMPUTED_STYLE_PROP(mask,                          Mask)
> +//// COMPUTED_STYLE_PROP(mask,                     Mask)

As I said above, this needs to stay (like overflow) because it was once a longhand.

::: layout/style/nsRuleNode.h:760
(Diff revision 13)
>    const void*
> +    ComputeMaskData(void* aStartStruct,
> +                        const nsRuleData* aRuleData,
> +                        nsStyleContext* aContext, nsRuleNode* aHighestNode,
> +                        RuleDetail aRuleDetail,
> +                        const mozilla::RuleNodeCacheConditions aConditions);
> +
> +  const void*

This is obsolete, you can remove it now that you no longer have a separate style struct.

::: layout/style/nsRuleNode.cpp:1917
(Diff revision 13)
> +static const uint32_t gMaskFlags[] = {
> +#define CSS_PROP_MASK FLAG_DATA_FOR_PROPERTY
> +#include "nsCSSPropList.h"
> +#undef CSS_PROP_MASK
> +};

This is also obsolete now that you no longer have a separate style struct.

::: layout/style/nsRuleNode.cpp:6437
(Diff revision 13)
> -SetLayerList(nsStyleContext* aStyleContext,
> +SetImageLayerList(nsStyleContext* aStyleContext,

This (and the series of changes that follow) should really move back to the patch that changed them from SetBackgroundList to SetLayerList.

::: layout/style/nsRuleNode.cpp:9434
(Diff revision 13)
> -  const nsCSSValue* maskValue = aRuleData->ValueForMask();
> -  if (eCSSUnit_URL == maskValue->GetUnit()) {
> -    svgReset->mMask = maskValue->GetURLValue();
> -  } else if (eCSSUnit_None == maskValue->GetUnit() ||
> -             eCSSUnit_Initial == maskValue->GetUnit() ||
> -             eCSSUnit_Unset == maskValue->GetUnit()) {
> +  const nsCSSValue* maskValue = aRuleData->ValueForMaskImage();
> +  if (eCSSUnit_List == maskValue->GetUnit() ||
> +      eCSSUnit_ListDep == maskValue->GetUnit()) {
> +    const nsCSSValue& item = maskValue->GetListValue()->mValue;
> +    if (eCSSUnit_URL == item.GetUnit() ||
> +        eCSSUnit_Image == item.GetUnit()) {
> +      svgReset->mMask = item.GetURLValue();
> +    } else if (eCSSUnit_None == item.GetUnit() ||
> +               eCSSUnit_Initial == item.GetUnit() ||
> +               eCSSUnit_Unset == item.GetUnit()) {
> -    svgReset->mMask = nullptr;
> +      svgReset->mMask = nullptr;
> -  } else if (eCSSUnit_Inherit == maskValue->GetUnit()) {
> +    } else if (eCSSUnit_Inherit == item.GetUnit()) {
> -    conditions.SetUncacheable();
> +      conditions.SetUncacheable();
> -    svgReset->mMask = parentSVGReset->mMask;
> +      svgReset->mMask = parentSVGReset->mMask;
> -  }
> +    }
> +  }

This should be explicitly marked as temporary, to be removed when we enable the new properties (as should mMask itself).

(I guess you have done part of the work to allow the preference.)

::: layout/style/nsRuleNode.cpp:9435
(Diff revision 13)
> -  if (eCSSUnit_URL == maskValue->GetUnit()) {
> -    svgReset->mMask = maskValue->GetURLValue();
> -  } else if (eCSSUnit_None == maskValue->GetUnit() ||
> -             eCSSUnit_Initial == maskValue->GetUnit() ||
> -             eCSSUnit_Unset == maskValue->GetUnit()) {
> +  if (eCSSUnit_List == maskValue->GetUnit() ||
> +      eCSSUnit_ListDep == maskValue->GetUnit()) {
> +    const nsCSSValue& item = maskValue->GetListValue()->mValue;
> +    if (eCSSUnit_URL == item.GetUnit() ||
> +        eCSSUnit_Image == item.GetUnit()) {
> +      svgReset->mMask = item.GetURLValue();
> +    } else if (eCSSUnit_None == item.GetUnit() ||
> +               eCSSUnit_Initial == item.GetUnit() ||
> +               eCSSUnit_Unset == item.GetUnit()) {
> -    svgReset->mMask = nullptr;
> +      svgReset->mMask = nullptr;

This doesn't get the right compatibility behavior since inherit, initial, and unset are alternatives to having a list rather than items in a list.  None, however, is an item in the list.

::: layout/style/nsRuleNode.cpp:9460
(Diff revision 13)
> +  uint32_t maxItemCount = 1;
> +  bool rebuild = false;

Move these 2 lines before the mask-image comment

::: layout/style/nsRuleNode.cpp:9466
(Diff revision 13)
> +                    initialImage, parentSVGReset->mLayers.mImageCount, svgReset->mLayers.mImageCount,

please wrap this long line if it's at or over 80 characters

::: layout/style/nsRuleNode.cpp:9483
(Diff revision 13)
> +                    uint8_t(NS_STYLE_IMAGELAYER_CLIP_BORDER), parentSVGReset->mLayers.mClipCount,

please wrap this long line if it's at or over 80 characters

::: layout/style/nsRuleNode.cpp:9490
(Diff revision 13)
> +                    uint8_t(NS_STYLE_IMAGELAYER_ORIGIN_PADDING), parentSVGReset->mLayers.mOriginCount,

please wrap this long line if it's at or over 80 characters

::: layout/style/nsRuleNode.cpp:9514
(Diff revision 13)
> +  // mask-mode: alpha | luminance | auto

This should say enum, inherit, initial [list], to match the comments for others.

::: layout/style/nsRuleNode.cpp:9521
(Diff revision 13)
> +  // mask-composition: add | subtract | intersect | exclude

The property's name is mask-composite, not mask-composition.  And again, this should say enum, inherit, initial [list], to match the comments for others.

::: layout/style/nsRuleNode.cpp:9525
(Diff revision 13)
> +                    uint8_t(NS_STYLE_COMPOSITE_MODE_ADD), parentSVGReset->mLayers.mCompositeCount,

Again, please wrap the line if it's more than 80 characters (I'm not sure if it is).

::: layout/style/nsStyleStruct.h:386
(Diff revision 13)
> +template <typename T> struct StyleLayerPropTrait;
> +
> +struct nsStyleBackground;
> +template <>
> +struct StyleLayerPropTrait<nsStyleBackground> {
> +  static const nsCSSProperty shortHand = eCSSProperty_background;
> +  static const nsCSSProperty color     = eCSSProperty_background_color;
> +  static const nsCSSProperty image     = eCSSProperty_background_image;
> +  static const nsCSSProperty repeat    = eCSSProperty_background_repeat;
> +  static const nsCSSProperty position  = eCSSProperty_background_position;
> +  static const nsCSSProperty clip      = eCSSProperty_background_clip;
> +  static const nsCSSProperty origin    = eCSSProperty_background_origin;
> +  static const nsCSSProperty size      = eCSSProperty_background_size;
> +  static const nsCSSProperty attach    = eCSSProperty_background_attachment;
> +  static const nsCSSProperty mode      = eCSSProperty_UNKNOWN;
> +  static const nsCSSProperty composite = eCSSProperty_UNKNOWN;
> +};
> +
> +struct nsStyleSVGReset;
> +template <>
> +struct StyleLayerPropTrait<nsStyleSVGReset> {
> +  static const nsCSSProperty shortHand = eCSSProperty_mask;
> +  static const nsCSSProperty color     = eCSSProperty_UNKNOWN;
> +  static const nsCSSProperty image     = eCSSProperty_mask_image;
> +  static const nsCSSProperty repeat    = eCSSProperty_mask_repeat;
> +  static const nsCSSProperty position  = eCSSProperty_mask_position;
> +  static const nsCSSProperty clip      = eCSSProperty_mask_clip;
> +  static const nsCSSProperty origin    = eCSSProperty_mask_origin;
> +  static const nsCSSProperty size      = eCSSProperty_mask_size;
> +  static const nsCSSProperty attach    = eCSSProperty_UNKNOWN;
> +  static const nsCSSProperty mode      = eCSSProperty_mask_mode;
> +  static const nsCSSProperty composite = eCSSProperty_mask_composite;
> +};
> +
>  struct nsStyleImageLayers {

As I said before, these should be structs rather than templates.

::: layout/style/nsStyleStruct.h:391
(Diff revision 13)
> +  static const nsCSSProperty shortHand = eCSSProperty_background;

shortHand should be shorthand (lowercase h rather than capital H)

::: layout/style/nsStyleStruct.h:546
(Diff revision 13)
> +    uint8_t       mComposite;     // [reset] See nsStyleConsts.h
> +                                  // mask layer property
> +    uint8_t       mMaskMode;      // [reset] See nsStyleConsts.h
> +                                  // mask layer property

Like for the background-only ones, these comments need to be clearer, and say what happens to these values in the background layer (hopefully they always have the initial value), and be clearer that "mask layer property" means it's a property that's only used for masks and not for backgrounds.

::: layout/style/nsStyleStruct.h:593
(Diff revision 13)
>             mSizeCount,
> -           mBlendModeCount;
> +           mMaskModeCount,
> +           mBlendModeCount,
> +           mCompositeCount;

Are the counts always 1 for the values that aren't currently used?  That seems the most sensible thing to do, but it should be documented.

::: layout/style/nsStyleStruct.h:3340
(Diff revision 13)
>    void Destroy(nsPresContext* aContext) {
> +    mLayers.UntrackImages(aContext);
> +
>      this->~nsStyleSVGReset();
>      aContext->PresShell()->
>        FreeByObjectID(mozilla::eArenaObjectID_nsStyleSVGReset, this);
>    }

Please move the Destroy method into the .cpp file, like other Destroy methods that have nontrivial contents (e.g., nsStyleBackground::Destroy).

::: layout/style/nsStyleStruct.h:3369
(Diff revision 13)
> +  const nsAutoTArray<nsStyleImageLayers::Layer, 1>& Layers() const {
> +    return mLayers.mLayers;
> +  }
> +
> +  nsAutoTArray<nsStyleImageLayers::Layer, 1>& Layers()  {
> +    return mLayers.mLayers;
> +  }

As I said for backgrounds, I'd like to see these methods go away.

::: layout/style/nsStyleStruct.cpp:1351
(Diff revision 13)
> +  const nsStyleImageLayers& moreLayers =
> +    mLayers.mImageCount > aOther.mLayers.mImageCount ?
> +      this->mLayers : aOther.mLayers;
> +  const nsStyleImageLayers& lessLayers =
> +    mLayers.mImageCount > aOther.mLayers.mImageCount ?
> +      aOther.mLayers : this->mLayers;
> +
> +  bool hasVisualDifference = false;
> +
> +  NS_FOR_VISIBLE_IMAGELAYER_BACK_TO_FRONT(i, moreLayers) {
> +    if (i < lessLayers.mImageCount) {
> +      if (moreLayers.mLayers[i] != lessLayers.mLayers[i]) {
> +        if ((moreLayers.mLayers[i].mImage.GetType() == eStyleImageType_Element) ||
> +            (lessLayers.mLayers[i].mImage.GetType() == eStyleImageType_Element))
> +          return NS_CombineHint(nsChangeHint_UpdateEffects,
> +                                nsChangeHint_RepaintFrame);
> +        hasVisualDifference = true;
> +      }
> +    } else {
> +      if (moreLayers.mLayers[i].mImage.GetType() == eStyleImageType_Element)
> +        return NS_CombineHint(nsChangeHint_UpdateEffects,
> +                              nsChangeHint_RepaintFrame);
> +      hasVisualDifference = true;
> +    }
> +  }
> +
> +  if (mLayers.mClipCount != aOther.mLayers.mClipCount ||
> +      mLayers.mOriginCount != aOther.mLayers.mOriginCount ||
> +      mLayers.mRepeatCount != aOther.mLayers.mRepeatCount ||
> +      mLayers.mPositionCount != aOther.mLayers.mPositionCount ||
> +      mLayers.mSizeCount != aOther.mLayers.mSizeCount||
> +      mLayers.mMaskModeCount != aOther.mLayers.mMaskModeCount||
> +      mLayers.mCompositeCount != aOther.mLayers.mCompositeCount) {
> +    NS_UpdateHint(hint, nsChangeHint_NeutralChange);
> +  }

Please make this share code with nsStyleBackground, probably with a CalcDifference method on nsStyleImageLayers.  Note that while you're there, you should fix the fact that there's an mBlendModeCount check missing.

If you ensure that the counts are always 1 for the properties that are unused, and that the values in the layer are always initial values when unused, then you don't need to worry about using a common method.  (But you should ensure those are true, which I think they are.)

::: layout/style/nsStyleStruct.cpp:1362
(Diff revision 13)
> +      if (moreLayers.mLayers[i] != lessLayers.mLayers[i]) {

This should also probably use the recently-added CalcDifference method on Layer.  But you'll see that when you merge it with the background code, assuming your tree is up-to-date enough.

::: layout/style/nsStyleStruct.cpp:1388
(Diff revision 13)
> +    NS_UpdateHint(hint, nsChangeHint_RepaintFrame);

Mask changes also need UpdateEffects and UpdateOverflow hints; see the current mMask handling.

I'm now done going through this patch (although, as I said, half my comments are, in reviewboard, associated with the previous patch, which I haven't looked at at all yet).
Attachment #8683706 - Flags: review?(dbaron)
Comment on attachment 8685499 [details]
MozReview Request: Bug 686281 - A static assertion to keep value correctness of NS_RULE_NODE_IS_ANIMATION_RULE; r=dbaron.

https://reviewboard.mozilla.org/r/24805/#review22427

::: layout/style/nsStyleStruct.h:99
(Diff revision 3)
> +  "NS_RULE_NODE_IS_ANIMATION_RULE cannot fit the number of style struct.");

instead of "cannot fit the number of style struct", how about "must not overlap the style struct bits".
Attachment #8685499 - Flags: review+
Comment on attachment 8685498 [details]
MozReview Request: Bug 686281 - Expands will-change of a shorthand prop to longhand ones; r=dbaron.

https://reviewboard.mozilla.org/r/24803/#review22429

::: layout/style/nsRuleNode.cpp:5903
(Diff revision 3)
> +            for (const nsCSSProperty *shorthands =

"\* " rather than " \*".

::: layout/style/nsRuleNode.cpp:5913
(Diff revision 3)
> +                                     CSS_PROPERTY_CREATES_STACKING_CONTEXT)) {

please fix indentation (unless that pushes it past 80 characters)
Attachment #8685498 - Flags: review?(dbaron) → review+
(In reply to David Baron [:dbaron] ⌚UTC+8 from comment #164)
> Comment on attachment 8685498 [details]
> MozReview Request: Bug 686281 - (Bug Fix:
> will-change-stacking-context-mask-1.html fail) Expands shorthand prop to
> longhands. r?dbaron
> 
> https://reviewboard.mozilla.org/r/24803/#review22429
> 
> ::: layout/style/nsRuleNode.cpp:5903
> (Diff revision 3)
> > +            for (const nsCSSProperty *shorthands =
> 
> "\* " rather than " \*".

This comment looks somehow unclear to me (especially that the stars are escaped, which I've filed bug 1223687 for).

To make it clearer, in Gecko code, we prefer declaring pointer types this way
> const nsCSSProperty* shorthands
> ~~~~~~~~~~~~~~~~~~~^
rather than
> const nsCSSProperty *shorthands
> ~~~~~~~~~~~~~~~~~~~~^

(We really should have some kind of style checker integrated in MozReview so that reviewers do not need to spend time on correcting things like this)
Comment on attachment 8684679 [details]
MozReview Request: Bug 686281 - Mask CSS animation; r=dbaron.

https://reviewboard.mozilla.org/r/24617/#review22431

::: layout/generic/nsFrame.cpp:861
(Diff revision 6)
> -      if ((!StyleMargin()->GetMargin(newValue) || oldValue != newValue) &&
> +    if ((!StyleMargin()->GetMargin(newValue) || oldValue != newValue) &&

Remove this change; the indentation was correct before, and this makes it incorrect.

::: layout/style/StyleAnimationValue.cpp:2982
(Diff revision 6)
> +ExtractStyleLayerPosition(const nsStyleImageLayers &layers,

The parameter should have an a prefix, e.g., aLayers.

Also, I'd name the function ExtractImageLayerPositionList.

::: layout/style/StyleAnimationValue.cpp:2999
(Diff revision 6)
> +ExtractStyleLayerSize(const nsStyleImageLayers &layers,

same 2 comments here

::: layout/style/StyleAnimationValue.cpp:3367
(Diff revision 6)
> -          const nsStyleBackground *bg =
> +          const nsStyleImageLayers &layers =

Again, please switch to & before space rather than space before &.

::: layout/style/StyleAnimationValue.cpp:3369
(Diff revision 6)
> -          nsAutoPtr<nsCSSValueList> result;
> +          MOZ_ASSERT(layers.mPositionCount > 0, "unexpected count");

This MOZ_ASSERT should move into the shared function.

::: layout/style/StyleAnimationValue.cpp:3384
(Diff revision 6)
> -                // leave it null
> +          MOZ_ASSERT(layers.mSizeCount > 0, "unexpected count");

This MOZ_ASSERT should also move into the shared function.
Attachment #8684679 - Flags: review?(dbaron) → review+
Comment on attachment 8684680 [details]
MozReview Request: Bug 686281 - Mask CSS webkit-alias; r=dbaron.

https://reviewboard.mozilla.org/r/24619/#review22433

::: layout/style/nsCSSPropAliasList.h:377
(Diff revision 6)
> +CSS_PROP_ALIAS(-webkit-mask-image,

you're missing -webkit-mask-composite; please add that
Attachment #8684680 - Flags: review+
Comment on attachment 8685499 [details]
MozReview Request: Bug 686281 - A static assertion to keep value correctness of NS_RULE_NODE_IS_ANIMATION_RULE; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24805/diff/3-4/
Attachment #8685499 - Flags: review+ → review?(dbaron)
Comment on attachment 8683558 [details]
MozReview Request: Bug 686281 - Implement CSS mask style; r=dbaron.

https://reviewboard.mozilla.org/r/24373/#review22423

One saved comment I made on the previous revision that I need to publish from MozReview:

::: layout/style/nsCSSProps.h:639
(Diff revision 15)
> -  static const KTableValue kBackgroundAttachmentKTable[];
> -  static const KTableValue kBackgroundOriginKTable[];
> -  static const KTableValue kBackgroundPositionKTable[];
> -  static const KTableValue kBackgroundRepeatKTable[];
> -  static const KTableValue kBackgroundRepeatPartKTable[];
> -  static const KTableValue kBackgroundSizeKTable[];
> +  static const KTableValue kLayerAttachmentKTable[];
> +  static const KTableValue kLayerOriginKTable[];
> +  static const KTableValue kLayerPositionKTable[];
> +  static const KTableValue kLayerRepeatKTable[];
> +  static const KTableValue kLayerRepeatPartKTable[];
> +  static const KTableValue kLayerSizeKTable[];

To fit with the other name changes, I think these should be kImageLayer...KTable[].
Attachment #8683558 - Flags: review?(dbaron)
Comment on attachment 8685499 [details]
MozReview Request: Bug 686281 - A static assertion to keep value correctness of NS_RULE_NODE_IS_ANIMATION_RULE; r=dbaron.

https://reviewboard.mozilla.org/r/24805/#review22437

::: layout/style/nsStyleStruct.h:99
(Diff revision 3)
> +  "NS_RULE_NODE_IS_ANIMATION_RULE cannot fit the number of style struct.");

Saying this a second time since MozReview is broken:

instead of "cannot fit the number of style struct", how about "must not overlap the style struct bits".
Attachment #8685499 - Flags: review?(dbaron) → review+
Comment on attachment 8683558 [details]
MozReview Request: Bug 686281 - Implement CSS mask style; r=dbaron.

https://reviewboard.mozilla.org/r/24373/#review22435

::: layout/base/nsCSSRendering.h:324
(Diff revision 16)
>     * A rectangle that one copy of the image tile is mapped onto. Same
>     * coordinate system as aBorderArea/aBGClipRect passed into
> -   * PrepareBackgroundLayer.
> +   * PrepareLayer.
>     */
>    nsRect mDestArea;
>    /**
>     * The actual rectangle that should be filled with (complete or partial)
>     * image tiles. Same coordinate system as aBorderArea/aBGClipRect passed into
> -   * PrepareBackgroundLayer.
> +   * PrepareLayer.
>     */
>    nsRect mFillArea;
>    /**
>     * The anchor point that should be snapped to a pixel corner. Same
>     * coordinate system as aBorderArea/aBGClipRect passed into
> -   * PrepareBackgroundLayer.
> +   * PrepareLayer.

These should say PrepareImageLayer to match the code.

::: layout/base/nsCSSRendering.h:548
(Diff revision 16)
> -  struct BackgroundClipState {
> +  struct ImageLayersClipState {

This should probably be ImageLayerClipState, actually, since it's only for a single layer.

::: layout/base/nsCSSRendering.cpp:1859
(Diff revision 16)
> -SetupBackgroundClip(nsCSSRendering::BackgroundClipState& aClipState,
> +SetupLayerClip(nsCSSRendering::ImageLayersClipState& aClipState,

This should be SetupImageLayerClip (as the indentation already suggests it is).  (Was this one of the changes you had in a later patch by mistake?)

::: layout/base/nsCSSRendering.cpp:2940
(Diff revision 16)
> -    GetBackgroundClip(layers.BottomLayer(),
> +    GetImageLayersClip(layers.BottomLayer(),
> -                      aForFrame, aBorder, aBorderArea,
> +                 aForFrame, aBorder, aBorderArea,
> -                      aDirtyRect, (aFlags & PAINTBG_WILL_PAINT_BORDER), appUnitsPerPixel,
> +                 aDirtyRect, (aFlags & PAINTBG_WILL_PAINT_BORDER),
> +                 appUnitsPerPixel,
> -                      &clipState);
> +                 &clipState);

This should also be GetImageLayerClip (singular), since it's for one layer.

And please fix the indentation.

::: layout/base/nsCSSRendering.cpp:3233
(Diff revision 16)
>     * These properties have the following dependencies upon each other when
>     * determining rendering:

You should document that background-blend-mode, mask-mode, and mask-composite have no dependencies.

::: layout/base/nsCSSRendering.cpp:3369
(Diff revision 16)
> -      PrepareBackgroundLayer(aPresContext, aForFrame, aFlags, borderArea,
> +      PrepareImageLayer(aPresContext, aForFrame, aFlags, borderArea,
>                               aClipRect, aLayer);

Please fix the indentation (unless I told you to change the name elsewhere).

::: layout/base/nsDisplayList.cpp:2159
(Diff revision 16)
> -    nsCSSRendering::PrepareBackgroundLayer(presContext, mFrame, flags,
> +    nsCSSRendering::PrepareImageLayer(presContext, mFrame, flags,
>                                             borderArea, borderArea, layer);

again, indentation, unless it matches another name fix

::: layout/base/nsDisplayList.cpp:2203
(Diff revision 16)
> -  nsCSSRendering::GetBackgroundClip(aLayer, aFrame, *aFrame->StyleBorder(),
> +  nsCSSRendering::GetImageLayersClip(aLayer, aFrame, *aFrame->StyleBorder(),
>                                      borderBox, borderBox, aWillPaintBorder,
>                                      aFrame->PresContext()->AppUnitsPerDevPixel(),
>                                      &clip);

Conveniently, this indentation is wrong, but dropping the "s" from the name as I suggested above will fix that.

::: layout/style/nsCSSProps.h:639
(Diff revision 16)
> -  static const KTableValue kBackgroundAttachmentKTable[];
> -  static const KTableValue kBackgroundOriginKTable[];
> -  static const KTableValue kBackgroundPositionKTable[];
> -  static const KTableValue kBackgroundRepeatKTable[];
> -  static const KTableValue kBackgroundRepeatPartKTable[];
> -  static const KTableValue kBackgroundSizeKTable[];
> +  static const KTableValue kLayerAttachmentKTable[];
> +  static const KTableValue kLayerOriginKTable[];
> +  static const KTableValue kLayerPositionKTable[];
> +  static const KTableValue kLayerRepeatKTable[];
> +  static const KTableValue kLayerRepeatPartKTable[];
> +  static const KTableValue kLayerSizeKTable[];

These should probably be kImageLayer...KTable; I think I said that already on a different patch, though!

::: layout/style/nsCSSProps.cpp:2181
(Diff revision 16)
> +
> +

Don't add these extra 2 lines.

::: layout/style/nsStyleConsts.h
(Diff revision 16)
> -// See nsStyleSVG
> -

Don't delete this (although fixing it to say nsStyleSVGReset would be good).
Attachment #8683558 - Flags: review+
Attachment #8684680 - Flags: review+ → review?(dbaron)
Comment on attachment 8684680 [details]
MozReview Request: Bug 686281 - Mask CSS webkit-alias; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24619/diff/6-7/
Attachment #8684815 - Flags: review?(dbaron)
Comment on attachment 8684815 [details]
MozReview Request: Bug 686281 - Mask mochitest; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24647/diff/4-5/
https://reviewboard.mozilla.org/r/24647/#review22441

I'm not marking this "Fix it and then ship it" because I'd like to understand how you didn't get test failures from mask-composite being missing.

I'm not entirely happy about so much copy and paste of tests here, but I guess it's ok; this isn't the first time that's happened in this file.

::: layout/style/test/property_database.js:3917
(Diff revision 4)
> +    type: CSS_TYPE_TRUE_SHORTHAND,

This should be CSS_TYPE_SHORTHAND_AND_LONGHAND given that the property was a longhand previously.

::: layout/style/test/property_database.js:3918
(Diff revision 4)
> +    subproperties: ["mask-clip", "mask-image", "mask-mode", "mask-origin", "mask-position", "mask-repeat", "mask-size" ],

mask-composite is missing in this list.

You should also add a fixme that all the mask-border properties should be added when we implement them.  (You should probably also add the same FIXME to the subproperty table in nsCSSProps.cpp, in the appropriate patch.)

::: layout/style/test/property_database.js:4017
(Diff revision 4)
> +  },

mask-composite is missing.

That should have led to test failures.  Did it?  If not, I'd like to understand why.

::: layout/style/test/property_database.js:4023
(Diff revision 4)
> -    other_values: [ "url(#mymask)" ],
> +    other_values: [

You should include "url(#mymask)" in the other_values line for both mask and mask-image, rather than dropping it.
You also need some reftests for CSS masking.  If there are some in the CSS WG repository, you could import them by modifying andusing layout/reftests/w3c-css/import-tests.py.  If not, you should write some for a new directory inside layout/reftests/w3c-css/submitted/ .

MozReview automatically makes new review requests to me when you revise patches.  Since many of the issues in MozReview are still open, I'm going to assume they're not actually ready for re-review.  Please let me know explicitly when you think that the patches that I didn't already review+ are ready for re-review.  (I may or may not go through and actually cancel the bugzilla requests.)

I'm also probably going to redirect the review for the rendering piece to someone else, probably mstange.
Flags: needinfo?(cku)
Attachment #8684292 - Flags: review?(dbaron) → review?(mstange)
Comment on attachment 8684680 [details]
MozReview Request: Bug 686281 - Mask CSS webkit-alias; r=dbaron.

https://reviewboard.mozilla.org/r/24619/#review22443

::: layout/style/nsCSSPropAliasList.h:377
(Diff revision 6)
> +CSS_PROP_ALIAS(-webkit-mask-image,

Saying this a second time to make MozReview happy: you're missing -webkit-mask-composite; please add that
Attachment #8684680 - Flags: review?(dbaron) → review+
Comment on attachment 8683556 [details]
MozReview Request: Bug 686281 - Implement nsStyleImageLayers; r=dbaron

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24369/diff/16-17/
Attachment #8683556 - Flags: review?(dbaron)
Attachment #8683557 - Flags: review?(dbaron)
Comment on attachment 8683557 [details]
MozReview Request: Bug 686281 - Rename *background* to *imagelayer*; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24371/diff/16-17/
Comment on attachment 8683558 [details]
MozReview Request: Bug 686281 - Implement CSS mask style; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24373/diff/16-17/
Attachment #8683558 - Flags: review+ → review?(dbaron)
Attachment #8683706 - Flags: review?(dbaron)
Comment on attachment 8683706 [details]
MozReview Request: Bug 686281 - Mask CSS parsing and Mask DOM API. r=dbaron

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24415/diff/14-15/
Attachment #8684292 - Flags: review?(mstange) → review?(dbaron)
Comment on attachment 8684292 [details]
MozReview Request: Bug 686281 - Mask CSS rendering; r=mstange

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24531/diff/12-13/
Attachment #8684679 - Flags: review+ → review?(dbaron)
Comment on attachment 8684679 [details]
MozReview Request: Bug 686281 - Mask CSS animation; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24617/diff/6-7/
Attachment #8684680 - Flags: review+ → review?(dbaron)
Comment on attachment 8684680 [details]
MozReview Request: Bug 686281 - Mask CSS webkit-alias; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24619/diff/6-7/
Comment on attachment 8684815 [details]
MozReview Request: Bug 686281 - Mask mochitest; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24647/diff/4-5/
Attachment #8685498 - Flags: review+ → review?(dbaron)
Comment on attachment 8685498 [details]
MozReview Request: Bug 686281 - Expands will-change of a shorthand prop to longhand ones; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24803/diff/3-4/
Comment on attachment 8685499 [details]
MozReview Request: Bug 686281 - A static assertion to keep value correctness of NS_RULE_NODE_IS_ANIMATION_RULE; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24805/diff/3-4/
Attachment #8685499 - Attachment description: MozReview Request: Bug 686281 - A static assertion to keep the rightness of NS_RULE_NODE_IS_ANIMATION_RULE. r=:dbaron. → MozReview Request: Bug 686281 - A static assertion to keep value correctness of NS_RULE_NODE_IS_ANIMATION_RULE. r=:dbaron.
Attachment #8685499 - Flags: review+ → review?(dbaron)
Could you please not request re-review on the ones I've already granted review to, and also change the r?:dbaron to r?:mstange on the one that I forwarded to him?  (I hope that changing the r? to r= will do that, assuming you've addressed the review comments.)

(I can fix the mstange one for now, at least.)


Also, please explicitly let me know if things are actually ready for re-review.

(But also, it's not so good to work so much late at night!)
Attachment #8684292 - Flags: review?(dbaron) → review?(mstange)
https://reviewboard.mozilla.org/r/24415/#review22425

> please list these properties in alphabetical order in nsCSSPropList.h

OK. May I ask is there any special reason that we need to list them in alphabetical order? Or, it's only for readness?
(In reply to C.J. Ku[:cjku] from comment #188)
> OK. May I ask is there any special reason that we need to list them in
> alphabetical order? Or, it's only for readness?

No special reason other than readability and maintainability.
(In reply to David Baron [:dbaron] ⌚UTC+8 from comment #187)
> Could you please not request re-review on the ones I've already granted
> review to, and also change the r?:dbaron to r?:mstange on the one that I
> forwarded to him?  (I hope that changing the r? to r= will do that, assuming
> you've addressed the review comments.)
No, unless you have Commit level 3, otherwise, r+ flag will be clear in each push review. Could you vouch for me?(Bug 1224089)
Attachment #8683556 - Attachment description: MozReview Request: Bug 686281 - Create nsStyleImageLayers. r?:dbaron. → MozReview Request: Bug 686281 - Create nsStyleImageLayers; r?:dbaron
Attachment #8683556 - Flags: review?(dbaron)
Comment on attachment 8683556 [details]
MozReview Request: Bug 686281 - Implement nsStyleImageLayers; r=dbaron

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24369/diff/17-18/
Attachment #8683557 - Attachment description: MozReview Request: Bug 686281 - Integrate nsStyleImageLayers with nsStyleBackground. r?:dbaron. → MozReview Request: Bug 686281 - Integrate nsStyleImageLayers with nsStyleBackground; r?:dbaron
Attachment #8683557 - Flags: review?(dbaron)
Comment on attachment 8683557 [details]
MozReview Request: Bug 686281 - Rename *background* to *imagelayer*; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24371/diff/17-18/
Attachment #8683558 - Attachment description: MozReview Request: Bug 686281 - Rename *background* to *imagelayer*. r?:dbaron. → MozReview Request: Bug 686281 - Rename *background* to *imagelayer*; r?:dbaron
Attachment #8683558 - Flags: review?(dbaron)
Comment on attachment 8683558 [details]
MozReview Request: Bug 686281 - Implement CSS mask style; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24373/diff/17-18/
Comment on attachment 8683706 [details]
MozReview Request: Bug 686281 - Mask CSS parsing and Mask DOM API. r=dbaron

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24415/diff/15-16/
Attachment #8683706 - Attachment description: MozReview Request: Bug 686281 - Implement CSS mask prop. r?:dbaron. → MozReview Request: Bug 686281 - Implement CSS mask prop; r?:dbaron.
Attachment #8683706 - Flags: review?(dbaron)
Attachment #8684292 - Attachment description: MozReview Request: Bug 686281 - Implement CSS mask rendering. r?:dbaron. → MozReview Request: Bug 686281 - Implement CSS mask rendering; r?:mstange
Attachment #8684292 - Flags: review?(mstange) → review?(dbaron)
Comment on attachment 8684292 [details]
MozReview Request: Bug 686281 - Mask CSS rendering; r=mstange

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24531/diff/13-14/
Comment on attachment 8684679 [details]
MozReview Request: Bug 686281 - Mask CSS animation; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24617/diff/7-8/
Attachment #8684679 - Attachment description: MozReview Request: Bug 686281 - Implement CSS mask animation. r?:dbaron → MozReview Request: Bug 686281 - Implement CSS mask animation; r?:dbaron
Attachment #8684679 - Flags: review?(dbaron)
Comment on attachment 8684680 [details]
MozReview Request: Bug 686281 - Mask CSS webkit-alias; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24619/diff/7-8/
Attachment #8684680 - Attachment description: MozReview Request: Bug 686281 - Implement CSS mask alias. r?:dbaron → MozReview Request: Bug 686281 - Implement CSS mask alias; r=:dbaron
Attachment #8684680 - Flags: review?(dbaron)
Comment on attachment 8684815 [details]
MozReview Request: Bug 686281 - Mask mochitest; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24647/diff/5-6/
Attachment #8685498 - Attachment description: MozReview Request: Bug 686281 - (Bug Fix: will-change-stacking-context-mask-1.html fail) Expands shorthand prop to longhands. r?dbaron → MozReview Request: Bug 686281 - (Bug Fix: will-change-stacking-context-mask-1.html fail) Expands shorthand prop to longhands; r:=dbaron
Attachment #8685498 - Flags: review?(dbaron)
Comment on attachment 8685498 [details]
MozReview Request: Bug 686281 - Expands will-change of a shorthand prop to longhand ones; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24803/diff/4-5/
Comment on attachment 8685499 [details]
MozReview Request: Bug 686281 - A static assertion to keep value correctness of NS_RULE_NODE_IS_ANIMATION_RULE; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24805/diff/4-5/
Attachment #8685499 - Attachment description: MozReview Request: Bug 686281 - A static assertion to keep value correctness of NS_RULE_NODE_IS_ANIMATION_RULE. r=:dbaron. → MozReview Request: Bug 686281 - A static assertion to keep value correctness of NS_RULE_NODE_IS_ANIMATION_RULE; r=:dbaron.
Attachment #8685499 - Flags: review?(dbaron)
Flags: needinfo?(cku)
Attachment #8683556 - Flags: review?(dbaron)
Comment on attachment 8683556 [details]
MozReview Request: Bug 686281 - Implement nsStyleImageLayers; r=dbaron

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24369/diff/18-19/
Comment on attachment 8683557 [details]
MozReview Request: Bug 686281 - Rename *background* to *imagelayer*; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24371/diff/18-19/
Attachment #8683557 - Flags: review?(dbaron)
Attachment #8683558 - Flags: review?(dbaron)
Comment on attachment 8683558 [details]
MozReview Request: Bug 686281 - Implement CSS mask style; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24373/diff/18-19/
Comment on attachment 8683706 [details]
MozReview Request: Bug 686281 - Mask CSS parsing and Mask DOM API. r=dbaron

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24415/diff/16-17/
Attachment #8683706 - Flags: review?(dbaron)
Comment on attachment 8684292 [details]
MozReview Request: Bug 686281 - Mask CSS rendering; r=mstange

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24531/diff/14-15/
Attachment #8684292 - Flags: review?(dbaron)
Attachment #8684679 - Flags: review?(dbaron)
Comment on attachment 8684679 [details]
MozReview Request: Bug 686281 - Mask CSS animation; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24617/diff/8-9/
Comment on attachment 8684680 [details]
MozReview Request: Bug 686281 - Mask CSS webkit-alias; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24619/diff/8-9/
Attachment #8684680 - Flags: review?(dbaron)
Comment on attachment 8685498 [details]
MozReview Request: Bug 686281 - Expands will-change of a shorthand prop to longhand ones; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24803/diff/5-6/
Attachment #8685498 - Flags: review?(dbaron)
Comment on attachment 8685499 [details]
MozReview Request: Bug 686281 - A static assertion to keep value correctness of NS_RULE_NODE_IS_ANIMATION_RULE; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24805/diff/5-6/
Attachment #8685499 - Flags: review?(dbaron)
Also, per bug 1224141, please stop using the ":" in the reviewer syntax in the commit messages.
Attachment #8683556 - Attachment description: MozReview Request: Bug 686281 - Create nsStyleImageLayers; r?:dbaron → MozReview Request: Bug 686281 - Create nsStyleImageLayers;
Comment on attachment 8683556 [details]
MozReview Request: Bug 686281 - Implement nsStyleImageLayers; r=dbaron

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24369/diff/18-19/
Comment on attachment 8683557 [details]
MozReview Request: Bug 686281 - Rename *background* to *imagelayer*; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24371/diff/18-19/
Attachment #8683557 - Attachment description: MozReview Request: Bug 686281 - Integrate nsStyleImageLayers with nsStyleBackground; r?:dbaron → MozReview Request: Bug 686281 - Integrate nsStyleImageLayers with nsStyleBackground;
Comment on attachment 8683558 [details]
MozReview Request: Bug 686281 - Implement CSS mask style; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24373/diff/18-19/
Attachment #8683558 - Attachment description: MozReview Request: Bug 686281 - Rename *background* to *imagelayer*; r?:dbaron → MozReview Request: Bug 686281 - Rename *background* to *imagelayer*;
Comment on attachment 8683706 [details]
MozReview Request: Bug 686281 - Mask CSS parsing and Mask DOM API. r=dbaron

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24415/diff/16-17/
Attachment #8683706 - Attachment description: MozReview Request: Bug 686281 - Implement CSS mask prop; r?:dbaron. → MozReview Request: Bug 686281 - Implement CSS mask prop;
Attachment #8684292 - Flags: review?(mstange)
Comment on attachment 8684292 [details]
MozReview Request: Bug 686281 - Mask CSS rendering; r=mstange

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24531/diff/14-15/
Comment on attachment 8684679 [details]
MozReview Request: Bug 686281 - Mask CSS animation; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24617/diff/8-9/
Attachment #8684679 - Attachment description: MozReview Request: Bug 686281 - Implement CSS mask animation; r?:dbaron → MozReview Request: Bug 686281 - (r+) Implement CSS mask animation;
Attachment #8684680 - Attachment description: MozReview Request: Bug 686281 - Implement CSS mask alias; r=:dbaron → MozReview Request: Bug 686281 - (r+) Implement CSS mask alias;
Comment on attachment 8684680 [details]
MozReview Request: Bug 686281 - Mask CSS webkit-alias; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24619/diff/8-9/
Comment on attachment 8684815 [details]
MozReview Request: Bug 686281 - Mask mochitest; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24647/diff/6-7/
Comment on attachment 8685498 [details]
MozReview Request: Bug 686281 - Expands will-change of a shorthand prop to longhand ones; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24803/diff/5-6/
Attachment #8685498 - Attachment description: MozReview Request: Bug 686281 - (Bug Fix: will-change-stacking-context-mask-1.html fail) Expands shorthand prop to longhands; r:=dbaron → MozReview Request: Bug 686281 - (r+) Expands shorthand prop to longhands;
Comment on attachment 8685499 [details]
MozReview Request: Bug 686281 - A static assertion to keep value correctness of NS_RULE_NODE_IS_ANIMATION_RULE; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24805/diff/5-6/
Attachment #8685499 - Attachment description: MozReview Request: Bug 686281 - A static assertion to keep value correctness of NS_RULE_NODE_IS_ANIMATION_RULE; r=:dbaron. → MozReview Request: Bug 686281 - (r+) A static assertion to keep value correctness of NS_RULE_NODE_IS_ANIMATION_RULE;
Comment on attachment 8684292 [details]
MozReview Request: Bug 686281 - Mask CSS rendering; r=mstange

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24531/diff/15-16/
Attachment #8684292 - Flags: review?(mstange)
https://reviewboard.mozilla.org/r/24415/#review22553

::: layout/style/Declaration.cpp:321
(Diff revision 17)
> +

This assertion is totally wrong. Remvoe it.
Blocks: mask-image
https://reviewboard.mozilla.org/r/24369/#review21991

> I believe this will fail tests because it's out of order.  Have you run this patch through try yet?

Will do, but can you enlighten me why the order here matter?
(In reply to C.J. Ku[:cjku] from comment #223)
> https://reviewboard.mozilla.org/r/24369/#review21991
> 
> > I believe this will fail tests because it's out of order.  Have you run this patch through try yet?
> 
> Will do, but can you enlighten me why the order here matter?

I believe the order of the properties in the big table in nsComputedDOMStyle.cpp is exposed to Web content as the enumeration order of JS "for (prop in element.style)", although maybe that's changed with new bindings.
Comment on attachment 8683556 [details]
MozReview Request: Bug 686281 - Implement nsStyleImageLayers; r=dbaron

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24369/diff/19-20/
Comment on attachment 8683557 [details]
MozReview Request: Bug 686281 - Rename *background* to *imagelayer*; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24371/diff/19-20/
Attachment #8683558 - Flags: review?(dbaron)
Comment on attachment 8683558 [details]
MozReview Request: Bug 686281 - Implement CSS mask style; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24373/diff/19-20/
Comment on attachment 8683706 [details]
MozReview Request: Bug 686281 - Mask CSS parsing and Mask DOM API. r=dbaron

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24415/diff/17-18/
Comment on attachment 8684292 [details]
MozReview Request: Bug 686281 - Mask CSS rendering; r=mstange

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24531/diff/15-16/
Attachment #8684292 - Attachment description: MozReview Request: Bug 686281 - Implement CSS mask rendering; r?:mstange → MozReview Request: Bug 686281 - Implement CSS mask rendering; r?mstange
Comment on attachment 8684679 [details]
MozReview Request: Bug 686281 - Mask CSS animation; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24617/diff/9-10/
Attachment #8684679 - Flags: review?(dbaron)
Attachment #8684680 - Flags: review?(dbaron)
Comment on attachment 8684680 [details]
MozReview Request: Bug 686281 - Mask CSS webkit-alias; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24619/diff/9-10/
Comment on attachment 8684815 [details]
MozReview Request: Bug 686281 - Mask mochitest; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24647/diff/7-8/
Attachment #8685498 - Flags: review?(dbaron)
Comment on attachment 8685498 [details]
MozReview Request: Bug 686281 - Expands will-change of a shorthand prop to longhand ones; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24803/diff/6-7/
Attachment #8685499 - Flags: review?(dbaron)
Comment on attachment 8685499 [details]
MozReview Request: Bug 686281 - A static assertion to keep value correctness of NS_RULE_NODE_IS_ANIMATION_RULE; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24805/diff/6-7/
Comment on attachment 8684292 [details]
MozReview Request: Bug 686281 - Mask CSS rendering; r=mstange

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24531/diff/16-17/
Attachment #8684292 - Flags: review?(mstange)
(In reply to C.J. Ku[:cjku] from comment #235)
> Comment on attachment 8684292 [details]
> MozReview Request: Bug 686281 - Implement CSS mask rendering; r?mstange
> 
> Review request updated; see interdiff:
> https://reviewboard.mozilla.org/r/24531/diff/16-17/

Since interface of gfxContext had changed, need to rework after rebase from master. Remove review?
As I said in comment 175, please let me know explicitly when you actually want me to review these again.

(That said, I'm not sure why you're pushing new versions to mozreview when they're not yet ready for re-review.)
Flags: needinfo?(cku)
https://reviewboard.mozilla.org/r/24647/#review22441

> mask-composite is missing.
> 
> That should have led to test failures.  Did it?  If not, I'd like to understand why.

Yes. Without listing mask-composite in gCSSProperties, I saw this kind of failures
79177 INFO TEST-UNEXPECTED-FAIL | layout/style/test/test_property_database.html | 'mask-composite' listed in gCSSProperties
80336 INFO TEST-UNEXPECTED-FAIL | layout/style/test/test_property_database.html | property 'mask' has a good subproperty list
Blocks: 1228280
https://reviewboard.mozilla.org/r/24415/#review22425

> This should be explicitly marked as temporary, to be removed when we enable the new properties (as should mMask itself).
> 
> (I guess you have done part of the work to allow the preference.)

I plan to keep nsStyleSVGReset::mMask here, move it to nsStyleImageLayers::Layer in "Bug 1228280 - (mask-image) Support multiple SVG mask." since it's not a trivial fix.

After phase in patches in this bug, what we have is:
1. If you write one svg-mask in "mask" property, FF can still parse and visually render it correctly as usual(backward compatible)
2. If you write one svg-mask in "mask-image", FF can parse and visually render it correcly(new feature).
3. If you write one or more image masks(none-SVG-mask) in "mask" or "mask-image" property, FF can prarse and visually render it correctly(new feature).
4. If you write more then one svg#mask in "mask" or "mask-image", FF can not render it correctly as usual(should be fix in bug 1228280)
Blocks: 1228354
Comment on attachment 8683556 [details]
MozReview Request: Bug 686281 - Implement nsStyleImageLayers; r=dbaron

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24369/diff/20-21/
Attachment #8683556 - Attachment description: MozReview Request: Bug 686281 - Create nsStyleImageLayers; → MozReview Request: Bug 686281 - Create nsStyleImageLayers; r?dbaron.
Attachment #8683556 - Flags: review?(dbaron)
Attachment #8683557 - Attachment description: MozReview Request: Bug 686281 - Integrate nsStyleImageLayers with nsStyleBackground; → MozReview Request: Bug 686281 - Integrate nsStyleImageLayers with nsStyleBackground; r?dbaron.
Attachment #8683557 - Flags: review?(dbaron)
Comment on attachment 8683557 [details]
MozReview Request: Bug 686281 - Rename *background* to *imagelayer*; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24371/diff/20-21/
Comment on attachment 8683558 [details]
MozReview Request: Bug 686281 - Implement CSS mask style; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24373/diff/20-21/
Attachment #8683558 - Attachment description: MozReview Request: Bug 686281 - Rename *background* to *imagelayer*; → MozReview Request: Bug 686281 - Rename *background* to *imagelayer*; r=dbaron.
Attachment #8683558 - Flags: review?(dbaron)
Attachment #8683706 - Attachment description: MozReview Request: Bug 686281 - Implement CSS mask prop; → MozReview Request: Bug 686281 - Implement CSS mask style; r?dbaron.
Attachment #8683706 - Flags: review?(dbaron)
Comment on attachment 8683706 [details]
MozReview Request: Bug 686281 - Mask CSS parsing and Mask DOM API. r=dbaron

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24415/diff/18-19/
Comment on attachment 8684292 [details]
MozReview Request: Bug 686281 - Mask CSS rendering; r=mstange

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24531/diff/16-17/
Attachment #8684292 - Attachment description: MozReview Request: Bug 686281 - Implement CSS mask rendering; r?mstange → MozReview Request: Bug 686281 - Mask CSS parsing; r?dbaron
Attachment #8684292 - Flags: review?(dbaron)
Comment on attachment 8684679 [details]
MozReview Request: Bug 686281 - Mask CSS animation; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24617/diff/10-11/
Attachment #8684679 - Attachment description: MozReview Request: Bug 686281 - (r+) Implement CSS mask animation; → MozReview Request: Bug 686281 - Mask style computing; r?dbaron
Attachment #8684679 - Flags: review?(dbaron)
Attachment #8684680 - Attachment description: MozReview Request: Bug 686281 - (r+) Implement CSS mask alias; → MozReview Request: Bug 686281 - Mask DOM API; r?dbaron
Attachment #8684680 - Flags: review?(dbaron)
Comment on attachment 8684680 [details]
MozReview Request: Bug 686281 - Mask CSS webkit-alias; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24619/diff/10-11/
Comment on attachment 8684815 [details]
MozReview Request: Bug 686281 - Mask mochitest; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24647/diff/8-9/
Attachment #8684815 - Attachment description: MozReview Request: Bug 686281 - Implement CSS mask test. (WIP) → MozReview Request: Bug 686281 - Mask CSS rendering; r?mstange
Attachment #8684815 - Flags: review?(mstange)
Comment on attachment 8685498 [details]
MozReview Request: Bug 686281 - Expands will-change of a shorthand prop to longhand ones; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24803/diff/7-8/
Attachment #8685498 - Attachment description: MozReview Request: Bug 686281 - (r+) Expands shorthand prop to longhands; → MozReview Request: Bug 686281 - Mask CSS animation; r=dbaron.
Attachment #8685498 - Flags: review?(dbaron)
Attachment #8685499 - Attachment description: MozReview Request: Bug 686281 - (r+) A static assertion to keep value correctness of NS_RULE_NODE_IS_ANIMATION_RULE; → MozReview Request: Bug 686281 - Mask CSS webkit-alias; r=dbaron.
Attachment #8685499 - Flags: review?(dbaron)
Comment on attachment 8685499 [details]
MozReview Request: Bug 686281 - A static assertion to keep value correctness of NS_RULE_NODE_IS_ANIMATION_RULE; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24805/diff/7-8/
Bug 686281 - Remove nsStyleSVGReset::mMask; r?dbaron
Attachment #8692847 - Flags: review?(dbaron)
Bug 686281 - Add mask property into property_database.js; r?dbaron.
Attachment #8692848 - Flags: review?(dbaron)
Bug 686281 - Expands will-change of a shorthand prop to longhand ones; r=dbaron.
Attachment #8692849 - Flags: review?(dbaron)
Bug 686281 - A static assertion to keep value correctness of NS_RULE_NODE_IS_ANIMATION_RULE; r=dbaron.
Attachment #8692850 - Flags: review?(dbaron)
Flags: needinfo?(cku)
Attachment #8683558 - Flags: review?(dbaron) → review+
Hi dbaron,
Had fixed issues that you mentioned, please help to review again.
1. Remove nsStyleSVGReset::mMask in "Bug 686281 - Remove nsStyleSVGReset::mMask"
2. Move Mask DOM relative change into "Bug 686281 - Mask DOM API"
3. Fix mochitest fail(property_database.js mainly).
Attachment #8684815 - Flags: review?(mstange)
Comment on attachment 8684815 [details]
MozReview Request: Bug 686281 - Mask mochitest; r=dbaron.

https://reviewboard.mozilla.org/r/24647/#review23767

::: layout/svg/nsSVGIntegrationUtils.cpp:548
(Diff revision 9)
> +    }
> +    else if (hasValidLayers) {

} else if (hasValidLayers) {

::: layout/svg/nsSVGIntegrationUtils.cpp:558
(Diff revision 9)
> +      RefPtr<DrawTarget> targetDT = aContext.GetDrawTarget()->CreateSimilarDrawTarget(drawRect.Size(), SurfaceFormat::B8G8R8A8);

This needs a null check; CreateSimilarDrawTarget can fail.

::: layout/svg/nsSVGIntegrationUtils.cpp:559
(Diff revision 9)
> +      RefPtr<gfxContext> target = new gfxContext(targetDT, drawRect.TopLeft());

Please try to avoid using the device offset. It's much better to multiply a translation into some other transform instead.

::: layout/svg/nsSVGIntegrationUtils.cpp:575
(Diff revision 9)
> +                                            true);

From this call it's really not obvious what true means here. Please use an enum instead, e.g. with DRAW_BACKGROUND and DRAW_MASK_IMAGE as the two possible values, or something along those lines.

::: layout/svg/nsSVGIntegrationUtils.cpp:576
(Diff revision 9)
> +      maskSurface = targetDT->Snapshot();

maskSurface will have the format B8G8R8A8 now. Doesn't it need to be A8? Do all our DrawTarget backends support masking with a B8G8R8A8 surface? What does that do?

::: layout/svg/nsSVGIntegrationUtils.cpp:578
(Diff revision 9)
> +      // Evalute mask transform.

Typo in evaluate; but I think "Compute mask transform." would be a better fit here.
Comment on attachment 8683556 [details]
MozReview Request: Bug 686281 - Implement nsStyleImageLayers; r=dbaron

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24369/diff/21-22/
Comment on attachment 8683557 [details]
MozReview Request: Bug 686281 - Rename *background* to *imagelayer*; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24371/diff/21-22/
Comment on attachment 8683558 [details]
MozReview Request: Bug 686281 - Implement CSS mask style; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24373/diff/21-22/
Attachment #8683558 - Flags: review+ → review?(dbaron)
Comment on attachment 8683706 [details]
MozReview Request: Bug 686281 - Mask CSS parsing and Mask DOM API. r=dbaron

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24415/diff/19-20/
Comment on attachment 8684292 [details]
MozReview Request: Bug 686281 - Mask CSS rendering; r=mstange

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24531/diff/17-18/
Comment on attachment 8684679 [details]
MozReview Request: Bug 686281 - Mask CSS animation; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24617/diff/11-12/
Comment on attachment 8684680 [details]
MozReview Request: Bug 686281 - Mask CSS webkit-alias; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24619/diff/11-12/
https://reviewboard.mozilla.org/r/24647/#review23767

> From this call it's really not obvious what true means here. Please use an enum instead, e.g. with DRAW_BACKGROUND and DRAW_MASK_IMAGE as the two possible values, or something along those lines.

In the new patch, I removed this new parameter(aMask), and pass draw-mask information in aFlag.
If you don't like this change, I am ok to add a new enum as what you said. Just let me know.
Comment on attachment 8684815 [details]
MozReview Request: Bug 686281 - Mask mochitest; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24647/diff/9-10/
Attachment #8684815 - Flags: review?(mstange)
Comment on attachment 8685498 [details]
MozReview Request: Bug 686281 - Expands will-change of a shorthand prop to longhand ones; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24803/diff/8-9/
Comment on attachment 8685499 [details]
MozReview Request: Bug 686281 - A static assertion to keep value correctness of NS_RULE_NODE_IS_ANIMATION_RULE; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24805/diff/8-9/
Comment on attachment 8692847 [details]
MozReview Request: Bug 686281 - Remove nsStyleSVGReset::mMask; r=dbaron

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/26343/diff/1-2/
Comment on attachment 8692848 [details]
MozReview Request: Bug 686281 - mask-composite reftests; r?dbaron

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/26345/diff/1-2/
Comment on attachment 8692849 [details]
MozReview Request: Bug 686281 - Expands will-change of a shorthand prop to longhand ones; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/26347/diff/1-2/
Comment on attachment 8692850 [details]
MozReview Request: Bug 686281 - A static assertion to keep value correctness of NS_RULE_NODE_IS_ANIMATION_RULE; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/26349/diff/1-2/
https://reviewboard.mozilla.org/r/24647/#review23767

> maskSurface will have the format B8G8R8A8 now. Doesn't it need to be A8? Do all our DrawTarget backends support masking with a B8G8R8A8 surface? What does that do?

My bad, you are right, we need only A8 here.
Comment on attachment 8684815 [details]
MozReview Request: Bug 686281 - Mask mochitest; r=dbaron.

https://reviewboard.mozilla.org/r/24647/#review23927

::: layout/svg/nsSVGIntegrationUtils.cpp:516
(Diff revision 10)
> +  // This soure is not a svg mask, but it still can be a correct mask image.

typo: source

::: layout/svg/nsSVGIntegrationUtils.cpp:583
(Diff revision 10)
> +      target->SetMatrix(aContext.CurrentMatrix());
> +      Matrix mat = ToMatrix(target->CurrentMatrix());

I think this should be
Matrix mat = ToMatrix(aContext.CurrentMatrix())
because "target" won't do anything with that new matrix at this point.

::: layout/svg/nsSVGIntegrationUtils.cpp:586
(Diff revision 10)
> +      Matrix deviceOffsetTranslation;
> +      deviceOffsetTranslation.PreTranslate(drawRect.x, drawRect.y);
> +      maskTransform = deviceOffsetTranslation * mat;

I'd leave out the deviceOffsetTranslation variable and just make this
maskTransform = Matrix::Translation(drawRect.x, drawRect.y) * mat;
Attachment #8684815 - Flags: review?(mstange) → review+
Comment on attachment 8683556 [details]
MozReview Request: Bug 686281 - Implement nsStyleImageLayers; r=dbaron

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24369/diff/22-23/
Comment on attachment 8683557 [details]
MozReview Request: Bug 686281 - Rename *background* to *imagelayer*; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24371/diff/22-23/
Comment on attachment 8683558 [details]
MozReview Request: Bug 686281 - Implement CSS mask style; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24373/diff/22-23/
Comment on attachment 8683706 [details]
MozReview Request: Bug 686281 - Mask CSS parsing and Mask DOM API. r=dbaron

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24415/diff/20-21/
Comment on attachment 8684292 [details]
MozReview Request: Bug 686281 - Mask CSS rendering; r=mstange

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24531/diff/18-19/
Comment on attachment 8684679 [details]
MozReview Request: Bug 686281 - Mask CSS animation; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24617/diff/12-13/
Comment on attachment 8684680 [details]
MozReview Request: Bug 686281 - Mask CSS webkit-alias; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24619/diff/12-13/
Comment on attachment 8684815 [details]
MozReview Request: Bug 686281 - Mask mochitest; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24647/diff/10-11/
Attachment #8684815 - Attachment description: MozReview Request: Bug 686281 - Mask CSS rendering; r?mstange → MozReview Request: Bug 686281 - Mask CSS rendering; r=mstange
Comment on attachment 8685498 [details]
MozReview Request: Bug 686281 - Expands will-change of a shorthand prop to longhand ones; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24803/diff/9-10/
Comment on attachment 8685499 [details]
MozReview Request: Bug 686281 - A static assertion to keep value correctness of NS_RULE_NODE_IS_ANIMATION_RULE; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24805/diff/9-10/
Comment on attachment 8692847 [details]
MozReview Request: Bug 686281 - Remove nsStyleSVGReset::mMask; r=dbaron

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/26343/diff/2-3/
Comment on attachment 8692848 [details]
MozReview Request: Bug 686281 - mask-composite reftests; r?dbaron

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/26345/diff/2-3/
Comment on attachment 8692849 [details]
MozReview Request: Bug 686281 - Expands will-change of a shorthand prop to longhand ones; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/26347/diff/2-3/
Comment on attachment 8692850 [details]
MozReview Request: Bug 686281 - A static assertion to keep value correctness of NS_RULE_NODE_IS_ANIMATION_RULE; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/26349/diff/2-3/
Normally, for a review like this, I would look at each patch, look at my previous comments on that patch, and go through only:
 (1) the differences between the current version and the patch that I previously reviewed, and
 (2) the comments that I made
rather than reviewing each patch from scratch.

However, the separation of the patches seems substantially different from the last time I reviewed the patches, and all the noise in the bug, plus the crossing of patch identities in MozReview, makes it pretty much impossible to figure out what relates to what.

I'm seriously considering giving up on trying to figure out how the patches relate to my previous review comments and to the previous patches that I reviewed, and just reviewing them all again from scratch.  This will probably mean I'll have a substantial number of comments that I didn't notice last time, but that's what happens when you do post-review reorganization of the patch series other than what was requested in the reviews.
Comment on attachment 8683556 [details]
MozReview Request: Bug 686281 - Implement nsStyleImageLayers; r=dbaron

https://reviewboard.mozilla.org/r/24369/#review24093

I don't think this makes sense as a separate patch in the way you've structured it.  You should move the code from one class to the other in a single patch -- this makes understanding history possible using hg annotate, etc., and it makes it clearer what you're changing.  Splitting this patch from the one that removed the code from nsStyleBackground::Layer should not have been done (and I didn't ask for that change).
I'm going to wait for comment 288 to be addressed before continuing the review; fixing that might make it easier for me to associate the patches with what they were the last time I reviewed them.
(In reply to David Baron [:dbaron] ⌚UTC-8 from comment #289)
> I'm going to wait for comment 288 to be addressed before continuing the
> review; fixing that might make it easier for me to associate the patches
> with what they were the last time I reviewed them.

Just want to double confirm:
You want these two patches to be merge as one just like before:
1. (The first patch) Bug 686281 - Create nsStyleImageLayers; r?dbaron. 
2. (The second patch) Bug 686281 - Integrate nsStyleImageLayers with nsStyleBackground; r?dbaron. 

And..
I also split two more patches
3. Bug 686281 - Mask DOM API
4. Bug 686281 - Mask style computing
Which original be part of "Bug 686281 - Mask CSS parsing"
Do you want me merge them back as well? Or, it's ok for you.
Flags: needinfo?(dbaron)
Here is a draft
https://reviewboard.mozilla.org/r/24369/
Please check whether it's acceptable for reviewing
(In reply to C.J. Ku[:cjku] from comment #290)
> (In reply to David Baron [:dbaron] ⌚UTC-8 from comment #289)
> > I'm going to wait for comment 288 to be addressed before continuing the
> > review; fixing that might make it easier for me to associate the patches
> > with what they were the last time I reviewed them.
> 
> Just want to double confirm:
> You want these two patches to be merge as one just like before:
> 1. (The first patch) Bug 686281 - Create nsStyleImageLayers; r?dbaron. 
> 2. (The second patch) Bug 686281 - Integrate nsStyleImageLayers with
> nsStyleBackground; r?dbaron. 

These should be put back together.

(If you were starting from the beginning, I'd have preferred them split in a different way -- the refactoring first (removal and addition), and then the addition of the mask bits in a separate patch.  But at this point in the process it's better to keep them together.)

> And..
> I also split two more patches
> 3. Bug 686281 - Mask DOM API
> 4. Bug 686281 - Mask style computing
> Which original be part of "Bug 686281 - Mask CSS parsing"
> Do you want me merge them back as well? Or, it's ok for you.

Those should also be back together.
Flags: needinfo?(dbaron)
Could you merge those patches back together so that I can start reviewing?

(I don't want to start without that, because MozReview is going to mess everything up, since the patches will get crossed given that you're using git.)
Flags: needinfo?(cku)
Comment on attachment 8683557 [details]
MozReview Request: Bug 686281 - Rename *background* to *imagelayer*; r=dbaron.

Please let me know when you've updated the patches per above comments.
Attachment #8683557 - Flags: review?(dbaron)
Hi Markus,
Here is the test failure I mentioned in the morning:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=104f7f9a3018&selectedJob=14488315

1. Change the source draw target format to A8
https://hg.mozilla.org/try/diff/ce834c5b9f6b/layout/svg/nsSVGIntegrationUtils.cpp#l1.149
2. Additional, modified nsLayoutUtils is also required..
https://hg.mozilla.org/try/diff/ce834c5b9f6b/layout/base/nsLayoutUtils.cpp#l1.13

Ignore dynamic-mask-pre-effects-bbox.html, since it cause by another patch.
As you can see, reftest(mask-composite-XX.html) are pass on Linux, but fail on Mac/ Windows.
The composition result of mask images with CompostionOp::OP_IN/OP_OUT/OP_XOR are the same with CompositionOp::OP_OVER, which is wrong.
I think the root cause should be located in platform drawing function such as DrawTargetCG::DrawSurface, but I am not figuring out yet.
Flags: needinfo?(cku) → needinfo?(mstange)
(In reply to David Baron [:dbaron] ⌚UTC-5 from comment #293)
> Could you merge those patches back together so that I can start reviewing?
> 
> (I don't want to start without that, because MozReview is going to mess
> everything up, since the patches will get crossed given that you're using
> git.)
Will update merged version soon(I am figuring out mask composition problem of my patches)
I talked to Bas and he told me that the Moz2D API MaskSurface doesn't guarantee correct results with a BGRA mask surface. And conceptually, A8 is the right format to use for a mask.
I'll try to debug where your problem is coming from. You're probably right that it's a problem with DrawSurface.
(In reply to Markus Stange [:mstange] from comment #297)
> I talked to Bas and he told me that the Moz2D API MaskSurface doesn't
> guarantee correct results with a BGRA mask surface. And conceptually, A8 is
> the right format to use for a mask.
> I'll try to debug where your problem is coming from. You're probably right
> that it's a problem with DrawSurface.

I discussed with him as well, be correct a logic error in previous patch, it seem to be a Mac specific problem. 
https://treeherder.mozilla.org/#/jobs?repo=try&revision=577792c6d8e7
I am going to fire a bug for tracing this issue.
In short, Compose a source surface onto a A8 source with CompostionOp::OP_IN/OUT/XOR on Mac(Quartz2D) get wrong result.
Attachment #8683558 - Attachment description: MozReview Request: Bug 686281 - Rename *background* to *imagelayer*; r=dbaron. → MozReview Request: Bug 686281 - Implement CSS mask style; r?dbaron.
Attachment #8683558 - Flags: review?(dbaron)
Attachment #8684292 - Attachment description: MozReview Request: Bug 686281 - Mask CSS parsing; r?dbaron → MozReview Request: Bug 686281 - Mask CSS rendering; r=mstange
Attachment #8684292 - Flags: review?(mstange)
Attachment #8684679 - Attachment description: MozReview Request: Bug 686281 - Mask style computing; r?dbaron → MozReview Request: Bug 686281 - Mask CSS animation; r=dbaron.
Attachment #8684679 - Flags: review?(dbaron)
Attachment #8685498 - Attachment description: MozReview Request: Bug 686281 - Mask CSS animation; r=dbaron. → MozReview Request: Bug 686281 - Expands will-change of a shorthand prop to longhand ones; r=dbaron.
Attachment #8685498 - Flags: review?(dbaron)
Attachment #8685499 - Attachment description: MozReview Request: Bug 686281 - Mask CSS webkit-alias; r=dbaron. → MozReview Request: Bug 686281 - A static assertion to keep value correctness of NS_RULE_NODE_IS_ANIMATION_RULE; r=dbaron.
Attachment #8685499 - Flags: review?(dbaron)
Attachment #8692850 - Attachment is obsolete: true
Attachment #8692849 - Attachment is obsolete: true
Hi dbaron,
I had merged back several patches, please check. And I also add several basic mask-composition reftests, prefix with fail-if(cocawidget) because of bug 1231643.
Hi Markus,
Since sequence number of rendering patch changes, r+ dismiss. I also fix a bug in nsCSSRendering.cpp while testing mask-composition, please help to review again.
Attachment #8683181 - Attachment is obsolete: true
Comment on attachment 8683556 [details]
MozReview Request: Bug 686281 - Implement nsStyleImageLayers; r=dbaron

https://reviewboard.mozilla.org/r/24369/#review25479

::: layout/base/nsCSSRendering.cpp:1750
(Diff revision 24)
> -nsCSSRendering::GetBackgroundClip(const nsStyleBackground::Layer& aLayer,
> -                                  nsIFrame* aForFrame, const nsStyleBorder& aBorder,
> -                                  const nsRect& aBorderArea, const nsRect& aCallerDirtyRect,
> -                                  bool aWillPaintBorder, nscoord aAppUnitsPerPixel,
> +nsCSSRendering::GetBackgroundClip(const nsStyleImageLayers::Layer& aLayer,
> +                                  nsIFrame* aForFrame,
> +                                  const nsStyleBorder& aBorder,
> +                                  const nsRect& aBorderArea,
> +                                  const nsRect& aCallerDirtyRect,
> +                                  bool aWillPaintBorder,
> +                                  nscoord aAppUnitsPerPixel,

Please don't do unnecessary reformatting here; just make the one change you need.

::: layout/base/nsCSSRendering.cpp:2998
(Diff revision 24)
> -    NS_FOR_VISIBLE_BACKGROUND_LAYERS_BACK_TO_FRONT_WITH_RANGE(i, bg, bg->mImageCount - 1,
> -                                                              nLayers + (bg->mImageCount -
> +    NS_FOR_VISIBLE_IMAGELAYER_BACK_TO_FRONT_WITH_RANGE(i, layers, layers.mImageCount - 1,
> +                                                       nLayers + (layers.mImageCount -
> -                                                                         startLayer - 1)) {
> +                                                       startLayer - 1)) {
> -      const nsStyleBackground::Layer &layer = bg->mLayers[i];
> +      const nsStyleImageLayers::Layer& layer = layers.mLayers[i];

Please rename this macro to NS_FOR_VISIBLE_IMAGE_LAYERS_BACK_TO_FRONT_WITH_RANGE (per comment 129).

And please keep the indentation of "startLayer - 1" matching the opening ( that it's in.

::: layout/generic/nsFrame.cpp:775
(Diff revision 24)
> -    NS_FOR_VISIBLE_BACKGROUND_LAYERS_BACK_TO_FRONT(i, oldBG) {
> +    NS_FOR_VISIBLE_IMAGELAYER_BACK_TO_FRONT(i, oldBGLayers) {

Again, please also rename this macro as previously requested in comment 129.

::: layout/style/nsComputedDOMStyle.h:38
(Diff revision 24)
>  struct nsStyleBackground;
> +struct nsStyleImageLayers;

These two lines should be dropped, and you should #include "nsStyleStruct.h" instead, given that the declarations below use nsStyleImageLayers::Layer, etc.

::: layout/style/nsStyleStruct.h:600
(Diff revision 24)
> +  #define NS_FOR_VISIBLE_IMAGELAYER_BACK_TO_FRONT(var_, layers_) \
> +    for (uint32_t var_ = layers_.mImageCount; var_-- != 0; )
> +  #define NS_FOR_VISIBLE_IMAGELAYER_BACK_TO_FRONT_WITH_RANGE(var_, layers_, start_, count_) \
> +    NS_ASSERTION((int32_t)(start_) >= 0 && (uint32_t)(start_) < (layers_.mImageCount), "Invalid layer start!"); \

Please restore the () around all macro arguments other than var\_ (which must be simply an ident) when used inside the macro.  This is good practice for making macros behave a bit more like functions, since it avoids having to worry about operator precedence in how macro arguments interact with their surroundings.

That is, put the () back around layers\_; stylebg\_ had () before.

::: layout/style/nsStyleStruct.cpp:2281
(Diff revision 24)
> -nsStyleBackground::nsStyleBackground()
> +nsStyleImageLayers::nsStyleImageLayers()

Please add MOZ_COUNT_CTOR(nsStyleImageLayers) in both nsStyleImageLayers constructors and MOZ_COUNT_DTOR(nsStyleImageLayers) in the destructor.

r=dbaron with those changes

(You may have noticed that for this patch, at least, I found it easier to re-review the entire patch than figure out where all of my previous comments were.  That's the result of the bug being such a mess.  And it resulted in a few, although not many, additional comments that I didn't make before.)
Comment on attachment 8683557 [details]
MozReview Request: Bug 686281 - Rename *background* to *imagelayer*; r=dbaron.

https://reviewboard.mozilla.org/r/24371/#review25485

::: layout/base/nsCSSRendering.cpp:2942
(Diff revision 24)
> -    GetBackgroundClip(layers.BottomLayer(),
> +    GetImageLayerClip(layers.BottomLayer(),
> -                      aForFrame, aBorder, aBorderArea,
> +                 aForFrame, aBorder, aBorderArea,
> -                      aDirtyRect, (aFlags & PAINTBG_WILL_PAINT_BORDER), appUnitsPerPixel,
> +                 aDirtyRect, (aFlags & PAINTBG_WILL_PAINT_BORDER),
> +                 appUnitsPerPixel,
> -                      &clipState);
> +                 &clipState);

Please keep the arguments lined up inside the (.

I mentioned this before in comment 171.

::: layout/base/nsCSSRendering.cpp:3217
(Diff revision 24)
> -   * The properties we need to keep in mind when drawing background
> +   * The properties we need to keep in mind when drawing style
>     * layers are:

"image layers" or "style image layers", not "style layers"

::: layout/style/nsCSSParser.cpp:866
(Diff revision 24)
> -  bool IsFunctionTokenValidForBackgroundImage(const nsCSSToken& aToken) const;
> +  bool IsFunctionTokenValidForStyleLayerImage(const nsCSSToken& aToken) const;

ImageLayerImage, not StyleLayerImage

::: layout/style/nsCSSParser.cpp:11536
(Diff revision 24)
> -  BackgroundParseState state(color, image.SetListValue(),
> +  ImageLayersShorthandParseState state(color, image.SetListValue(),
>                                repeat.SetPairListValue(),
>                                attachment.SetListValue(), clip.SetListValue(),
>                                origin.SetListValue(), position.SetListValue(),
>                                size.SetPairListValue(), composite.SetListValue(),
>                                mode.SetListValue());

Please fix indentation to line up with the (.  (The prior-to-patch state appears to be off by one.)

::: layout/style/nsCSSProps.cpp:884
(Diff revision 24)
>  // Note: Don't change this table unless you update
>  // parseBackgroundPosition!

Please update this comment to refer to ParseImageLayerPosition (and fix the initial p -> P).

::: layout/style/nsStyleStruct.h:513
(Diff revision 24)
> -   nsStyleImage  mImage;         // [reset]
> +    nsStyleImage  mImage;         // [reset]

seems like this is fixing an indentation mistake in the previous patch, and it should actually be fixed there

r=dbaron with those changes

(again, this was a complete re-review)
Attachment #8683557 - Flags: review?(dbaron) → review+
https://reviewboard.mozilla.org/r/24369/#review25479

Really sorry for that. It's my first time using reviewboard, I will split patches more accurately before asking review next time.
Please stop updating the patches so often (and mid-review); please only submit new patches to reviewboard when you want a different set of patches to be reviewed.
(In reply to David Baron [:dbaron] ⌚️UTC-5 from comment #327)
> Please stop updating the patches so often (and mid-review); please only
> submit new patches to reviewboard when you want a different set of patches
> to be reviewed.
By reading mozreview user manual, 
http://mozilla-version-control-tools.readthedocs.org/en/latest/mozreview/review-requests.html#choosing-what-commits-to-review
I think you want me to use "hg push -c $patch-id review" to update the patch that changed.
(In reply to C.J. Ku[:cjku] from comment #328)
> (In reply to David Baron [:dbaron] ⌚️UTC-5 from comment #327)
> > Please stop updating the patches so often (and mid-review); please only
> > submit new patches to reviewboard when you want a different set of patches
> > to be reviewed.
> By reading mozreview user manual, 
> http://mozilla-version-control-tools.readthedocs.org/en/latest/mozreview/
> review-requests.html#choosing-what-commits-to-review
> I think you want me to use "hg push -c $patch-id review" to update the patch
> that changed.

"hg push -c" will change review-id, and that's not I want.
Since I use git as source control tool, I can't leverage the power of hg evolve. Need to find another way to prevent update all patches(or, using hg directly)
Comment on attachment 8683558 [details]
MozReview Request: Bug 686281 - Implement CSS mask style; r=dbaron.

https://reviewboard.mozilla.org/r/24373/#review25579

OK, previous comments on this patch are in comment 131 and comment 162.

The version I was reviewing in comment 131 was https://reviewboard.mozilla.org/r/24373/diff/13/ or https://reviewboard.mozilla.org/r/24373/diff/13/raw/

The version I was reviewing in comment 162 was https://reviewboard.mozilla.org/r/24415/diff/13/ or https://reviewboard.mozilla.org/r/24415/diff/13/raw/

The current review request is https://reviewboard.mozilla.org/r/24373/diff/25/ or https://reviewboard.mozilla.org/r/24373/diff/25/raw/


Except you've now split that patch into two parts, despite that I asked
for them to be merged back together in comment 292.

Please merge them back before landing, but not before I've finished reviewing the whole series.

::: layout/generic/nsFrame.cpp:736
(Diff revision 25)
> +AssociateStyleImageWithFrame(nsFrame* aFrame,
> +                             const nsStyleImageLayers* aOldLayers,
> +                             const nsStyleImageLayers* aNewLayers)

Given what the caller has, these parameters should be "const nsStyleImageLayers&" instead of "const nsStyleImageLayers\*".


Also, since this both adds and removes associations, I think the method should be called AddAndRemoveImageAssociations or something like that; the current name suggests that it only adds.

::: layout/generic/nsFrame.cpp:743
(Diff revision 25)
> +  // If the old context had a background image image, or mask image image,

It's an existing comment, but maybe fix it to read "background-image image" and "mask-image image" with dashes.

::: layout/generic/nsFrame.cpp:745
(Diff revision 25)
> +  // notifier(which keeps the image loading, if it still is) for the frame.

space before the (

::: layout/generic/nsFrame.cpp:808
(Diff revision 25)
> -  ImageLoader* imageLoader = PresContext()->Document()->StyleImageLoader();
> +  // Setup relation between background style images and this frame.

Drop this comment; it doesn't add anything to the code.

::: layout/generic/nsFrame.cpp:815
(Diff revision 25)
> -        imageLoader->DisassociateRequestFromFrame(oldImage.GetImageData(),
> +  // Setup relation between mask style images and this frame.

same for this comment

::: layout/style/nsRuleNode.cpp:9649
(Diff revision 25)
> -  const nsCSSValue* maskValue = aRuleData->ValueForMask();
> -  if (eCSSUnit_URL == maskValue->GetUnit()) {
> -    svgReset->mMask = maskValue->GetURLValue();
> +  const nsCSSValue* maskValue = aRuleData->ValueForMaskImage();
> +  if (eCSSUnit_List == maskValue->GetUnit() ||
> +      eCSSUnit_ListDep == maskValue->GetUnit()) {
> +    const nsCSSValue& item = maskValue->GetListValue()->mValue;
> +    if (eCSSUnit_URL == item.GetUnit() ||
> +        eCSSUnit_Image == item.GetUnit()) {
> +      svgReset->mMask = item.GetURLValue();
> +    }

I guess the goal of this is to keep the existing SVG mask code working?

But is it also to be part of allowing the new code to be pref-controlled?  It seems like it should be -- although I think all the pref stuff has disappeared from the patch, despite comment 100, comment 162, although I don't understand the response in comment 239 (why do we need both mMask and mMaskImage in the long term?).  If it is intended to allow landing of this new feature behind a pref (or perhaps even if not), I think the assignment to svgReset->mMask should also be conditional on the linked list being a single-item list.

::: layout/style/nsStyleConsts.h:1151
(Diff revision 25)
> +// composite
> +#define NS_STYLE_COMPOSITE_MODE_ADD                 0
> +#define NS_STYLE_COMPOSITE_MODE_SUBSTRACT           1
> +#define NS_STYLE_COMPOSITE_MODE_INTERSECT           2
> +#define NS_STYLE_COMPOSITE_MODE_EXCLUDE             3

Please replace COMPOSITE_MODE with MASK_COMPOSITE in all of these names, since the property is called mask-composite and doesn't have the word mode in it.

::: layout/style/nsStyleStruct.h:403
(Diff revision 25)
> +  enum {
> +    shorthand = 0,
> +    color,
> +    image,
> +    repeat,
> +    position,
> +    clip,
> +    origin,
> +    size,
> +    attachment,
> +    maskMode,
> +    composite
> +  };

Please add a comment saying that these are the indices into kBackgroundLayerTable and kMaskLayerTable.

::: layout/style/nsStyleStruct.h:426
(Diff revision 25)
> +    static bool IsInitialValue(const Position& aPosition);

This shouldn't be a static method; it should be:

bool IsInitialValue() const;

::: layout/style/nsStyleStruct.h:464
(Diff revision 25)
> +    static bool IsInitialValue(const Size& aSize) {
> +      return (aSize.mWidthType == eAuto && aSize.mHeightType == eAuto);
> +    }

Again, this shouldn't be a static method; it should be bool IsInitialValue() const.

::: layout/style/nsStyleStruct.h:521
(Diff revision 25)
> +    static bool IsInitialValue(const Repeat& aRepeat);

Same here; not a static method; instead, bool IsInitialValue() const.

::: layout/style/nsStyleStruct.h:654
(Diff revision 25)
> +
> +

Please don't insert these 2 blank lines.

::: layout/style/nsStyleStruct.h:3407
(Diff revision 25)
> +  nsStyleImageLayers    mLayers;

This should really be called mMask, but you're better off fixing that when you get rid of the current mMask.  (That said, I'm not sure why you need to keep it.)

So please just add a FIXME for now, and file a followup bug blocking enabling the feature.

::: layout/style/nsStyleStruct.cpp:1358
(Diff revision 25)
> +  mLayers.CalcDifference(aOther.mLayers, hint);

Instead of passing the hint to nsStyleImageLayers::CalcDifference, please making nsStyleImageLayers::CalcDifference return the hint, and use |= here (and at the other caller).

::: layout/style/nsStyleStruct.cpp:2412
(Diff revision 25)
> +bool
> +nsStyleImageLayers::HasValidLayers() const
> +{
> +  for (uint32_t i = 0; i < mImageCount; i++) {
> +    if (!mLayers[i].mImage.IsEmpty()) {
> +      return true;
> +    }
> +  }
> +
> +  return false;
> +}

Please rename HasValidLayers to HasLayerWithImage to make it clear what it means.

::: layout/style/nsStyleStruct.cpp:2429
(Diff revision 25)
> +      aPosition.mXPosition.mLength == 0&&

space before &&

::: layout/style/nsStyleStruct.cpp:2568
(Diff revision 25)
> +  return (aRepeat.mXRepeat == NS_STYLE_IMAGELAYER_REPEAT_REPEAT) &&
> +         (aRepeat.mYRepeat == NS_STYLE_IMAGELAYER_REPEAT_REPEAT);

Please remove all of the () here.

r=dbaron with the above comments addressed, *and* with a bug filed on fixing the pref-control of these properties that you intend to fix in the *same cycle* as landing these patches.  In fact, it should probably be written and reviewed, on top of these patches, before these land, so that it can land at the same time.

It's NOT ACCEPTABLE to try fixing it in this bug because I'm simply not willing to deal with another round of review here.
Attachment #8683558 - Flags: review?(dbaron) → review+
https://reviewboard.mozilla.org/r/24373/#review25655

::: layout/style/nsStyleStruct.h:465
(Diff revision 25)
> +      return (aSize.mWidthType == eAuto && aSize.mHeightType == eAuto);

Also, drop the () here.
Comment on attachment 8683706 [details]
MozReview Request: Bug 686281 - Mask CSS parsing and Mask DOM API. r=dbaron

https://reviewboard.mozilla.org/r/24415/#review25669

OK, previous comments on this patch are in comment 131 and comment 162.

The version I was reviewing in comment 131 was https://reviewboard.mozilla.org/r/24373/diff/13/ or https://reviewboard.mozilla.org/r/24373/diff/13/raw/

The version I was reviewing in comment 162 was https://reviewboard.mozilla.org/r/24415/diff/13/ or https://reviewboard.mozilla.org/r/24415/diff/13/raw/

You've now split that patch into two parts, despite that I asked for
them to be merged back together in comment 292.

The other half of the patch is
https://reviewboard.mozilla.org/r/24373/diff/25/ or
https://reviewboard.mozilla.org/r/24373/diff/25/raw/

This half of the patch is
https://reviewboard.mozilla.org/r/24415/diff/23/
https://reviewboard.mozilla.org/r/24415/diff/23/raw/

::: layout/style/Declaration.cpp:211
(Diff revision 23)
> +  // We know from above that all subproperties were specified.

"above" should now be "our caller" or similar

::: layout/style/Declaration.cpp:252
(Diff revision 23)
> +      if (aTable[nsStyleImageLayers::color] !=  eCSSProperty_UNKNOWN) {

one space rather than two, please

::: layout/style/Declaration.cpp:352
(Diff revision 23)
> +      // This layer is an mask layer
> +      } else {

Please assert about the value of aTable here.

::: layout/style/Declaration.cpp:370
(Diff revision 23)
> +    // This layer is an mask layer
> +    } else {

again, please assert here

::: layout/style/nsCSSParser.cpp:11603
(Diff revision 23)
> +#define APPENDVALUE(propID_, propValue_) \
> +  if (aTable[propID_] != eCSSProperty_UNKNOWN) { \
> +    AppendValue(aTable[propID_],  propValue_); \
> +  }

I think it would be clearer to either:
 (1) have the macro just take one argument, and write the "nsStyleImageLayers::" inside the macro, or
 (2) write the aTable[] in the uses of the macro

::: layout/style/nsCSSParser.cpp:11717
(Diff revision 23)
> -      } else if (nsCSSProps::FindKeyword(keyword,
> +      } else if (aTable[nsStyleImageLayers::attachment] !=
> +                 eCSSProperty_UNKNOWN &&

Please indent the eCSSProperty_UNKNOWN by 2 more spaces.

::: layout/style/nsCSSParser.cpp:11824
(Diff revision 23)
>        } else {
> +        if (aTable[nsStyleImageLayers::color] != eCSSProperty_UNKNOWN) {

Please merge this into a single "else if", and un-indent by 2 spaces everything up to and including the else { return false; } that's about 10 lines lower.

::: layout/style/nsCSSParser.cpp:11869
(Diff revision 23)
>      } else {
> +      if (aTable[nsStyleImageLayers::color] != eCSSProperty_UNKNOWN) {

same here

::: layout/style/nsComputedDOMStyle.cpp:5842
(Diff revision 23)
> +  if (svg->mLayers.mImageCount > 1 ||
> +      firstLayer.mClip != NS_STYLE_IMAGELAYER_CLIP_BORDER ||
> +      firstLayer.mOrigin != NS_STYLE_IMAGELAYER_ORIGIN_PADDING ||
> +      firstLayer.mAttachment != NS_STYLE_IMAGELAYER_ATTACHMENT_SCROLL ||
> +      firstLayer.mBlendMode != NS_STYLE_BLEND_NORMAL ||
> +      firstLayer.mComposite != NS_STYLE_COMPOSITE_MODE_ADD ||
> +      firstLayer.mMaskMode != NS_STYLE_MASK_MODE_AUTO ||
> +      !nsStyleImageLayers::Position::IsInitialValue(firstLayer.mPosition) ||
> +      !nsStyleImageLayers::Repeat::IsInitialValue(firstLayer.mRepeat) ||
> +      !nsStyleImageLayers::Size::IsInitialValue(firstLayer.mSize)){
> +    return nullptr;
> +  }

Could you add a comment here saying that mask is now a shorthand, but it used to be a longhand, and thus we need to support computed style for it for the cases where it used to be a longhand.

r=dbaron with those changes
Comment on attachment 8684679 [details]
MozReview Request: Bug 686281 - Mask CSS animation; r=dbaron.

https://reviewboard.mozilla.org/r/24617/#review25739

Previous comments on this patch are in comment 166.

The version I was reviewing in comment 162 was https://reviewboard.mozilla.org/r/24617/diff/6/ or https://reviewboard.mozilla.org/r/24617/diff/6/raw/

This patch is
https://reviewboard.mozilla.org/r/24617/diff/15/
https://reviewboard.mozilla.org/r/24617/diff/15/raw/

::: layout/style/StyleAnimationValue.cpp:2982
(Diff revision 15)
> +ExtractImageLayerPositionList(const nsStyleImageLayers& aLayer,
> +                          StyleAnimationValue& aComputedValue)

Please fix indentation of the second line following the rename requested before.

Also call the parameter aLayers rather than aLayer, as requested before (when I said to change layers to aLayers).

::: layout/style/StyleAnimationValue.cpp:3001
(Diff revision 15)
> +ExtractImageLayerSizePairList(const nsStyleImageLayers& aLayer,
> +                      StyleAnimationValue& aComputedValue)

Same 2 comments here.

::: layout/style/StyleAnimationValue.cpp:3381
(Diff revision 15)
> -          nsAutoPtr<nsCSSValueList> result;
> +          MOZ_ASSERT(layers.mPositionCount > 0, "unexpected count");

This MOZ_ASSERT should be removed from its old location now that it's moved into the shared function.

r=dbaron with that

This patch was quite easy to review by diffing against the old version and comparing to my comments (although reviewboard doesn't make that easy).
Attachment #8684679 - Flags: review?(dbaron) → review+
Comment on attachment 8684815 [details]
MozReview Request: Bug 686281 - Mask mochitest; r=dbaron.

https://reviewboard.mozilla.org/r/24647/#review25745

Previous comments on this patch are in comment 174.

The version I was reviewing in comment 174 was https://reviewboard.mozilla.org/r/24647/diff/4/ or https://reviewboard.mozilla.org/r/24647/diff/4/raw/

This patch is https://reviewboard.mozilla.org/r/24647/diff/13/ or https://reviewboard.mozilla.org/r/24647/diff/13/raw/


There appear to be numerous other changes here than the ones requested in the previous cycle of review.  I'm guessing that's because you requested review the previous time without the tests actually passing... which probably wasn't a good idea.

::: layout/style/test/property_database.js:4076
(Diff revision 13)
> +  /* FIXME: All mask-border-* should be add whe we implement them. */

This FIXME should be right above the subproperties line, since that's where they're missing.

Also, add -> added, and whe -> when.

::: layout/style/test/property_database.js:4080
(Diff revision 13)
> +    type: CSS_TYPE_TRUE_SHORTHAND,

I asked last time for this to be CSS_TYPE_SHORTHAND_AND_LONGHAND.  Did that not work?

::: layout/style/test/property_database.js:4142
(Diff revision 13)
> +      "linear-gradient(red -99, yellow, alpha, blue 120%)",

This "alpha" should still be "green".

r=dbaron with that
Attachment #8684815 - Flags: review?(dbaron) → review+
Comment on attachment 8685498 [details]
MozReview Request: Bug 686281 - Expands will-change of a shorthand prop to longhand ones; r=dbaron.

https://reviewboard.mozilla.org/r/24803/#review25779

Previous comments on this patch are in comment 164.

The version I was reviewing in comment 164 was https://reviewboard.mozilla.org/r/24803/diff/3/ or https://reviewboard.mozilla.org/r/24803/diff/3/raw/

This patch is https://reviewboard.mozilla.org/r/24803/diff/12/ or https://reviewboard.mozilla.org/r/24803/diff/12/raw/




This needs some substantial merging with trunk (for the FIXPOS_CB and ABSPOS_CB changes), which probably means you want to move the chunk that handles the three bits into a separate function so that you can call it from both the shorthand and non-shorthand cases.

r=dbaron with that, although I should probably look at the revised version
Attachment #8685498 - Flags: review?(dbaron) → review+
Comment on attachment 8685499 [details]
MozReview Request: Bug 686281 - A static assertion to keep value correctness of NS_RULE_NODE_IS_ANIMATION_RULE; r=dbaron.

https://reviewboard.mozilla.org/r/24805/#review25781
https://reviewboard.mozilla.org/r/24415/#review25789

::: layout/style/nsComputedDOMStyle.cpp:5836
(Diff revision 23)
>  CSSValue*
>  nsComputedDOMStyle::DoGetMask()
>  {
> -  nsROCSSPrimitiveValue* val = new nsROCSSPrimitiveValue;
> -
>    const nsStyleSVGReset* svg = StyleSVGReset();
> +  const nsStyleImageLayers::Layer& firstLayer = svg->mLayers.mLayers[0];
> +
> +  if (svg->mLayers.mImageCount > 1 ||
> +      firstLayer.mClip != NS_STYLE_IMAGELAYER_CLIP_BORDER ||
> +      firstLayer.mOrigin != NS_STYLE_IMAGELAYER_ORIGIN_PADDING ||
> +      firstLayer.mAttachment != NS_STYLE_IMAGELAYER_ATTACHMENT_SCROLL ||
> +      firstLayer.mBlendMode != NS_STYLE_BLEND_NORMAL ||
> +      firstLayer.mComposite != NS_STYLE_COMPOSITE_MODE_ADD ||
> +      firstLayer.mMaskMode != NS_STYLE_MASK_MODE_AUTO ||
> +      !nsStyleImageLayers::Position::IsInitialValue(firstLayer.mPosition) ||
> +      !nsStyleImageLayers::Repeat::IsInitialValue(firstLayer.mRepeat) ||
> +      !nsStyleImageLayers::Size::IsInitialValue(firstLayer.mSize)){
> +    return nullptr;
> +  }

One more comment here (noticed while reviewing https://reviewboard.mozilla.org/r/26343/diff/5 ):

These conditions in the if should also check one more condition, which is:

!(firstLayer.mImage.GetType() == eStyleImageType_Null || firstLayer.mImage.GetType() == eStyleImageType_Image)

since otherwise you'll end up incorrectly reporting "none" here when a gradient or -moz-element() was given as the value of mask or mask-image.
Comment on attachment 8692847 [details]
MozReview Request: Bug 686281 - Remove nsStyleSVGReset::mMask; r=dbaron

https://reviewboard.mozilla.org/r/26343/#review25787

This patch wasn't previously reviewed.

::: layout/style/nsRuleNode.cpp:6451
(Diff revision 5)
> +    if (eCSSUnit_URL == aSpecifiedValue->mValue.GetUnit() ||
> +       eCSSUnit_Image == aSpecifiedValue->mValue.GetUnit()) {

You shouldn't need to handle eCSSUnit_URL here, only eCSSUnit_Image.  If you do, there's something wrong with the flags that are used by ShouldStartImageLoads in nsCSSDataBlock.cpp.

(But if you do need it, you'd want to fix the indentation on the second line.)

::: layout/style/nsRuleNode.cpp:6454
(Diff revision 5)
> +    }

This also needs an:

else if (eCSSUnit_Null != aSpecifiedValue->mValue.GetUnit()) {
  aComputedValue = null;
}

so that non-url values correctly override url values.

::: layout/style/nsRuleNode.cpp:6689
(Diff revision 5)
> +  case eCSSUnit_None:

I don't understand why you need this addition.  Could you explain?

::: layout/style/nsRuleNode.cpp:6757
(Diff revision 5)
> +  case eCSSUnit_None:

Same here.

::: layout/style/nsStyleStruct.cpp:2409
(Diff revision 5)
>  bool
>  nsStyleImageLayers::HasValidLayers() const
>  {
>    for (uint32_t i = 0; i < mImageCount; i++) {
> -    if (!mLayers[i].mImage.IsEmpty()) {
> +    if (mLayers[i].mSourceURI) {
>        return true;

Given this change, the comment I made in a previous review about renaming HasValidLayers() should probably be changed to a different name, such as HasLayerWithSourceURI.

::: layout/style/nsStyleStruct.cpp:2627
(Diff revision 5)
> +    NS_UpdateHint(hint, nsChangeHint_UpdateEffects);
> +    NS_UpdateHint(hint, nsChangeHint_RepaintFrame);
> +    // Mask changes require that we update the PreEffectsBBoxProperty,
> +    // which is done during overflow computation.
> +    NS_UpdateHint(hint, nsChangeHint_UpdateOverflow);

Are CSS masks implemented differently enough at the rendering level that they don't need the UpdateEffects or UpdateOverflow hints?  (I think that seems likely, but I'd like to make sure.)

::: layout/svg/nsSVGEffects.cpp:555
(Diff revision 5)
> -  EffectProperties result;
> +  EffectProperties result{nullptr, nullptr, nullptr};

Instead of doing this, could you just set result.mMask to nullptr in an else, to match the rest of the code?

r=dbaron with that, although I'd like to understand the eCSSUnit_None stuff and the question about change hints
Attachment #8692847 - Flags: review?(dbaron) → review+
Comment on attachment 8692848 [details]
MozReview Request: Bug 686281 - mask-composite reftests; r?dbaron

https://reviewboard.mozilla.org/r/26345/#review25795

::: layout/reftests/mask/mask-composite-1b.html:18
(Diff revision 5)
> +  -webkit-mask-composite: source-over;

If they go in the w3c-css folder, we probably don't want the -webkit- CSS in them.

::: layout/reftests/mask/reftest.list:4
(Diff revision 5)
> +fails-if(cocoaWidget) == mask-composite-2a.html mask-composite-2-ref.html
> +fails-if(cocoaWidget) == mask-composite-2b.html mask-composite-2-ref.html

How and why do these fail?  There should be a comment in the manifest and probably a bug number.

::: layout/reftests/reftest.list:253
(Diff revision 5)
> +# mask/
> +include mask/reftest.list

It would have been better to write these in the CSSWG format (which requires a little bit of additional metadata), and put them in layout/reftests/w3c-css/submitted/masking

Or is there a reason not to do that?


Also, why are these tests only for mask-composite, and not for other aspects of masking?  Do those not need testing?
https://reviewboard.mozilla.org/r/26345/#review25795

> How and why do these fail?  There should be a comment in the manifest and probably a bug number.

Will comment(bug 1231643)

> It would have been better to write these in the CSSWG format (which requires a little bit of additional metadata), and put them in layout/reftests/w3c-css/submitted/masking
> 
> Or is there a reason not to do that?
> 
> 
> Also, why are these tests only for mask-composite, and not for other aspects of masking?  Do those not need testing?

1. Will move to w3c-css/submitted/masking
2. Except mask-mode and mask-composite, the rendering path of all the rest mask properties are the same with backbround properties. So I decided add mask-composite reftest first(reftest of mask-mode should be done in bug 1228354). I will construct reftest for the rests in the follow-up bugs.
https://reviewboard.mozilla.org/r/26343/#review25787

> I don't understand why you need this addition.  Could you explain?

I should remove this(and the next).

> Are CSS masks implemented differently enough at the rendering level that they don't need the UpdateEffects or UpdateOverflow hints?  (I think that seems likely, but I'd like to make sure.)

If the mask-image is a raster image/ gradient/ moz-element, then there is no need to have UpdateEffects and UpdateOverflow(I think). That says if we know the source of mask-image is not a SVG mask, then we can skip these two hints.

To figure out whether a mask-image is a non-svg-mask mask, we may search '#' in source-URI, if no matching, then we are sure that this mask-image does not refer to a SVG mask or SVG fragment, then we can skip these two hints. I filed another bug to tracing this issue(Bug 1235494)
https://reviewboard.mozilla.org/r/24373/#review25579

> Given what the caller has, these parameters should be "const nsStyleImageLayers&" instead of "const nsStyleImageLayers\*".
> 
> 
> Also, since this both adds and removes associations, I think the method should be called AddAndRemoveImageAssociations or something like that; the current name suggests that it only adds.

1. aOldLayers can be nullptr if aOldStyleContext is nullptr, so it has to be pointer type.
2. Will rename AssociateStyleImageWithFrame.

> I guess the goal of this is to keep the existing SVG mask code working?
> 
> But is it also to be part of allowing the new code to be pref-controlled?  It seems like it should be -- although I think all the pref stuff has disappeared from the patch, despite comment 100, comment 162, although I don't understand the response in comment 239 (why do we need both mMask and mMaskImage in the long term?).  If it is intended to allow landing of this new feature behind a pref (or perhaps even if not), I think the assignment to svgReset->mMask should also be conditional on the linked list being a single-item list.

yes, it's to keep exising svg mask working, and being removed in "Bug 686281 - Remove nsStyleSVGReset::mMask."

> This should really be called mMask, but you're better off fixing that when you get rid of the current mMask.  (That said, I'm not sure why you need to keep it.)
> 
> So please just add a FIXME for now, and file a followup bug blocking enabling the feature.

Since mMask is removed in patch - "Bug 686281 - Remove nsStyleSVGReset::mMask", I will do the renaming in that patch.
(In reply to C.J. Ku[:cjku] from comment #343)
> > This should really be called mMask, but you're better off fixing that when you get rid of the current mMask.  (That said, I'm not sure why you need to keep it.)
> > 
> > So please just add a FIXME for now, and file a followup bug blocking enabling the feature.
> 
> Since mMask is removed in patch - "Bug 686281 - Remove
> nsStyleSVGReset::mMask", I will do the renaming in that patch.

Please add an additional patch on top of the queue rather than adding additional changes to already-reviewed patches.
https://reviewboard.mozilla.org/r/24647/#review25745

> I asked last time for this to be CSS_TYPE_SHORTHAND_AND_LONGHAND.  Did that not work?

It's work with two changes in property_database.js and test_value_storage.html
1. https://dxr.mozilla.org/mozilla-central/source/layout/style/test/property_database.js#4858
   if (info.type == CSS_TYPE_TRUE_SHORTHAND ||
      (info.type == CSS_TYPE_SHORTHAND_AND_LONGHAND &&
       (property == "text-decoration" || property == "mask"))
   This change fix most of test failure after change mask.type to CSS_TYPE_SHORTHAND_AND_LONGHAND, "except"
2. https://dxr.mozilla.org/mozilla-central/source/layout/style/test/test_value_storage.html#225
   if (info.type != CSS_TYPE_TRUE_SHORTHAND && property != "mask")  << need to skip "mask" in parse+compute+serialize test.
   The reason of failure
   a. e.style.setProperty("mask", "repeat-x 100%"); (PS: The property value can be any mask longhand properties.)
   b. e.style.getProperty("mask") return "", empty string, because of the logic I modified in nsComputedDOMStyle::DoGetMask
   c. set the value we get in #b, which is "", into style again(e.style.setProperty("mask", ""))
   d. e.style.getProperty("mask") return "none".
   f. the retrn value of #b and #c is not the same, test case failed.
   
And I do have a question here. I notice if you try to get the value of background prop, you always get ""(since we do not define nsComputeDOMStyle::DoGetBackground?). But according to the spec https://drafts.csswg.org/cssom/#dom-cssstyledeclaration-getpropertyvalue, we should return longhand value list. Is that a bug?
(In reply to C.J. Ku[:cjku] from comment #345)
> https://reviewboard.mozilla.org/r/24647/#review25745
> 
> > I asked last time for this to be CSS_TYPE_SHORTHAND_AND_LONGHAND.  Did that not work?
> 
> It's work with two changes in property_database.js and
> test_value_storage.html
> 1.
> https://dxr.mozilla.org/mozilla-central/source/layout/style/test/
> property_database.js#4858
>    if (info.type == CSS_TYPE_TRUE_SHORTHAND ||
>       (info.type == CSS_TYPE_SHORTHAND_AND_LONGHAND &&
>        (property == "text-decoration" || property == "mask"))
>    This change fix most of test failure after change mask.type to
> CSS_TYPE_SHORTHAND_AND_LONGHAND, "except"
> 2.
> https://dxr.mozilla.org/mozilla-central/source/layout/style/test/
> test_value_storage.html#225
>    if (info.type != CSS_TYPE_TRUE_SHORTHAND && property != "mask")  << need
> to skip "mask" in parse+compute+serialize test.
>    The reason of failure
>    a. e.style.setProperty("mask", "repeat-x 100%"); (PS: The property value
> can be any mask longhand properties.)
>    b. e.style.getProperty("mask") return "", empty string, because of the
> logic I modified in nsComputedDOMStyle::DoGetMask
Should be gComputedStyle.getPropertyValue("mask")
>    c. set the value we get in #b, which is "", into style
> again(e.style.setProperty("mask", ""))
>    d. e.style.getProperty("mask") return "none".
Should be gComputedStyle.getPropertyValue("mask")
>    f. the retrn value of #b and #c is not the same, test case failed.
>    
> And I do have a question here. I notice if you try to get the value of
> background prop, you always get ""(since we do not define
> nsComputeDOMStyle::DoGetBackground?). But according to the spec
> https://drafts.csswg.org/cssom/#dom-cssstyledeclaration-getpropertyvalue, we
> should return longhand value list. Is that a bug?
https://reviewboard.mozilla.org/r/24373/#review25579

Just want to double confirm: "with a bug filed on fixing the pref-control"
Do you mean have another preference(don't use layout.css.mask.enabled) to enable/ disable all these mask properties, such as mask-clip/position etc.., added in these patches?
> And I do have a question here. I notice if you try to get the value of
> background prop, you always get ""(since we do not define
> nsComputeDOMStyle::DoGetBackground?). But according to the spec
> https://drafts.csswg.org/cssom/#dom-cssstyledeclaration-getpropertyvalue, we
> should return longhand value list. Is that a bug?
Had a discussion with heycom, bug 137699 and bug 58638 is relative to my question here.
.. Have no idea what happen here.
Single push create more 40 comments...
Since dbaron had r+ most patches. Except
1. Bug 686281 - mask-composite reftests.
2. ug 686281 - Rename nsStyleSVGReset::mLayers to nsStyleSVGReset::mMask - new one. Only renaming.
And I have no idea what happen on reviewboard, so I am going to switch back to manually attach patches on bugizlla, and ask review for these two patches.
Attachment #8683556 - Attachment is obsolete: true
Attachment #8683557 - Attachment is obsolete: true
Attachment #8683558 - Attachment is obsolete: true
Attachment #8683706 - Attachment is obsolete: true
Attachment #8684292 - Attachment is obsolete: true
Attachment #8684679 - Attachment is obsolete: true
Attachment #8684680 - Attachment is obsolete: true
Attachment #8684815 - Attachment is obsolete: true
Attachment #8685498 - Attachment is obsolete: true
Attachment #8685499 - Attachment is obsolete: true
Attachment #8692847 - Attachment is obsolete: true
Attachment #8692848 - Attachment is obsolete: true
Attachment #8684292 - Flags: review?(mstange)
Attachment #8684815 - Flags: review?(mstange)
Flags: needinfo?(mstange)
Attachment #8704027 - Flags: review+
Attachment #8704030 - Flags: review+
Attachment #8704033 - Flags: review+
Attachment #8704035 - Flags: review+
Attachment #8704036 - Flags: review+
Attachment #8704038 - Flags: review+
Attachment #8704041 - Flags: review+
Did you mean to request review for some of those?
Flags: needinfo?(cku)
(In reply to David Baron [:dbaron] ⌚️UTC-8 from comment #441)
> Did you mean to request review for some of those?

Yes, #12 and #13.
1. I had fix issues you filed from patch #1 ~ #12. And those patches are in "Fix it, then Ship it!" status.
2. I had moved mask-composite test case into w3c-css folder. Please help to review #12 again.
3. #13 is to rename nsStyleSVGReset::mLayers ot nsStyleSVGReset::mMask as you suggest.

And
4. In patch #8, I change type of mask from CSS_TYPE_TRUE_SHORTHAND to CSS_TYPE_SHORTHAND_AND_LONGHAND. According to this change, I need to change test_transitions_per_property.html and test_value_storage.html. Please see comment 345.
5. Please see my question in comment 347.
Flags: needinfo?(cku)
Attachment #8704043 - Flags: review?(dbaron)
Attachment #8704044 - Flags: review?(dbaron)
Depends on: 1242617
What bug contains the additional patch requested at the end of comment 330?
Flags: needinfo?(cku)
Comment on attachment 8704044 [details] [diff] [review]
0013-Bug-686281-Rename-nsStyleSVGReset-mLayers-to-nsStyle.patch

This is the additional patch requested in comment 344 and has not
been previously reviewed.

I didn't ask for you to rename nsStyleBackground::mLayers to mImage,
but I guess it's fine.  But you really shouldn't have added additional
changes to this bug that weren't requested -- this bug should get
finished and landed.

Please fix the commit message to reflect that you're also renaming
nsStyleBackground::mLayers to mImage.

r=dbaron with that
Attachment #8704044 - Flags: review?(dbaron) → review+
Comment on attachment 8704043 [details] [diff] [review]
0012-Bug-686281-mask-composite-reftests-r-dbaron.patch

The previous review of this patch was in comment 340.

The previous version of this patch was:
https://reviewboard.mozilla.org/r/26345/diff/5/raw/

mask-composite-1-ref.html and mask-composite-2-ref.html are missing one
of the 2 author lines that the other tests have (it needs the line for
author Mozilla)

Please file a bug on adding additional reftests for other features other
than mask-composite, and make it block the bug on enabling CSS masking
support.  (I'd asked for that in comment 340, but I guess you should get
this stuff landed.)

r=dbaron with that
Attachment #8704043 - Flags: review?(dbaron) → review+
(In reply to David Baron [:dbaron] ⌚️UTC+13 (busy, returning 8 February) from comment #443)
> What bug contains the additional patch requested at the end of comment 330?

Since I am not that clear on your request, so I asked a question in Comment 347:
> Just want to double confirm: "with a bug filed on fixing the pref-control"
> Do you mean have another preference(other than layout.css.mask.enabled) to enable/ disable all these mask > shorthand properties, such as mask-clip/position etc.., added in these patches?
Flags: needinfo?(cku) → needinfo?(dbaron)
The pref should enable all of this new work when it's set to true, but make us support only what we supported before this patch when set to false (with the possible exception of having a shorthand and one subproperty rather than just a single property).  Thus you can land with the pref false and not significantly change the behavior of Web content, and then once the necessary bugs are fixed, flip the pref to true.
Flags: needinfo?(dbaron)
Blocks: 1243675
Attachment #8704027 - Attachment is obsolete: true
Attachment #8704029 - Attachment is obsolete: true
Attachment #8704030 - Attachment is obsolete: true
Attachment #8704032 - Attachment is obsolete: true
Attachment #8704033 - Attachment is obsolete: true
Attachment #8704035 - Attachment is obsolete: true
Attachment #8704036 - Attachment is obsolete: true
Attachment #8704038 - Attachment is obsolete: true
Attachment #8704039 - Attachment is obsolete: true
Attachment #8704040 - Attachment is obsolete: true
Attachment #8704041 - Attachment is obsolete: true
Attachment #8704043 - Attachment is obsolete: true
Attachment #8704044 - Attachment is obsolete: true
Attachment #8713173 - Flags: review+
Attachment #8713176 - Flags: review+
Attachment #8713177 - Flags: review+
Address issues in comment 444 445/ rebase/ and checkin-needed
Keywords: checkin-needed
Reminder that this needs to happen:

(In reply to David Baron [:dbaron] ⌚️UTC+13 (busy, returning 8 February) from comment #330)
> r=dbaron with the above comments addressed, *and* with a bug filed on fixing
> the pref-control of these properties that you intend to fix in the *same
> cycle* as landing these patches.  In fact, it should probably be written and
> reviewed, on top of these patches, before these land, so that it can land at
> the same time.
> 
> It's NOT ACCEPTABLE to try fixing it in this bug because I'm simply not
> willing to deal with another round of review here.

and all of the followup bugs I requested need to be filed.
Flags: needinfo?(cku)
(In reply to David Baron [:dbaron] ⌚️UTC+13 (busy, returning 8 February) from comment #464)
> Reminder that this needs to happen:
> 
> (In reply to David Baron [:dbaron] ⌚️UTC+13 (busy, returning 8 February)
> from comment #330)
> > r=dbaron with the above comments addressed, *and* with a bug filed on fixing
> > the pref-control of these properties that you intend to fix in the *same
> > cycle* as landing these patches.  In fact, it should probably be written and
> > reviewed, on top of these patches, before these land, so that it can land at
> > the same time.
> > 
> > It's NOT ACCEPTABLE to try fixing it in this bug because I'm simply not
> > willing to deal with another round of review here.
> 
> and all of the followup bugs I requested need to be filed.

Yes, bug 1243734, which is the one that I am going to fix
Flags: needinfo?(cku)
(In reply to C.J. Ku[:cjku] from comment #40)
> 1. mask-mode: luminance mask is not supported yet.

This is bug 1228354.

> 2. mask-size & mask-position animation are not supported yet.

Is this done, or is there a followup bug on it?

> 3. svg-mask: mask properties, such as mask-repeat/origin/clip, are not apply
> to a mask-image if that image is a SVG#mask element.

Is this bug 1228280, or does a separate bug need to be filed?

(In reply to C.J. Ku[:cjku] from comment #341)
> 2. Except mask-mode and mask-composite, the rendering path of all the rest
> mask properties are the same with backbround properties. So I decided add
> mask-composite reftest first(reftest of mask-mode should be done in bug
> 1228354). I will construct reftest for the rests in the follow-up bugs.

Is this bug 1243675, or does a separate bug need to be filed?
Flags: needinfo?(cku)
(In reply to David Baron [:dbaron] ⌚️UTC+13 (busy, returning 8 February) from comment #466)
> (In reply to C.J. Ku[:cjku] from comment #40)
> > 1. mask-mode: luminance mask is not supported yet.
> 
> This is bug 1228354.
> 
> > 2. mask-size & mask-position animation are not supported yet.
> 
> Is this done, or is there a followup bug on it?
It's done in 0006 patch.
> > 3. svg-mask: mask properties, such as mask-repeat/origin/clip, are not apply
> > to a mask-image if that image is a SVG#mask element.
> 
> Is this bug 1228280, or does a separate bug need to be filed?
Yes, it should be bug 1228280 
> (In reply to C.J. Ku[:cjku] from comment #341)
> > 2. Except mask-mode and mask-composite, the rendering path of all the rest
> > mask properties are the same with backbround properties. So I decided add
> > mask-composite reftest first(reftest of mask-mode should be done in bug
> > 1228354). I will construct reftest for the rests in the follow-up bugs.
> 
> Is this bug 1243675, or does a separate bug need to be filed?
Yes, it's bug 1243675
Flags: needinfo?(cku)
When importing the tests to the CSS test suite, I get:

remote: vendor-imports/mozilla/mozilla-central-reftests/masking/blue-100x50-transparent-100X50.png status changed to 'Submitted for Review'
remote: vendor-imports/mozilla/mozilla-central-reftests/masking/blue-100x50-transparent-100X50.svg status changed to 'Needs Work' due to error:
remote: Missing <g id='testmeta'> element Title not specified. Not linked to a specification. No author specified.
remote: vendor-imports/mozilla/mozilla-central-reftests/masking/mask-composite-1-ref.html status changed to 'Submitted for Review'
remote: vendor-imports/mozilla/mozilla-central-reftests/masking/mask-composite-1a.html status changed to 'Submitted for Review'
remote: vendor-imports/mozilla/mozilla-central-reftests/masking/mask-composite-1b.html status changed to 'Submitted for Review'
remote: vendor-imports/mozilla/mozilla-central-reftests/masking/mask-composite-2-ref.html status changed to 'Submitted for Review'
remote: vendor-imports/mozilla/mozilla-central-reftests/masking/mask-composite-2a.html status changed to 'Submitted for Review'
remote: vendor-imports/mozilla/mozilla-central-reftests/masking/mask-composite-2b.html status changed to 'Submitted for Review'
remote: vendor-imports/mozilla/mozilla-central-reftests/masking/reftest.list status changed to 'Submitted for Review'
remote: vendor-imports/mozilla/mozilla-central-reftests/masking/transparent-100x50-blue-100X50.png status changed to 'Submitted for Review'
remote: vendor-imports/mozilla/mozilla-central-reftests/masking/transparent-100x50-blue-100X50.svg status changed to 'Needs Work' due to error:
remote: Missing <g id='testmeta'> element Title not specified. Not linked to a specification. No author specified.


The images (svg and png) should probably be moved to a support/ subdirectory to avoid these errors.
Flags: needinfo?(cku)
(In reply to David Baron [:dbaron] ⌚️UTC+11 (busy, returning 8 February) from comment #162)
> > +  if (mLayers.mClipCount != aOther.mLayers.mClipCount ||
> > +      mLayers.mOriginCount != aOther.mLayers.mOriginCount ||
> > +      mLayers.mRepeatCount != aOther.mLayers.mRepeatCount ||
> > +      mLayers.mPositionCount != aOther.mLayers.mPositionCount ||
> > +      mLayers.mSizeCount != aOther.mLayers.mSizeCount||
> > +      mLayers.mMaskModeCount != aOther.mLayers.mMaskModeCount||
> > +      mLayers.mCompositeCount != aOther.mLayers.mCompositeCount) {
> > +    NS_UpdateHint(hint, nsChangeHint_NeutralChange);
> > +  }
> 
> Please make this share code with nsStyleBackground, probably with a
> CalcDifference method on nsStyleImageLayers.  Note that while you're there,
> you should fix the fact that there's an mBlendModeCount check missing.

Note that you ended up losing the mAttachmentCount check in nsStyleBackground when you did this.  I missed this in the later patch.  This needs to be fixed.
(In reply to David Baron [:dbaron] ⌚️UTC+11 (busy, returning 8 February) from comment #469)
> When importing the tests to the CSS test suite, I get:
> 
> remote:
> vendor-imports/mozilla/mozilla-central-reftests/masking/blue-100x50-
> transparent-100X50.png status changed to 'Submitted for Review'
> remote:
> vendor-imports/mozilla/mozilla-central-reftests/masking/blue-100x50-
> transparent-100X50.svg status changed to 'Needs Work' due to error:
> remote: Missing <g id='testmeta'> element Title not specified. Not linked to
> a specification. No author specified.
> remote:
> vendor-imports/mozilla/mozilla-central-reftests/masking/mask-composite-1-ref.
> html status changed to 'Submitted for Review'
> remote:
> vendor-imports/mozilla/mozilla-central-reftests/masking/mask-composite-1a.
> html status changed to 'Submitted for Review'
> remote:
> vendor-imports/mozilla/mozilla-central-reftests/masking/mask-composite-1b.
> html status changed to 'Submitted for Review'
> remote:
> vendor-imports/mozilla/mozilla-central-reftests/masking/mask-composite-2-ref.
> html status changed to 'Submitted for Review'
> remote:
> vendor-imports/mozilla/mozilla-central-reftests/masking/mask-composite-2a.
> html status changed to 'Submitted for Review'
> remote:
> vendor-imports/mozilla/mozilla-central-reftests/masking/mask-composite-2b.
> html status changed to 'Submitted for Review'
> remote: vendor-imports/mozilla/mozilla-central-reftests/masking/reftest.list
> status changed to 'Submitted for Review'
> remote:
> vendor-imports/mozilla/mozilla-central-reftests/masking/transparent-100x50-
> blue-100X50.png status changed to 'Submitted for Review'
> remote:
> vendor-imports/mozilla/mozilla-central-reftests/masking/transparent-100x50-
> blue-100X50.svg status changed to 'Needs Work' due to error:
> remote: Missing <g id='testmeta'> element Title not specified. Not linked to
> a specification. No author specified.
> 
> 
> The images (svg and png) should probably be moved to a support/ subdirectory
> to avoid these errors.

Change made in Bug 1244598.
Flags: needinfo?(cku)
(In reply to David Baron [:dbaron] ⌚️UTC+11 (busy, returning 8 February) from comment #470)
> Note that you ended up losing the mAttachmentCount check in
> nsStyleBackground when you did this.  I missed this in the later patch. 
> This needs to be fixed.
Filed bug 1244628 to fix it.
Whiteboard: p=0 → p=0, [DocArea=CSS]
Depends on: 1247560
Depends on: 1245499
Depends on: 1251414
that one’s fixed, so what’s the progress?
The mask-* longhands were put behind a compiler flag (see bug 1243734).
Those will ship with bug 1251161. And bug 1224422 is the overall tracking bug.

Sebastian
These are documented.
You need to log in before you can comment on or make changes to this bug.