Astley Chen (inactive) Assignee
	Description • 8 years ago

mask-* longhand properties support currently are disabled by default by compiled flag. In bugs 1291231 and 1284169, we are observing potential feature broken simply because developers was unaware that their patches had broken mask feature.
It'd caused an increased effort to keep our feature running before formally enabling it.

To resolve this problem, before fully enabling mask-* longhand properties(bug 1251161), we'd prefer to enable this feature on nightly and aurora channels first.
It can help us bail out the situation of potential feature broken but also give us a better test coverage to stress out unknown bugs and solve them before ship.

	Ting-Yu Lin [:TYLin] (PDT, UTC-7)
	Comment 1 • 8 years ago

If there are still major bugs to fix before releasing to aurora testers, we can enable it only on nightly.

	Astley Chen (inactive) Assignee
	Comment 2 • 8 years ago

(In reply to Ting-Yu Lin [:TYLin] (UTC+8) from comment #1)
> If there are still major bugs to fix before releasing to aurora testers, we
> can enable it only on nightly.

So far there is only bug 1234485 which is talking about performance optimization for smooth mask animation. I think it's not fatal and also considering most of developers or authors are on aurora, it's probably a good way to collect the feedback from them as well. Since we will not uplift so that we still have enough time to go through nightly 51 before it turns to aurora.

	Tom Tromey :tromey
	Comment 7 • 8 years ago
mozreview-review

Comment on attachment 8781042 [details]
Bug 1294660: Part 3 - regenerate CSS_PROPERTIES list in devtools/shared/css-properties-db.js.

https://reviewboard.mozilla.org/r/71548/#review69416

Thank you for doing this.  It looks good to me.

	Astley Chen (inactive) Assignee
	Comment 8 • 8 years ago

(In reply to Tom Tromey :tromey from comment #7)
> Comment on attachment 8781042 [details]
> Bug 1294660: Part 3 - regenerate CSS_PROPERTIES list in
> devtools/shared/css-properties-db.js.
> 
> https://reviewboard.mozilla.org/r/71548/#review69416
> 
> Thank you for doing this.  It looks good to me.

Hi Tom, given that we will enable mask longhand properties on nightly & aurora only, so we would need to make CSS_PROPERTIES as is now for beta and release to ensure no assertion on devtool test. Could you let me know what's the best way to do it ? Thanks.

	Tom Tromey :tromey
	Comment 9 • 8 years ago

Greg recently did this, so I'm forwarding the NI to him.

	Greg Tatum [:gregtatum]
	Comment 10 • 8 years ago

I'm working on a solution for automatically generating this list as I wasn't aware at the time of the release flags. I'm planning on having this in place before the next uplift. In the meantime the css properties list will need to be re-generated on aurora and nightly separately. That code isn't in beta.

	Astley Chen (inactive) Assignee
	Comment 11 • 8 years ago

(In reply to Greg Tatum [:gregtatum] [@gregtatum] from comment #10)
> I'm working on a solution for automatically generating this list as I wasn't
> aware at the time of the release flags. I'm planning on having this in place
> before the next uplift. In the meantime the css properties list will need to
> be re-generated on aurora and nightly separately. That code isn't in beta.
Thanks for the feedback.
I'm planning to enable it starting from nightly FF51 and won't uplift to aurora FF50.
So in this case, as soon as FF51 goes to beta, I'll need to re-generate the list to avoid test assertions, am I correct ?

	Astley Chen (inactive) Assignee
	Comment 12 • 8 years ago

Hi Ted, could you help to review first patch ? Thanks.

	Cameron McCormack (:heycam)
	Comment 13 • 8 years ago
mozreview-review

Comment on attachment 8781041 [details]
Bug 1294660: Part 2 - update mask test cases and enable mask shorthand reftest.

https://reviewboard.mozilla.org/r/71546/#review70058

::: layout/reftests/w3c-css/submitted/masking/reftest.list:19
(Diff revision 1)
>  # mask-image test cases
> -fails == mask-image-1a.html mask-image-1-ref.html
> -fails == mask-image-1b.html mask-image-1-ref.html
> -fails == mask-image-1c.html mask-image-1-ref.html
> +== mask-image-1a.html mask-image-1-ref.html
> +== mask-image-1b.html mask-image-1-ref.html
> +== mask-image-1c.html mask-image-1-ref.html
>  == mask-image-1d.html mask-image-1-ref.html
> -fails == mask-image-2.html mask-image-2-ref.html
> +fuzzy-if(skiaContent,1,20000) == mask-image-2.html mask-image-2-ref.html

Nit: I would either consistently add the "bug 1231643#c10" comment to all the lines with fuzzy-if(skiaContent,...) or just add a single comment somewhere at the top of the file mentioning it.

	Cameron McCormack (:heycam)
	Comment 14 • 8 years ago
mozreview-review

Comment on attachment 8781041 [details]
Bug 1294660: Part 2 - update mask test cases and enable mask shorthand reftest.

https://reviewboard.mozilla.org/r/71546/#review70060

	Astley Chen (inactive) Assignee
	Comment 18 • 8 years ago
mozreview-review-reply

Comment on attachment 8781041 [details]
Bug 1294660: Part 2 - update mask test cases and enable mask shorthand reftest.

https://reviewboard.mozilla.org/r/71546/#review70058

> Nit: I would either consistently add the "bug 1231643#c10" comment to all the lines with fuzzy-if(skiaContent,...) or just add a single comment somewhere at the top of the file mentioning it.

That totally makes sense. Thanks for your review. Will update the patch accordingly.

	u459114
	Comment 19 • 8 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d0c92a89dafd

	Greg Tatum [:gregtatum]
	Comment 23 • 8 years ago

(In reply to Astley Chen [:astley] (UTC+8) from comment #11)
> Thanks for the feedback.
> I'm planning to enable it starting from nightly FF51 and won't uplift to
> aurora FF50.
> So in this case, as soon as FF51 goes to beta, I'll need to re-generate the
> list to avoid test assertions, am I correct ?

The only time you should worry about it is when you are changing the CSS properties within your own version. The uplift issues are my problem until I can land my fix. I'll let you know once I have my solution in place and you will not have to worry about this test failing again.

	Astley Chen (inactive) Assignee
	Comment 27 • 8 years ago

Hi gps, could you help to review patch part 1 from build module perspective ? The intention is to use $RELEASE_BUILD variable to determine if the build is for nightly and aurora and enable the feature on demand. Thanks.

	Mike Hommey [:glandium]
	Comment 28 • 8 years ago
mozreview-review

Comment on attachment 8781040 [details]
Bug 1294660: Part 1 - enable CSS positioned mask on nightly and aurora.

https://reviewboard.mozilla.org/r/71544/#review71306

	u459114
	Comment 32 • 8 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5f839734523c

	Astley Chen (inactive) Assignee
	Comment 36 • 8 years ago

Update latest try result:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=48d552e08804

Good to go.

	u459114
	Comment 37 • 8 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/51fc7aec63174687f64fb57518dcfafc65d83a0d
Bug 1294660: Part 1 - enable CSS positioned mask on nightly and aurora. r=glandium

https://hg.mozilla.org/integration/mozilla-inbound/rev/4e7efbee95fd20f08d988a2f383976b52c821566
Bug 1294660: Part 2 - update mask test cases and enable mask shorthand reftest. r=heycam

https://hg.mozilla.org/integration/mozilla-inbound/rev/ade443e6455749cf6cca6f2224d3bf07a871e420
Bug 1294660: Part 3 - regenerate CSS_PROPERTIES list in devtools/shared/css-properties-db.js. r=tromey

	Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)
	Comment 38 • 8 years ago

Backed out for build failures:

https://hg.mozilla.org/integration/mozilla-inbound/rev/3b4d6ed9dbec830bb9e5a3d76b93281ad381976c
https://hg.mozilla.org/integration/mozilla-inbound/rev/059ac65e6fceb4ad40cd00ddb3e2bac59b3f3e7e
https://hg.mozilla.org/integration/mozilla-inbound/rev/8f8bfe32eebc7f771039a65a086ade212a37bd42

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=34554987&repo=mozilla-inbound

> /home/worker/workspace/build/src/obj-firefox/layout/style/nsCSSPropsGenerated.inc:891:1: error: static_assert failed "GenerateCSSPropsGenerated.py did not list properties in nsCSSPropertyID order"

	Astley Chen (inactive) Assignee
	Comment 39 • 8 years ago

pulled from mozilla-inbound and rebuild with my patches, no build bustage on my local macOS 10.11.
no idea why the failure only appeared on try. Looking for cause now.

	Pulsebot
	Comment 40 • 8 years ago

Pushed by archaeopteryx@coole-files.de:
https://hg.mozilla.org/integration/mozilla-inbound/rev/41225d00af33
Part 1 - enable CSS positioned mask on nightly and aurora. r=glandium
https://hg.mozilla.org/integration/mozilla-inbound/rev/06f05485b3fd
Part 2 - update mask test cases and enable mask shorthand reftest. r=heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/617e22a259ab
Part 3 - regenerate CSS_PROPERTIES list in devtools/shared/css-properties-db.js. r=tromey
https://hg.mozilla.org/integration/mozilla-inbound/rev/df48165b6b64
Part 4 - Touch clobber file to prevent bustage. r=me

	Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)
	Comment 41 • 8 years ago

This change needed clobber, so I added that and relanded.

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 42 • 8 years ago

Backed out again for Windows reftest failures like https://treeherder.mozilla.org/logviewer.html#?job_id=34597675&repo=mozilla-inbound

https://hg.mozilla.org/integration/mozilla-inbound/rev/d0f1300fe232

	Astley Chen (inactive) Assignee
	Comment 43 • 8 years ago

I think the failure you found is an intermittent orange tracked by bug 1297919 and 1207900. It seems not related to my patches.
From the failure log, it looks like the timeout threshold is too short to run over the test. I'll check what I can do further to solve this intermittent orange.

[1] https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&selectedJob=34605775
[2] https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&fromchange=df8d6a12b605745eead88fca245a6d8c99bf2cba&selectedJob=34603202
[3] https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&fromchange=df8d6a12b605745eead88fca245a6d8c99bf2cba&selectedJob=34597389

	Astley Chen (inactive) Assignee
	Comment 48 • 8 years ago
mozreview-review

Comment on attachment 8784665 [details]
Bug 1294660: Part 4 - Touch clobber file to prevent bustage.

https://reviewboard.mozilla.org/r/74018/#review71906

Reviewed by :aryx & me.

	Ting-Yu Lin [:TYLin] (PDT, UTC-7)
	Comment 49 • 8 years ago
mozreview-review

Comment on attachment 8784665 [details]
Bug 1294660: Part 4 - Touch clobber file to prevent bustage.

https://reviewboard.mozilla.org/r/74018/#review71908

r+ for Astley per comment 48.

	Phil Ringnalda (:philor)
	Comment 50 • 8 years ago

He just picked the wrong failure log to paste, you were backed out for the non-intermittent Win8 mask failures in https://treeherder.mozilla.org/logviewer.html#?job_id=34607327&repo=mozilla-inbound not the 7200 second thing.

	Astley Chen (inactive) Assignee
	Comment 51 • 8 years ago

(In reply to Phil Ringnalda (:philor) from comment #50)
> He just picked the wrong failure log to paste, you were backed out for the
> non-intermittent Win8 mask failures in
> https://treeherder.mozilla.org/logviewer.html#?job_id=34607327&repo=mozilla-
> inbound not the 7200 second thing.

Thank you to point his out clearly. Problem addressed and will come out a new try with results of Win8 reftest to ensure it's fixed.

	Astley Chen (inactive) Assignee
	Comment 56 • 8 years ago
mozreview-review

Comment on attachment 8784665 [details]
Bug 1294660: Part 4 - Touch clobber file to prevent bustage.

https://reviewboard.mozilla.org/r/74018/#review72274

LGTM.

	Astley Chen (inactive) Assignee
	Comment 57 • 8 years ago
mozreview-review

Comment on attachment 8784665 [details]
Bug 1294660: Part 4 - Touch clobber file to prevent bustage.

https://reviewboard.mozilla.org/r/74018/#review72276

	Pulsebot
	Comment 58 • 8 years ago

Pushed by cku@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/adfd8fb2faca
Part 1 - enable CSS positioned mask on nightly and aurora. r=glandium
https://hg.mozilla.org/integration/autoland/rev/0764147ff82b
Part 2 - update mask test cases and enable mask shorthand reftest. r=heycam
https://hg.mozilla.org/integration/autoland/rev/051453bf9c54
Part 3 - regenerate CSS_PROPERTIES list in devtools/shared/css-properties-db.js. r=tromey
https://hg.mozilla.org/integration/autoland/rev/64b92e33713c
Part 4 - Touch clobber file to prevent bustage. r=TYLin

	Phil Ringnalda (:philor)
	Comment 59 • 8 years ago

Backed out in https://hg.mozilla.org/integration/autoland/rev/8281d8ce0050 for Linux failures in mask-composite-2c.html

	u459114
	Comment 60 • 8 years ago

(In reply to Phil Ringnalda (:philor) from comment #59)
> Backed out in https://hg.mozilla.org/integration/autoland/rev/8281d8ce0050
> for Linux failures in mask-composite-2c.html
These patches depend on a check-in in inbound repo
https://hg.mozilla.org/integration/mozilla-inbound/rev/e78454e242072bbcada626874f2900bbe1c3e647

I should manually push patches into inbound instead of using autoland.

	u459114
	Comment 61 • 8 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/c6ea6cfdf70f6aca2febfc242bbe60553d4c6d29
Bug 1294660: Part 1 - enable CSS positioned mask on nightly and aurora. r=glandium

https://hg.mozilla.org/integration/mozilla-inbound/rev/51fc5a46d6d4c5f713a2d6411bbb273254a8efee
Bug 1294660: Part 2 - update mask test cases and enable mask shorthand reftest. r=heycam

https://hg.mozilla.org/integration/mozilla-inbound/rev/9fae889966e39e5b03a0ad3399a67d77c809d0b1
Bug 1294660: Part 3 - regenerate CSS_PROPERTIES list in devtools/shared/css-properties-db.js. r=tromey

https://hg.mozilla.org/integration/mozilla-inbound/rev/bb8f2e88d99176437cdee6f4d50e6c5cd6a91f60
Bug 1294660: Part 4 - Touch clobber file to prevent bustage. r=astley

	Ryan VanderMeulen [:RyanVM]
	Comment 62 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/c6ea6cfdf70f
https://hg.mozilla.org/mozilla-central/rev/51fc5a46d6d4
https://hg.mozilla.org/mozilla-central/rev/9fae889966e3
https://hg.mozilla.org/mozilla-central/rev/bb8f2e88d991

	Jean-Yves Perrier [:teoli]
	Comment 63 • 8 years ago

Updated:
https://developer.mozilla.org/en-US/Firefox/Experimental_features
and the note in the browser compat section of:
https://developer.mozilla.org/en-US/docs/Web/CSS/mask-repeat
https://developer.mozilla.org/en-US/docs/Web/CSS/mask-size
https://developer.mozilla.org/en-US/docs/Web/CSS/mask-position
https://developer.mozilla.org/en-US/docs/Web/CSS/mask-origin
https://developer.mozilla.org/en-US/docs/Web/CSS/mask-mode
https://developer.mozilla.org/en-US/docs/Web/CSS/mask-image
https://developer.mozilla.org/en-US/docs/Web/CSS/mask-composite
https://developer.mozilla.org/en-US/docs/Web/CSS/mask-clip
https://developer.mozilla.org/en-US/docs/Web/CSS/mask