Note: I read files in random order.
http://gwt-code-reviews.appspot.com/1621803/diff/1/user/src/com/google/gwt/typedarrays/client/ArrayBuffer.java File user/src/com/google/gwt/typedarrays/client/ArrayBuffer.java (right): http://gwt-code-reviews.appspot.com/1621803/diff/1/user/src/com/google/gwt/typedarrays/client/ArrayBuffer.java#newcode36 user/src/com/google/gwt/typedarrays/client/ArrayBuffer.java:36: return null; How about throwing an UnsupportedOperationExceptin instead? or maybe even better, not doing anything special and let it fail in the JSNI: users should have already checked for isSupported() in their own code (possibly use an 'assert' here to catch mistakes in dev mode without impacting prod mode) http://gwt-code-reviews.appspot.com/1621803/diff/1/user/src/com/google/gwt/typedarrays/client/DataView.java File user/src/com/google/gwt/typedarrays/client/DataView.java (right): http://gwt-code-reviews.appspot.com/1621803/diff/1/user/src/com/google/gwt/typedarrays/client/DataView.java#newcode128 user/src/com/google/gwt/typedarrays/client/DataView.java:128: private static native boolean isSupportedRuntime() /*-{ Is there really a need for a distinct check besides TypedArray.isSupported()? AFAICT, only Chrome m7 and m8 supported "typed arrays" without supporting DataView, and those versions are no longer used on the web; or are there mobile browsers out there with similar "partial support"? http://gwt-code-reviews.appspot.com/1621803/diff/1/user/src/com/google/gwt/typedarrays/client/DataView.java#newcode188 user/src/com/google/gwt/typedarrays/client/DataView.java:188: public final native int getInt16(int byteOffset) /*-{ Why an 'int' and not a 'short'? http://gwt-code-reviews.appspot.com/1621803/diff/1/user/src/com/google/gwt/typedarrays/client/DataView.java#newcode261 user/src/com/google/gwt/typedarrays/client/DataView.java:261: public final native int getUint32(int byteOffset) /*-{ Should probably be a 'double' here (or a 'long', using a JSNI method that returns a double, and a cast from double to long in Java), as an 'int' in Java is 32-bit, so cannot represent all values of an UInt32. http://gwt-code-reviews.appspot.com/1621803/diff/1/user/src/com/google/gwt/typedarrays/client/Int16Array.java File user/src/com/google/gwt/typedarrays/client/Int16Array.java (right): http://gwt-code-reviews.appspot.com/1621803/diff/1/user/src/com/google/gwt/typedarrays/client/Int16Array.java#newcode24 user/src/com/google/gwt/typedarrays/client/Int16Array.java:24: public class Int16Array extends IntBasedTypedArray<Int16Array> { I'm not sure the IntBasedTypedArray intermediate type is worth it, at least for reading (the TypedArrays spec defines how values are converted, so you can technically put a 'double' into an Int16Array; but for reading, I think the getter return value should be of the type of the stored value: i.e. 'short' for an Int16Array). http://gwt-code-reviews.appspot.com/1621803/diff/1/user/src/com/google/gwt/typedarrays/client/Int16Array.java#newcode113 user/src/com/google/gwt/typedarrays/client/Int16Array.java:113: public static Int16Array create(int[] array) { Shouldn't it rather be short[] ? or at least have a short[] overload? http://gwt-code-reviews.appspot.com/1621803/diff/1/user/src/com/google/gwt/typedarrays/client/IntBasedTypedArray.java File user/src/com/google/gwt/typedarrays/client/IntBasedTypedArray.java (right): http://gwt-code-reviews.appspot.com/1621803/diff/1/user/src/com/google/gwt/typedarrays/client/IntBasedTypedArray.java#newcode58 user/src/com/google/gwt/typedarrays/client/IntBasedTypedArray.java:58: public final native byte getByte(int index) /*-{ What's the use case for getByte() vs. get() + a cast from int to byte? (note there's no JsArrayByte or JsArrayShort, only JsArrayInteger) http://gwt-code-reviews.appspot.com/1621803/diff/1/user/src/com/google/gwt/typedarrays/client/IntBasedTypedArray.java#newcode116 user/src/com/google/gwt/typedarrays/client/IntBasedTypedArray.java:116: public final native void set(int index, byte value) /*-{ There's no need to provide overloads for all narrower types of 'int', as you can simply pass a 'byte' value to set(int,int). http://gwt-code-reviews.appspot.com/1621803/diff/1/user/src/com/google/gwt/typedarrays/client/IntBasedTypedArray.java#newcode176 user/src/com/google/gwt/typedarrays/client/IntBasedTypedArray.java:176: public final void set(int[] array) { How about using varargs here? Could make things easier for a Java developer: set(1, 2, 3) instead of set(new int[] { 1, é, 3 }) http://gwt-code-reviews.appspot.com/1621803/diff/1/user/src/com/google/gwt/typedarrays/client/JsArrayUtil.java File user/src/com/google/gwt/typedarrays/client/JsArrayUtil.java (right): http://gwt-code-reviews.appspot.com/1621803/diff/1/user/src/com/google/gwt/typedarrays/client/JsArrayUtil.java#newcode29 user/src/com/google/gwt/typedarrays/client/JsArrayUtil.java:29: * The methods take care of the scope (dev mode vs. production mode). Javadoc is HTML, there should be a <p> here (well, I believe the last part is not needed actually) http://gwt-code-reviews.appspot.com/1621803/diff/1/user/src/com/google/gwt/typedarrays/client/JsArrayUtil.java#newcode32 user/src/com/google/gwt/typedarrays/client/JsArrayUtil.java:32: public final class JsArrayUtil { I believe this class should rather live in the c.g.g.core.client package (next to JsArrayXxx). http://gwt-code-reviews.appspot.com/1621803/diff/1/user/src/com/google/gwt/typedarrays/client/JsArrayUtil.java#newcode45 user/src/com/google/gwt/typedarrays/client/JsArrayUtil.java:45: public static long getLongFromJsArrayInteger(JsArrayInteger array, int index) { Wouldn't it be better/faster to use (or cast to) a JsArrayNumber and cast the 'double' to a 'long'? Numbers in ECMAScript have the same value-range as doubles in Java AFAIK, so there's no risk of loosing data here; and the GWT compiler would probably have more room for optimizations. http://gwt-code-reviews.appspot.com/1621803/diff/1/user/src/com/google/gwt/typedarrays/client/JsArrayUtil.java#newcode56 user/src/com/google/gwt/typedarrays/client/JsArrayUtil.java:56: public static native String getStringFromJsArrayInteger(JsArrayInteger array, int index) /*-{ Is that used somewhere else that getLongFromJsArrayInteger above? (i.e. does it need to be 'public'? could it be removed if getLongFromJsArrayInteger were reworked around double-to-long casts?) http://gwt-code-reviews.appspot.com/1621803/diff/1/user/src/com/google/gwt/typedarrays/client/JsArrayUtil.java#newcode87 user/src/com/google/gwt/typedarrays/client/JsArrayUtil.java:87: * Converts a JsArrayString to a String[]. Javadoc should make it *very* clear that it makes a copy in dev mode but a "cast" in prod mode, so the returned array should only be used for reading, and the array passed as argument should also only be used for reading while the returned array is in use: any change to one of the arrays would induce different behaviors in dev mode vs. prod mode. That means the method name ("unwrapArray") does not accurately describes its behavior. In my own code, I call these "bridge" methods "forIteration" because that's basically all they should be used for. http://gwt-code-reviews.appspot.com/1621803/diff/1/user/src/com/google/gwt/typedarrays/client/JsArrayUtil.java#newcode97 user/src/com/google/gwt/typedarrays/client/JsArrayUtil.java:97: for (int i = 0; i < jsArrayString.length(); i++) { Given the length doesn't change, storing it in a local variable would avoid going back and forth to JSNI (which implies a roundtrip over the wire between the browser and the DevMode) on each iteration (this is still needed for the get() call, but it'd cut by half the number of JSNI calls) http://gwt-code-reviews.appspot.com/1621803/diff/1/user/src/com/google/gwt/typedarrays/client/JsArrayUtil.java#newcode114 user/src/com/google/gwt/typedarrays/client/JsArrayUtil.java:114: for (int i = 0; i < srcArray.length; i++) { How about a "for each" loop w/ JsArrayBoolean.push()? I doubt it'd change anything performance-wise but it would make the code slightly more readable and less error-prone: for (boolean b : srcArray) { result.push(b); } http://gwt-code-reviews.appspot.com/1621803/diff/1/user/src/com/google/gwt/typedarrays/client/JsArrayUtil.java#newcode219 user/src/com/google/gwt/typedarrays/client/JsArrayUtil.java:219: public static JsArrayInteger wrapArray(long[] srcArray) { Really not sure long[] support is a good idea. Using 'long' in GWT is not a good idea overall, you should use 'double' for "greater than int" values when possible; and if you really need to "cast" a long[] to a JsArrayNumber, then you can make a copy. At the very least, given that it would always involve a copy (contrary to the other "wrapArray" methods), naming the method differently (such as "copyAsJsArray") would make it clear it's not the same as just "casting" arrays with no performance impact (at least in prod mode). http://gwt-code-reviews.appspot.com/1621803/diff/1/user/src/com/google/gwt/typedarrays/client/JsArrayUtil.java#newcode326 user/src/com/google/gwt/typedarrays/client/JsArrayUtil.java:326: return eval(arrayString); Couldn't JsonUtils.safeEval() be used instead? (in the case the wrapArray(long[]) is kept) I wonder how it would perform vs. iterating over the values to copy them to a JsArrayNumber. I'm concerned that this eval/JSON.parse method would probably fail with 'long' values that don't fit into a 'double', leading to weird errors in prod mode only; using a cast would make it behave the same in dev mdoe and prod mode. http://gwt-code-reviews.appspot.com/1621803/diff/1/user/src/com/google/gwt/typedarrays/client/TypedArray.java File user/src/com/google/gwt/typedarrays/client/TypedArray.java (right): http://gwt-code-reviews.appspot.com/1621803/diff/1/user/src/com/google/gwt/typedarrays/client/TypedArray.java#newcode78 user/src/com/google/gwt/typedarrays/client/TypedArray.java:78: return isSupportedRuntime(); Why isn't this baked into TypedArrayCompileTimeSupportTrue (that I would rename to TypedArraySupportMaybe) Also, how about doing this on clinit, storing the result in a private static field? (like JsonUtils does when checking for JSON.parse) http://gwt-code-reviews.appspot.com/1621803/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
