Closed Bug 1613010 Opened 2 years ago Closed 2 years ago

Don't use a binary tree representation for calc() sums / products.

Categories

(Core :: CSS Parsing and Computation, task, P3)

task

Tracking

()

RESOLVED FIXED
mozilla74
Tracking Status
firefox74 --- fixed

People

(Reporter: emilio, Assigned: emilio)

References

Details

Attachments

(1 file)

No description provided.

Keep a flat list of sum members. Simpify product and division ASAP.

I want to preserve the tree for a bit longer to implement min / max / clamp.
This doesn't do anything for it that we weren't doing already, but it helps to
eventually keep this specified representation and the equivalent computed
representation for <length-percentage> values.

Type: defect → task
Depends on: 1613006
Priority: -- → P3

(Sorry for posting here and not on Phabricator, but the latter hates me.)

Lines 260-272 straightforwardly distribute scalar multiplication over min/max/clamp, but that's not actually right for negative values: -1 * min(a, b) = max(-a, -b) and vice-versa; -1 * clamp(a, x, b) = clamp(-b, -x, -a). (This might make for a good test case, if there's not one already.)

(In reply to quasicomputational from comment #2)

(Sorry for posting here and not on Phabricator, but the latter hates me.)

Lines 260-272 straightforwardly distribute scalar multiplication over min/max/clamp, but that's not actually right for negative values: -1 * min(a, b) = max(-a, -b) and vice-versa; -1 * clamp(a, x, b) = clamp(-b, -x, -a). (This might make for a good test case, if there's not one already.)

Oh, indeed, d'oh! Will add a test for this.

Blocks: 1613491

(In reply to quasicomputational from comment #2)

(Sorry for posting here and not on Phabricator, but the latter hates me.)

Lines 260-272 straightforwardly distribute scalar multiplication over min/max/clamp, but that's not actually right for negative values: -1 * min(a, b) = max(-a, -b) and vice-versa; -1 * clamp(a, x, b) = clamp(-b, -x, -a). (This might make for a good test case, if there's not one already.)

FWIW, this was fixed, and some existing tests caught it. I enabled those tests in automation to get some coverage.

Pushed by ealvarez@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/8e28baddb10b
Don't use a binary tree representation for calc() sums / products. r=heycam
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla74
You need to log in before you can comment on or make changes to this bug.