Bug 4980 – CSS2: Counters not supported

• Component: CSS
• Status: RESOLVED FIXED

[Mac-arena the Bored Zo]

test case:
ul#bookmarksbar li:after { float: right; content: "\2318" counter(cmdkey, decimal); counter-increment: 
cmdkey; }

expected behaviour:
the li has the place of interest sign followed by a number after it.

current behaviour:
the li has nothing after it.

work-around:
remove the counter. any content: not including a counter works.

[Mac-arena the Bored Zo]

(oops, meant to set the priority to P3, sorry)

[Mac-arena the Bored Zo]

OK, I think I finally have this bug set up correctly now. sorry about all the email :)

[Mac-arena the Bored Zo]

confirmed in CVS as of end-of-day, 2005-09-15, PDT.

[Joost de Valk (AlthA)]

Confirmed, adding testcase in a sec. Reassigning to webkit-unassigned

[Joost de Valk (AlthA)]

Created attachment 5354 [details]
Testcase

[Beth Dakin]

Created attachment 10717 [details]
Preliminary counter implementation

Here is a patch to implement counters. It is based largely on the KHTML implementation. A few things about this patch:

1. There are definitely still bugs with counters. There are a bunch of great counters test cases at http://dbaron.org/css2.1/tests/imported-20050702/ I plan to check in a bunch of them as layout tests, and file bugs on all of the ones that don't yet work.

2. List items should be re-written to be rendered with counters. KHTML does this, and I have a patch that would make them switch over. I do not want to switch them over yet, though, because of the bugs that still exist in this counters patch (see above!). I will file an additional bug for this switch. It doesn't seem worth it to do the switch now and introduce regressions into lists when we can just switch it later when counters are more robust.

3. I don't think the test case attached to this bug is valid. Unless I am mis-reading the spec, of course. Anyway, as I mentioned before, there are lots of great tests at http://dbaron.org/css2.1/tests/imported-20050702/

[Dave Hyatt]

I definitely think switching lists over to counters can be a post-Leopard thing. :)

[Darin Adler]

Comment on attachment 10717 [details]
Preliminary counter implementation

The part of this where we add 4 specific functions to the base class Node to distinguish ordered list, unordered list, menu, and directory elements seems wrong to me.

Instead I think these functions should be higher level, and not on Node (perhaps on HTMLElement or Element). When I say higher level I mean a function like resetsCounter().

If all we want to do is check the type of element, we can use hasTagName(olTag) which is the style used in all the rest of the code.

In the code for the ordered list element, we should be using the HTMLOListElement::start() function instead of calling getAttribute.

I haven't reviewed other aspects of this patch yet, but this is an immediately-obvious design mistake.

[Dave Hyatt]

Need to implement getComputedStyle stuff for counters it looks like.

[Beth Dakin]

Created attachment 10763 [details]
Patch with minor adjustments

Here is a patch that addresses Darin's comments. It also adds computed style information for counter-increment and counter-reset. I didn't do anything for actual counters since they are a part of the content property, and we have not yet done anything to compute style for the content property. Seems like taking care of the whole content property may be out of the scope of this bug, but if anyone disagrees, I am happy to take care of it.

[Beth Dakin]

Okay, I filed a whole bunch of bugs for the test cases that still fail. Specifically,

http://bugzilla.opendarwin.org/show_bug.cgi?id=11028
http://bugzilla.opendarwin.org/show_bug.cgi?id=11029
http://bugzilla.opendarwin.org/show_bug.cgi?id=11030
http://bugzilla.opendarwin.org/show_bug.cgi?id=11031
http://bugzilla.opendarwin.org/show_bug.cgi?id=11032
http://bugzilla.opendarwin.org/show_bug.cgi?id=11033
http://bugzilla.opendarwin.org/show_bug.cgi?id=11034
http://bugzilla.opendarwin.org/show_bug.cgi?id=11035
http://bugzilla.opendarwin.org/show_bug.cgi?id=11036

[Darin Adler]

Comment on attachment 10763 [details]
Patch with minor adjustments

+    CounterNode *newNode = lookupCounter(counter);

Need to move the * on that one.

The function named getCounter I would name findCounter. I find it very difficult to understand the "view" and "counters" parameters to that function. Perhaps they need different names.

I don't understand why a linked list is a good data structure for something we are looking up by name.

The function destroyCounters walks the linked list in what seems to be a slightly obfuscated way.

The class CounterList is not well named for a linked list node. I think it should be named CounterListNode perhaps.

Why does CounterList have one member named m_counterNode and two others named counter and next. It's a brand new class so its members should be named consistently.

Why does this brand new code use DeprecatedString? Performance will be better if it doesn't.

+    RenderCounter(Node* node, CounterData* counter);
+    CounterResetNode(RenderObject* o);

We omit parameter names in cases like this.

+#ifndef _Counter_Reset_Node_h_

Names with a leading underscore like this are reserved for the implementation. Please leave it off.

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

We don't put spaces before parentheses like this.

In RenderCounter.h:

+#include "config.h"
+#include "CounterNode.h"
+#include "CounterResetNode.h"
+#include "RenderCounter.h"
+#include "RenderStyle.h"

We put the include of the file's own header first to make sure it compiles standalone. So RenderCounter.h needs to be first before the other headers.

+using namespace WebCore;

The entire file implements something in namespace WebCore, so it should be inside a namespace WebCore declaration (with braces), not after a "using" declaration.

+// -------------------------------------------------------------------------

I don't think this long line adds anything.

+CounterNode::CounterNode(RenderObject *o)
+    : m_hasCounters(false), m_isVisual(false),
+      m_value(0), m_count(0), m_parent(0), m_previous(0), m_next(0),
+      m_renderer(o) {}

Strange formatting here.

I think the counter nodes and counter list nodes should be arena-allocated like the rest of the render tree.

+                String v = static_cast<Element*>(element())->getAttribute("value");
+                if (!v.isEmpty()) {

The above code is unnecessarily inefficient. Instead it should use const AtomicString& for v.

\ No newline at end of file

Should not have any of those.

+        if(!value->isValueList())

I see a lot of if statements without spaces after the "if" like this one.

Sorry, I didn't get through every last bit of the patch, but those are my comments so far.

[Beth Dakin]

Created attachment 10788 [details]
Patch the addresses Darin's comments, but leaks

So here is a patch that, I believe, addressess all of Darin's comments. However, I am leaking CounterNodes all over the place. I think that I need to do something to make the HashMap clean them up correctly. Since there is so much other stuff in this patch too, I thought I'd post it anyway while I am working on that.

[Beth Dakin]

Created attachment 10793 [details]
better

This should take care of the leaking...

[Darin Adler]

Comment on attachment 10793 [details]
better

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?

[Beth Dakin]

Created attachment 10821 [details]
Patch for Darin's most recent comments

Here is a patch that addresses nearly all of Darin's most recent comments. I may have missed a few, but I will take a cleaner pass at it in the morning. One thing I know for sure is that I haven't yet addressed any of the potential overflow issues. I am hoping to talk with Darin about that in person since I am not sure how best to deal with it. Also, instead of adding a HashMap to the RenderView that keeps track of all of the objects and their CounterNodeMaps, I made a static map in RenderObject. This seems more intuitive to me. Anywaym here is what I got.

[Darin Adler]

Comment on attachment 10821 [details]
Patch for Darin's most recent comments

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().

[Beth Dakin]

Created attachment 10831 [details]
another round

This patch addresses Darin's last round of comments. I am pretty sure that I have addressed everything, thought I could be wrong. In Counter.h, I left the listStyle as an integer because I check to make sure it is a valid list style number from within the parser. Perhaps that is insufficient, though? What do you think?

(Oh, and Darin, just so you know, I fixed that crazy crasher that I showed to you earlier today, so that is all set. It was utterly bizarre; I will tell you about it later.)

[Beth Dakin]

Eep! I just realized I forgot to finish taking care of the overflow stuff. Investigating now.

[Beth Dakin]

Hmm...looks like Firefox just lets everything overflow and gives totally bogus values. I wonder what behavior we want...

[Darin Adler]

Comment on attachment 10831 [details]
another round

Looking good!

I checked, and every use of RenderObject::hasCounter is immediately followed by a call to RenderObject::findCounter. Perhaps we should just add yet another boolean parameter to findCounter to determine whether or not it should create a counter if it doesn't already exist. Then we could eliminate hasCounter entirely.

Is there a way to avoid recounting everything in the tree as the tree is initially parsed and when the tree is destroyed and the entire document is going away? I'm worried that there's too much recounting.

+    if (pos >= m_length) {
+        append(*str);
+        return;
+    }

That needs to be append(str, length), not append(*str), which will append only the first character!

+        int newlen = m_length + length;

newlen should be a size_t rather than an int.

+static RenderObjectsToCounterNodeMaps* getRenderObjectsToCounterNodeMaps()

This could return a reference instead of a pointer. Either way is OK.

+                counterNode->remove();
+                delete counterNode;
+                counterNode = 0;

1) No reason to set counterNode to 0.

2) Why doesn't the CounterNode destructor handle the counterNode->remove() part? If it did, we could go back to writing this more simply using deleteAllValues.

+    int val = 0;
+    RenderObjectsToCounterNodeMaps* objectsMap = getRenderObjectsToCounterNodeMaps();

The definition of "val" should really be moved down to just before it's first used. I also think it would read beter if the values of the nodes were set where they are allocated as needed instead of sharing a single call to setValue().

+        CounterNodeMap* counterNodesMap = objectsMap->get(this);
+        if (counterNodesMap)
+            newNode = counterNodesMap->get(counterName);

In general, I like putting the definition inside the if statement in all the cases like this, but it's an arguably matter of style.

+    objectsMap->set(this, nodeMap);

This line should go inside the if statement. It's only needed for a newly-created node map.

+        RenderObject* n = !isListItem() && previousSibling() ? 
+            previousSibling()->previousSibling() : previousSibling();

I think this reads better with the ? on the start of the second line.

+        while (n) {
+            if (n->hasCounter(counterName)) {
+                current = n->findCounter(counterName);
+                break;
+            } else
+                n = n->previousSibling();
+        }

I would have written this as a for loop:

    for (; n; n = n->previousSibling())
        if (n->hasCounter(counterName)) {
            current = n->findCounter(counterName);
            break;
        }

I think that makes it a little easier to see what's going on.

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

This local variable needs a better name. I can't see how a boolean could be named "counters"!

+            // Found render-sibling, now search for later counter-siblings among its render-children

I suspect that the loop below this comment could be written much more simply by using previousInPreOrder or a similar function. Not sure though.

I noticed this comment:

    // note: do not add unnecessary bitflags, we have 32 bit already!

You should remove it -- it's inaccurate!

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

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

You should remove the typecasts since they're not needed any more.

+            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?

+    str = new StringImpl(m_item.characters(), m_item.length());

Can this be str = m_item.impl()->copy() instead?

+    , m_next (0)

Stray space here.

The CounterNode::recountAndGetNext does not seem to handle overflow. Is that OK?

+        assert(newChild->m_next->m_previous == refChild);

Should use ASSERT, not asssert.

+        assert (m_last == refChild);

Should use ASSERT and omit the extra space.

+// Implementation of counter-increment and counter-content

What good is that comment?

+    bool hasSeparator() const { return m_hasSeparator; };
+    bool willNeedLayout() const { return m_willNeedLayout; };
+    void setWillNeedLayout() { m_willNeedLayout = true; };
+    void setRenderer(RenderObject* o) { m_renderer = o; };
+    RenderObject* renderer() const { return m_renderer; };

Remove incorrect trailing semicolons here, please.

+        ASSERT(pair);

Should move this up one line so we assert before we dereference pair (two places in RenderStyle).

+    RefPtr<CSSValue> parsedValue;

Why not put parsedValue back inside the loop where it used to be? Is there a reason to move it out? To avoid a tiny bit of reference count churn, the code below this should be append(parsedValue.release()) instead of just append(parsedValue). Same with the call to addProperty, which should pass values.release() instead of values.

These comments are all about optional cleanup and obscure edge case questions except for the append(*str) mistake, but I need to do a review- because of that mistake.

[Beth Dakin]

Created attachment 10857 [details]
Most newest...est

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...

[Darin Adler]

Comment on attachment 10857 [details]
Most newest...est

+        append(new StringImpl(str, length));

This is no good. It leaks a StringImpl!

Instead, append(str, length) should work fine.

[Darin Adler]

(In reply to comment #22)
> 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 =?
> 
> because += makes things be in the wrong order.

If you're relying on the order, then you may need separate statements. In an assignment statement there's no guarantee that the different parameters to the + are evaluated from left to right. I don't see any ordering guarantees you'd get from + that you wouldn't get from += so I don't understand the comment.

Anyway, not a big issue. The only thing that needs fixing is that append and then I think you can land.

[Beth Dakin]

Created attachment 10869 [details]
Yet another patch

Fixed the bug in StringImpl...will fix it better after I check in. Did some additional clean-up.

[Beth Dakin]

Comment on attachment 10869 [details]
Yet another patch

Wait! Ignore this patch! It crashes! BRB!

[Beth Dakin]

Created attachment 10870 [details]
Okay, this one!

Okay, fixed the crash from the last one. This one should be good.

[Darin Adler]

Comment on attachment 10870 [details]
Okay, this one!

r=me

[Beth Dakin]

I committed this with r16721