Closed Bug 826582 Opened 7 years ago Closed 7 years ago

upstream CSS calc() reftests

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla21

People

(Reporter: heycam, Assigned: heycam)

References

Details

Attachments

(5 files, 3 obsolete files)

There was some interest in seeing our CSS calc() tests upstreamed to the official CSS test suite.
Assignee: nobody → cam
Status: NEW → ASSIGNED
Attachment #697788 - Flags: review?(dbaron)
And these last two tests for calc() in gradient image functions.  I had to modify background-image-gradient-1 a bit to get it to work with the new linear-gradient() syntax, so I copied this one to w3c-css/ instead of moving it.
Attachment #697796 - Flags: review?(dbaron)
Move not copy one of the tests.
Attachment #698120 - Flags: review?(dbaron)
Attachment #697796 - Attachment is obsolete: true
Attachment #697796 - Flags: review?(dbaron)
Comment on attachment 697788 [details] [diff] [review]
Part 1: Unprefix -moz-calc() in reftests for that feature.

r=dbaron
Attachment #697788 - Flags: review?(dbaron) → review+
Comment on attachment 697789 [details] [diff] [review]
Part 2: Unprefix -moz-transform{,-origin} in calc() reftests.

r=dbaron
Attachment #697789 - Flags: review?(dbaron) → review+
Comment on attachment 697790 [details] [diff] [review]
Part 3: Move calc() tests to be upstreamed to the CSS test suite.

These should be represented as file moves, not removes and adds.  (Once they're represented as moves, it should be easier to review.)

Also, why are the new file names moz-calc rather than just calc?
Attachment #697790 - Flags: review?(dbaron) → review-
Comment on attachment 697791 [details] [diff] [review]
Part 4: Make upstreamed calc() tests conform to the test template.

The rel="author" should point to http://dbaron.org rather than the bug numbers.  Otherwise looks fine.

(Thanks for doing all this.)
Attachment #697791 - Flags: review?(dbaron) → review+
Comment on attachment 698120 [details] [diff] [review]
Part 5: Upstream calc-in-gradient tests. (v1.1)

Again, file moves and weird moz-calc names.
Attachment #698120 - Flags: review?(dbaron) → review-
I named them with "moz-calc" prefixes because that's what those in layout/reftests/w3c-css/submitted/multicol3/ did.  But I can use "calc" if you prefer.

Regarding the patches not encoding the moves of the files, this was unfortunately due to the fact that I'm working out of a git repository, and git doesn't track file renames (it instead detects them itself).  I'll regenerate the patches from my hg repo.
Attachment #698120 - Attachment is obsolete: true
Attachment #702088 - Flags: review?(dbaron)
Comment on attachment 702086 [details] [diff] [review]
Part 3: Move calc() tests to be upstreamed to the CSS test suite. (v1.1)

r=dbaron

(And please make sure before hg pushing that whatever you use for git->hg export doesn't lose the fact that these are moves.)
Attachment #702086 - Flags: review?(dbaron) → review+
Comment on attachment 702088 [details] [diff] [review]
Part 5: Upstream calc-in-gradient tests. (v1.2)

So it looks like you're intending to leave one of the tests but not the other in the old directory.  I think that makes sense if it's testing a gradient feature that's in -moz-linear-gradient but not linear-gradient (I didn't check if this was the case, though).

You need to get permission from the author of the other test (who was not me) for the relicensing, though.  (Before landing!)  Ideally, ask the author to comment in this bug agreeing to the licensing as described in http://hg.mozilla.org/mozilla-central/file/56ff556e74d9/layout/reftests/w3c-css/submitted/README .

You also need to get the -moz- prefixes out of the other test.

It also looks like you're converting my test into another test that tests the same thing as the gradient stop positions test.  I'm not sure that makes sense -- the positions stuff can no longer be tested with linear-gradient, but it can still be tested with radil-gradient, and probably should be.  (So still worth leaving the old test.)

And see comment 15.

r=dbaron with all of those things fixed.  Thanks for doing all this.
Attachment #702088 - Flags: review?(dbaron) → review+
(In reply to David Baron [:dbaron] from comment #16)
> So it looks like you're intending to leave one of the tests but not the
> other in the old directory.  I think that makes sense if it's testing a
> gradient feature that's in -moz-linear-gradient but not linear-gradient (I
> didn't check if this was the case, though).

I believe it is the case.

> You need to get permission from the author of the other test (who was not
> me) for the relicensing, though.  (Before landing!)  Ideally, ask the author
> to comment in this bug agreeing to the licensing as described in
> http://hg.mozilla.org/mozilla-central/file/56ff556e74d9/layout/reftests/w3c-
> css/submitted/README .

Thomasy: are you OK to license your css-calc/background-linear-gradient-1.html test according to the above README, so that we can submit it to the official CSS test suite?

> You also need to get the -moz- prefixes out of the other test.

Missed that when recreating the patch.

> It also looks like you're converting my test into another test that tests
> the same thing as the gradient stop positions test.  I'm not sure that makes
> sense -- the positions stuff can no longer be tested with linear-gradient,
> but it can still be tested with radil-gradient, and probably should be.  (So
> still worth leaving the old test.)

Ah OK.
Flags: needinfo?(thomas)
(In reply to Cameron McCormack (:heycam) from comment #17)
> (In reply to David Baron [:dbaron] from comment #16)
> > So it looks like you're intending to leave one of the tests but not the
> > other in the old directory.  I think that makes sense if it's testing a
> > gradient feature that's in -moz-linear-gradient but not linear-gradient (I
> > didn't check if this was the case, though).
> 
> I believe it is the case.
> 
> > You need to get permission from the author of the other test (who was not
> > me) for the relicensing, though.  (Before landing!)  Ideally, ask the author
> > to comment in this bug agreeing to the licensing as described in
> > http://hg.mozilla.org/mozilla-central/file/56ff556e74d9/layout/reftests/w3c-
> > css/submitted/README .
> 
> Thomasy: are you OK to license your
> css-calc/background-linear-gradient-1.html test according to the above
> README, so that we can submit it to the official CSS test suite?
> 

Fine. I agree the license.

> > You also need to get the -moz- prefixes out of the other test.
> 
> Missed that when recreating the patch.
> 
> > It also looks like you're converting my test into another test that tests
> > the same thing as the gradient stop positions test.  I'm not sure that makes
> > sense -- the positions stuff can no longer be tested with linear-gradient,
> > but it can still be tested with radil-gradient, and probably should be.  (So
> > still worth leaving the old test.)
> 
> Ah OK.
Flags: needinfo?(thomas)
Landed a followup to fix some errors reported by the test suite system (link rel=help in references):
https://hg.mozilla.org/integration/mozilla-inbound/rev/1cd56379b9f5
Blocks: 1502510
You need to log in before you can comment on or make changes to this bug.