Closed Bug 1375536 Opened 3 years ago Closed 2 years ago

remove nsStyleVariables

Categories

(Core :: CSS Parsing and Computation, enhancement, P4)

Product:
Core
Component:
CSS Parsing and Computation
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla61
Tracking Flags:
Tracking Status
firefox57 --- wontfix
firefox58 --- wontfix
firefox59 --- ?
firefox61 --- fixed

People

(Reporter: ferjm, Assigned: heycam)

References

Details

Attachments

(1 file)

Bug 1375536 - Remove nsStyleVariables.
59 bytes, text/x-review-board-request
emilio
: review+
xidorn
: review+
Details 
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.
Blocks: stylo
Priority: -- → P4
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 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)
(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 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 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+
(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.)
Er, given it's not a struct.
Pushed by cam@mcc.id.au:
https://hg.mozilla.org/integration/autoland/rev/1ec9d6648322
Remove nsStyleVariables. r=emilio,xidorn
https://hg.mozilla.org/mozilla-central/rev/1ec9d6648322
Status: ASSIGNED → RESOLVED
Closed: 2 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.