[webkit-reviews] review granted: [Bug 14848] DOM table rules are not updated when changed : [Attachment 15878] Different approach

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Rob Buis <rwlbuis at gmail.com>'s
request for review:
Bug 14848: DOM table rules are not updated when changed
http://bugs.webkit.org/show_bug.cgi?id=14848

Attachment 15878: Different approach
http://bugs.webkit.org/attachment.cgi?id=15878&action=edit

------- Additional Comments from Darin Adler <darin at apple.com>
I think setTableCellsChanged could be written so it's not recursive, but I
guess it's OK as-is. I do worry about adding more and more recursive algorithms
that could be a problem if someone makes a deeply nested DOM tree, but Hyatt
tells me not to worry.

+    bool result = false;

I think "cellChanged" might be a better name for this local variable. Also, I'd
put a space after this to match the paragraphing of the rest of the function.

+    if (n->hasTagName(theadTag) || n->hasTagName(tbodyTag) ||
+	 n->hasTagName(tfootTag) || n->hasTagName(trTag) ||
+	  n->hasTagName(thTag)) {

I'd prefer a helper function for this (inline if necessary) -- it's hard to
read a long list like this.

+	 for (Node *child = n->firstChild();child;child = child->nextSibling())
{
+	     result |= setTableCellsChanged(child);
+	 }

There should not be space between Node and the * after it. There should be
spaces after the semicolons. There should not be braces around the single line
block inside the loop.

+    } else if (n->hasTagName(tdTag)) {
+	 result = true;
+    }

We normally don't put braces around single line statements like this one.
Putting the tdTag case first before the other tags might make the formatting
easier to read.

What about cases where we don't have actual <td> elements, but rather things
styled as table-cell by CSS. Does this bug happen in that case?

r=me anyway, even though I had a lot of questions.