[gwt-contrib] [google-web-toolkit] [EMAIL PROTECTED] commented on branch /branches/1_6_clean_events.

[codesite-noreply]

[google-web-toolkit] [EMAIL PROTECTED] commented on branch  
/branches/1_6_clean_events.
Details are at  
http://code.google.com/p/google-web-toolkit/source/branch?spec=issue3083&branch=%2Fbranches%2F1_6_clean_events

General Comment:
CloseEvent:
http://code.google.com/p/google-web-toolkit/source/browse/branches/1_6_clean_events/user/src/com/google/gwt/event/logical/shared/CloseEvent.java?spec=issue3083&r=3969

Line-by-line comments:

File:  
/branches/1_6_clean_events/user/src/com/google/gwt/event/logical/shared/CloseEvent.java
  
(r3969)
===

Line 59:* @param  the target type
---
I don't understand the difference between the target and the source for  
this event.

Line 84:* Constructor. Should only be used by subclasses, almost always  
for testing.
---
See my comments in BeforeSelectionEvent.

Line 107:   // @param autoClosed was the close event triggered automatically
---
I think this comment took a wrong turn; it doesn't seem to apply.

Line 126:   protected final void setAutoClosed(boolean autoClosed) {
---
It really seems like autoClosed and target should both be construction time  
invariants.

Respond to these comments at  
http://code.google.com/p/google-web-toolkit/source/branch?spec=issue3083&branch=%2Fbranches%2F1_6_clean_events
--
You received this message because you starred this review, or because
your project has directed all notifications to a mailing list that you
subscribe to.
You may adjust your review notification preferences at:
http://code.google.com/hosting/settings

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

[gwt-contrib] [google-web-toolkit] [EMAIL PROTECTED] commented on branch /branches/1_6_clean_events.

[codesite-noreply]

[google-web-toolkit] [EMAIL PROTECTED] commented on branch  
/branches/1_6_clean_events.
Details are at  
http://code.google.com/p/google-web-toolkit/source/branch?spec=issue3083&branch=%2Fbranches%2F1_6_clean_events

General Comment:
JsHandlerRegistry:
http://code.google.com/p/google-web-toolkit/source/browse/branches/1_6_clean_events/user/src/com/google/gwt/event/shared/JsHandlerRegistry.java?spec=issue3083&r=4009

Line-by-line comments:

File:  
/branches/1_6_clean_events/user/src/com/google/gwt/event/shared/JsHandlerRegistry.java
  
(r4009)
===

Line 36: return (JsHandlerRegistry) JavaScriptObject.createObject();
---
This could be JavaScriptObject.createObject().cast() if you like that style.

Line 45:   public final  void  
addHandler(HandlerManager manager,
---
What do we get out of trying to flatten these? Comparing this to the  
simpler approach of just always keeping unflattened structure seems  
complex. What are the tradeoffs that led us to adopt this approach?

Line 87:   // unsafe cast instead.
---
I don't see how getHandler could get inlined at all. Doesn't the possible  
throwing of IndexOutOfBounds exception prevent that?

Now that we have some benchmark baselines, we can use these to guide out  
decisions about doing linguistically ugly things like unchecked type  
conversions.

Line 114:   // unsafe cast instead.
---
Same comment here as the last method.

Line 121:   throw new IndexOutOfBoundsException("index: " + index);
---
I really don't know the right answer here, but should we consider making  
this an assertion?

Line 145:   final native int getCount(int index) /*-{
---
private

Line 150:   final native void unflatten(int base) /*-{
---
private

Line 154:  for(var i = 0; i < count;i++){
---
Indentation is off.

Line 174: return  this[base + 1];
---
Extra space after return.

Respond to these comments at  
http://code.google.com/p/google-web-toolkit/source/branch?spec=issue3083&branch=%2Fbranches%2F1_6_clean_events
--
You received this message because you starred this review, or because
your project has directed all notifications to a mailing list that you
subscribe to.
You may adjust your review notification preferences at:
http://code.google.com/hosting/settings

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

[gwt-contrib] [google-web-toolkit] [EMAIL PROTECTED] commented on branch /branches/1_6_clean_events.

[codesite-noreply]

[google-web-toolkit] [EMAIL PROTECTED] commented on branch  
/branches/1_6_clean_events.
Details are at  
http://code.google.com/p/google-web-toolkit/source/branch?spec=issue3083&branch=%2Fbranches%2F1_6_clean_events

General Comment:
JavaHandlerRegistry:
http://code.google.com/p/google-web-toolkit/source/browse/branches/1_6_clean_events/user/src/com/google/gwt/event/shared/JavaHandlerRegistry.java?spec=issue3083&r=3969

Line-by-line comments:

File:  
/branches/1_6_clean_events/user/src/com/google/gwt/event/shared/JavaHandlerRegistry.java
  
(r3969)
===

Line 27: class JavaHandlerRegistry extends HashMap,  
ArrayList> {
---
I think this class should contain a HashMap and not be a HashMap. It offers  
a much simpler API than HashMap.

Line 33:   super.put(type, l);
---
super.put? We don't override put so super isn't necessary, but this also  
becomes a non-issue if this isn't a subclass of HashMap.

Line 54: assert (index < handlerCount);
---
shouldn't this be
assert index < getHandlerCount(eventKey)
Otherwise you'll always compute the handlerCount even when assertions are  
turned off.

Line 61: if (l == null) {
---
would probably be clearer as return (l == null) ? 0 : l.size();

Line 70: if (l != null) {
---
Given the existence of HandlerRegistrations, why is it even valid to ask to  
remove something that isn't there?

Line 77: // This cast is safe because we control the puts.
---
We don't actually control the puts, someone could accidently put some  
invalid gunk in by calling put directly in HandlerManager. Now that's not  
highly likely, but the fact that it's possible is another strike against  
the subclass.

Respond to these comments at  
http://code.google.com/p/google-web-toolkit/source/branch?spec=issue3083&branch=%2Fbranches%2F1_6_clean_events
--
You received this message because you starred this review, or because
your project has directed all notifications to a mailing list that you
subscribe to.
You may adjust your review notification preferences at:
http://code.google.com/hosting/settings

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

[gwt-contrib] [google-web-toolkit] [EMAIL PROTECTED] commented on branch /branches/1_6_clean_events.

[codesite-noreply]

[google-web-toolkit] [EMAIL PROTECTED] commented on branch  
/branches/1_6_clean_events.
Details are at  
http://code.google.com/p/google-web-toolkit/source/branch?spec=issue3083&branch=%2Fbranches%2F1_6_clean_events

General Comment:
HandlerManager:
http://code.google.com/p/google-web-toolkit/source/browse/branches/1_6_clean_events/user/src/com/google/gwt/event/shared/HandlerManager.java?spec=issue3083&r=4006

Line-by-line comments:

File:  
/branches/1_6_clean_events/user/src/com/google/gwt/event/shared/HandlerManager.java
  
(r4006)
===

Line 30:   static int EXPECTED_HANDLERS = 5;
---
Why is this package-private? testability?

Line 31:   private static final boolean useJs = GWT.isClient();
---
This should probably be useJsRegistry

Line 40:   public static boolean isLive(GwtEvent event) {
---
Why make GwtEvent.isLive protected if we're going to make a public delegate  
to it?

Line 49:   public static void revive(GwtEvent event) {
---
Why make GwtEvent.revive protected if we're going to offer a public  
delegate to it?

Line 50: if (event.isLive() == false) {
---
if (!event.isLive()) {


Line 58: index += EXPECTED_HANDLERS + 2;
---
It seems weird that HandlerManager has to be concerned with the stride of  
the underlying registry. And the EXPECTED_HANDLERS is really a detail of  
JsHandlerRegistry. Seems like this should defer to the registry for the  
next key index. That would allow EXPECTED_HANDLERS to move into  
JsHandlerRegistry and JavaHandlerRegistry can return serial indexes if it  
wants to.

Line 63:   int firingDepth = 0;
---
There seems something unholy about the union between HandlerManager and the  
registries. They seem to share logic in a way that suggests that they  
aren't properly encapsulated ... or that they are the same thing and that  
the registries should become nested classes. firingDepth, like  
EXPECTED_HANDLERS, seems unnecessarily package-private, but I'll have to  
keep looking to understand exactly why the relationship is so intimate.

Line 76:   private ArrayList addQueue;
---
I think this should be a List as well.

I have to admit that I don't really like the concurrent modification  
scheme. Did we consider doing a simple copy of the handler collection on  
modification when firing is active? While that will likely be slower to add  
a handler during firings, it would likely be faster in the common case. And  
since web mode could take advantage of Array.slice, I'm not sure that there  
would be a significant difference in the mod-while-firing case. Plus, there  
are a few things that trouble me about this scheme:

1 - It does seem kind of complex having to enqueue mods into lazily created  
Lists.

2 - The fact that we end up with List