Closed
Bug 1375536
Opened 6 years ago
Closed 5 years ago
remove nsStyleVariables
Categories
(Core :: CSS Parsing and Computation, enhancement, P4)
Core
CSS Parsing and Computation
Tracking
()
RESOLVED
FIXED
mozilla61
People
(Reporter: ferjm, Assigned: heycam)
References
Details
Attachments
(1 file)
Instead of getting nsStyleVariables through Servo_GetStyleVariables for CalcStyleDifference, we can implement a method to compare nsStyleVariables in Servo and get rid of Servo_GetStyleVariables.
Updated•6 years ago
|
Priority: -- → P4
Updated•5 years ago
|
status-firefox57:
--- → wontfix
status-firefox58:
--- → fix-optional
Comment 1•5 years ago
|
||
https://wiki.mozilla.org/Bug_Triage/Projects/Bug_Handling/Bug_Husbandry#Move_fix-optionals
status-firefox59:
--- → ?
Assignee | ||
Comment 2•5 years ago
|
||
Morphing this into the removal of nsStyleVariables, which is no longer used.
Assignee: nobody → cam
Status: NEW → ASSIGNED
Summary: stylo: Implement method to compare nsStyleVariables and use it from CalcStyleDifference → remove nsStyleVariables
Comment hidden (mozreview-request) |
Assignee | ||
Comment 4•5 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4ecf4eb4d42bcd58ca1e5b255d5c8f75984a4501
Comment 5•5 years ago
|
||
mozreview-review |
Comment on attachment 8966828 [details] Bug 1375536 - Remove nsStyleVariables. https://reviewboard.mozilla.org/r/235508/#review241242 ::: layout/style/ComputedStyle.cpp:116 (Diff revision 1) > - *aEqualStructs |= NS_STYLE_INHERIT_BIT(Variables); > + DebugOnly<int> styleStructCount = 0; // count Variables already > - DebugOnly<int> styleStructCount = 1; // count Variables already This is going to break the logic in Gecko_CalcStyleDifference I suppose. I actually asked emilio to remove it in bug 1448559 comment 4, but he said it needs more thought and thus should be done in a followup. You may need to change NS_STYLE_INHERIT_MASK, and maybe some other places.
Attachment #8966828 -
Flags: review?(xidorn+moz)
Assignee | ||
Comment 6•5 years ago
|
||
(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #5) > This is going to break the logic in Gecko_CalcStyleDifference I suppose. Yes, good point. > I actually asked emilio to remove it in bug 1448559 comment 4, but he said > it needs more thought and thus should be done in a followup. OK. I'll post the updated patch and get Emilio's review too to see what I've missed... > You may need to change NS_STYLE_INHERIT_MASK, and maybe some other places. Hmm, I forgot that wasn't automatically generated.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 9•5 years ago
|
||
mozreview-review |
Comment on attachment 8966828 [details] Bug 1375536 - Remove nsStyleVariables. https://reviewboard.mozilla.org/r/235508/#review241250 It'd be great if you could double-check that we don't incorrectly end up thinking reset property changes are inherited ones. But I think it shouldn't be the case with this patch. Thank you for the cleanup! ::: layout/style/ComputedStyle.h:28 (Diff revision 3) > // Includes nsStyleStructID. > #include "nsStyleStructFwd.h" > > // Bits for each struct. > // NS_STYLE_INHERIT_BIT defined in nsStyleStructFwd.h > -#define NS_STYLE_INHERIT_MASK 0x000ffffff > +#define NS_STYLE_INHERIT_MASK 0x0007fffff Please document that this doesn't account for variables whatsoever? I guess there's no variables struct so maybe not worth it.
Attachment #8966828 -
Flags: review?(emilio) → review+
Comment 10•5 years ago
|
||
mozreview-review |
Comment on attachment 8966828 [details] Bug 1375536 - Remove nsStyleVariables. https://reviewboard.mozilla.org/r/235508/#review241252 Reading the code of Gecko_CalcStyleDifference, it seems to me changing NS_STYLE_INHERIT_MASK seems to be the only thing we need to do to make it works correctly.
Attachment #8966828 -
Flags: review?(xidorn+moz) → review+
Assignee | ||
Comment 11•5 years ago
|
||
(In reply to Emilio Cobos Álvarez [:emilio] from comment #9) > Please document that this doesn't account for variables whatsoever? I guess > there's no variables struct so maybe not worth it. Yeah, might be more confusing to mention it given it's a struct. (We should probably rename NS_STYLE_INHERIT_MASK, since it's no longer a bitfield about which structs are inherited in the style context tree.)
Assignee | ||
Comment 12•5 years ago
|
||
Er, given it's not a struct.
Comment hidden (mozreview-request) |
Comment 14•5 years ago
|
||
Pushed by cam@mcc.id.au: https://hg.mozilla.org/integration/autoland/rev/1ec9d6648322 Remove nsStyleVariables. r=emilio,xidorn
Comment 15•5 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/1ec9d6648322
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
status-firefox61:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
You need to log in
before you can comment on or make changes to this bug.
Description
•