[webkit-reviews] review requested: [Bug 31723] CSS Counter Nesting still does not work according to the spec. : [Attachment 44101] Proposed Patch

[bugzilla-daemon at webkit.org]

Carol Szabo <carol.szabo at nokia.com> has asked  for review:
Bug 31723: CSS Counter Nesting still does not work according to the spec.
https://bugs.webkit.org/show_bug.cgi?id=31723

Attachment 44101: Proposed Patch
https://bugs.webkit.org/attachment.cgi?id=44101&action=review

------- Additional Comments from Carol Szabo <carol.szabo at nokia.com>
I have addressed all style related comments that Darin made.
I did not address the readability issue as my understanding is that Darin's
concern not withstanding the current form is acceptable and I wasn't able to
find a solution that would make the code significantly smaller and more
readable.

One thought that I had was to codify in all 7 decision points as bits in an
offset and build a decision vector of chars, where the 8 bits would represent
the about 8 different actions in the algorithm. This way the code size would be
probably the same, maybe somewhat smaller, the speed might be slightly lower,
but probably not significantly, 90% of the code would be easy to read, the
entire complexity of the algorithm being packaged in the decision vector which
would be as hard to figure out as the current code is to read if not harder
hence in the end I decided against this solution.

If I have not made my self clear above here is what I had in mind:

const char ActionSetParentToCurrent=1;
const char ActionSetParentToCurrentsParent=2;
const char ActionSetPreviousSiblingToCurrent=4;
const char ActionReturnParent=8;
.....

const char DecisionIsReset=1
const char DecisionIsCurrentReset=2
const char DecisionCurrentAndOwnerSiblings=4;
const char DecisionCurrentIsEndOfSearch=8;
.....

const char decisionVector[128] = {
    0,
    0,
    ActionSetPreviousSiblingToCurrent,
    ActionSetPreviousSiblingToZero,
    ActionSetPreviousSiblingToZero | ActionSetParentToCurrent,
    ActionSetPreviousSiblingToZero | ActionSetParentToCurrent | ReturnParent,
.....
};
 while(currentRenderer) {
  current = makeCounterNode(...);
  action = decisionVector [ DecisionIsReset * isReset + DecisionIsCurrentReset
* current->actsAsReset() + ... ];
  if (action & ActionSetParentToCurrent)
      parent=current;
  if (action & ActionSetPreviousSiblingToCurrent)
      previousSibling=current;
  if (action & ActionReturnParent)
      return parent;
.....
  if (previousSibling)
      currentRenderer = previousSiblingOrParent(currentRenderer);
  else
      currentRenderer = currentRenderer->previousInPreOrder();
}
return false;