[webkit-reviews] review granted: [Bug 21152] Speedup static property get/put : [Attachment 23858] patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Fri Sep 26 15:29:00 PDT 2008
Darin Adler <darin at apple.com> has granted Sam Weinig <sam at webkit.org>'s request
for review:
Bug 21152: Speedup static property get/put
https://bugs.webkit.org/show_bug.cgi?id=21152
Attachment 23858: patch
https://bugs.webkit.org/attachment.cgi?id=23858&action=edit
------- Additional Comments from Darin Adler <darin at apple.com>
+++ JavaScriptCore/kjs/PropertySlot.h (working copy)
Please take out the forward declaration of the HashEntry struct.
I'd love to see the actual functions (getRightContext) inlined as much as
possible. It seems a shame to pay an extra function call for these little
"trampolines".
+ void setInput(const UString&);
These look indented wrong. What happened?
+ // FIXME: other than sharing code, there is no reason this get function
can't be simpler.
I think that "other than sharing code" is unnecessarily vague here. You need to
be more specific about what is being shared.
-JSValue* nonCachingStaticFunctionGetter(ExecState* exec, const Identifier&
propertyName, const PropertySlot& slot)
I think you forgot to remove the declaration in the JSDOMBinding.h header.
+ if
(!static_cast<JSDOMWindowBase*>(slot.slotBase())->allowsAccessFrom(exec))
+ return jsUndefined();
+ return static_cast<JSDOMWindowBase*>(slot.slotBase())->getListener(exec,
mousemoveEvent);
These could all be implemented by a call to a single inline helper instead of
repeating this code over and over again.
+void setJSDOMWindowBaseEvent(ExecState*, JSObject*, JSValue*)
+{
+}
These empty functions probably need a comment to explain why they're here but
empty.
+ void setListener(JSC::ExecState*, const AtomicString& eventType,
JSC::JSValue* function);
private:
I'd prefer to always have blank lines before private. I've noticed others
recently deleting them, but I want them visually grouped with whatever comes
after the private, not the things before.
+static JSValue* jsEventTargetNodeOnAbort(ExecState*, const Identifier&, const
PropertySlot&);
You could use JS_EVENT_LISTENER_FOR_EACH_LISTENER to avoid having to write
these all out.
+ # FIXME: this casts to int to match our previous behavior which
turned 0xFFFFFFFF in -1 for NodeFilter.SHOW_ALL
You probably mean "into" here. Also, is there a cleaner way to handle this?
+ push(@implContent, " { \"$key\", @$specials[$i],
(intptr_t)@$value1[$i], (intptr_t)@$value2[$i] },\n");
C++ casts instead?
+CONSOLE MESSAGE: line 1: Unsafe JavaScript attempt to access frame with URL
http://localhost:8000/security/resources/cross-frame-iframe-for-put-test.html
from frame with URL http://127.0.0.1:8000/security/cross-frame-access-put.html.
Domains, protocols and ports must match.
What? Why no explanation in the ChangeLog.
r=me -- all comments above optional
More information about the webkit-reviews
mailing list