[webkit-reviews] review denied: [Bug 35401] Unsafe casting of Method types in bridge code. : [Attachment 49530] The patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Thu Feb 25 14:01:07 PST 2010
Alexey Proskuryakov <ap at webkit.org> has denied Gavin Barraclough
<barraclough at apple.com>'s request for review:
Bug 35401: Unsafe casting of Method types in bridge code.
https://bugs.webkit.org/show_bug.cgi?id=35401
Attachment 49530: The patch
https://bugs.webkit.org/attachment.cgi?id=49530&action=review
------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
> + if (!asObject(runtimeMethod)->inherits(&CRuntimeMethod::info))
> + return throwError(exec, TypeError, "Attempt to invoke non-C method
on C object.");
The "C" hierarchy of classes is a huge misnomer, it's bindings for NPAPI
plug-ins. Please don't call it "C" in text visible to Web developers.
+ const MethodList &methodList = *runtimeMethod->methods();
Misplaced &. Repeated several times in the patch.
+class ObjcRuntimeMethod : public RuntimeMethod {
The modern spelling is "ObjC". I used that for RuntimeObject, even though old
code used "Objc".
+ (WebKit::):
This is just a result of a bug in the script, please delete it. Per-function
comments would be even better.
+class PluginRuntimeMethod : public RuntimeMethod {
Maybe "ProxyRuntimeMethod", to match ProxyInstance and ProxyRuntimeObject?
+public:
+ PluginRuntimeMethod(ExecState* exec, const Identifier& name,
Bindings::MethodList& list)
+ : RuntimeMethod(exec, name, list)
+ {
+ }
+
+ static const ClassInfo s_info;
+};
Don't you need to override classInfo() in all those classes?
Looks good, r- for lack of tests.
More information about the webkit-reviews
mailing list