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

RESOLVED FIXED in Firefox 47

Status

()

Core
Layout
--
enhancement
RESOLVED FIXED
7 years ago
10 months ago

People

(Reporter: Alexei, Assigned: u459114)

Tracking

(Depends on: 1 bug, Blocks: 3 bugs, {dev-doc-complete})

Trunk
mozilla47
dev-doc-complete
Points:
---
Dependency tree / graph
Bug Flags:
firefox-backlog +

Firefox Tracking Flags

(firefox47 fixed)

Details

(Whiteboard: p=0, [DocArea=CSS], URL)

Attachments

(13 attachments, 41 obsolete attachments)

84.13 KB, patch
u459114
: review+
Details | Diff | Splinter Review
81.44 KB, patch
u459114
: review+
Details | Diff | Splinter Review
30.28 KB, patch
u459114
: review+
Details | Diff | Splinter Review
58.44 KB, patch
u459114
: review+
Details | Diff | Splinter Review
14.89 KB, patch
u459114
: review+
Details | Diff | Splinter Review
9.95 KB, patch
u459114
: review+
Details | Diff | Splinter Review
1.72 KB, patch
u459114
: review+
Details | Diff | Splinter Review
15.37 KB, patch
u459114
: review+
Details | Diff | Splinter Review
2.56 KB, patch
u459114
: review+
Details | Diff | Splinter Review
864 bytes, patch
u459114
: review+
Details | Diff | Splinter Review
10.72 KB, patch
u459114
: review+
Details | Diff | Splinter Review
14.25 KB, patch
u459114
: review+
Details | Diff | Splinter Review
42.45 KB, patch
u459114
: review+
Details | Diff | Splinter Review
(Reporter)

Description

7 years ago
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/
(Reporter)

Updated

7 years ago
Version: 7 Branch → unspecified
(Reporter)

Updated

7 years ago
Severity: normal → enhancement
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Windows 7 → All
Hardware: x86 → All
Version: unspecified → Trunk
Summary: CSS Mask Image (-moz-mask) proporse → Implement CSS Mask Image (-moz-mask) proposal

Updated

7 years ago
Component: General → Layout
Product: Firefox → Core
QA Contact: general → layout
We already support this, using SVG masks, no?

Comment 2

7 years ago
Two different approaches. SVG is probably more powerful, but using an image is easier.
Supporting the image syntax as well as SVG makes sense.
(Reporter)

Comment 4

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

Comment 5

6 years ago
Will mask-image accept the -moz-element() image type? I'd love to use text as a mask for an image (=text with a pattern).
SVG masks already let you use text as a mask.

Comment 7

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

Comment 8

6 years ago
I was first hoping I would be able to use a CSS gradient for a mask. When I found I couldn't do that I was then hoping I could use an image, preferably a data uri I could embed right into the CSS, but alas it currently requires an external SVG file, a file type I would rather not have to deal with.
See also the related W3C Working Draft at http://www.w3.org/TR/css-masking/
I just voted for this bug, so let me explain:
SVG masks are theoretically more powerful and versatile. It's only when you use it in practice that you notice its main issue: performance. When you try to use it for browser-chrome (applied to XUL), you notice that hardware-accelerated rendering is problematic and slow.
I have no benchmarks backing this statement at the moment, but I believe when you ask the UX or fx-desktop team members, you'll get a confirmation nonetheless.

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

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

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

Epic, on both counts! I guess this won't just happen overnight, but good to hear nonetheless.

Updated

5 years ago
Assignee: nobody → ncameron
Blocks: 877294
Depends on: 700926
Whiteboard: [Australis:P?][Australis:M?]
Whiteboard: [Australis:P?][Australis:M?]
Keywords: dev-doc-needed
Summary: Implement CSS Mask Image (-moz-mask) proposal → Implement CSS Mask Image properties (mask, mask-image, etc.)

Comment 13

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

Updated

4 years ago
Blocks: 950073
Whiteboard: p=0

Updated

4 years ago
No longer blocks: 950073
Flags: firefox-backlog+

Comment 14

4 years ago
Is mask-size in the spec and proposition?

Comment 15

4 years ago
(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

4 years ago
Picking this up until I can find a proper assignee.
Assignee: ncameron → bugs
This issue should be blocking bug 913153.

Sebastian
Comment hidden (me-too)
Comment hidden (me-too)
(Assignee)

Updated

3 years ago
Assignee: bugs → cku
(Assignee)

Comment 20

3 years ago
Created attachment 8667692 [details] [diff] [review]
WIP - mask-image + mask-repeat
(Assignee)

Comment 21

3 years ago
Created attachment 8668867 [details] [diff] [review]
WIP - mask image longhand properties
Attachment #8667692 - Attachment is obsolete: true
(Assignee)

Comment 22

3 years ago
Created attachment 8669592 [details] [diff] [review]
WIP - mask-image/repeat/mode/position/clip/origin/size/composite
Attachment #8668867 - Attachment is obsolete: true
(Assignee)

Comment 23

3 years ago
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.
(Assignee)

Comment 24

3 years ago
Created attachment 8671293 [details] [diff] [review]
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.
Attachment #8669592 - Attachment is obsolete: true
(Assignee)

Comment 25

3 years ago
Created attachment 8674131 [details] [diff] [review]
WIP - no mask shorthand and mask-composite
Attachment #8671293 - Attachment is obsolete: true
(Assignee)

Comment 26

3 years ago
Created attachment 8674796 [details] [diff] [review]
WIP 5
Attachment #8674131 - Attachment is obsolete: true
(Assignee)

Comment 27

3 years ago
Created attachment 8677956 [details] [diff] [review]
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.
Attachment #8674796 - Attachment is obsolete: true
(Assignee)

Comment 28

3 years ago
Created attachment 8678121 [details] [diff] [review]
WIP 7: mask image - image list/repeat/composite/repeat/position/clip/size/origin

Rebase
Attachment #8677956 - Attachment is obsolete: true
(Assignee)

Comment 29

3 years ago
Created attachment 8681281 [details] [diff] [review]
WIP 8 - create nsStyleMask structure
Attachment #8678121 - Attachment is obsolete: true
(Assignee)

Comment 30

3 years ago
Created attachment 8681973 [details] [diff] [review]
WIP 9 - merge mask & background properties into layer property
Attachment #8681281 - Attachment is obsolete: true
Comment on attachment 8681973 [details] [diff] [review]
WIP 9 - merge mask & background properties into layer property

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

Also I'm not inclined to think we want a separate style struct.  That would perhaps be desirable for if the mask properties were widely used, but I don't see them being as widely used as the background properties.  (It is sometimes nice for rule tree optimizations if a single shorthand sets all of the properties in a struct, although it's less important for non-inherited properties like these.)  I'd probably suggest just adding to nsStyleSVGReset for now.
It's also going to be a *lot* easier to review this if it is done as a stack of patches rather than a single patch.
(Assignee)

Comment 33

3 years ago
(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
(Assignee)

Comment 34

3 years ago
(In reply to David Baron [:dbaron] ⌚UTC+8 from comment #32)
> It's also going to be a *lot* easier to review this if it is done as a stack
> of patches rather than a single patch.

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

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

But given how many properties are common, having a single class (without a hierarchy) seems fine, as long as there are comments explaining which members are/aren't used in which situations.
(Assignee)

Comment 36

3 years ago
Created attachment 8682995 [details] [diff] [review]
1. Mask CSS property
Attachment #8681973 - Attachment is obsolete: true
(Assignee)

Comment 37

3 years ago
Created attachment 8682996 [details] [diff] [review]
2. Mask Style
(Assignee)

Comment 38

3 years ago
Created attachment 8682999 [details] [diff] [review]
3. Mask Rendering
(Assignee)

Comment 39

3 years ago
Created attachment 8683181 [details]
Git hub working branch
(Assignee)

Comment 40

3 years ago
Things not ready
1. mask-mode: luminance mask is not supported yet.
2. mask-size & mask-position animation are not supported yet.
3. svg-mask: mask properties, such as mask-repeat/origin/clip, are not apply to a mask-image if that image is a SVG#mask element.

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

It's nearly always fine to separate work like that into followup bugs when the plan is to ship the feature after the followup bugs land (that is, until they're done, have it behind a pref that's either disabled or enabled only for nightly and aurora).  If what you're asking is whether it's ok to ship the feature without those, then I'd have to actually think about it...
(Assignee)

Comment 42

3 years ago
Created attachment 8683556 [details]
MozReview Request: Bug 686281 - Implement nsStyleImageLayers; r=dbaron

Bug 686281 - CSS Mask prop.
(Assignee)

Comment 43

3 years ago
Created attachment 8683557 [details]
MozReview Request: Bug 686281 - Rename *background* to *imagelayer*; r=dbaron.

Bug 686281 - CSS Mask Style
(Assignee)

Comment 44

3 years ago
Created attachment 8683558 [details]
MozReview Request: Bug 686281 - Implement CSS mask style; r=dbaron.

Bug 686281 - CSS Mask rendering.
(Assignee)

Updated

3 years ago
Attachment #8682995 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Attachment #8682996 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Attachment #8682999 - Attachment is obsolete: true
(Assignee)

Comment 45

3 years ago
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/
(Assignee)

Comment 46

3 years ago
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/
(Assignee)

Comment 47

3 years ago
Comment on attachment 8683558 [details]
MozReview Request: Bug 686281 - Implement CSS mask style; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24373/diff/1-2/
Did you intend to request review?
Flags: needinfo?(cku)
(Assignee)

Comment 49

3 years ago
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/
(Assignee)

Comment 50

3 years ago
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/
(Assignee)

Comment 51

3 years ago
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/
(Assignee)

Comment 52

3 years ago
Created attachment 8683706 [details]
MozReview Request: Bug 686281 - Mask CSS parsing and Mask DOM API. r=dbaron

Bug 686281 - CSS Mask animation.
(Assignee)

Updated

3 years ago
Attachment #8683556 - Attachment description: MozReview Request: Bug 686281 - CSS Mask prop. → MozReview Request: Bug 686281- Implement CSS mask properties. r=:dbaron
Attachment #8683556 - Flags: review?(dbaron)
(Assignee)

Comment 53

3 years ago
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/
(Assignee)

Updated

3 years ago
Attachment #8683557 - Attachment description: MozReview Request: Bug 686281 - CSS Mask Style → MozReview Request: Bug 686281- Implement CSS mask style. r=:dbaron
Attachment #8683557 - Flags: review?(dbaron)
(Assignee)

Comment 54

3 years ago
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/
(Assignee)

Updated

3 years ago
Attachment #8683558 - Attachment description: MozReview Request: Bug 686281 - CSS Mask rendering. → MozReview Request: Bug 686281- Implement CSS mask rendering. r=:dbaron
Attachment #8683558 - Flags: review?(dbaron)
(Assignee)

Comment 55

3 years ago
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/
(Assignee)

Updated

3 years ago
Attachment #8683706 - Attachment description: MozReview Request: Bug 686281 - CSS Mask animation. → MozReview Request: Bug 686281- Implement CSS mask animation. r=:dbaron
Attachment #8683706 - Flags: review?(dbaron)
(Assignee)

Comment 56

3 years ago
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/
(Assignee)

Comment 57

3 years ago
(In reply to David Baron [:dbaron] ⌚UTC+8 from comment #48)
> Did you intend to request review?
I am still working on test case... but yes, if you have time, please help to review mask implementation.
Flags: needinfo?(cku)
Attachment #8683556 - Flags: review?(dbaron)
Comment on attachment 8683556 [details]
MozReview Request: Bug 686281 - Implement nsStyleImageLayers; r=dbaron

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

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

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

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

Is it possible for this to share code with background?

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

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

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

Do you need to deal with this TBD?

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

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

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

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

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

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

Also, addition of new properties should have corresponding changes in layout/style/test/property_database.js -- and then the tests should pass with those changes.
Attachment #8683557 - Flags: review?(dbaron)
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).
Attachment #8683558 - Flags: review?(dbaron)
Comment on attachment 8683558 [details]
MozReview Request: Bug 686281 - Implement CSS mask style; r=dbaron.

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

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

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

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

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

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

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

Why does this need to be a separate unit from eUnit_BackgroundPosition?
Attachment #8683706 - Flags: review?(dbaron)
(Assignee)

Updated

3 years ago
Attachment #8683556 - Attachment description: MozReview Request: Bug 686281- Implement CSS mask properties. r=:dbaron → MozReview Request: Bug 686281- Implement nsStyleLayers and modify nsStyleBackground. r=:dbaron.
Attachment #8683556 - Flags: review?(dbaron)
(Assignee)

Comment 64

3 years ago
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/
(Assignee)

Updated

3 years ago
Attachment #8683557 - Attachment description: MozReview Request: Bug 686281- Implement CSS mask style. r=:dbaron → MozReview Request: Bug 686281 - Rename *background* to *layer*. r=:dbaron
Attachment #8683557 - Flags: review?(dbaron)
(Assignee)

Comment 65

3 years ago
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/
(Assignee)

Updated

3 years ago
Attachment #8683558 - Attachment description: MozReview Request: Bug 686281- Implement CSS mask rendering. r=:dbaron → MozReview Request: Bug 686281- Implement CSS mask properties. r=:dbaron
Attachment #8683558 - Flags: review?(dbaron)
(Assignee)

Comment 66

3 years ago
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/
(Assignee)

Updated

3 years ago
Attachment #8683706 - Attachment description: MozReview Request: Bug 686281- Implement CSS mask animation. r=:dbaron → MozReview Request: Bug 686281- Implement CSS mask rendering. r=:dbaron
Attachment #8683706 - Flags: review?(dbaron)
(Assignee)

Comment 67

3 years ago
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/
(Assignee)

Comment 68

3 years ago
Created attachment 8684292 [details]
MozReview Request: Bug 686281 - Mask CSS rendering; r=mstange

Bug 686281- Implement CSS mask animation. r=:dbaron
Attachment #8684292 - Flags: review?(dbaron)
(Assignee)

Comment 69

3 years ago
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/
(Assignee)

Comment 70

3 years ago
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/
(Assignee)

Comment 71

3 years ago
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/
(Assignee)

Comment 72

3 years ago
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/
(Assignee)

Comment 73

3 years ago
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/
(Assignee)

Comment 74

3 years ago
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
(Assignee)

Comment 75

3 years ago
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/
(Assignee)

Comment 76

3 years ago
Comment on attachment 8683557 [details]
MozReview Request: Bug 686281 - Rename *background* to *imagelayer*; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24371/diff/6-7/
Attachment #8683557 - Attachment description: MozReview Request: Bug 686281 - Rename *background* to *layer*. r=:dbaron → MozReview Request: Bug 686281- Rename *background* to *layer*. r=:dbaron
(Assignee)

Comment 77

3 years ago
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/
(Assignee)

Comment 78

3 years ago
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/
(Assignee)

Comment 79

3 years ago
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/
(Assignee)

Comment 80

3 years ago
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
(Assignee)

Comment 81

3 years ago
https://reviewboard.mozilla.org/r/24373/#review22089

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

Fix this TODO
(Assignee)

Comment 82

3 years ago
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/
(Assignee)

Comment 83

3 years ago
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/
(Assignee)

Comment 84

3 years ago
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/
(Assignee)

Comment 85

3 years ago
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/
(Assignee)

Comment 86

3 years ago
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/
(Assignee)

Comment 87

3 years ago
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.
(Assignee)

Comment 88

3 years ago
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
(Assignee)

Comment 89

3 years ago
Comment on attachment 8683556 [details]
MozReview Request: Bug 686281 - Implement nsStyleImageLayers; r=dbaron

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24369/diff/8-9/
Attachment #8683556 - Attachment description: MozReview Request: Bug 686281- Implement nsStyleLayers and modify nsStyleBackground. r=:dbaron. → MozReview Request: Bug 686281 - Implement nsStyleLayers and modify nsStyleBackground. r?:dbaron.
(Assignee)

Comment 90

3 years ago
Comment on attachment 8683557 [details]
MozReview Request: Bug 686281 - Rename *background* to *imagelayer*; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24371/diff/8-9/
Attachment #8683557 - Attachment description: MozReview Request: Bug 686281- Rename *background* to *layer*. r=:dbaron → MozReview Request: Bug 686281 - Rename *background* to *layer*. r?:dbaron
(Assignee)

Comment 91

3 years ago
Comment on attachment 8683558 [details]
MozReview Request: Bug 686281 - Implement CSS mask style; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24373/diff/8-9/
Attachment #8683558 - Attachment description: MozReview Request: Bug 686281- Implement CSS mask properties. r=:dbaron → MozReview Request: Bug 686281 - Implement CSS mask properties. r?:dbaron
(Assignee)

Comment 92

3 years ago
Comment on attachment 8683706 [details]
MozReview Request: Bug 686281 - Mask CSS parsing and Mask DOM API. r=dbaron

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24415/diff/6-7/
Attachment #8683706 - Attachment description: MozReview Request: Bug 686281- Implement CSS mask rendering. r=:dbaron → MozReview Request: Bug 686281 - Implement CSS mask rendering. r?:dbaron
(Assignee)

Comment 93

3 years ago
Comment on attachment 8684292 [details]
MozReview Request: Bug 686281 - Mask CSS rendering; r=mstange

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24531/diff/4-5/
Attachment #8684292 - Attachment description: MozReview Request: Bug 686281- Implement CSS mask animation. r=:dbaron → MozReview Request: Bug 686281 - Implement CSS mask animation. r?:dbaron
(Assignee)

Comment 94

3 years ago
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/
(Assignee)

Comment 95

3 years ago
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/
(Assignee)

Comment 96

3 years ago
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/
(Assignee)

Comment 97

3 years ago
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/
(Assignee)

Comment 98

3 years ago
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/
(Assignee)

Comment 99

3 years ago
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
(Assignee)

Comment 100

3 years ago
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.
(Assignee)

Comment 101

3 years ago
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/
(Assignee)

Comment 102

3 years ago
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/
(Assignee)

Comment 103

3 years ago
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/
(Assignee)

Comment 104

3 years ago
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/
(Assignee)

Comment 105

3 years ago
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/
(Assignee)

Comment 106

3 years ago
Created attachment 8684679 [details]
MozReview Request: Bug 686281 - Mask CSS animation; r=dbaron.

Bug 686281 - Implement CSS mask alias. r?:dbaron
Attachment #8684679 - Flags: review?(dbaron)
(Assignee)

Comment 107

3 years ago
Created attachment 8684680 [details]
MozReview Request: Bug 686281 - Mask CSS webkit-alias; r=dbaron.

Bug 686281 - Implement CSS mask test. (WIP)
(Assignee)

Comment 108

3 years ago
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)
(Assignee)

Comment 109

3 years ago
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/
(Assignee)

Comment 110

3 years ago
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/
(Assignee)

Comment 111

3 years ago
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/
(Assignee)

Comment 112

3 years ago
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/
(Assignee)

Comment 113

3 years ago
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/
(Assignee)

Comment 114

3 years ago
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/
(Assignee)

Comment 115

3 years ago
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/
(Assignee)

Comment 116

3 years ago
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/
(Assignee)

Comment 117

3 years ago
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/
(Assignee)

Comment 118

3 years ago
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/
(Assignee)

Comment 119

3 years ago
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/
(Assignee)

Comment 120

3 years ago
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/
(Assignee)

Comment 121

3 years ago
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/
(Assignee)

Comment 122

3 years ago
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/
(Assignee)

Comment 123

3 years ago
Created attachment 8684815 [details]
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
Attachment #8684815 - Flags: review?(dbaron)
Attachment #8683556 - Flags: review?(dbaron)
Comment on attachment 8683556 [details]
MozReview Request: Bug 686281 - Implement nsStyleImageLayers; r=dbaron

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

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

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

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


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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

I'm not really happy about that name, though, and would like to find something better.
(Assignee)

Comment 125

3 years ago
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
(Assignee)

Comment 126

3 years ago
https://reviewboard.mozilla.org/r/24369/#review22177

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

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

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

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

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

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

Missed the "s" here in nsStyleLayers

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

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

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

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

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

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

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

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

Probably COMPOSITE -> COMPOSITE_MODE

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

Please move this comment rather than deleting it.

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

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

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

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

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

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

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

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

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

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

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

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

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

again, _ at the end

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

again, no need for templatization

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

space before !=

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

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

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

same here

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

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

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

I'm only partly done with this review; I need to continue starting at nsCSSPropList.h.
Attachment #8683558 - Flags: review?(dbaron)
(Assignee)

Comment 132

3 years ago
Comment on attachment 8683556 [details]
MozReview Request: Bug 686281 - Implement nsStyleImageLayers; r=dbaron

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24369/diff/13-14/
Attachment #8683556 - Attachment description: MozReview Request: Bug 686281 - Implement nsStyleLayers and modify nsStyleBackground. r?:dbaron. → MozReview Request: Bug 686281 - Create nsStyleImageLayers. r?:dbaron
Attachment #8683556 - Flags: review?(dbaron)
(Assignee)

Comment 133

3 years ago
Comment on attachment 8683557 [details]
MozReview Request: Bug 686281 - Rename *background* to *imagelayer*; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24371/diff/13-14/
Attachment #8683557 - Attachment description: MozReview Request: Bug 686281 - Rename *background* to *layer*. r?:dbaron → MozReview Request: Bug 686281 - Integrate nsStyleImageLayers with nsStyleBackground. r?:dbaron.
(Assignee)

Comment 134

3 years ago
Comment on attachment 8683558 [details]
MozReview Request: Bug 686281 - Implement CSS mask style; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24373/diff/13-14/
Attachment #8683558 - Attachment description: MozReview Request: Bug 686281 - Implement CSS mask properties. r?:dbaron → MozReview Request: Bug 686281 - Rename *background* to *imagelayer*. r?:dbaron
Attachment #8683558 - Flags: review?(dbaron)
(Assignee)

Comment 135

3 years ago
Comment on attachment 8683706 [details]
MozReview Request: Bug 686281 - Mask CSS parsing and Mask DOM API. r=dbaron

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24415/diff/11-12/
Attachment #8683706 - Attachment description: MozReview Request: Bug 686281 - Implement CSS mask rendering. r?:dbaron → MozReview Request: Bug 686281 - Implement CSS mask prop. r?:dbaron
(Assignee)

Comment 136

3 years ago
Comment on attachment 8684292 [details]
MozReview Request: Bug 686281 - Mask CSS rendering; r=mstange

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24531/diff/9-10/
Attachment #8684292 - Attachment description: MozReview Request: Bug 686281 - Implement CSS mask animation. r?:dbaron → MozReview Request: Bug 686281 - Implement CSS mask rendering. r?:dbaron
(Assignee)

Comment 137

3 years ago
Comment on attachment 8684679 [details]
MozReview Request: Bug 686281 - Mask CSS animation; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24617/diff/3-4/
Attachment #8684679 - Attachment description: MozReview Request: Bug 686281 - Implement CSS mask alias. r?:dbaron → MozReview Request: Bug 686281 - Implement CSS mask animation. r?:dbaron
(Assignee)

Comment 138

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24619/diff/3-4/
Attachment #8684680 - Attachment description: MozReview Request: Bug 686281 - Implement CSS mask test. (WIP) → MozReview Request: Bug 686281 - Implement CSS mask alias. r?:dbaron
(Assignee)

Comment 139

3 years ago
Comment on attachment 8684815 [details]
MozReview Request: Bug 686281 - Mask mochitest; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24647/diff/1-2/
Attachment #8684815 - Attachment description: MozReview Request: Bug 686281 - (Bug Fix: will-change-stacking-context-mask-1.html fail) Expands shorthad to longhand. r?dbaron → MozReview Request: Bug 686281 - Implement CSS mask test. (WIP)
(Assignee)

Comment 140

3 years ago
Created attachment 8685498 [details]
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
Attachment #8685498 - Flags: review?(dbaron)
(Assignee)

Comment 141

3 years ago
Created attachment 8685499 [details]
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.
(Assignee)

Updated

3 years ago
Attachment #8683556 - Attachment description: MozReview Request: Bug 686281 - Create nsStyleImageLayers. r?:dbaron → MozReview Request: Bug 686281 - Create nsStyleImageLayers. r?:dbaron.
(Assignee)

Comment 142

3 years ago
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/
(Assignee)

Comment 143

3 years ago
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/
(Assignee)

Comment 144

3 years ago
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/
(Assignee)

Comment 145

3 years ago
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/
(Assignee)

Comment 146

3 years ago
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/
(Assignee)

Comment 147

3 years ago
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/
(Assignee)

Comment 148

3 years ago
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/
(Assignee)

Comment 149

3 years ago
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/
(Assignee)

Comment 150

3 years ago
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/
(Assignee)

Comment 151

3 years ago
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/
(Assignee)

Comment 152

3 years ago
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/
(Assignee)

Comment 153

3 years ago
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/
(Assignee)

Comment 154

3 years ago
Comment on attachment 8683558 [details]
MozReview Request: Bug 686281 - Implement CSS mask style; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24373/diff/15-16/
Attachment #8683558 - Attachment description: MozReview Request: Bug 686281 - Rename *background* to *imagelayer*. r?:dbaron → MozReview Request: Bug 686281 - Rename *background* to *imagelayer*. r?:dbaron.
(Assignee)

Updated

3 years ago
Attachment #8683706 - Attachment description: MozReview Request: Bug 686281 - Implement CSS mask prop. r?:dbaron → MozReview Request: Bug 686281 - Implement CSS mask prop. r?:dbaron.
(Assignee)

Comment 155

3 years ago
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/
(Assignee)

Comment 156

3 years ago
Comment on attachment 8684292 [details]
MozReview Request: Bug 686281 - Mask CSS rendering; r=mstange

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24531/diff/11-12/
Attachment #8684292 - Attachment description: MozReview Request: Bug 686281 - Implement CSS mask rendering. r?:dbaron → MozReview Request: Bug 686281 - Implement CSS mask rendering. r?:dbaron.
(Assignee)

Comment 157

3 years ago
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/
(Assignee)

Comment 158

3 years ago
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/
(Assignee)

Comment 159

3 years ago
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/
(Assignee)

Comment 160

3 years ago
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/
(Assignee)

Comment 161

3 years ago
Comment on attachment 8685499 [details]
MozReview Request: Bug 686281 - A static assertion to keep value correctness of NS_RULE_NODE_IS_ANIMATION_RULE; r=dbaron.

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

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

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

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

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

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

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

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

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

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

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

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

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

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

please list these properties in alphabetical order in nsCSSPropList.h

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

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

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

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

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

This should share code with DoGetBackgroundImage.

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

This should share code with DoGetBackgroundPosition.

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

This should share code with DoGetBackgroundRepeat.

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

This should share code with DoGetBackgroundSize.

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

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

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

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

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

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

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

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

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

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

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

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

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

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

Move these 2 lines before the mask-image comment

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

"\* " rather than " \*".

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

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

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

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

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

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

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

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

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

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

Also, I'd name the function ExtractImageLayerPositionList.

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

same 2 comments here

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

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

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

This MOZ_ASSERT should move into the shared function.

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

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

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

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

you're missing -webkit-mask-composite; please add that
Attachment #8684680 - Flags: review+
(Assignee)

Comment 168

3 years ago
Comment on attachment 8685499 [details]
MozReview Request: Bug 686281 - A static assertion to keep value correctness of NS_RULE_NODE_IS_ANIMATION_RULE; r=dbaron.

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

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

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

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

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

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

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

Saying this a second time since MozReview is broken:

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

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

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

These should say PrepareImageLayer to match the code.

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

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

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

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

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

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

And please fix the indentation.

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

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

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

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

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

again, indentation, unless it matches another name fix

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

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

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

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

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

Don't add these extra 2 lines.

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

Don't delete this (although fixing it to say nsStyleSVGReset would be good).
Attachment #8683558 - Flags: review+
(Assignee)

Updated

3 years ago
Attachment #8684680 - Flags: review+ → review?(dbaron)
(Assignee)

Comment 172

3 years ago
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/
(Assignee)

Updated

3 years ago
Attachment #8684815 - Flags: review?(dbaron)
(Assignee)

Comment 173

3 years ago
Comment on attachment 8684815 [details]
MozReview Request: Bug 686281 - Mask mochitest; r=dbaron.

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

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

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

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

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

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

mask-composite is missing in this list.

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

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

mask-composite is missing.

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

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

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

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

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

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

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

Saying this a second time to make MozReview happy: you're missing -webkit-mask-composite; please add that
Attachment #8684680 - Flags: review?(dbaron) → review+
Attachment #8683556 - Flags: review?(dbaron)
Attachment #8683557 - Flags: review?(dbaron)
(Assignee)

Comment 177

3 years ago
Comment on attachment 8683556 [details]
MozReview Request: Bug 686281 - Implement nsStyleImageLayers; r=dbaron

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24369/diff/16-17/
Attachment #8683556 - Flags: review?(dbaron)
(Assignee)

Updated

3 years ago
Attachment #8683557 - Flags: review?(dbaron)
(Assignee)

Comment 178

3 years ago
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/
(Assignee)

Comment 179

3 years ago
Comment on attachment 8683558 [details]
MozReview Request: Bug 686281 - Implement CSS mask style; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24373/diff/16-17/
Attachment #8683558 - Flags: review+ → review?(dbaron)
(Assignee)

Updated

3 years ago
Attachment #8683706 - Flags: review?(dbaron)
(Assignee)

Comment 180

3 years ago
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/
(Assignee)

Updated

3 years ago
Attachment #8684292 - Flags: review?(mstange) → review?(dbaron)
(Assignee)

Comment 181

3 years ago
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/
(Assignee)

Updated

3 years ago
Attachment #8684679 - Flags: review+ → review?(dbaron)
(Assignee)

Comment 182

3 years ago
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/
(Assignee)

Updated

3 years ago
Attachment #8684680 - Flags: review+ → review?(dbaron)
(Assignee)

Comment 183

3 years ago
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/
(Assignee)

Comment 184

3 years ago
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/
(Assignee)

Updated

3 years ago
Attachment #8685498 - Flags: review+ → review?(dbaron)
(Assignee)

Comment 185

3 years ago
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/
(Assignee)

Comment 186

3 years ago
Comment on attachment 8685499 [details]
MozReview Request: Bug 686281 - A static assertion to keep value correctness of NS_RULE_NODE_IS_ANIMATION_RULE; r=dbaron.

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

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


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

(But also, it's not so good to work so much late at night!)
Attachment #8684292 - Flags: review?(dbaron) → review?(mstange)
Attachment #8683556 - Flags: review?(dbaron)
Attachment #8683557 - Flags: review?(dbaron)
Attachment #8683558 - Flags: review?(dbaron)
Attachment #8683706 - Flags: review?(dbaron)
Attachment #8684679 - Flags: review?(dbaron)
Attachment #8684680 - Flags: review?(dbaron)
Attachment #8685498 - Flags: review?(dbaron)
Attachment #8685499 - Flags: review?(dbaron)
(Assignee)

Comment 188

3 years ago
https://reviewboard.mozilla.org/r/24415/#review22425

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

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

No special reason other than readability and maintainability.
(Assignee)

Comment 190

3 years ago
(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)
(Assignee)

Updated

3 years ago
Attachment #8683556 - Attachment description: MozReview Request: Bug 686281 - Create nsStyleImageLayers. r?:dbaron. → MozReview Request: Bug 686281 - Create nsStyleImageLayers; r?:dbaron
Attachment #8683556 - Flags: review?(dbaron)
(Assignee)

Comment 191

3 years ago
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/
(Assignee)

Updated

3 years ago
Attachment #8683557 - Attachment description: MozReview Request: Bug 686281 - Integrate nsStyleImageLayers with nsStyleBackground. r?:dbaron. → MozReview Request: Bug 686281 - Integrate nsStyleImageLayers with nsStyleBackground; r?:dbaron
Attachment #8683557 - Flags: review?(dbaron)
(Assignee)

Comment 192

3 years ago
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/
(Assignee)

Updated

3 years ago
Attachment #8683558 - Attachment description: MozReview Request: Bug 686281 - Rename *background* to *imagelayer*. r?:dbaron. → MozReview Request: Bug 686281 - Rename *background* to *imagelayer*; r?:dbaron
Attachment #8683558 - Flags: review?(dbaron)
(Assignee)

Comment 193

3 years ago
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/
(Assignee)

Comment 194

3 years ago
Comment on attachment 8683706 [details]
MozReview Request: Bug 686281 - Mask CSS parsing and Mask DOM API. r=dbaron

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24415/diff/15-16/
Attachment #8683706 - Attachment description: MozReview Request: Bug 686281 - Implement CSS mask prop. r?:dbaron. → MozReview Request: Bug 686281 - Implement CSS mask prop; r?:dbaron.
Attachment #8683706 - Flags: review?(dbaron)
(Assignee)

Updated

3 years ago
Attachment #8684292 - Attachment description: MozReview Request: Bug 686281 - Implement CSS mask rendering. r?:dbaron. → MozReview Request: Bug 686281 - Implement CSS mask rendering; r?:mstange
Attachment #8684292 - Flags: review?(mstange) → review?(dbaron)
(Assignee)

Comment 195

3 years ago
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/
(Assignee)

Comment 196

3 years ago
Comment on attachment 8684679 [details]
MozReview Request: Bug 686281 - Mask CSS animation; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24617/diff/7-8/
Attachment #8684679 - Attachment description: MozReview Request: Bug 686281 - Implement CSS mask animation. r?:dbaron → MozReview Request: Bug 686281 - Implement CSS mask animation; r?:dbaron
Attachment #8684679 - Flags: review?(dbaron)
(Assignee)

Comment 197

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24619/diff/7-8/
Attachment #8684680 - Attachment description: MozReview Request: Bug 686281 - Implement CSS mask alias. r?:dbaron → MozReview Request: Bug 686281 - Implement CSS mask alias; r=:dbaron
Attachment #8684680 - Flags: review?(dbaron)
(Assignee)

Comment 198

3 years ago
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/
(Assignee)

Updated

3 years ago
Attachment #8685498 - Attachment description: MozReview Request: Bug 686281 - (Bug Fix: will-change-stacking-context-mask-1.html fail) Expands shorthand prop to longhands. r?dbaron → MozReview Request: Bug 686281 - (Bug Fix: will-change-stacking-context-mask-1.html fail) Expands shorthand prop to longhands; r:=dbaron
Attachment #8685498 - Flags: review?(dbaron)
(Assignee)

Comment 199

3 years ago
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/
(Assignee)

Comment 200

3 years ago
Comment on attachment 8685499 [details]
MozReview Request: Bug 686281 - A static assertion to keep value correctness of NS_RULE_NODE_IS_ANIMATION_RULE; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24805/diff/4-5/
Attachment #8685499 - Attachment description: MozReview Request: Bug 686281 - A static assertion to keep value correctness of NS_RULE_NODE_IS_ANIMATION_RULE. r=:dbaron. → MozReview Request: Bug 686281 - A static assertion to keep value correctness of NS_RULE_NODE_IS_ANIMATION_RULE; r=:dbaron.
Attachment #8685499 - Flags: review?(dbaron)
(Assignee)

Updated

3 years ago
Flags: needinfo?(cku)
Attachment #8683556 - Flags: review?(dbaron)
(Assignee)

Comment 201

3 years ago
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/
(Assignee)

Comment 202

3 years ago
Comment on attachment 8683557 [details]
MozReview Request: Bug 686281 - Rename *background* to *imagelayer*; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24371/diff/18-19/
Attachment #8683557 - Flags: review?(dbaron)
(Assignee)

Updated

3 years ago
Attachment #8683558 - Flags: review?(dbaron)
(Assignee)

Comment 203

3 years ago
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/
(Assignee)

Comment 204

3 years ago
Comment on attachment 8683706 [details]
MozReview Request: Bug 686281 - Mask CSS parsing and Mask DOM API. r=dbaron

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24415/diff/16-17/
Attachment #8683706 - Flags: review?(dbaron)
(Assignee)

Comment 205

3 years ago
Comment on attachment 8684292 [details]
MozReview Request: Bug 686281 - Mask CSS rendering; r=mstange

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24531/diff/14-15/
Attachment #8684292 - Flags: review?(dbaron)
(Assignee)

Updated

3 years ago
Attachment #8684679 - Flags: review?(dbaron)
(Assignee)

Comment 206

3 years ago
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/
(Assignee)

Comment 207

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24619/diff/8-9/
Attachment #8684680 - Flags: review?(dbaron)
(Assignee)

Comment 208

3 years ago
Comment on attachment 8685498 [details]
MozReview Request: Bug 686281 - Expands will-change of a shorthand prop to longhand ones; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24803/diff/5-6/
Attachment #8685498 - Flags: review?(dbaron)
(Assignee)

Comment 209

3 years ago
Comment on attachment 8685499 [details]
MozReview Request: Bug 686281 - A static assertion to keep value correctness of NS_RULE_NODE_IS_ANIMATION_RULE; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24805/diff/5-6/
Attachment #8685499 - Flags: review?(dbaron)
Attachment #8684292 - Flags: review?(mstange)
Also, per bug 1224141, please stop using the ":" in the reviewer syntax in the commit messages.
(Assignee)

Updated

3 years ago
Attachment #8683556 - Attachment description: MozReview Request: Bug 686281 - Create nsStyleImageLayers; r?:dbaron → MozReview Request: Bug 686281 - Create nsStyleImageLayers;
(Assignee)

Comment 211

3 years ago
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/
(Assignee)

Comment 212

3 years ago
Comment on attachment 8683557 [details]
MozReview Request: Bug 686281 - Rename *background* to *imagelayer*; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24371/diff/18-19/
Attachment #8683557 - Attachment description: MozReview Request: Bug 686281 - Integrate nsStyleImageLayers with nsStyleBackground; r?:dbaron → MozReview Request: Bug 686281 - Integrate nsStyleImageLayers with nsStyleBackground;
(Assignee)

Comment 213

3 years ago
Comment on attachment 8683558 [details]
MozReview Request: Bug 686281 - Implement CSS mask style; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24373/diff/18-19/
Attachment #8683558 - Attachment description: MozReview Request: Bug 686281 - Rename *background* to *imagelayer*; r?:dbaron → MozReview Request: Bug 686281 - Rename *background* to *imagelayer*;
(Assignee)

Comment 214

3 years ago
Comment on attachment 8683706 [details]
MozReview Request: Bug 686281 - Mask CSS parsing and Mask DOM API. r=dbaron

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24415/diff/16-17/
Attachment #8683706 - Attachment description: MozReview Request: Bug 686281 - Implement CSS mask prop; r?:dbaron. → MozReview Request: Bug 686281 - Implement CSS mask prop;
(Assignee)

Updated

3 years ago
Attachment #8684292 - Flags: review?(mstange)
(Assignee)

Comment 215

3 years ago
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/
(Assignee)

Comment 216

3 years ago
Comment on attachment 8684679 [details]
MozReview Request: Bug 686281 - Mask CSS animation; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24617/diff/8-9/
Attachment #8684679 - Attachment description: MozReview Request: Bug 686281 - Implement CSS mask animation; r?:dbaron → MozReview Request: Bug 686281 - (r+) Implement CSS mask animation;
(Assignee)

Updated

3 years ago
Attachment #8684680 - Attachment description: MozReview Request: Bug 686281 - Implement CSS mask alias; r=:dbaron → MozReview Request: Bug 686281 - (r+) Implement CSS mask alias;
(Assignee)

Comment 217

3 years ago
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/
(Assignee)

Comment 218

3 years ago
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/
(Assignee)

Comment 219

3 years ago
Comment on attachment 8685498 [details]
MozReview Request: Bug 686281 - Expands will-change of a shorthand prop to longhand ones; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24803/diff/5-6/
Attachment #8685498 - Attachment description: MozReview Request: Bug 686281 - (Bug Fix: will-change-stacking-context-mask-1.html fail) Expands shorthand prop to longhands; r:=dbaron → MozReview Request: Bug 686281 - (r+) Expands shorthand prop to longhands;
(Assignee)

Comment 220

3 years ago
Comment on attachment 8685499 [details]
MozReview Request: Bug 686281 - A static assertion to keep value correctness of NS_RULE_NODE_IS_ANIMATION_RULE; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24805/diff/5-6/
Attachment #8685499 - Attachment description: MozReview Request: Bug 686281 - A static assertion to keep value correctness of NS_RULE_NODE_IS_ANIMATION_RULE; r=:dbaron. → MozReview Request: Bug 686281 - (r+) A static assertion to keep value correctness of NS_RULE_NODE_IS_ANIMATION_RULE;
(Assignee)

Comment 221

3 years ago
Comment on attachment 8684292 [details]
MozReview Request: Bug 686281 - Mask CSS rendering; r=mstange

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

Comment 222

3 years ago
https://reviewboard.mozilla.org/r/24415/#review22553

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

This assertion is totally wrong. Remvoe it.
(Assignee)

Updated

3 years ago
Blocks: 1224422
(Assignee)

Comment 223

3 years ago
https://reviewboard.mozilla.org/r/24369/#review21991

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

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

I believe the order of the properties in the big table in nsComputedDOMStyle.cpp is exposed to Web content as the enumeration order of JS "for (prop in element.style)", although maybe that's changed with new bindings.
(Assignee)

Comment 225

3 years ago
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/
(Assignee)

Comment 226

3 years ago
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/
(Assignee)

Updated

3 years ago
Attachment #8683558 - Flags: review?(dbaron)
(Assignee)

Comment 227

3 years ago
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/
(Assignee)

Comment 228

3 years ago
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/
(Assignee)

Comment 229

3 years ago
Comment on attachment 8684292 [details]
MozReview Request: Bug 686281 - Mask CSS rendering; r=mstange

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24531/diff/15-16/
Attachment #8684292 - Attachment description: MozReview Request: Bug 686281 - Implement CSS mask rendering; r?:mstange → MozReview Request: Bug 686281 - Implement CSS mask rendering; r?mstange
(Assignee)

Comment 230

3 years ago
Comment on attachment 8684679 [details]
MozReview Request: Bug 686281 - Mask CSS animation; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24617/diff/9-10/
Attachment #8684679 - Flags: review?(dbaron)
(Assignee)

Updated

3 years ago
Attachment #8684680 - Flags: review?(dbaron)
(Assignee)

Comment 231

3 years ago
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/
(Assignee)

Comment 232

3 years ago
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/
(Assignee)

Updated

3 years ago
Attachment #8685498 - Flags: review?(dbaron)
(Assignee)

Comment 233

3 years ago
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/
(Assignee)

Updated

3 years ago
Attachment #8685499 - Flags: review?(dbaron)
(Assignee)

Comment 234

3 years ago
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/
(Assignee)

Comment 235

3 years ago
Comment on attachment 8684292 [details]
MozReview Request: Bug 686281 - Mask CSS rendering; r=mstange

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

Comment 236

3 years ago
(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?
Attachment #8683558 - Flags: review?(dbaron)
Attachment #8684679 - Flags: review?(dbaron)
Attachment #8684680 - Flags: review?(dbaron)
Attachment #8685498 - Flags: review?(dbaron)
Attachment #8685499 - Flags: review?(dbaron)
As I said in comment 175, please let me know explicitly when you actually want me to review these again.

(That said, I'm not sure why you're pushing new versions to mozreview when they're not yet ready for re-review.)
Flags: needinfo?(cku)
(Assignee)

Comment 238

3 years ago
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
(Assignee)

Updated

3 years ago
Blocks: 1228280
(Assignee)

Comment 239

3 years ago
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)
(Assignee)

Updated

3 years ago
Blocks: 1228354
(Assignee)

Comment 240

3 years ago
Comment on attachment 8683556 [details]
MozReview Request: Bug 686281 - Implement nsStyleImageLayers; r=dbaron

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24369/diff/20-21/
Attachment #8683556 - Attachment description: MozReview Request: Bug 686281 - Create nsStyleImageLayers; → MozReview Request: Bug 686281 - Create nsStyleImageLayers; r?dbaron.
Attachment #8683556 - Flags: review?(dbaron)
(Assignee)

Updated

3 years ago
Attachment #8683557 - Attachment description: MozReview Request: Bug 686281 - Integrate nsStyleImageLayers with nsStyleBackground; → MozReview Request: Bug 686281 - Integrate nsStyleImageLayers with nsStyleBackground; r?dbaron.
Attachment #8683557 - Flags: review?(dbaron)
(Assignee)

Comment 241

3 years ago
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/
(Assignee)

Comment 242

3 years ago
Comment on attachment 8683558 [details]
MozReview Request: Bug 686281 - Implement CSS mask style; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24373/diff/20-21/
Attachment #8683558 - Attachment description: MozReview Request: Bug 686281 - Rename *background* to *imagelayer*; → MozReview Request: Bug 686281 - Rename *background* to *imagelayer*; r=dbaron.
Attachment #8683558 - Flags: review?(dbaron)
(Assignee)

Updated

3 years ago
Attachment #8683706 - Attachment description: MozReview Request: Bug 686281 - Implement CSS mask prop; → MozReview Request: Bug 686281 - Implement CSS mask style; r?dbaron.
Attachment #8683706 - Flags: review?(dbaron)
(Assignee)

Comment 243

3 years ago
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/
(Assignee)

Comment 244

3 years ago
Comment on attachment 8684292 [details]
MozReview Request: Bug 686281 - Mask CSS rendering; r=mstange

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

Comment 245

3 years ago
Comment on attachment 8684679 [details]
MozReview Request: Bug 686281 - Mask CSS animation; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24617/diff/10-11/
Attachment #8684679 - Attachment description: MozReview Request: Bug 686281 - (r+) Implement CSS mask animation; → MozReview Request: Bug 686281 - Mask style computing; r?dbaron
Attachment #8684679 - Flags: review?(dbaron)
(Assignee)

Updated

3 years ago
Attachment #8684680 - Attachment description: MozReview Request: Bug 686281 - (r+) Implement CSS mask alias; → MozReview Request: Bug 686281 - Mask DOM API; r?dbaron
Attachment #8684680 - Flags: review?(dbaron)
(Assignee)

Comment 246

3 years ago
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/
(Assignee)

Comment 247

3 years ago
Comment on attachment 8684815 [details]
MozReview Request: Bug 686281 - Mask mochitest; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24647/diff/8-9/
Attachment #8684815 - Attachment description: MozReview Request: Bug 686281 - Implement CSS mask test. (WIP) → MozReview Request: Bug 686281 - Mask CSS rendering; r?mstange
Attachment #8684815 - Flags: review?(mstange)
(Assignee)

Comment 248

3 years ago
Comment on attachment 8685498 [details]
MozReview Request: Bug 686281 - Expands will-change of a shorthand prop to longhand ones; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24803/diff/7-8/
Attachment #8685498 - Attachment description: MozReview Request: Bug 686281 - (r+) Expands shorthand prop to longhands; → MozReview Request: Bug 686281 - Mask CSS animation; r=dbaron.
Attachment #8685498 - Flags: review?(dbaron)
(Assignee)

Updated

3 years ago
Attachment #8685499 - Attachment description: MozReview Request: Bug 686281 - (r+) A static assertion to keep value correctness of NS_RULE_NODE_IS_ANIMATION_RULE; → MozReview Request: Bug 686281 - Mask CSS webkit-alias; r=dbaron.
Attachment #8685499 - Flags: review?(dbaron)
(Assignee)

Comment 249

3 years ago
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/
(Assignee)

Comment 250

3 years ago
Created attachment 8692847 [details]
MozReview Request: Bug 686281 - Remove nsStyleSVGReset::mMask; r=dbaron

Bug 686281 - Remove nsStyleSVGReset::mMask; r?dbaron
Attachment #8692847 - Flags: review?(dbaron)
(Assignee)

Comment 251

3 years ago
Created attachment 8692848 [details]
MozReview Request: Bug 686281 - mask-composite reftests; r?dbaron

Bug 686281 - Add mask property into property_database.js; r?dbaron.
Attachment #8692848 - Flags: review?(dbaron)
(Assignee)

Comment 252

3 years ago
Created attachment 8692849 [details]
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.
Attachment #8692849 - Flags: review?(dbaron)
(Assignee)

Comment 253

3 years ago
Created attachment 8692850 [details]
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.
Attachment #8692850 - Flags: review?(dbaron)
(Assignee)

Updated

3 years ago
Flags: needinfo?(cku)
Attachment #8683558 - Flags: review?(dbaron) → review+
(Assignee)

Comment 254

3 years ago
Hi dbaron,
Had fixed issues that you mentioned, please help to review again.
1. Remove nsStyleSVGReset::mMask in "Bug 686281 - Remove nsStyleSVGReset::mMask"
2. Move Mask DOM relative change into "Bug 686281 - Mask DOM API"
3. Fix mochitest fail(property_database.js mainly).
Attachment #8684815 - Flags: review?(mstange)
Comment on attachment 8684815 [details]
MozReview Request: Bug 686281 - Mask mochitest; r=dbaron.

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

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

} else if (hasValidLayers) {

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

This needs a null check; CreateSimilarDrawTarget can fail.

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

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

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

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

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

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

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

Typo in evaluate; but I think "Compute mask transform." would be a better fit here.
(Assignee)

Comment 256

3 years ago
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/
(Assignee)

Comment 257

3 years ago
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/
(Assignee)

Comment 258

3 years ago
Comment on attachment 8683558 [details]
MozReview Request: Bug 686281 - Implement CSS mask style; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24373/diff/21-22/
Attachment #8683558 - Flags: review+ → review?(dbaron)
(Assignee)

Comment 259

3 years ago
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/
(Assignee)

Comment 260

3 years ago
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/
(Assignee)

Comment 261

3 years ago
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/
(Assignee)

Comment 262

3 years ago
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/
(Assignee)

Comment 263

3 years ago
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.
(Assignee)

Comment 264

3 years ago
Comment on attachment 8684815 [details]
MozReview Request: Bug 686281 - Mask mochitest; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24647/diff/9-10/
Attachment #8684815 - Flags: review?(mstange)
(Assignee)

Comment 265

3 years ago
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/
(Assignee)

Comment 266

3 years ago
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/
(Assignee)

Comment 267

3 years ago
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/
(Assignee)

Comment 268

3 years ago
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/
(Assignee)

Comment 269

3 years ago
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/
(Assignee)

Comment 270

3 years ago
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/
(Assignee)

Comment 271

3 years ago
https://reviewboard.mozilla.org/r/24647/#review23767

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

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

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

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

typo: source

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

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

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

I'd leave out the deviceOffsetTranslation variable and just make this
maskTransform = Matrix::Translation(drawRect.x, drawRect.y) * mat;
Attachment #8684815 - Flags: review?(mstange) → review+
(Assignee)

Comment 273

3 years ago
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/
(Assignee)

Comment 274

3 years ago
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/
(Assignee)

Comment 275

3 years ago
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/
(Assignee)

Comment 276

3 years ago
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/
(Assignee)

Comment 277

3 years ago
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/
(Assignee)

Comment 278

3 years ago
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/
(Assignee)

Comment 279

3 years ago
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/
(Assignee)

Comment 280

3 years ago
Comment on attachment 8684815 [details]
MozReview Request: Bug 686281 - Mask mochitest; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24647/diff/10-11/
Attachment #8684815 - Attachment description: MozReview Request: Bug 686281 - Mask CSS rendering; r?mstange → MozReview Request: Bug 686281 - Mask CSS rendering; r=mstange
(Assignee)

Comment 281

3 years ago
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/
(Assignee)

Comment 282

3 years ago
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/
(Assignee)

Comment 283

3 years ago
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/
(Assignee)

Comment 284

3 years ago
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/
(Assignee)

Comment 285

3 years ago
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/
(Assignee)

Comment 286

3 years ago
Comment on attachment 8692850 [details]
MozReview Request: Bug 686281 - A static assertion to keep value correctness of NS_RULE_NODE_IS_ANIMATION_RULE; r=dbaron.

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

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

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

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

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

Comment 290

3 years ago
(In reply to David Baron [:dbaron] ⌚UTC-8 from comment #289)
> I'm going to wait for comment 288 to be addressed before continuing the
> review; fixing that might make it easier for me to associate the patches
> with what they were the last time I reviewed them.

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

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

Comment 291

3 years ago
Here is a draft
https://reviewboard.mozilla.org/r/24369/
Please check whether it's acceptable for reviewing
(In reply to C.J. Ku[:cjku] from comment #290)
> (In reply to David Baron [:dbaron] ⌚UTC-8 from comment #289)
> > I'm going to wait for comment 288 to be addressed before continuing the
> > review; fixing that might make it easier for me to associate the patches
> > with what they were the last time I reviewed them.
> 
> Just want to double confirm:
> You want these two patches to be merge as one just like before:
> 1. (The first patch) Bug 686281 - Create nsStyleImageLayers; r?dbaron. 
> 2. (The second patch) Bug 686281 - Integrate nsStyleImageLayers with
> nsStyleBackground; r?dbaron. 

These should be put back together.

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

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

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

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

Please let me know when you've updated the patches per above comments.
Attachment #8683557 - Flags: review?(dbaron)
Attachment #8683558 - Flags: review?(dbaron)
Attachment #8683706 - Flags: review?(dbaron)
Attachment #8684292 - Flags: review?(dbaron)
Attachment #8684679 - Flags: review?(dbaron)
Attachment #8684680 - Flags: review?(dbaron)