[webkit-reviews] review granted: [Bug 35401] Unsafe casting of Method types in bridge code. : [Attachment 49603] New awesomier patch.

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Fri Feb 26 12:16:15 PST 2010


Alexey Proskuryakov <ap at webkit.org> has granted 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 49603: New awesomier patch.
https://bugs.webkit.org/attachment.cgi?id=49603&action=review

------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
 __ZN7WebCore6String8fromUTF8EPKcm
+__ZTVN3JSC13RuntimeMethodE

__ZNK3JSC8Bindings13RuntimeObject12defaultValueEPNS_9ExecStateENS_22PreferredPr
imitiveTypeE

This doesn't look sorted properly.

+    virtual const ClassInfo* classInfo() const { return &info; }

In RuntimeMethod, it's s_info. Why does this work (or what is broken)?

+class CRuntimeMethod : public RuntimeMethod {

I didn't notice it last time, but we really prefer to have classes in their own
files in new code.

+#import "JavaScriptCore/Error.h"
 #import "NetscapePluginHostProxy.h"
 #import "ProxyRuntimeObject.h"
 #import <WebCore/IdentifierRep.h>
 #import <WebCore/JSDOMWindow.h>
 #import <WebCore/npruntime_impl.h>
 #import <runtime/PropertyNameArray.h>
+#import "WebCore/runtime_method.h"
 
This looks like a mess. I think it should be "runtime/Error.h" and
<WebCore/runtime_method.h>.

+	 * java/java-and-plugins.html: Add tests for passing mismatched this
objects to methdods.

The patch and ChangeLog don't include changes for expected results.

With the exception of "one class per file", the comments above clearly must be
addressed before landing, r=me assuming you make the appropriate changes, and
also fix Qt build.


More information about the webkit-reviews mailing list