[webkit-reviews] review denied: [Bug 4980] CSS2: Counters not supported : [Attachment 10793] better

[bugzilla-request-daemon at opendarwin.org]

Darin Adler <darin at apple.com> has denied Darin Adler <darin at apple.com>'s
request for review:
Bug 4980: CSS2: Counters not supported
http://bugzilla.opendarwin.org/show_bug.cgi?id=4980

Attachment 10793: better
http://bugzilla.opendarwin.org/attachment.cgi?id=10793&action=edit

------- Additional Comments from Darin Adler <darin at apple.com>
Another round of comments:

Is counter support really worth 4 bytes in every single RenderObject? Perhaps
we could put the counters into a per-document hash table (maybe in the
RenderView object) and save the space in the RenderObject instances at the cost
of a hash table lookup to find the counter. The main cost here would be time in
the RenderObject destruction code to look for a counter entry in the hash
table. That cost can be offset either by making sure it doesn't run when the
entire tree is destroyed (the "document being destroyed" optimization) or by
devoting a bit to "has an entry in the per-document hash table". A bit is
probably more affordable than the 32 bits we are spending on it in this patch.

+    CSSValueList* counterResetList;
+    CSSValueList* counterIncrementList;

Can we afford these 8 bytes in every RenderStyle? Are there any alternatives?

My suggestion to arena-allocate CounterNode objects is incompatible with the
use of deleteAllValues in the RenderObject destructor, so either we have to
back off on the arena allocation or write our own code to delete all the values
that uses arenaDelete. Since the hash map itself won't be in the arena, I think
the use of a hash might eliminate the need for use of arena allocation.

+    virtual ~RenderCounter() {};

No semicolon is needed for a function like this. But more importantly there's
no reason to explicitly declare a destructor if it's just going to be empty in
the header. That's the same behavior you get if you just leave out the explicit
destructor definition altogether.

+class RenderCounter : public RenderText
+{

We put the brace on the same line as the class declaration, not its own line.

+protected:

We should always use private rather than protected, unless we have a derived
class that needs access to some of these fields.

+    String toListStyleType(int value, int total, EListStyleType type);

I think we could come up with a better name for this function. For one thing,
it doesn't produce a "list style type". I think we need a verb here or a name
that describes the result. Also no need for the name "type" in the declaration,
since EListStyleType speaks for itself.

+    virtual void recount(bool first = false);

It's quite unclear what the "first" boolean means. We need to give this
parameter a better name.

CounterListItem.h includes RenderListItem.h, but it shouldn't. It should
include CounterNode.h.

+    : RenderText(node,0), m_counter(counter), m_counterNode(0)

Need a space after the comma.

+const int cMarkerPadding = 7;

I think constants should be at the top of the file, not amidst the functions.

+		 c = UChar(digits[d]);
+		 roman.insert(String(&c, 1), 0);

No need for the conversion to UChar here; in fact we don't need the local
variable "c" at all since digits[d] is already a UChar.

I'd like to see insert overloaded to take a UChar so we don't have to contruct
a string every time. This function is going to be really slow the way it's
written, in fact, so we might want to think about the idiom for building up a
String. For example, we could build a Vector<UChar> and then reverse it before
making the string to avoid the expensive prepend operation.

+static String toLetterString(int number, int letterA)

The "letterA" parameter should be a UChar not an int.

+	 c = UChar(1511 + 3);
+	     c = UChar(0x2022);

Most of the UChar casts are unnecessary. A plain integer an be assigned to a
UChar without a cast.

+	 letter += String(&c, 1);

There's an append on String that takes a UChar already -- we should use that
instead of constructing a string.

+	     // ### unsupported, we use decimal instead

The "###" is the KDE way of doing what we do with FIXME. Lets use FIXME.

+    bool counters;
+    counters = !m_counter->separator().isNull();

Should merge these into one line.

+struct CounterData {
+    CounterData() {}
+    CounterData(String i, int l, String sep) :ident(i), ls(l), s(sep) {}
+    ~CounterData() {}

The parameters should be const String&, not String. There's no need to
explicitly define the destructor. And given that the list style enum is defined
in this same file, there's no justification for using "int" here. It should be
EListStyleType instead.

+    String identifier() { return ident; }

There's no point in having these accessor functions for a struct where the data
members are also public. Either we should make the data members private or omit
the accessor functions.

+CounterNode::~CounterNode() {};

Should omit this bogus semicolon and possibly the entire destructor definition.


+CounterNode::CounterNode(RenderObject *o) 

I'd like to see member initialization syntax used here instead of assignment
statements.

CounterNode::setHasCounters and CounterNode::recount use recursive
implementations instead of iterative. We should use iterative implementations
instead for brand new code like this.

+    short counterIncrement; //ok, so these are not visual mode spesific
+    short counterReset;     //can't go to inherited, since these are not
inherited

May as well fix the comment formatting (no spaces after //) as long as you're
touching these lines. And also fix the spelling.

+    void setContent(CounterData* c, bool add = false);
+    bool counterDataEquivalent(RenderStyle* otherStyle);
+    void setCounterReset(CSSValueList* v);
+    void setCounterIncrement(CSSValueList* v);
+    void addCounterReset(CSSPrimitiveValue* c);
+    void addCounterIncrement(CSSPrimitiveValue* c);
+    bool hasCounterReset(const String& c) const;
+    bool hasCounterIncrement(const String& c) const;
+    short counterReset(const String& c) const;
+    short counterIncrement(const String& c) const;

Should omit most of these variable names, although for some of the ones named
"c" it would be helpful to use a name like "counterName".

I'm disappointed that we're writing yet another tree implementation for
CounterNode. Functions like CounterResetNode::removeChild are tricky to write
correctly and we've had buts in such functions in the past. I wonder if there's
an alternative. Do we really need the efficient removal you get by using a
tree? Could we consider using vectors of children instead? Those are more
efficient for most other operations.

+	 CounterNode* n = firstChild();
+	 for(; n; n = n->nextSibling())
+	     n->setParentDirty();

That should be written with the variable inside the for loop, not just before
it. But this is another example of a recursive algorithm where I'd like to see
iterative ones.

In RenderContainer.cpp I can't figure out of the style leaks in some code
paths. Are you sure it doesn't?

As far as I can tell, there is no use of the functions that remove elements
from the counter tree. Is that right? If so, can we omit this dead code?

+    bool isRoot() { return m_renderer && m_renderer->isRoot(); };

This should be const.

+    bool m_hasCounters : 1;
+    bool m_isVisual : 1;

I'm not sure using bit fields here is the right tradeoff.

+    int value() const { return m_value; }
+    int count() const { return m_count; }

Why do these return int when the member itself is a short? In general, I'm
concerned when I see short that there's an issue with values that fit in an int
and don't fit in a short. Does this code handle that right?

I don't understand the RenderStyle copy constructor. Why is it OK to just copy
things like the content, counterResetList, and counterIncrementList pointers
without making a copy or doing some sort of ref counting. I think that works
only if they're always 0?

Don't counterResetList and counterIncrementList have to be looked at in
functions like RenderStyle::diff?

Why doesn't ContentData::clearContent need to do something with the old counter
like it does with text? Who owns the counter object?

+    // ### Should we compare content?

Again, we use FIXME for this rather than ###. I'd also like a more intelligent
comment about this.

+	     total += (short)counterValue->getFloatValue();

Where's the guarantee this won't just overflow?

Why aren't we using RefPtr for counterResetList and counterIncrementList?

Also, setCounterReset and setCounterIncrement should take PassRefPtr
parameters.

I think it's very confusing that setCounterIncrement takes a list and
addCounterIncrement adds a counter to that list. What's the list's name and
what is the names of the items in that list? We should consider that when
naming these functions. Same for setCounterReset and addCounterReset.

+	 bool parseCounter(int propId, bool increment, bool important);

I don't think a bool is very good here, because it's unclear what false means.
I think the name "increment" here is also unclear because it's a verb!

+		 CounterData counter =	CounterData(counterValue->identifier(),

+		     counterValue->listStyleNumber(),
counterValue->separator());

Should probably use constructor syntax here instead of assignment to avoid
repeating the type name twice.

+	 CSSValueList* list = static_cast<CSSValueList *>(value);
+	 style->setCounterIncrement(list);

Merging this into one line would be clearer, I think, and avoid the need for
braces around the code.

Since Counter has no classes derived from it, I don't think it needs a virtual
destructor. The code generated will be more efficient if there are no virtual
functions in the class.

+    String identifier() const { return m_identifier.get() ?
m_identifier.get()->getStringValue() : String(); }

I think there's too much get here. Does it compile like this?

+    String identifier() const { return m_identifier ?
m_identifier->getStringValue() : String(); }

If so, please do that.

+    delete identifier;
+    delete separator;
+    delete listStyle;

+    delete counterName;
+    delete numValue;
+    delete list;

These are wrong. The objects are reference-counted, and deleting them is not
the correct way to deal with those. Instead the variables should be RefPtr and
we can get rid of "goto invalid" and change those sites to instead say "return
0".

Since parseCounterContent returns a new object, it should return a
PassRefPtr<CSSValue> instead of a CSSValue*.

+    if (counters || (args->size() != 1 && args->size() != 3))
+	 if (!counters || (args->size() != 3 && args->size() != 5))
+	     goto invalid;

This is twisted logic, and args->size() can be really expensive to compute.
Lets rewrite this to be human-readable and not call args->size() over and over
again.

+		     i = (short)val->fValue;

What about values that don't fit in a short?