[Webkit-unassigned] [Bug 20928] Speed up JS property enumeration by caching entire PropertyNameArray

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=20928





------- Comment #3 from sam at webkit.org  2008-09-19 16:40 PDT -------
(In reply to comment #2)
> (From update of attachment 23556 [edit])
> JSPropertyNameIterator::invalidate is so simple now it should probably be
> inlined. Maybe also use clear() instead of assigning 0?
> 
> +    , m_data(0)
> 
> Don't need that for RefPtr.

Fixed.

> 
> +inline JSPropertyNameIterator::JSPropertyNameIterator(JSObject* object,
> PropertyNameArrayData* propertyNameArrayData)

Fixed.

> 
> Should PropertyNameArrayData* should be PassRefPtr, since this takes ownership.
> 
> +        void setCachedPrototypeChain(StructureIDChain* cachedPrototypeChain) {
> m_cachedPrototypeChain = cachedPrototypeChain; }
> 
> +        void setData(PropertyNameArrayData* data) { m_data = data; } 
> 
> Should be PassRefPtr since these take ownership.

I don't think this is true, because the ownership is actually shared.

> 
> +        if ((*a).get() != (*b).get())
> +            return false;
> 
> These would read better with -> but can't you just omit the get() altogether?
> 
> +        if (!(*a).get())
> +            return true;
> 
> This would read better with -> but can't you just omit the get() altogether?

Fixed

> 
> +    ASSERT_NOT_REACHED();
> +    return false;
> 
> This will just create a warning on Windows and make the build fail. Leave it
> out.
> 

Fixed.

> +    // FIXME: This breaks the optimization for the JSGlobalObject. 
> 
> I think you should write a clearer comment. Does this really need a FIXME?

Removed.

> 
> I'm going to say review- because of the number of actionable comments.
> 


-- 
Configure bugmail: https://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.