332335 - replace nsRuleNode::Compute##struct##Data with table-driven computation

Status: RESOLVED WONTFIX

(Core :: CSS Parsing and Computation, defect)

Description

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

We should replace nsRuleNode::Compute##struct##Data with table-driven computation.  This will avoid some if-else chains that cause performance problems and make it easier to implement various things in css3-values and my cycle() proposal.  It will also make 'initial' and initial values easier to implement consistently, and avoid the need for copy-constructors on nsStyleStructs.

Comment 1

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

An additional dependency, like bug 332334, is backing out bug 339358.

Comment 2

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

This should also avoid the duplication of initial values between nsStyleStruct constructors and nsRuleNode -moz-initial implementations; if it doesn't, a separate bug should be filed.

Comment 3

[[e-mail]]

can you elaborate on what you had in mind? i've been poking around nsRuleNode but i'm not sure exactly what needs to be table-driven.

Comment 4

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

All the repetitive code in Compute*Data that computes the value of each property.  And Get*Data as well, although that might be filed as a separate bug already.

Comment 5

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

So, the basic idea here is the following:

There are tons of if-else chains in the Compute*Data methods in nsRuleNode.cpp, like this:

  // user-input: auto, none, enum, inherit
  if (eCSSUnit_Enumerated == uiData.mUserInput.GetUnit()) {
    ui->mUserInput = uiData.mUserInput.GetIntValue();
  }
  else if (eCSSUnit_Auto == uiData.mUserInput.GetUnit() ||
           eCSSUnit_Initial == uiData.mUserInput.GetUnit()) {
    ui->mUserInput = NS_STYLE_USER_INPUT_AUTO;
  }
  else if (eCSSUnit_None == uiData.mUserInput.GetUnit()) {
    ui->mUserInput = NS_STYLE_USER_INPUT_NONE;
  }
  else if (eCSSUnit_Inherit == uiData.mUserInput.GetUnit()) {
    inherited = PR_TRUE;
    ui->mUserInput = parentUI->mUserInput;
  }

some things, however, use the SetCoord function.  The first step here is to gradually migrate all of these if-else chains over to something like the SetCoord function (perhaps to coexist alongside SetCoord, since SetCoord is for setting nsStyleCoord).

A good first step would be to just take a handful of these if-else chains and write a function that's capable of replacing each one with a single function call.

Having this function is really what's needed for implementing calc(), attr(), etc., since we don't want to add yet another case (and a complicated one) to all of these if-else chains to handle calc(), attr(), etc.

Eventually, once *everything* was replaced by calls to this new function or to SetCoord (which would probably be desired to implement calc() and attr() for all properties, which may not be necessary to do in the first pass of implementing those features), we could replace all the nsRuleNode::Compute*Data functions with a data structure that contains the parameters passed to those function calls, which would let us then replace the Compute*Data functions themselves with just one or two functions.  But doing that isn't necessarily even a blocker for calc(), attr(), etc.

(I think replacing all the Get*Data functions is already filed as a separate bug, and is mostly separate from this.)

Comment 6

[Xidorn Quan [:xidorn] UTC+11]

nsRuleNode is gone.

Comment 7

[Florian Rivoal]

Could you go through the open bugs that depended on this, and hopefully remove the dependency (or say something about what we can/should do if it cannot be removed)?

Having open bugs depend on a WONTFIX bug isn't great…