Closed Bug 1358710 Opened 6 years ago Closed 6 years ago

Unprefix -moz-linear-gradient / -moz-radial-gradient in unit tests

Categories

(Core :: CSS Parsing and Computation, enhancement)

Product:
Core
Component:
CSS Parsing and Computation
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: dholbert, Assigned: canova)

References

Details

Attachments

(1 file)

Bug 1358710 - Unprefix -moz-linear-gradient / -moz-radial-gradient in unit tests
59 bytes, text/x-review-board-request
dholbert
: review+
Details 
Before we can disable -moz-linear-gradient / -moz-radial-gradient (in bug 1337655), we need to fix our unit tests to stop depending on them, and to test the modern syntax instead (where applicable).

Happily, we have a patch to do this from ~2 years ago which still seems to apply pretty cleanly, at least for its reftest changes:
https://hg.mozilla.org/mozilla-central/rev/acb7eb7f5ad4

We should take that patch's reftest-changes, and then grep for any other remaining instance of -moz-linear-gradient/-moz-radial-gradient in the tree and fix them up to modern syntax as well (and remove all the moz-prefixed gradient entries in property-database.js)

(After that, we should be able to flip "layout.css.prefixes.gradients" over in bug 1337655 without breaking anything. And stylo should be able to run all our gradient tests without needing -moz prefix support.)
xidorn, perhaps you'd be up for taking this, as part of driving in bug 1337655?

If not, I could probably take it at some point in the next few weeks (thought I've already spent a bit more time on gradient hackery than I'd intended to this past week, since the other dependencies of bug bug 1357117 were more involved than I was anticipating).
Flags: needinfo?(xidorn+moz)
Xidorn is partly on PTO I think. Nazim, this is just some test changes, which will allow us to test our support for this stuff in stylo without implemented the deprecated syntax. Do you have time to fix up the tests this week?
Flags: needinfo?(xidorn+moz) → needinfo?(canaltinova)
Sure, I can fix up these tests.
Assignee: nobody → canaltinova
Flags: needinfo?(canaltinova)
Note that I think there's a partial/bitrotted patch for this in bug 1176496 that may or may not be a good starting point.
Oh, looks like Nazim just posted a patch. Nice!
(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #5)
> Note that I think there's a partial/bitrotted patch for this in bug 1176496
> that may or may not be a good starting point.

(Yeah, that's the same changeset I linked to in comment 0 -- I should've mentioned the bug number there too.)

Thanks for taking this, nazim!
I updated the commit and triggered another try run. Let's see again.

(In reply to Daniel Holbert [:dholbert] from comment #7)
> (In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #5)
> > Note that I think there's a partial/bitrotted patch for this in bug 1176496
> > that may or may not be a good starting point.
> 
> (Yeah, that's the same changeset I linked to in comment 0 -- I should've
> mentioned the bug number there too.)
> 
> Thanks for taking this, nazim!

My pleasure :)
Comment on attachment 8860760 [details]
Bug 1358710 - Unprefix -moz-linear-gradient / -moz-radial-gradient in unit tests

https://reviewboard.mozilla.org/r/132702/#review135822

::: layout/reftests/css-gradients/aja-linear-2b.html:7
(Diff revision 2)
>  <!-- Derived from http://a-ja.net/newgrad.html -->
>  <style>
>  div {
>    height: 200px;
>    width: 100px;
> -  background: -moz-linear-gradient(0 0, yellow, blue);
> +  background: linear-gradient(to top left, blue, yellow);

This one should be "to bottom right", not "to top left".

Though you've already got that in -2a, so I guess you should either:
 (1) mix up the syntax to keep this useful -- perhaps with "to right bottom" (flipping the keywords around)
...OR:
 (2) get rid of this -2b testcase, and rename -2a to just -2 since there wouldn't be any variants anymore.


(-moz-linear-gradient was more expressive than linear-gradient, so it had more edge cases that needed testing.  It makes sense that we can get rid of some testcases now that we're testing a less-expressive syntax.)
Comment on attachment 8860760 [details]
Bug 1358710 - Unprefix -moz-linear-gradient / -moz-radial-gradient in unit tests

https://reviewboard.mozilla.org/r/132702/#review135822

> This one should be "to bottom right", not "to top left".
> 
> Though you've already got that in -2a, so I guess you should either:
>  (1) mix up the syntax to keep this useful -- perhaps with "to right bottom" (flipping the keywords around)
> ...OR:
>  (2) get rid of this -2b testcase, and rename -2a to just -2 since there wouldn't be any variants anymore.
> 
> 
> (-moz-linear-gradient was more expressive than linear-gradient, so it had more edge cases that needed testing.  It makes sense that we can get rid of some testcases now that we're testing a less-expressive syntax.)

Oh, right. I got rid of the 2b testcase.
Also fixed stylo list. Testing one more time :)
Heads-up: I added a few new moz-prefixed gradient testcases in the patch stack that I just landed on bug 1357117. (That's on autoland now, and should be on central by tomorrow.)  Specifically, see the two mochitest-changes at the bottom of this cset:
 https://hg.mozilla.org/integration/autoland/rev/706f342e2942

You'll want to rip those out (the moz-prefixed parts) as part of the patches here.
(sorry for creating more work for you -- I felt it was worth including moz-prefixed tests to atomically test the changes in that bug & to be at a "known good state", even if those testcases were tentatively-doomed, in case we discover that we can't deprecate moz-prefixed gradients after all & end up having to keep the implementation enabled/somewhat-tested
(In reply to Daniel Holbert [:dholbert] from comment #16)
> (sorry for creating more work for you -- I felt it was worth including
> moz-prefixed tests to atomically test the changes in that bug & to be at a
> "known good state", even if those testcases were tentatively-doomed, in case
> we discover that we can't deprecate moz-prefixed gradients after all & end
> up having to keep the implementation enabled/somewhat-tested

Np :) Rebased on top of central and removed the -moz- prefixed ones. I think it's ready for review now.
Btw, I've tried to r? dholbert but it didn't change since it's cleared before. Could you review when you're available please? :)
Flags: needinfo?(dholbert)
It probably didn't change because I've blocked review requests on Bugzilla while I'm away for a few days. :)

I'll take a look early next week.
Comment on attachment 8860760 [details]
Bug 1358710 - Unprefix -moz-linear-gradient / -moz-radial-gradient in unit tests

https://reviewboard.mozilla.org/r/132702/#review140890

Sorry -- I think this may have bitrotted a bit since you posted it (due to time passing from me being away).

Hopefully it's not too much trouble to unbitrot.  Feel free to tag me for needinfo if any of that is non-trivial & you'd like a sanity-check.

::: layout/style/test/property_database.js:244
(Diff revision 5)
>    "-moz-linear-gradient(30deg red, blue)",
>    "-moz-linear-gradient(5px 5px 30deg red, blue)",
>    "-moz-linear-gradient(5px 5px red, blue)",
>    "-moz-linear-gradient(top left 30deg red, blue)",

These -moz-linear-gradient lines need removing as part of this patch.  (Sorry -- I added them in bug 1358586.)
Attachment #8860760 - Flags: review+
(transferring needinfo back to :canaltinova, to address comment 21 & get this landed. Thanks again!)
Flags: needinfo?(dholbert) → needinfo?(canaltinova)
Sorry, I missed the previous mail. Rebased on top of m-c. But after removal of `reftest-stylo.list`s, I change these lists a lot. I triggered a try build to see if everything is ok.
Flags: needinfo?(canaltinova)
Ok, fixed some parts according to try results. I think we can land this now.
Pushed by dholbert@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/19f0b1e8f287
Unprefix -moz-linear-gradient / -moz-radial-gradient in unit tests r=dholbert
Backed out for failing reftest aja-linear-2.html on OS X with e10s:

https://hg.mozilla.org/integration/autoland/rev/62d05055505d286aff5f069e515b4a261b1813ea

Recent push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=25ddfb854eec24c9d4a32667f9ac7529ef32baa1&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=99001847&repo=autoland
> REFTEST TEST-UNEXPECTED-FAIL | file:///builds/slave/test/build/tests/reftest/tests/layout/reftests/css-gradients/aja-linear-2.html == file:///builds/slave/test/build/tests/reftest/tests/layout/reftests/css-gradients/aja-linear-2-ref.html | image comparison, max difference: 1, number of differing pixels: 74
Flags: needinfo?(canaltinova)
Oops, sorry. I accidentally deleted if conditions from this test expectation. Fixed it now.
:dholbert could you push this again?
Flags: needinfo?(canaltinova)
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 86e483b65f60 -d 25a7e2672c99: rebasing 395827:86e483b65f60 "Bug 1358710 - Unprefix -moz-linear-gradient / -moz-radial-gradient in unit tests r=dholbert" (tip)
merging devtools/client/inspector/shared/test/browser_styleinspector_output-parser.js
merging gfx/tests/crashtests/557348-1.html
merging layout/base/crashtests/595039-1.html
merging layout/generic/crashtests/767765.html
merging layout/reftests/backgrounds/gradient/reftest.list
merging layout/reftests/backgrounds/gradient/scaled-color-stop-position-ref.html
merging layout/reftests/backgrounds/gradient/scaled-color-stop-position.html
merging layout/reftests/bugs/538909-1-ref.html
merging layout/reftests/bugs/538909-1.html
merging layout/reftests/bugs/605138-1-ref.html
merging layout/reftests/bugs/605138-1.html
merging layout/reftests/bugs/reftest.list
merging layout/reftests/css-calc/background-image-gradient-1-ref.html
merging layout/reftests/css-calc/background-image-gradient-1.html
merging layout/reftests/css-calc/reftest.list
merging layout/reftests/css-gradients/aja-linear-1a.html
merging layout/reftests/css-gradients/aja-linear-1b.html
merging layout/reftests/css-gradients/aja-linear-1e.html
merging layout/reftests/css-gradients/aja-linear-1f.html
merging layout/reftests/css-gradients/aja-linear-2-ref.html
merging layout/reftests/css-gradients/aja-linear-2a.html and layout/reftests/css-gradients/aja-linear-2.html to layout/reftests/css-gradients/aja-linear-2.html
merging layout/reftests/css-gradients/aja-linear-4a.html
merging layout/reftests/css-gradients/aja-linear-4b.html
merging layout/reftests/css-gradients/aja-linear-5a.html
merging layout/reftests/css-gradients/height-dependence-1-ref.html
merging layout/reftests/css-gradients/height-dependence-1.html
merging layout/reftests/css-gradients/height-dependence-2-ref.html
merging layout/reftests/css-gradients/height-dependence-2.html
merging layout/reftests/css-gradients/height-dependence-3-ref.html
merging layout/reftests/css-gradients/height-dependence-3.html
merging layout/reftests/css-gradients/linear-1a.html
merging layout/reftests/css-gradients/linear-diagonal-1a.html
merging layout/reftests/css-gradients/linear-diagonal-2a.html
merging layout/reftests/css-gradients/linear-diagonal-3a.html
merging layout/reftests/css-gradients/linear-diagonal-4a.html
merging layout/reftests/css-gradients/linear-flipped-1-ref.html
merging layout/reftests/css-gradients/linear-flipped-1.html
merging layout/reftests/css-gradients/linear-onestopposition-1.html
merging layout/reftests/css-gradients/linear-position-1a.html
merging layout/reftests/css-gradients/linear-repeat-1a.html
merging layout/reftests/css-gradients/linear-repeat-1b.html
merging layout/reftests/css-gradients/linear-repeat-1c.html
merging layout/reftests/css-gradients/linear-repeat-1d.html
merging layout/reftests/css-gradients/linear-repeat-1e.html
merging layout/reftests/css-gradients/linear-repeat-1f.html
merging layout/reftests/css-gradients/linear-repeat-1g.html
merging layout/reftests/css-gradients/linear-size-1-ref.html
merging layout/reftests/css-gradients/linear-size-1a.html
merging layout/reftests/css-gradients/linear-stops-1-ref.html
merging layout/reftests/css-gradients/linear-stops-1a.html
merging layout/reftests/css-gradients/linear-stops-1b.html
merging layout/reftests/css-gradients/linear-stops-1c.html
merging layout/reftests/css-gradients/linear-stops-1d.html
merging layout/reftests/css-gradients/linear-stops-1e.html
merging layout/reftests/css-gradients/linear-stops-1f.html
merging layout/reftests/css-gradients/linear-vertical-1a.html
merging layout/reftests/css-gradients/linear-vertical-1b.html
merging layout/reftests/css-gradients/linear-vertical-1c.html
merging layout/reftests/css-gradients/linear-vertical-1d.html
merging layout/reftests/css-gradients/linear-vertical-subpixel-1-ref.html
merging layout/reftests/css-gradients/linear-vertical-subpixel-1.html
merging layout/reftests/css-gradients/linear-viewport-ref.html
merging layout/reftests/css-gradients/linear-viewport.html
merging layout/reftests/css-gradients/radial-shape-closest-corner-1-ref.html
merging layout/reftests/css-gradients/radial-shape-closest-side-1-ref.html
merging layout/reftests/css-gradients/radial-shape-farthest-corner-1-ref.html
merging layout/reftests/css-gradients/radial-size-1a.html
merging layout/reftests/css-gradients/reftest.list
merging layout/reftests/css-gradients/repeated-final-stop-1-ref.html
merging layout/reftests/css-gradients/repeated-final-stop-1.html
merging layout/reftests/css-gradients/repeating-linear-1a.html
merging layout/reftests/css-gradients/repeating-linear-1b.html
merging layout/reftests/css-gradients/repeating-linear-2a.html
merging layout/reftests/css-gradients/repeating-radial-onestopposition-1a.html
merging layout/reftests/css-gradients/repeating-radial-onestopposition-1b.html
merging layout/reftests/css-gradients/twostops-1a.html
merging layout/reftests/css-gradients/twostops-1b.html
merging layout/reftests/css-gradients/twostops-1c.html
merging layout/reftests/css-gradients/twostops-1d.html
merging layout/reftests/css-gradients/twostops-1e.html
merging layout/reftests/forms/meter/default-style/style.css
merging layout/reftests/image-element/element-paint-continuation-ref.html
merging layout/reftests/image-element/element-paint-multiple-backgrounds-01-ref.html
merging layout/reftests/image-element/gradient-html-06b.html
merging layout/reftests/layers/component-alpha-exit-1-ref.html
merging layout/reftests/layers/component-alpha-exit-1.html
merging layout/reftests/native-theme/checkbox-dynamic-1-ref.html
merging layout/reftests/native-theme/checkbox-dynamic-1.html
merging layout/reftests/scrolling/opacity-mixed-scrolling-2.html
merging layout/reftests/scrolling/reftest.list
merging layout/reftests/transform/601894-1.html
merging layout/reftests/transform/601894-2.html
merging layout/reftests/transform/reftest.list
merging layout/style/crashtests/large_border_image_width.html
merging layout/style/test/property_database.js
merging layout/style/test/stylo-failures.md
merging layout/style/test/test_computed_style.html
merging layout/style/test/test_specified_value_serialization.html
merging layout/style/test/test_unclosed_parentheses.html
warning: conflicts while merging layout/reftests/css-gradients/reftest.list! (edit, then use 'hg resolve --mark')
warning: conflicts while merging layout/style/test/stylo-failures.md! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
Pushed by canaltinova@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/f7d5aedd57e4
Unprefix -moz-linear-gradient / -moz-radial-gradient in unit tests r=dholbert
Fixed merge conflicts and pushed to autoland. (yay! I can push now :) )
https://hg.mozilla.org/mozilla-central/rev/f7d5aedd57e4
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox55: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Hmm, you probably shouldn't remove the values from property_database.js until we completely remove -moz-prefixed gradients from the code base. I'm going to add them back but guarded by the pref.
You need to log in before you can comment on or make changes to this bug.