[webkit-reviews] review requested: [Bug 4980] CSS2: Counters not supported : [Attachment 10857] Most newest...est

[bugzilla-request-daemon at macosforge.org]

Beth Dakin <bdakin at apple.com> has asked  for review:
Bug 4980: CSS2: Counters not supported
http://bugs.webkit.org/show_bug.cgi?id=4980

Attachment 10857: Most newest...est
http://bugs.webkit.org/attachment.cgi?id=10857&action=edit

------- Additional Comments from Beth Dakin <bdakin at apple.com>
Okay here is a new patch. A few comments on things I did NOT address:

1. I decided not to address overflow at all. I even removed a few lines of code
that I had previously added to RenderStyle.cpp to address it. It seems like we
don't really gain anything by addressing overflow with counters. Even if we
give 0 or 1 instead of some ridiculous number, it will still be incorrect.
Firefox doesn't do anything to handle overflow. In general, it seems like we
would have to patch this code in a lot of places to really make sure we were
always returning 0/1 in the overflow case, and for very little gain. It's not
that I mind putting in the effort (I am happy to if anyone disagrees), it's
that checking for overflow in all of these places sort of clutters up the code
making it harder to read, etc. In the end it just doesn't seem like we gain
much.

2. Darin, I did not address your first comment about hadCounter()/findCounter()
because I was confused and don't think I understand what you mean. We can talk
about this in person if you want me to do it still.

3. I ran into a bunch of crashes when I had the CounterNode destructor handle
removing the nodes. It would crash most often in deletAllValues(). I am not
entirely sure why, and it wasn't consistent. It seems to be timing-related, so
I just left the removal in RenderObject::destroy()

4. I did not do this:

+	     m_item = convertValueToType(value, total,
(EListStyleType)m_counter->listStyle()) 
+		 + m_counter->separator() + m_item;

How about a += here instead of an =? Does this handle the overflow case? Is
that needed?

because += makes things be in the wrong order.

Okay, I think that's all I wanted to mention...