[Webkit-unassigned] [Bug 9508] JavaScript Comparing a form element group to itself returns false.

[bugzilla-daemon at webkit.org]

http://bugs.webkit.org/show_bug.cgi?id=9508


darin at apple.com changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #12780|review?                     |review-
               Flag|                            |




------- Comment #12 from darin at apple.com  2007-01-30 13:52 PDT -------
(From update of attachment 12780)
Good work! It looks like you're on the right track.

If you're moving the code you should probably clean things up a little bit.
Details follow.

The patch has a combination of 4-character and 2-character indentation. Better
to do 4-character consistently.

+    DOMNamedNodesCollection(ExecState *exec, const Vector<RefPtr<Node> >&
nodes, const AtomicString& name);

Just ExecState* here, no space before * and no need for the variable name exec.
Also no need to give the Vector the name "nodes".

+    static JSValue *lengthGetter(ExecState* exec, JSObject *, const
Identifier&, const PropertySlot& slot);

JSValue* and JSObject*. No need for the name "exec" or "slot".

+    Vector<RefPtr<WebCore::Node> > m_nodes;

No need for the WebCore:: prefix. Probably best to use a typedef for the node
vector type.

+    AtomicString propertyName;

Should call this m_propertyName. I'm not sure why this is an AtomicString. The
only role for this is to be a key for the hash table, and the hash table uses a
StringImpl* rather than an AtomicStringImpl*. We really can go either way -- if
the strings are unique each time, then AtomicString doesn't add too much value.

To avoid aliasing during the lifetime of the DOMNamedNodesCollection, we have
to do something to keep its node alive. Thus I think it needs to hold a RefPtr
to the node it was made from, not just the nodes in the collection. Without
this, the %p technique we are currently using is not sufficient to prevent
collisions.

+typedef HashMap<StringImpl*,DOMNamedNodesCollection*> NamedNodesCache;

Need a space after the comma.

+NamedNodesCache& getNamedNodesCache()

Need to mark this static so the function gets internal linkage.

+    // Mangle the identifier string with the node pointer to the first
element.

This approach is ugly. If we really need a key that is a string plus a node
pointer, we can do that. HashMap allows arbitrary keys and I can help you make
a appropriate hash function. But a simpler thing that will also work is to use
a hash of hashes -- a primary hash table that uses the node pointer as a key
and the values are pointers to hash tables that use a string as a key.

+    char ptrBuffer[15];

Strange choice of length here. On 64-bit platforms, the hex form of a pointer
will typically be 18 bytes long, so you'd need a 19-byte buffer, and on 32-bit
it will typically be 10 bytes long and require an 11-byte buffer. So 19 or 20
would make more sense.

+        getNamedNodesCache().add(tempString.impl(), nodeCollection);

This line of code is incorrect. The tempString will go away, and its
StringImpl* might be deallocated. What you need to use as the hash key is the
pointer to the string that's actually stored inside the DOMNamedNodesCollection
object. You'll probably need a member function named propertyName() to get at
it.

+  DOMNamedNodesCollection *thisObj =
static_cast<DOMNamedNodesCollection*>(slot.slotBase());

Should move the * next to the type.

Ideally we'd move DOMNamedNodesCollection to its own source file. We're trying
to phase out these kjs_xxx files. Maybe even use an idl to generate its
JavaScript wrapper; we could at least auto-generate length even though we'd
still need custom code for getting the array index and by-name properties.

review- because of the need to ref the node and the incorrect call to add. I'd
really like to see a version that does not use %p.


-- 
Configure bugmail: http://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.