LGTM

Everything looks good, but there are some places where you can simplify
the Impl classes.


http://gwt-code-reviews.appspot.com/1374803/diff/1/user/src/com/google/gwt/storage/client/StorageImpl.java
File user/src/com/google/gwt/storage/client/StorageImpl.java (right):

http://gwt-code-reviews.appspot.com/1374803/diff/1/user/src/com/google/gwt/storage/client/StorageImpl.java#newcode193
user/src/com/google/gwt/storage/client/StorageImpl.java:193:
@com.google.gwt.storage.client.StorageImpl::jsHandler = function(event)
{
This function assignment should be wrapped with $entry to ensure that it
GWT can capture events properly.  See DOMImplStandard for an example.

@com.google.gwt.storage.client.StorageImpl::jsHandler =
$entry(function(event) {
...
}}});

http://gwt-code-reviews.appspot.com/1374803/diff/1/user/src/com/google/gwt/storage/client/StorageImplIE8.java
File user/src/com/google/gwt/storage/client/StorageImplIE8.java (right):

http://gwt-code-reviews.appspot.com/1374803/diff/1/user/src/com/google/gwt/storage/client/StorageImplIE8.java#newcode33
user/src/com/google/gwt/storage/client/StorageImplIE8.java:33: public
native String key(String storage, int index) /*-{
This is that same as StorageImplNonNativeEvents.key().  We can just get
ride of this Impl version.

http://gwt-code-reviews.appspot.com/1374803/diff/1/user/src/com/google/gwt/storage/client/StorageImplMozilla.java
File user/src/com/google/gwt/storage/client/StorageImplMozilla.java
(right):

http://gwt-code-reviews.appspot.com/1374803/diff/1/user/src/com/google/gwt/storage/client/StorageImplMozilla.java#newcode34
user/src/com/google/gwt/storage/client/StorageImplMozilla.java:34:
public native String key(String storage, int index) /*-{
This is that same as StorageImplNonNativeEvents.key().  We can just get
ride of this Impl version.

http://gwt-code-reviews.appspot.com/1374803/diff/1/user/src/com/google/gwt/storage/client/StorageImplNonNativeEvents.java
File
user/src/com/google/gwt/storage/client/StorageImplNonNativeEvents.java
(right):

http://gwt-code-reviews.appspot.com/1374803/diff/1/user/src/com/google/gwt/storage/client/StorageImplNonNativeEvents.java#newcode29
user/src/com/google/gwt/storage/client/StorageImplNonNativeEvents.java:29:
class StorageImplNonNativeEvents extends StorageImpl {
Most of the overrides in this class could be made non-native and use
calls to the super.method() plus some other stuff.

http://gwt-code-reviews.appspot.com/1374803/diff/1/user/src/com/google/gwt/storage/client/StorageImplNonNativeEvents.java#newcode53
user/src/com/google/gwt/storage/client/StorageImplNonNativeEvents.java:53:
final StorageEvent.Handler handler) {
It might be cleaner to just override
addStorageEventHandler0/removeStorageEventHandler0 to be no-op methods.

http://gwt-code-reviews.appspot.com/1374803/diff/1/user/src/com/google/gwt/storage/client/StorageImplNonNativeEvents.java#newcode64
user/src/com/google/gwt/storage/client/StorageImplNonNativeEvents.java:64:
$wnd[storage].clear();
Make non-native:
super.clear();
fireStorageEvent(...);

http://gwt-code-reviews.appspot.com/1374803/diff/1/user/src/com/google/gwt/storage/client/StorageImplNonNativeEvents.java#newcode87
user/src/com/google/gwt/storage/client/StorageImplNonNativeEvents.java:87:
StorageEvent.Handler handler) {
Override  removeStorageEventHandler0 instead of this method.

http://gwt-code-reviews.appspot.com/1374803/

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

Reply via email to