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) {
On 2011/07/01 18:38:17, jat wrote:
Given the new method added, please change this to something like
getById.
Done.
http://gwt-code-reviews.appspot.com/1469803/diff/1/plugins/npapi/LocalObjectTable.h#newcode109
plugins/npapi/LocalObjectTable.h:109: int get(NPObject* jsObject) {
On 2011/07/01 18:38:17, jat wrote:
Please use a more descriptive name, such as getObjectId since there is
now
collision with the above.
Done.
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);
On 2011/07/01 18:38:17, jat wrote:
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?
Kelly's patch to chrome is not present in Chrome13 builds. Running the
GWT tests shows these passing with Chrome13+NewPlugin and failing in
Chrome12+NewPlugin
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.
On 2011/07/01 18:38:17, jat wrote:
2011
Done.
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
cod...@google.com (John McDole)
On 2011/07/01 18:38:17, jat wrote:
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?
Ran the code formatter on the whole file and created it in Eclipse. I
did not run checktyle, but will.
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";
On 2011/07/01 18:38:17, jat wrote:
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.
The .contains() test was suggested by another team that was running into
that problem.
testJsoJavaComparison() does your testJstoJava().
testJavaObjectStorage() is similar enough to the first test, just using
== and === in JavaScript instead of indexOf(). It was a simple test
suggested by someone else.
The last test, testJavaArrayArray(), was something I was experiencing
working with scheduled tasks leaking. I can certainly remove it.
http://gwt-code-reviews.appspot.com/1469803/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
