Last Comment Bug 779010 - nsStyleSides comparison always assume Calc values are different
: nsStyleSides comparison always assume Calc values are different
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: CSS Parsing and Computation (show other bugs)
: unspecified
: x86 Linux
: -- normal (vote)
: mozilla17
Assigned To: Matt Woodrow (:mattwoodrow)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-07-30 17:43 PDT by Matt Woodrow (:mattwoodrow)
Modified: 2012-08-02 16:35 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Compare calc values (1.02 KB, patch)
2012-07-30 17:43 PDT, Matt Woodrow (:mattwoodrow)
dbaron: review-
Details | Diff | Review
Compare calc values v2 (2.09 KB, patch)
2012-07-30 18:44 PDT, Matt Woodrow (:mattwoodrow)
dbaron: review+
Details | Diff | Review
Mochitest for this bug (3.57 KB, patch)
2012-07-31 13:34 PDT, Matt Woodrow (:mattwoodrow)
dbaron: review+
Details | Diff | Review

Description Matt Woodrow (:mattwoodrow) 2012-07-30 17:43:59 PDT
Created attachment 647374 [details] [diff] [review]
Compare calc values

This is causing unnecessary invalidation in Gaia.
Comment 1 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2012-07-30 18:06:24 PDT
Comment on attachment 647374 [details] [diff] [review]
Compare calc values

Could you just get rid of this macro and construct an nsStyleCoord for each side?  nsStyleCoord::nsStyleCoord(const nsStyleUnion&, nsStyleUnit) is inline and quite cheap, and then this logic will be in one place (nsStyleCoord::operator==) rather than two.
Comment 2 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2012-07-30 18:10:41 PDT
Comment on attachment 647374 [details] [diff] [review]
Compare calc values

Good catch, though, but I'd like this not to happen in the future (see previous comment).

(Alternatively, if you think that there would be a performance difference, you could refactor the macro so it's used by both places and there's only one thing to change.  But I don't think it's worth it.)
Comment 3 Matt Woodrow (:mattwoodrow) 2012-07-30 18:44:33 PDT
Created attachment 647393 [details] [diff] [review]
Compare calc values v2

Seems reasonable, updated the patch
Comment 4 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2012-07-30 19:09:21 PDT
Comment on attachment 647393 [details] [diff] [review]
Compare calc values v2

r=dbaron.  Thanks for finding and fixing this.
Comment 5 Matt Woodrow (:mattwoodrow) 2012-07-30 19:27:57 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/a57c83db51a7
Comment 6 Ed Morley [:emorley] 2012-07-31 06:10:47 PDT
https://hg.mozilla.org/mozilla-central/rev/a57c83db51a7
Comment 7 Matt Woodrow (:mattwoodrow) 2012-07-31 13:34:53 PDT
Created attachment 647659 [details] [diff] [review]
Mochitest for this bug
Comment 8 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2012-08-02 16:35:52 PDT
Comment on attachment 647659 [details] [diff] [review]
Mochitest for this bug

r=dbaron

Note You need to log in before you can comment on or make changes to this bug.