http://gwt-code-reviews.appspot.com/1469803/diff/1/plugins/npapi/LocalObjectTable.h File plugins/npapi/LocalObjectTable.h (right):
http://gwt-code-reviews.appspot.com/1469803/diff/1/plugins/npapi/LocalObjectTable.h#newcode100 plugins/npapi/LocalObjectTable.h:100: NPObject* get(int id) { Given the new method added, please change this to something like getById. http://gwt-code-reviews.appspot.com/1469803/diff/1/plugins/npapi/LocalObjectTable.h#newcode109 plugins/npapi/LocalObjectTable.h:109: int get(NPObject* jsObject) { Please use a more descriptive name, such as getObjectId since there is now collision with the above. http://gwt-code-reviews.appspot.com/1469803/diff/1/plugins/npapi/ScriptableInstance.cpp File plugins/npapi/ScriptableInstance.cpp (right): http://gwt-code-reviews.appspot.com/1469803/diff/1/plugins/npapi/ScriptableInstance.cpp#newcode434 plugins/npapi/ScriptableInstance.cpp:434: int id = localObjects.get(obj); So how does this fix the identity problem? The reason this was added was to avoid the identity problem, since the same JS object passed to the plugin twice will get different NPObject wrappers. Has that been fixed now? http://gwt-code-reviews.appspot.com/1469803/diff/1/user/test/com/google/gwt/core/client/JsIdentityTest.java File user/test/com/google/gwt/core/client/JsIdentityTest.java (right): http://gwt-code-reviews.appspot.com/1469803/diff/1/user/test/com/google/gwt/core/client/JsIdentityTest.java#newcode2 user/test/com/google/gwt/core/client/JsIdentityTest.java:2: * Copyright 2008 Google Inc. 2011 http://gwt-code-reviews.appspot.com/1469803/diff/1/user/test/com/google/gwt/core/client/JsIdentityTest.java#newcode24 user/test/com/google/gwt/core/client/JsIdentityTest.java:24: * @author [email protected] (John McDole) We don't use @author tags in GWT, and the first line should have a period. Did you run checkstyle or have Eclipse configured to use it? http://gwt-code-reviews.appspot.com/1469803/diff/1/user/test/com/google/gwt/core/client/JsIdentityTest.java#newcode34 user/test/com/google/gwt/core/client/JsIdentityTest.java:34: return "com.google.gwt.core.Core"; I'm not sure I follow what all these tests are looking for. The most basic tests are: testJavaToJs() { obj=...; assertTrue(jsID(obj, obj)); } native jsID(a, b) { return a === b; } testJsToJava() { initJsObj(); a=getJsObj(); b=getJsObj(); assertSame(a, b); } native initJsObj() { obj = {}; } native getJsObj() { return obj; } Having additional tests that store objects and return them later are fine, but it is hard for me to tell these cases are covered from the test and I would prefer a more direct test for them. http://gwt-code-reviews.appspot.com/1469803/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
