1375536 - remove nsStyleVariables

Status: RESOLVED FIXED

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

Description

[Fernando Jiménez Moreno [:ferjm]]

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.

Comment 1

[Firefox Bug Husbandry Bot]

https://wiki.mozilla.org/Bug_Triage/Projects/Bug_Handling/Bug_Husbandry#Move_fix-optionals

Comment 2

[Cameron McCormack (:heycam)]

Morphing this into the removal of nsStyleVariables, which is no longer used.

Comment 3

[Cameron McCormack (:heycam)]

Attachment: Bug 1375536 - Remove nsStyleVariables.

Review commit: https://reviewboard.mozilla.org/r/235508/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/235508/

Comment 4

[Cameron McCormack (:heycam)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4ecf4eb4d42bcd58ca1e5b255d5c8f75984a4501

Comment 5

[Xidorn Quan [:xidorn] UTC+10]

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.

Comment 6

[Cameron McCormack (:heycam)]

(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 7

[Cameron McCormack (:heycam)]

Comment on attachment 8966828 [details]
Bug 1375536 - Remove nsStyleVariables.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/235508/diff/1-2/

Comment 8

[Cameron McCormack (:heycam)]

Comment on attachment 8966828 [details]
Bug 1375536 - Remove nsStyleVariables.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/235508/diff/2-3/

Comment 9

[Emilio Cobos Álvarez [:emilio]]

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.

Comment 10

[Xidorn Quan [:xidorn] UTC+10]

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.

Comment 11

[Cameron McCormack (:heycam)]

(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.)

Comment 12

[Cameron McCormack (:heycam)]

Er, given it's not a struct.

Comment 13

[Cameron McCormack (:heycam)]

Comment on attachment 8966828 [details]
Bug 1375536 - Remove nsStyleVariables.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/235508/diff/3-4/

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1ec9d6648322
Remove nsStyleVariables. r=emilio,xidorn

Comment 15

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/1ec9d6648322