332333 - macro-ize nsRuleNode::Compute##struct##Data start and end sections

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

We should macro-ize nsRuleNode::Compute*Data start and end sections.  nsStyleFont and nsStyleSVG* need the specialized em/currentColor checks moved into a check-properties callback.

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: testcase for color:currentColor

I had to move some stuff into FontCheckCallback, and realized I needed the same for color to make color:currentColor work correctly.  (I'm questioning whether those should be callbacks rather than a 2-item if-else chain, though.)

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch

The introduction of CheckColorCallback fixes the bug in the testcase above; moving the stuff that was in the preamble of ComputeFontData into CheckFontCallback made me realize it was needed.  There were only a few other minor tweaks:
 * padding, margin, and outline had a RecalcData call in the end part that's now a little earlier
 * the parentFlags stuff in ComputeBackgroundData is not in the preamble, although it was always at the end (this should go away with bug 332334, I think)
 * the generatedContent test in ComputeDisplayData is now later (this one might be messy later on)
 * ComputePaddingData used delete instead of Destroy in the OOM case at the end
 * ComputeTableData was missing an NS_UNLIKELY that the others had
and perhaps something else that I've forgotten.

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 216875 [details] [diff] [review]
patch

>   if (specified == total) {

I fixed this so it's an |else if|.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 216875 [details] [diff] [review]
patch

>Index: nsRuleNode.cpp

> typedef nsRuleNode::RuleDetail
>-  (* PR_CALLBACK CheckCallbackFn)(const nsRuleDataStruct& aData);
>+  (* PR_CALLBACK CheckCallbackFn)(const nsRuleDataStruct& aData,
>+                                  nsRuleNode::RuleDetail aResult);

It'd be nice to add some documentation here.  Specifically, what the callback is expected (or allowed?) to do, and what the aResult argument means, given that it's an in param.

>@@ -990,38 +1033,46 @@ nsRuleNode::CheckSpecifiedProperties(con

>+  nsRuleNode::RuleDetail result;

It might be worth documenting why we have the tests in the order we have them in (e.g. why given specified == inherited == 0 we prefer eRuleNone to eRulePartialInherited).  Something about handing back as much information as possible or something?

>+#define COMPUTE_START_INHERITED(type_, ctorargs_, data_, parentdata_, rdtype_, rdata_) \

Again, might be nice to document what the args are.

>+    if (aRuleDetail != eRuleFullMixed && aRuleDetail != eRuleFullReset) {     \
...
>+    else                                                                      \

I think this branch should assert that aRuleDetail is one of eRuleNone, eRulePartialReset, eRulePartialMixed, eRulePartialInherited, and eRuleFullInherited.  Just to make sure that nothing unexpected happens.

>+#define COMPUTE_START_RESET(type_, ctorargs_, data_, parentdata_, rdtype_, rdata_) \

Documentation for the args?

>+  if (parentContext &&                                                        \
>+      aRuleDetail != eRuleFullReset &&                                        \
>+      aRuleDetail != eRulePartialReset &&                                     \
>+      aRuleDetail != eRuleNone)                                               \

Assert that aRuleDetail is one of eRulePartialMixed, eRulePartialInherited, eRuleFullMixed, or eRuleFullInherited.

> nsRuleNode::ComputeDisplayData(nsStyleStruct* aStartStruct,
>   PRBool generatedContent = (pseudoTag == nsCSSPseudoElements::before || 
>                              pseudoTag == nsCSSPseudoElements::after);

I guess add:

  NS_ASSERTION(!generatedContent || parentContext,
               "Must have parent style for generated content");

or something?

r+sr=bzbarsky with those nits picked.

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Checked in with all of the comments addressed except for adding the assertions about the enum being within the enum values, which I'd rather not bother with.

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch

Patch that was checked in.

Comment 7

[jag (Peter Annema)]

Could this have kicked btek from ~944ms to ~950ms?

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Yeah.  Backing out CheckColorCallback (which was a bug fix) got about half the difference back.

Comment 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I put the change back in -- I hope bug 332335 may get some of the performance back, and we need this, and that, for some new css3-values stuff that I think is important.

But CheckSpecifiedProperties is called a *lot*.

I suspect the other part of the loss is the bit added to the end of CheckFontProperties -- technically that only needs to happen for the last call in the loop in WalkRuleTree, not all the calls.  Perhaps I should try to do something about that as well.

Comment 10

[Boris Zbarsky [:bzbarsky]]

> except for adding the assertions about the enum being within the enum values

I guess my point was that if we add enum values (unlikely, I know) we're implicitly going to put them into one of the two buckets at those if checks.  It'd be good to have the implicitness be explicit (as in, the asserts will fire and we'll have to make a conscious decision as to which branch to take).

I guess it's not a big deal, since as I said we're not really planning to add enum values....

As for perf, another change was that now we examine all the properties for the Font struct even if font-family is a system font.  We didn't use to do that; not sure whether we use system fonts enough for that to matter.