[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