Closed
Bug 1294660
Opened 8 years ago
Closed 8 years ago
Enable mask-* longhand properties support on nightly & aurora channels
Categories
(Core :: Layout, defect)
Core
Layout
Tracking
()
RESOLVED
FIXED
mozilla51
Tracking | Status | |
---|---|---|
firefox51 | --- | fixed |
People
(Reporter: bmo, Assigned: bmo)
References
Details
(Keywords: dev-doc-complete)
Attachments
(4 files)
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.
Assignee | ||
Updated•8 years ago
|
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.
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.
Updated•8 years ago
|
Keywords: dev-doc-needed
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•8 years ago
|
Attachment #8781040 -
Flags: review?(ted)
Attachment #8781041 -
Flags: review?(cam)
Attachment #8781042 -
Flags: review?(ttromey)
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.
Attachment #8781042 -
Flags: review?(ttromey) → review+
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.
Flags: needinfo?(ttromey)
Comment 9•8 years ago
|
||
Greg recently did this, so I'm forwarding the NI to him.
Flags: needinfo?(ttromey) → needinfo?(gtatum)
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.
Flags: needinfo?(gtatum)
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 ?
Assignee | ||
Comment 12•8 years ago
|
||
Hi Ted, could you help to review first patch ? Thanks.
Flags: needinfo?(ted)
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.
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
Attachment #8781041 -
Flags: review?(cam) → review+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
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.
Comment 19•8 years ago
|
||
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
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.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•8 years ago
|
Attachment #8781040 -
Flags: review?(cam)
Assignee | ||
Updated•8 years ago
|
Attachment #8781040 -
Flags: review?(ted) → review?(gps)
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.
Flags: needinfo?(ted)
Updated•8 years ago
|
Attachment #8781040 -
Flags: review?(gps) → review?(mh+mozilla)
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
Attachment #8781040 -
Flags: review?(mh+mozilla) → review+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•8 years ago
|
Attachment #8781040 -
Flags: review?(cam)
Comment 32•8 years ago
|
||
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 36•8 years ago
|
||
Update latest try result:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=48d552e08804
Good to go.
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
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"
Flags: needinfo?(aschen)
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.
Flags: needinfo?(aschen)
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
Comment 41•8 years ago
|
||
This change needed clobber, so I added that and relanded.
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
Flags: needinfo?(aschen)
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
Flags: needinfo?(aschen)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•8 years ago
|
Attachment #8784665 -
Flags: review?(aschen)
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.
Attachment #8784665 -
Flags: review?(aschen) → review+
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.
Attachment #8784665 -
Flags: review+
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.
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.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
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.
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
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
Comment 59•8 years ago
|
||
Backed out in https://hg.mozilla.org/integration/autoland/rev/8281d8ce0050 for Linux failures in mask-composite-2c.html
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.
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
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
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox51:
--- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
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
Keywords: dev-doc-needed → dev-doc-complete
You need to log in
before you can comment on or make changes to this bug.
Description
•