[webkit-reviews] review denied: [Bug 4980] CSS2: Counters not supported : [Attachment 10821] Patch for Darin's most recent comments

[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 10821: Patch for Darin's most recent comments
http://bugzilla.opendarwin.org/attachment.cgi?id=10821&action=edit

------- Additional Comments from Darin Adler <darin at apple.com>
Looks good. Here's a new round of comments:

To get the performance boost from String::insert we really need to implement a
StringImpl::insert too, but that can wait until another patch.

+    static RenderObjectsToCounterNodeMaps* _renderObjectsToCounterNodeMaps;

No need for an underscore here. It's a local variable so it can have a nice
simple name. Also, since it's a local it can just be a HashMap instead of a
pointer to a HashMap, simplifying the function.

Should also have a space between the typedefs and the functions.

I still think that the "view" and "counters" parameters of findCounter are hard
to understand. Maybe we could change their names.

I think the counter parameters to hasCounter and findCounter should be
counterName instead.

+class CounterResetNode : public CounterNode
+{

We put braces on the same line.

+    virtual void insertAfter (CounterNode* newChild, CounterNode* refChild);

No space needed after the function name.

+    virtual bool isReset() { return true; };

I think isReset() should be a const member function.

+    str = new StringImpl((static_cast<String>(m_item)).characters(),
m_item.length());

Do we need that static_cast? I think this can be str = m_item.impl().copy()
instead. But is the copy() even needed?

If CounterData has all private members, then I think it should be marked class
instead of struct (just a minor style issue). Also, the members should have
nice names like m_identifier, m_listStyle, m_separator instead of ident, ls, s.


+    PassRefPtr<CSSValueList> counterResetValueList() { return
counterResetList; }
+    PassRefPtr<CSSValueList> counterIncrementValueList() { return
counterIncrementList; }

These two should not return PassRefPtr because they return a list that's owned
by the object returning. They are not trying to pass ownership to the caller.

On the other hand, the setters are correctly using PassRefPtr to express the
fact that the callee takes ownership from the caller.

+CounterResetNode::CounterResetNode(RenderObject* o) : CounterNode(o),
m_total(0), m_first(0), m_last(0) {}
+CounterResetNode::~CounterResetNode() {};

Should make this normal formatting instead of wacky formatting and consider
removing the destructor declaration since I believe it's redundant. There are
only two reason to have an empty destructor declaration in a .cpp file:

    1) In cases where some data member can't be destroyed without including
additional headers. For example, if I have a RefPtr<Node> in a class but don't
want to require clients to include "Node.h" the destructor has to be in the
.cpp file because otherwise clients trying to destroy will get errors or have
to include "Node.h".

    2) If you want to be able to add code there later without recompiling all
the files that include this one.

I don't think either of these apply to CounterResetNode.

If we had it to do all over again, we might do this tree using vectors for the
lists of children. I'm not sure a next/prev/parent/child tree is best for this.
But I don't think that's worth reworking all the code for.

I'm still not sure the use of "short" in CounterNode makes sense. It just saves
a few bytes per node and we've had trouble in the past with short vs. int
issues.

+	 ASSERT(pair && counterName);
+	 ASSERT(pair && counterName && counterValue);

These should be broken up int separate assertions. That's true of any ASSERT
that has a simple && in it -- it's better to know which failed.

+	     int totalInt = total + counterValue->getFloatValue();
+	     if (totalInt < SHRT_MAX && totalInt > SHRT_MIN)

Seems to me that this still could overflow an int. If we want to safely check
for overflow I think we need to use a floating point type. Also, this doesn't
check if getFloatValue itself overflows a short, which could happen even if the
sum doesn't overflow a short. I'd also like to see at least one overflow test
case.

+	 PassRefPtr<CSSValue> parseCounterContent(ValueList *args, bool
counters);

Star should be next to ValueList*. I don't understand what a boolean "counters"
means here. Better name?

+    void setContent(CounterData*, bool add = false);

This should take a PassRefPtr<CounterData> I believe. I still don't see how the
CounterData object gets deallocated when the RenderStyle is deallocated. I'd
like to understand who owns that object. I see it being created in
cssstyleselector.cpp and I can't see who's responsible for deallocating it.

+		 CounterData counter(counterValue->identifier()
+				   
,(EListStyleType)counterValue->listStyleNumber()
+				    ,counterValue->separator());
+		 CounterData* c = new CounterData(counter);

These should be merged into a single statement like this:

    CounterData* c = new CounterData(counterValue->identifier(),
	counterValue->listStyleNumber(), counterValue->separator());

But also you should almost never put a new object into a raw pointer. This
should either be passed directly to a function taking a PassRefPtr or perhaps
put into a local RefPtr and then passed to a function taking a PassRefPtr (and
in that case you can do a release() for efficiency too).

+	     // ### add list-style and separator

Please use FIXME, not ###.

+    int listStyleNumber() const { return m_listStyle.get() ? (int)
m_listStyle.get()->getFloatValue() : 0; }

Why is this an int? What guarantees it will be a valid list style number? No
need for .get() here.

+    RefPtr<CSSPrimitiveValue> m_identifier; //string
+    RefPtr<CSSPrimitiveValue> m_listStyle; //int
+    RefPtr<CSSPrimitiveValue> m_separator; //string

Lets put spaces after the "//" to match our usual style.

 #include "CSSBorderImageValue.h"
+#include "Counter.h"
 #include "CSSImageValue.h"

Would be better to put this in alphabetical order (I usually use case-sensitive
alphabetical order because that's what Xcode does with the Sort Selection menu
item).

+     case CSS_PROP_COUNTER_RESET:	  // [ <identifier> <integer>? ]+ |
none | inherit
+	  if (id == CSS_VAL_NONE)
+	      valid_primitive = true;
+	 else
+	     return parseCounter(propId, 0, important);
     case CSS_PROP_FONT_FAMILY:

There's a "break" missing here so we'll end up falling into the font family
code. I suggest formatting this differently to make that easier to see. Maybe:

    case CSS_PROP_COUNTER_RESET:	// [ <identifier> <integer>? ]+ | none
| inherit
	if (id != CSS_VAL_NONE)
	    return parseCounter(propId, 0, important);
	valid_primitive = true;
	break;

+		 Value *a = args->current();

The * should be next to Value, not a.

+		 parsedValue = parseCounterContent(args, false).get();

+		 parsedValue = parseCounterContent(args, true).get();

The .get() here is wrong and using it creates unnecessary ref-count churn;
PassRefPtr will pass to a RefPtr without a ref/deref, but by doing get() we
prevent that.

+		 if (!parsedValue) return false;

Lets break these up into two lines using the traditional formatting.

+	     } else
+	     if (fname == "counter(") {

+	     } else
+	     if (fname == "counters(") {

Strange "else" formatting here. Please fix.

+	 addProperty(propId, values.get(), important);

CSSParser::addProperty's value parameter should be changed to a PassRefPtr.
That would obviate this get() and be cleaner that what we have today.

+    RefPtr<CSSPrimitiveValue> identifier;

You can make the code better and more efficient by declaring this where it's
initialized. It's also better style, I think.

+    RefPtr<CSSPrimitiveValue> separator;
+    RefPtr<CSSPrimitiveValue> listStyle;

And these can be moved down to just before where they're set up (before the if
statements).

+    Counter* counter = new Counter(identifier.get(), listStyle.get(),
separator.get());
+    return new CSSPrimitiveValue(counter);

The constructor for Counter should take PassRefPtr and then you should call
release() instead of get() here. Also, it's not great style to have a new
object in a raw pointer. I think merging this into a single statement would be
best.

+    Value* val;

This should be declared where it's initialized, not outside the loop.

+    RefPtr<CSSPrimitiveValue> numValue;

Same here.

+		 Pair* pair = new Pair(counterName.get(), numValue.get());

Again, we don't want to use get() here because it creates unnecessary ref-count
churn. Instead we should call release() on both of these. And again it's not
good to have a new object that gets put into a raw pointer. Merging into one
big statement would be better. I think with the right formatting you could get
rid of the numValue local variable too and it would look fine:

    list->append(new CSSPrimitiveValue(new Pair(counterName.release(),
	new CSSPrimitiveValue(i, CSSPrimitiveValue::CSS_NUMBER))));

+		     i = (short)val->fValue;

The cast to short, and I'm not clear on why it's OK to discard bits if the
value is outside the short range.

+	 addProperty(propId, list.get(), important);

Another case of get() where what we want is release().