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

Status: RESOLVED FIXED

(Core :: Layout, enhancement)

Description

[Alexei]

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/

Comment 1

[Boris Zbarsky [:bzbarsky]]

We already support this, using SVG masks, no?

Comment 2

[Paul Rouget [:paul]]

Two different approaches. SVG is probably more powerful, but using an image is easier.

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Supporting the image syntax as well as SVG makes sense.

Comment 4

[Alexei]

Да лучше бы реализовали CSS Content, а то JNG приходится чинить за счет места в src. А из-за этого сохранить JNG нельзя!

Comment 5

[bop]

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).

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

SVG masks already let you use text as a mask.

Comment 7

[ishmael.ahmed]

Han any on had experience in animating or using transition on a svg mask?

Comment 8

[Greg Houston]

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.

Comment 9

[Sergey Sokoloff]

See also the related W3C Working Draft at http://www.w3.org/TR/css-masking/

Comment 10

[Mike de Boer [:mikedeboer]]

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!

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

We will do this since there's a spec for it now, but we should also accelerate rendering of SVG masks.

Comment 12

[Mike de Boer [:mikedeboer]]

(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.

Comment 13

[Florian Bender]

The spec (http://www.w3.org/TR/css-masking/) is in last call, the editor's version has been updated since then, though.

Comment 14

[adrian]

Is mask-size in the spec and proposition?

Comment 15

[Dirk Schulze]

(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/

Comment 16

[Jet Villegas (inactive)]

Picking this up until I can find a proper assignee.

Comment 17

[Sebastian Zartner [:sebo]]

This issue should be blocking bug 913153.

Sebastian

Comment 18

[eric]

I am finding the lack of mask-size, mask-type, mask-repeat, mask-position, and mask-origin to be very problematic.

Although I quite prefer SVG masks, there are numerous instances where bitmap masks are necessary.

Comment 19

[u459114]

1. Clip
clip-path basic-shape is not supported
2. Mask
mask-image=<image> is not support. (example, http://tympanus.net/codrops/css_reference/mask-image/)
mask-box-image is not supported. (example, http://www.html5rocks.com/en/tutorials/masking/adobe/, search for "These parts are now used to mask the corner and edges of the content and result in the following view").

Comment 20

[u459114]

Attachment: WIP - mask-image + mask-repeat

Comment 21

[u459114]

Attachment: WIP - mask image longhand properties

Comment 22

[u459114]

Attachment: WIP - mask-image/repeat/mode/position/clip/origin/size/composite

Comment 23

[u459114]

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.

Comment 24

[u459114]

Attachment: WIP - mask css parsing + rendering 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.

Comment 25

[u459114]

Attachment: WIP - no mask shorthand and mask-composite

Comment 26

[u459114]

Attachment: WIP 5

Comment 27

[u459114]

Attachment: WIP 6: mask image - image list/repeat/composite/repeat/position/clip/size/origin

TBD:
1. Most requirements are implemented, except mask-mode(luminance) and short-hand.
2. Change PaintBackground semantic to PaintCSSLayer.

Comment 28

[u459114]

Attachment: WIP 7: mask image - image list/repeat/composite/repeat/position/clip/size/origin

Rebase

Comment 29

[u459114]

Attachment: WIP 8 - create nsStyleMask structure

Comment 30

[u459114]

Attachment: WIP 9 - merge mask & background properties into layer property

Comment 31

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 32

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 33

[u459114]

(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

Comment 34

[u459114]

(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.

Comment 35

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(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.

Comment 36

[u459114]

Attachment: 1. Mask CSS property

Comment 37

[u459114]

Attachment: 2. Mask Style

Comment 38

[u459114]

Attachment: 3. Mask Rendering

Comment 39

[u459114]

Attachment: Git hub working branch

Comment 40

[u459114]

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?

Comment 41

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(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...

Comment 42

[u459114]

Attachment: MozReview Request: Bug 686281 - Implement nsStyleImageLayers; r=dbaron

Bug 686281 - CSS Mask prop.

Comment 43

[u459114]

Attachment: MozReview Request: Bug 686281 - Rename *background* to *imagelayer*; r=dbaron.

Bug 686281 - CSS Mask Style

Comment 44

[u459114]

Attachment: MozReview Request: Bug 686281 - Implement CSS mask style; r=dbaron.

Bug 686281 - CSS Mask rendering.

Comment 45

[u459114]

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 46

[u459114]

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 47

[u459114]

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/

Comment 48

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Did you intend to request review?

Comment 49

[u459114]

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 50

[u459114]

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 51

[u459114]

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/

Comment 52

[u459114]

Attachment: MozReview Request: Bug 686281 - Mask CSS parsing and Mask DOM API. r=dbaron

Bug 686281 - CSS Mask animation.

Comment 53

[u459114]

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/

Comment 54

[u459114]

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/

Comment 55

[u459114]

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/

Comment 56

[u459114]

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/

Comment 57

[u459114]

(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.

Comment 58

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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?

Comment 59

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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 60

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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 61

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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 62

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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?

Comment 63

[u459114]

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

Done

Comment 64

[u459114]

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/

Comment 65

[u459114]

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/

Comment 66

[u459114]

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/

Comment 67

[u459114]

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/

Comment 68

[u459114]

Attachment: MozReview Request: Bug 686281 - Mask CSS rendering; r=mstange

Bug 686281- Implement CSS mask animation. r=:dbaron

Comment 69

[u459114]

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 70

[u459114]

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 71

[u459114]

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 72

[u459114]

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 73

[u459114]

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/

Comment 74

[u459114]

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 75

[u459114]

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 76

[u459114]

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/

Comment 77

[u459114]

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 78

[u459114]

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 79

[u459114]

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/

Comment 80

[u459114]

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

Comment 81

[u459114]

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

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

Fix this TODO

Comment 82

[u459114]

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 83

[u459114]

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 84

[u459114]

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 85

[u459114]

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 86

[u459114]

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/

Comment 87

[u459114]

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.

Comment 88

[u459114]

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 89

[u459114]

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/

Comment 90

[u459114]

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/

Comment 91

[u459114]

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/

Comment 92

[u459114]

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/

Comment 93

[u459114]

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/

Comment 94

[u459114]

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 95

[u459114]

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 96

[u459114]

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 97

[u459114]

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 98

[u459114]

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/

Comment 99

[u459114]

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

Comment 100

[u459114]

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 101

[u459114]

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 102

[u459114]

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 103

[u459114]

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 104

[u459114]

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 105

[u459114]

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/

Comment 106

[u459114]

Attachment: MozReview Request: Bug 686281 - Mask CSS animation; r=dbaron.

Bug 686281 - Implement CSS mask alias. r?:dbaron

Comment 107

[u459114]

Attachment: MozReview Request: Bug 686281 - Mask CSS webkit-alias; r=dbaron.

Bug 686281 - Implement CSS mask test. (WIP)

Comment 108

[u459114]

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 109

[u459114]

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 110

[u459114]

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 111

[u459114]

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 112

[u459114]

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 113

[u459114]

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 114

[u459114]

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 115

[u459114]

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 116

[u459114]

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 117

[u459114]

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 118

[u459114]

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 119

[u459114]

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 120

[u459114]

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 121

[u459114]

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 122

[u459114]

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/

Comment 123

[u459114]

Attachment: MozReview Request: Bug 686281 - Mask mochitest; r=dbaron.

Bug 686281 - (Bug Fix: will-change-stacking-context-mask-1.html fail) Expands shorthad to longhand. r?dbaron

Comment 124

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 125

[u459114]

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

Comment 126

[u459114]

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.

Comment 127

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(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).

Comment 128

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 129

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 130

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(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 131

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 132

[u459114]

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/

Comment 133

[u459114]

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/

Comment 134

[u459114]

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/

Comment 135

[u459114]

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/

Comment 136

[u459114]

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/

Comment 137

[u459114]

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/

Comment 138

[u459114]

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/

Comment 139

[u459114]

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/

Comment 140

[u459114]

Attachment: MozReview Request: Bug 686281 - Expands will-change of a shorthand prop to longhand ones; r=dbaron.

Bug 686281 - (Bug Fix: will-change-stacking-context-mask-1.html fail) Expands shorthand prop to longhands. r?dbaron

Comment 141

[u459114]

Attachment: MozReview Request: Bug 686281 - A static assertion to keep value correctness of NS_RULE_NODE_IS_ANIMATION_RULE; r=dbaron.

Bug 686281 - A static assertion to keep the rightness of NS_RULE_NODE_IS_ANIMATION_RULE. r=:dbaron.

Comment 142

[u459114]

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 143

[u459114]

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 144

[u459114]

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 145

[u459114]

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 146

[u459114]

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 147

[u459114]

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 148

[u459114]

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 149

[u459114]

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 150

[u459114]

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 151

[u459114]

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 152

[u459114]

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 153

[u459114]

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 154

[u459114]

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/

Comment 155

[u459114]

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 156

[u459114]

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/

Comment 157

[u459114]

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 158

[u459114]

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 159

[u459114]

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 160

[u459114]

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 161

[u459114]

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 162

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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).

Comment 163

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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".

Comment 164

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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)

Comment 165

[Xidorn Quan [:xidorn] UTC+10]

(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 166

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 167

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 168

[u459114]

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/

Comment 169

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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[].

Comment 170

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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".

Comment 171

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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).

Comment 172

[u459114]

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 173

[u459114]

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/

Comment 174

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 175

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 176

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 177

[u459114]

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/

Comment 178

[u459114]

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 179

[u459114]

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/

Comment 180

[u459114]

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/

Comment 181

[u459114]

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/

Comment 182

[u459114]

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/

Comment 183

[u459114]

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 184

[u459114]

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/

Comment 185

[u459114]

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 186

[u459114]

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/

Comment 187

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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!)

Comment 188

[u459114]

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?

Comment 189

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(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.

Comment 190

[u459114]

(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)

Comment 191

[u459114]

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/

Comment 192

[u459114]

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/

Comment 193

[u459114]

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 194

[u459114]

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/

Comment 195

[u459114]

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 196

[u459114]

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/

Comment 197

[u459114]

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/

Comment 198

[u459114]

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/

Comment 199

[u459114]

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 200

[u459114]

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/

Comment 201

[u459114]

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 202

[u459114]

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/

Comment 203

[u459114]

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 204

[u459114]

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/

Comment 205

[u459114]

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 206

[u459114]

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 207

[u459114]

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 208

[u459114]

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/

Comment 209

[u459114]

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/

Comment 210

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Also, per bug 1224141, please stop using the ":" in the reviewer syntax in the commit messages.

Comment 211

[u459114]

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 212

[u459114]

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/

Comment 213

[u459114]

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 214

[u459114]

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/

Comment 215

[u459114]

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 216

[u459114]

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 217

[u459114]

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 218

[u459114]

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 219

[u459114]

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/

Comment 220

[u459114]

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/

Comment 221

[u459114]

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/

Comment 222

[u459114]

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

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

This assertion is totally wrong. Remvoe it.

Comment 223

[u459114]

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?

Comment 224

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(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 225

[u459114]

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 226

[u459114]

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/

Comment 227

[u459114]

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 228

[u459114]

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 229

[u459114]

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/

Comment 230

[u459114]

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/

Comment 231

[u459114]

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 232

[u459114]

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/

Comment 233

[u459114]

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/

Comment 234

[u459114]

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 235

[u459114]

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/

Comment 236

[u459114]

(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?

Comment 237

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.)

Comment 238

[u459114]

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

Comment 239

[u459114]

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)

Comment 240

[u459114]

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/

Comment 241

[u459114]

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 242

[u459114]

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/

Comment 243

[u459114]

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 244

[u459114]

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/

Comment 245

[u459114]

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/

Comment 246

[u459114]

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 247

[u459114]

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/

Comment 248

[u459114]

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/

Comment 249

[u459114]

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/

Comment 250

[u459114]

Attachment: MozReview Request: Bug 686281 - Remove nsStyleSVGReset::mMask; r=dbaron

Bug 686281 - Remove nsStyleSVGReset::mMask; r?dbaron

Comment 251

[u459114]

Attachment: MozReview Request: Bug 686281 - mask-composite reftests; r?dbaron

Bug 686281 - Add mask property into property_database.js; r?dbaron.

Comment 252

[u459114]

Attachment: MozReview Request: Bug 686281 - Expands will-change of a shorthand prop to longhand ones; r=dbaron.

Bug 686281 - Expands will-change of a shorthand prop to longhand ones; r=dbaron.

Comment 253

[u459114]

Attachment: MozReview Request: Bug 686281 - A static assertion to keep value correctness of NS_RULE_NODE_IS_ANIMATION_RULE; r=dbaron.

Bug 686281 - A static assertion to keep value correctness of NS_RULE_NODE_IS_ANIMATION_RULE; r=dbaron.

Comment 254

[u459114]

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).

Comment 255

[Markus Stange [: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 256

[u459114]

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 257

[u459114]

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 258

[u459114]

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/

Comment 259

[u459114]

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 260

[u459114]

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 261

[u459114]

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 262

[u459114]

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/

Comment 263

[u459114]

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 264

[u459114]

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/

Comment 265

[u459114]

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 266

[u459114]

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 267

[u459114]

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 268

[u459114]

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 269

[u459114]

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 270

[u459114]

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/

Comment 271

[u459114]

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 272

[Markus Stange [:mstange]]

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;

Comment 273

[u459114]

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 274

[u459114]

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 275

[u459114]

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 276

[u459114]

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 277

[u459114]

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 278

[u459114]

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 279

[u459114]

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 280

[u459114]

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/

Comment 281

[u459114]

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 282

[u459114]

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 283

[u459114]

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 284

[u459114]

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 285

[u459114]

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 286

[u459114]

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/

Comment 287

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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 288

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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).

Comment 289

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 290

[u459114]

(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.

Comment 291

[u459114]

Here is a draft
https://reviewboard.mozilla.org/r/24369/
Please check whether it's acceptable for reviewing

Comment 292

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(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.

Comment 293

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.)

Comment 294

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 295

[u459114]

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.

Comment 296

[u459114]

(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)

Comment 297

[Markus Stange [:mstange]]

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.

Comment 298

[u459114]

(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.

Comment 311

[u459114]

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.

Comment 312

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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 313

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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)

Comment 326

[u459114]

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.

Comment 327

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 328

[u459114]

(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.

Comment 329

[u459114]

(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 330

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 331

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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 332

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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 333

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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).

Comment 334

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8684680 [details]
MozReview Request: Bug 686281 - Mask CSS webkit-alias; r=dbaron.

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

Previous comments on this patch are in comment 167.

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

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

Again, easy to review by comparing.

Comment 335

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 336

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 337

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 338

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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 339

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 340

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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?

Comment 341

[u459114]

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.

Ｗill 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.

Comment 342

[u459114]

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)

Comment 343

[u459114]

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.

Comment 344

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(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.

Comment 345

[u459114]

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?

Comment 346

[u459114]

(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?

Comment 347

[u459114]

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?

Comment 348

[u459114]

> 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.

Comment 415

[u459114]

.. Have no idea what happen here.
Single push create more 40 comments...

Comment 427

[u459114]

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.

Comment 428

[u459114]

Attachment: 0001-Bug-686281-Implement-nsStyleImageLayers-r-dbaron.patch

Comment 429

[u459114]

Attachment: 0002-Bug-686281-Rename-background-to-imagelayer-r-dbaron.patch

Comment 430

[u459114]

Attachment: 0003-Bug-686281-Implement-CSS-mask-style-r-dbaron.patch

Comment 431

[u459114]

Attachment: 0004-Bug-686281-Mask-CSS-parsing-and-Mask-DOM-API.-r-dbar.patch

Comment 432

[u459114]

Attachment: 0005-Bug-686281-Mask-CSS-rendering-r-mstange.patch

Comment 433

[u459114]

Attachment: 0006-Bug-686281-Mask-CSS-animation-r-dbaron.patch

Comment 434

[u459114]

Attachment: 0007-Bug-686281-Mask-CSS-webkit-alias-r-dbaron.patch

Comment 435

[u459114]

Attachment: 0008-Bug-686281-Mask-mochitest-r-dbaron.patch

Comment 436

[u459114]

Attachment: 0009-Bug-686281-Expands-will-change-of-a-shorthand-prop-t.patch

Comment 437

[u459114]

Attachment: 0010-Bug-686281-A-static-assertion-to-keep-value-correctn.patch

Comment 438

[u459114]

Attachment: 0011-Bug-686281-Remove-nsStyleSVGReset-mMask-r-dbaron.patch

Comment 439

[u459114]

Attachment: 0012-Bug-686281-mask-composite-reftests-r-dbaron.patch

Comment 440

[u459114]

Attachment: 0013-Bug-686281-Rename-nsStyleSVGReset-mLayers-to-nsStyle.patch

Comment 441

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Did you mean to request review for some of those?

Comment 442

[u459114]

(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.

Comment 443

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

What bug contains the additional patch requested at the end of comment 330?

Comment 444

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 445

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 446

[u459114]

(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?

Comment 447

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 448

[u459114]

Try result https://treeherder.mozilla.org/#/jobs?repo=try&revision=a823a87fe614&selectedJob=16038238

Comment 449

[u459114]

Attachment: 0001-Bug-686281-Implement-nsStyleImageLayers-r-dbaron.patch

Comment 450

[u459114]

Attachment: 0002-Bug-686281-Rename-background-to-imagelayer-r-dbaron.patch

Comment 451

[u459114]

Attachment: 0003-Bug-686281-Implement-CSS-mask-style-r-dbaron.patch

Comment 452

[u459114]

Attachment: 0004-Bug-686281-Mask-CSS-parsing-and-Mask-DOM-API.-r-dbar.patch

Comment 453

[u459114]

Attachment: 0005-Bug-686281-Mask-CSS-rendering-r-mstange.patch

Comment 454

[u459114]

Attachment: 0006-Bug-686281-Mask-CSS-animation-r-dbaron.patch

Comment 455

[u459114]

Attachment: 0007-Bug-686281-Mask-CSS-webkit-alias-r-dbaron.patch

Comment 456

[u459114]

Attachment: 0008-Bug-686281-Mask-mochitest-r-dbaron.patch

Comment 457

[u459114]

Attachment: 0009-Bug-686281-Expands-will-change-of-a-shorthand-prop-t.patch

Comment 458

[u459114]

Attachment: 0010-Bug-686281-A-static-assertion-to-keep-value-correctn.patch

Comment 459

[u459114]

Attachment: 0011-Bug-686281-Remove-nsStyleSVGReset-mMask-r-dbaron.patch

Comment 460

[u459114]

Attachment: 0012-Bug-686281-mask-composite-reftests-r-dbaron.patch

Comment 461

[u459114]

Attachment: 0013-Bug-686281-Rename-nsStyleSVGReset-mLayers-to-nsStyle.patch

Comment 462

[u459114]

Address issues in comment 444 445/ rebase/ and checkin-needed

Comment 463

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/61b2c6e53923
https://hg.mozilla.org/integration/mozilla-inbound/rev/6debcbde2776
https://hg.mozilla.org/integration/mozilla-inbound/rev/6eceaeb0ec70
https://hg.mozilla.org/integration/mozilla-inbound/rev/918df1f41e40
https://hg.mozilla.org/integration/mozilla-inbound/rev/f6f05e41a0ab
https://hg.mozilla.org/integration/mozilla-inbound/rev/a5feb879a4f1
https://hg.mozilla.org/integration/mozilla-inbound/rev/f6eba571ce59
https://hg.mozilla.org/integration/mozilla-inbound/rev/280c5933d1ce
https://hg.mozilla.org/integration/mozilla-inbound/rev/ac79326c8b74
https://hg.mozilla.org/integration/mozilla-inbound/rev/337f7b90e385
https://hg.mozilla.org/integration/mozilla-inbound/rev/42e90ed0f40e
https://hg.mozilla.org/integration/mozilla-inbound/rev/4bad4727057f
https://hg.mozilla.org/integration/mozilla-inbound/rev/45bcd98357bd

Comment 464

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 465

[u459114]

(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

Comment 466

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(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?

Comment 467

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/61b2c6e53923
https://hg.mozilla.org/mozilla-central/rev/6debcbde2776
https://hg.mozilla.org/mozilla-central/rev/6eceaeb0ec70
https://hg.mozilla.org/mozilla-central/rev/918df1f41e40
https://hg.mozilla.org/mozilla-central/rev/f6f05e41a0ab
https://hg.mozilla.org/mozilla-central/rev/a5feb879a4f1
https://hg.mozilla.org/mozilla-central/rev/f6eba571ce59
https://hg.mozilla.org/mozilla-central/rev/280c5933d1ce
https://hg.mozilla.org/mozilla-central/rev/ac79326c8b74
https://hg.mozilla.org/mozilla-central/rev/337f7b90e385
https://hg.mozilla.org/mozilla-central/rev/42e90ed0f40e
https://hg.mozilla.org/mozilla-central/rev/4bad4727057f
https://hg.mozilla.org/mozilla-central/rev/45bcd98357bd

Comment 468

[u459114]

(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

Comment 469

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 470

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(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.

Comment 471

[u459114]

(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.

Comment 472

[u459114]

(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.

Comment 473

[flying sheep]

that one’s fixed, so what’s the progress?

Comment 474

[Sebastian Zartner [:sebo]]

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

Comment 475

[Chris Mills [:cmills]]

These are documented.