[gwt-contrib] Re: Review: JsArrays patch
Actually, I didn't mean that it was going to pull in arbitrary amounts of code. I just literally meant that the body of that toString() is pretty big for a micro-app, like a small gadget based on GQuery. I just don't see the value of adding that code into a web mode production build. On Tue, Mar 31, 2009 at 7:55 AM, Freeland Abbott fabb...@google.com wrote: Okay. I'll look into sort and toSource tomorrow; right now I'm away from that project code to see whether I want to try to fight for sort. So this patch should, I hope, be just the easy stuff. Usually when I say something rash like that it turns out I'm very wrong, but we'll see. Regarding JSO, I pulled toSource, but left the I-think-helpful toString(). I know Scott worried about pulling in other types' toString(), but in separate private email I think his worry is unfounded---best I know, we don't analyze JSNI bodies, so while this implementation references toString() if available, it can't change code size by pulling anything in that wasn't already coming for the ride. I think; I'm sure he or someone will correct me if I'm wrong on that! On Tue, Mar 31, 2009 at 5:44 AM, Kelly Norton knor...@google.com wrote: Few things: Overall, I'd like to be more conservative landing things in JavaScriptObject for a couple of reasons: (1) It's hard to take a mulligan with these because of their constraints (2) there is always a trivial work around to create application specific subclasses (with toll free casting). From r5082: I don't think toSource is appropriate for JavaScriptObject. It only works on mozilla browsers. JsArray.push: As I recall, this[this.length] = value is faster than this.push(value) on all browsers. It's not a complexity change like array.pop() is, but it can be significant. (How I do wish we had continuous perf testing). javadoc: The javadoc for these should be written to describe what the function does. Direct mapping to underlying sort is a good implementation note, but we should actually way what it does and not rely on developers having an understanding of the JavaScript equivalent. sort(JavaScriptObject): I'd like to avoid this one if we can. It just opens up larger questions about the right way to do this. We don't currently have a convention for representing JavaScript functions in Java. Someone should probably have a good story there before we add something like this to JavaScriptObject. /kel On Fri, Mar 27, 2009 at 2:15 PM, Freeland Abbott fabb...@google.comwrote: I think the argument is more for unnecessary rather than bad... although without JsArrayBase (we can make it package-protected, and call it JsArrayImpl if anyone cares), we duplicate the JSNI implementation for a couple trivial methods. I thought refactoring them into one place was nice, but trivial enough that I'm not fighting over it. Revised patch attached; I can go either way on this. On Fri, Mar 27, 2009 at 2:06 PM, Scott Blum sco...@google.com wrote: I'm going to punt this review to Bruce Kelly, 'cause I have no idea why having JsArrayBase would be bad. :) On Fri, Mar 27, 2009 at 1:28 PM, Freeland Abbott gwt.team.fabb...@gmail.com wrote: Scott, we already talked about this, but here's the patch for public review. The basic goal is to surface the native length, sort, push, and shift operators for JsArrays... I know you mentioned that IE6's push may be slower than indexed extension, and thus a candidate for deferred binding, but I wanted to get a basic implementation in first. There should be only checkstyle changes from what we discussed (though that obviously doesn't help the rest GWTC). I also added some checkstyle fixes to JavaScriptObject, introduced by my c5082. -- If you received this communication by mistake, you are entitled to one free ice cream cone on me. Simply print out this email including all relevant SMTP headers and present them at my desk to claim your creamy treat. We'll have a laugh at my emailing incompetence, and play a game of ping pong. (offer may not be valid in all States). --~--~-~--~~~---~--~~ http://groups.google.com/group/Google-Web-Toolkit-Contributors -~--~~~~--~~--~--~---
[gwt-contrib] Re: Review: JsArrays patch
On 31 mar, 22:29, Ray Cromwell cromwell...@gmail.com wrote:
Wouldn't this cause a problem when you want more than one JSO to implement
Iterable? I wouldn't make the class itself implement Iterable, I'd add a
helper method to return an Iterable, e.g.
public class JsArrayT extends JavaScriptObject {
public IterableT iterable() { ... }
}
for(T foo : someJsArray.iterable()) {
}
The risk of adding it to the class type itself is that you potentially block
anyone else from making a JSO that implements Iterable.
For better performance in web mode, returning a T[] would be
preferable (as it would just return the JsArray unchanged, without
wrapping class).
See
http://code.google.com/p/gwt-in-the-air/source/browse/trunk/src/net/ltgt/gwt/jscollections/client/JsArrays.java#124
(that's something we discussed already a few months ago and I believe
that's what you're using in GQuery too; the array should be considered
read-only, the main use case is iterating using a for(:) loop)
Or maybe, i don't if it would work: can a super-source class
implementation return a different type as a normal source class?
i.e. in src/com/google/gwt/core/client/JsArrayT: IterableT iterable
()
while in super/com/google/gwt/core/client/JsArrayT: T[] iterable()
though it can break at compile time if not used in a for(:) loop (e.g.
assigned to an intermediate IterableT variable and/or explicitly
calling IterableT methods)
--~--~-~--~~~---~--~~
http://groups.google.com/group/Google-Web-Toolkit-Contributors
-~--~~~~--~~--~--~---
[gwt-contrib] Re: Review: JsArrays patch
The for (T x: array) ... use case is precisely what's motivating me, yes; I
don't have any intention of e.g. implementing Iterator.remove(). We allow
that syntax on real Java arrays, but not on JsArrays.
Does returning T[] actually avoid creating an Iterator object, or does it
just create an Iterator of different type?
I'm not sure I understand Ray's concern... JSO wouldn't implement iterable,
so another subclass of JSO would do whatever the author made it do. Most of
the methods on JsArrayT and e.g. JsArrayString[1] are final, but we
wouldn't *have* to do that for iterator(), so a subclass could still
override.
[1] why don't we just use JsArrayString instead of JsArrayString?
Pre-generics compatibility? If so, should we re-implement JsArrayString as
a trivial extension of JsArrayString?
On Wed, Apr 1, 2009 at 9:22 AM, Thomas Broyer t.bro...@gmail.com wrote:
On 31 mar, 22:29, Ray Cromwell cromwell...@gmail.com wrote:
Wouldn't this cause a problem when you want more than one JSO to
implement
Iterable? I wouldn't make the class itself implement Iterable, I'd add a
helper method to return an Iterable, e.g.
public class JsArrayT extends JavaScriptObject {
public IterableT iterable() { ... }
}
for(T foo : someJsArray.iterable()) {
}
The risk of adding it to the class type itself is that you potentially
block
anyone else from making a JSO that implements Iterable.
For better performance in web mode, returning a T[] would be
preferable (as it would just return the JsArray unchanged, without
wrapping class).
See
http://code.google.com/p/gwt-in-the-air/source/browse/trunk/src/net/ltgt/gwt/jscollections/client/JsArrays.java#124
(that's something we discussed already a few months ago and I believe
that's what you're using in GQuery too; the array should be considered
read-only, the main use case is iterating using a for(:) loop)
Or maybe, i don't if it would work: can a super-source class
implementation return a different type as a normal source class?
i.e. in src/com/google/gwt/core/client/JsArrayT: IterableT iterable
()
while in super/com/google/gwt/core/client/JsArrayT: T[] iterable()
though it can break at compile time if not used in a for(:) loop (e.g.
assigned to an intermediate IterableT variable and/or explicitly
calling IterableT methods)
--~--~-~--~~~---~--~~
You received this message because you are subscribed to the Google Groups
Google Web Toolkit Contributors group.
To post to this group, send email to
Google-Web-Toolkit-Contributors@googlegroups.com
To unsubscribe from this group, send email to
google-web-toolkit-contributors+unsubscr...@googlegroups.com
For more options, visit this group at
http://groups.google.com/group/Google-Web-Toolkit-Contributors?hl=en
-~--~~~~--~~--~--~---
[gwt-contrib] Re: Review: JsArrays patch
Yes, My concern is, if you don't make a method that returns a T[], and instead make a JSO that implements IterableT, you can only do this once. Java treats T[] differently than IterableT with regard to the for-each loop. As usual in Java, the primitive types are treated non-orthogonally. -Ray On Tue, Mar 31, 2009 at 3:50 PM, Ian Petersen ispet...@gmail.com wrote: On Tue, Mar 31, 2009 at 3:42 PM, Freeland Abbott fabb...@google.com wrote: I'm not sure I understand Ray's concern... JSO wouldn't implement iterable, so another subclass of JSO would do whatever the author made it do. Most of the methods on JsArrayT and e.g. JsArrayString[1] are final, but we wouldn't have to do that for iterator(), so a subclass could still override. I assumed Ray's concern is based around the fact that only one subclass of JSO can implement any given interface so, if JsArrayT implements IterableT, no other subclass of JSO can implement it. Am I right? Ian --~--~-~--~~~---~--~~ http://groups.google.com/group/Google-Web-Toolkit-Contributors -~--~~~~--~~--~--~---
[gwt-contrib] Re: Review: JsArrays patch
On Tue, Mar 31, 2009 at 6:42 PM, Freeland Abbott fabb...@google.com wrote:
The for (T x: array) ... use case is precisely what's motivating me, yes; I
don't have any intention of e.g. implementing Iterator.remove(). We allow
that syntax on real Java arrays, but not on JsArrays.
Does returning T[] actually avoid creating an Iterator object, or does it
just create an Iterator of different type?
I'm not sure I understand Ray's concern... JSO wouldn't implement iterable,
so another subclass of JSO would do whatever the author made it do. Most of
the methods on JsArrayT and e.g. JsArrayString[1] are final, but we
wouldn't *have* to do that for iterator(), so a subclass could still
override.
[1] why don't we just use JsArrayString instead of JsArrayString?
Pre-generics compatibility? If so, should we re-implement JsArrayString as
a trivial extension of JsArrayString
I believe this is because String is not a subclass of JavaScriptObject.
It's not just a matter of changing the generic declaration of JsArray,
because passing JSO's is different between from passing Strings in JSNI
(hosted mode will barf.)
On Wed, Apr 1, 2009 at 9:22 AM, Thomas Broyer t.bro...@gmail.com wrote:
On 31 mar, 22:29, Ray Cromwell cromwell...@gmail.com wrote:
Wouldn't this cause a problem when you want more than one JSO to
implement
Iterable? I wouldn't make the class itself implement Iterable, I'd add a
helper method to return an Iterable, e.g.
public class JsArrayT extends JavaScriptObject {
public IterableT iterable() { ... }
}
for(T foo : someJsArray.iterable()) {
}
The risk of adding it to the class type itself is that you potentially
block
anyone else from making a JSO that implements Iterable.
For better performance in web mode, returning a T[] would be
preferable (as it would just return the JsArray unchanged, without
wrapping class).
See
http://code.google.com/p/gwt-in-the-air/source/browse/trunk/src/net/ltgt/gwt/jscollections/client/JsArrays.java#124
(that's something we discussed already a few months ago and I believe
that's what you're using in GQuery too; the array should be considered
read-only, the main use case is iterating using a for(:) loop)
Or maybe, i don't if it would work: can a super-source class
implementation return a different type as a normal source class?
i.e. in src/com/google/gwt/core/client/JsArrayT: IterableT iterable
()
while in super/com/google/gwt/core/client/JsArrayT: T[] iterable()
though it can break at compile time if not used in a for(:) loop (e.g.
assigned to an intermediate IterableT variable and/or explicitly
calling IterableT methods)
--
Eric Z. Ayers - GWT Team - Atlanta, GA USA
http://code.google.com/webtoolkit/
--~--~-~--~~~---~--~~
http://groups.google.com/group/Google-Web-Toolkit-Contributors
-~--~~~~--~~--~--~---
[gwt-contrib] Re: Review: JsArrays patch
Few things: Overall, I'd like to be more conservative landing things in JavaScriptObject for a couple of reasons: (1) It's hard to take a mulligan with these because of their constraints (2) there is always a trivial work around to create application specific subclasses (with toll free casting). From r5082: I don't think toSource is appropriate for JavaScriptObject. It only works on mozilla browsers. JsArray.push: As I recall, this[this.length] = value is faster than this.push(value) on all browsers. It's not a complexity change like array.pop() is, but it can be significant. (How I do wish we had continuous perf testing). javadoc: The javadoc for these should be written to describe what the function does. Direct mapping to underlying sort is a good implementation note, but we should actually way what it does and not rely on developers having an understanding of the JavaScript equivalent. sort(JavaScriptObject): I'd like to avoid this one if we can. It just opens up larger questions about the right way to do this. We don't currently have a convention for representing JavaScript functions in Java. Someone should probably have a good story there before we add something like this to JavaScriptObject. /kel On Fri, Mar 27, 2009 at 2:15 PM, Freeland Abbott fabb...@google.com wrote: I think the argument is more for unnecessary rather than bad... although without JsArrayBase (we can make it package-protected, and call it JsArrayImpl if anyone cares), we duplicate the JSNI implementation for a couple trivial methods. I thought refactoring them into one place was nice, but trivial enough that I'm not fighting over it. Revised patch attached; I can go either way on this. On Fri, Mar 27, 2009 at 2:06 PM, Scott Blum sco...@google.com wrote: I'm going to punt this review to Bruce Kelly, 'cause I have no idea why having JsArrayBase would be bad. :) On Fri, Mar 27, 2009 at 1:28 PM, Freeland Abbott gwt.team.fabb...@gmail.com wrote: Scott, we already talked about this, but here's the patch for public review. The basic goal is to surface the native length, sort, push, and shift operators for JsArrays... I know you mentioned that IE6's push may be slower than indexed extension, and thus a candidate for deferred binding, but I wanted to get a basic implementation in first. There should be only checkstyle changes from what we discussed (though that obviously doesn't help the rest GWTC). I also added some checkstyle fixes to JavaScriptObject, introduced by my c5082. -- If you received this communication by mistake, you are entitled to one free ice cream cone on me. Simply print out this email including all relevant SMTP headers and present them at my desk to claim your creamy treat. We'll have a laugh at my emailing incompetence, and play a game of ping pong. (offer may not be valid in all States). --~--~-~--~~~---~--~~ http://groups.google.com/group/Google-Web-Toolkit-Contributors -~--~~~~--~~--~--~---
[gwt-contrib] Re: Review: JsArrays patch
Let's not add this extra type JsArrayBase into the hierarchy. Why can't we just push the various methods down? We can always factor upward in the future. If we need shared implementation, we can factor that out into a package-private JsArrayImpl class. On Fri, Mar 27, 2009 at 1:28 PM, Freeland Abbott gwt.team.fabb...@gmail.com wrote: Scott, we already talked about this, but here's the patch for public review. The basic goal is to surface the native length, sort, push, and shift operators for JsArrays... I know you mentioned that IE6's push may be slower than indexed extension, and thus a candidate for deferred binding, but I wanted to get a basic implementation in first. There should be only checkstyle changes from what we discussed (though that obviously doesn't help the rest GWTC). I also added some checkstyle fixes to JavaScriptObject, introduced by my c5082. --~--~-~--~~~---~--~~ http://groups.google.com/group/Google-Web-Toolkit-Contributors -~--~~~~--~~--~--~---
[gwt-contrib] Re: Review: JsArrays patch
FWIW, in another little project I used a pattern for this that avoids
implementation inheritance that I call self-delegation.
Here's an example:
/** Not put API, but it includes the impl for al getters and setters for all
types. **/
final class JsArray extends JavaScriptObject {
...
public int getInt(int index) {
assert indexIsInBounds(index);
return getIntImpl(index);
}
public void setNumber(int index, double value) {
assert isNumber(value);
setNumberImpl(index, value);
}
private native int getIntImpl(int index) /*-{
return this[index];
}-*/;
private native void setNumberImpl(int index, double value) /*-{
this[index] = value;
}-*/;
...
}
/** Public API */
public final class IntArray extends DataStructure {
public static IntArray create() {
return JavaScriptObject.createArray().cast();
}
protected IntArray() {
}
public int get(int index) {
return this.JsArraycast().getInt(index);
}
public int getSize() {
return this.JsArraycast().getSize();
}
public void set(int index, int value) {
this.JsArraycast().setNumber(index, value);
}
}
/kel
On Fri, Mar 27, 2009 at 1:41 PM, Bruce Johnson br...@google.com wrote:
Let's not add this extra type JsArrayBase into the hierarchy. Why can't we
just push the various methods down? We can always factor upward in the
future. If we need shared implementation, we can factor that out into a
package-private JsArrayImpl class.
On Fri, Mar 27, 2009 at 1:28 PM, Freeland Abbott
gwt.team.fabb...@gmail.com wrote:
Scott, we already talked about this, but here's the patch for public
review.
The basic goal is to surface the native length, sort, push, and shift
operators for JsArrays... I know you mentioned that IE6's push may be slower
than indexed extension, and thus a candidate for deferred binding, but I
wanted to get a basic implementation in first.
There should be only checkstyle changes from what we discussed (though
that obviously doesn't help the rest GWTC). I also added some checkstyle
fixes to JavaScriptObject, introduced by my c5082.
--
If you received this communication by mistake, you are entitled to one free
ice cream cone on me. Simply print out this email including all relevant
SMTP headers and present them at my desk to claim your creamy treat. We'll
have a laugh at my emailing incompetence, and play a game of ping pong.
(offer may not be valid in all States).
--~--~-~--~~~---~--~~
http://groups.google.com/group/Google-Web-Toolkit-Contributors
-~--~~~~--~~--~--~---
[gwt-contrib] Re: Review: JsArrays patch
I'm going to punt this review to Bruce Kelly, 'cause I have no idea why having JsArrayBase would be bad. :) On Fri, Mar 27, 2009 at 1:28 PM, Freeland Abbott gwt.team.fabb...@gmail.com wrote: Scott, we already talked about this, but here's the patch for public review. The basic goal is to surface the native length, sort, push, and shift operators for JsArrays... I know you mentioned that IE6's push may be slower than indexed extension, and thus a candidate for deferred binding, but I wanted to get a basic implementation in first. There should be only checkstyle changes from what we discussed (though that obviously doesn't help the rest GWTC). I also added some checkstyle fixes to JavaScriptObject, introduced by my c5082. --~--~-~--~~~---~--~~ http://groups.google.com/group/Google-Web-Toolkit-Contributors -~--~~~~--~~--~--~---
[gwt-contrib] Re: Review: JsArrays patch
I think the argument is more for unnecessary rather than bad...
although without JsArrayBase (we can make it package-protected, and call it
JsArrayImpl if anyone cares), we duplicate the JSNI implementation for a
couple trivial methods. I thought refactoring them into one place was nice,
but trivial enough that I'm not fighting over it. Revised patch attached; I
can go either way on this.
On Fri, Mar 27, 2009 at 2:06 PM, Scott Blum sco...@google.com wrote:
I'm going to punt this review to Bruce Kelly, 'cause I have no idea why
having JsArrayBase would be bad. :)
On Fri, Mar 27, 2009 at 1:28 PM, Freeland Abbott
gwt.team.fabb...@gmail.com wrote:
Scott, we already talked about this, but here's the patch for public
review.
The basic goal is to surface the native length, sort, push, and shift
operators for JsArrays... I know you mentioned that IE6's push may be slower
than indexed extension, and thus a candidate for deferred binding, but I
wanted to get a basic implementation in first.
There should be only checkstyle changes from what we discussed (though
that obviously doesn't help the rest GWTC). I also added some checkstyle
fixes to JavaScriptObject, introduced by my c5082.
--~--~-~--~~~---~--~~
http://groups.google.com/group/Google-Web-Toolkit-Contributors
-~--~~~~--~~--~--~---
Index: user/src/com/google/gwt/core/client/JsArrayString.java
===
--- user/src/com/google/gwt/core/client/JsArrayString.java (revision 5095)
+++ user/src/com/google/gwt/core/client/JsArrayString.java (working copy)
@@ -1,12 +1,12 @@
/*
* Copyright 2008 Google Inc.
- *
+ *
* Licensed under the Apache License, Version 2.0 (the License); you may not
* use this file except in compliance with the License. You may obtain a copy of
* the License at
- *
+ *
* http://www.apache.org/licenses/LICENSE-2.0
- *
+ *
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an AS IS BASIS, WITHOUT
* WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
@@ -17,10 +17,10 @@
/**
* A simple wrapper around a homogeneous native array of string values.
- *
+ *
* This class may not be directly instantiated, and can only be returned from a
* native method. For example,
- *
+ *
* code
* native JsArrayString getNativeArray() /*-{
* return ['foo', 'bar', 'baz'];
@@ -34,7 +34,7 @@
/**
* Gets the value at a given index.
- *
+ *
* @param index the index to be retrieved
* @return the value at the given index, or codenull/code if none exists
*/
@@ -44,7 +44,7 @@
/**
* Gets the length of the array.
- *
+ *
* @return the array length
*/
public final native int length() /*-{
@@ -52,15 +52,38 @@
}-*/;
/**
+ * call underlying push method.
+ */
+ public final native void push(String value) /*-{
+this.push(value);
+ }-*/;
+
+ /**
* Sets the value value at a given index.
- *
+ *
* If the index is out of bounds, the value will still be set. The array's
* length will be updated to encompass the bounds implied by the added value.
- *
+ *
* @param index the index to be set
* @param value the value to be stored
*/
public final native void set(int index, String value) /*-{
this[index] = value;
}-*/;
+
+ /**
+ * Shifts the first value off the array.
+ * @return the shifted value
+ */
+ public final native String shift() /*-{
+return this.shift();
+ }-*/;
+
+ /**
+ * direct mapping to underlying sort method.
+ */
+ public final native void sort(JavaScriptObject sortFunc) /*-{
+this.sort(sortFunc);
+ }-*/;
+
}
Index: user/src/com/google/gwt/core/client/JsArrayBoolean.java
===
--- user/src/com/google/gwt/core/client/JsArrayBoolean.java (revision 5095)
+++ user/src/com/google/gwt/core/client/JsArrayBoolean.java (working copy)
@@ -1,12 +1,12 @@
/*
* Copyright 2008 Google Inc.
- *
+ *
* Licensed under the Apache License, Version 2.0 (the License); you may not
* use this file except in compliance with the License. You may obtain a copy of
* the License at
- *
+ *
* http://www.apache.org/licenses/LICENSE-2.0
- *
+ *
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an AS IS BASIS, WITHOUT
* WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
@@ -17,10 +17,10 @@
/**
* A simple wrapper around a homogeneous native array of boolean values.
- *
+ *
* This class may not be directly instantiated, and can only be returned from a
* native method. For example,
- *
+ *
* code
* native JsArrayBoolean getNativeArray() /*-{
* return [true, false, true];
@@ -34,11 +34,11 @@
/**
*
[gwt-contrib] Re: Review: JsArrays patch
Kelly, since you have experience with this, I'd like you to be the decider (i.e. Freeland is now waiting on your LGTM). On Fri, Mar 27, 2009 at 2:16 PM, Freeland Abbott gwt.team.fabb...@gmail.com wrote: I think the argument is more for unnecessary rather than bad... although without JsArrayBase (we can make it package-protected, and call it JsArrayImpl if anyone cares), we duplicate the JSNI implementation for a couple trivial methods. I thought refactoring them into one place was nice, but trivial enough that I'm not fighting over it. Revised patch attached; I can go either way on this. On Fri, Mar 27, 2009 at 2:06 PM, Scott Blum sco...@google.com wrote: I'm going to punt this review to Bruce Kelly, 'cause I have no idea why having JsArrayBase would be bad. :) On Fri, Mar 27, 2009 at 1:28 PM, Freeland Abbott gwt.team.fabb...@gmail.com wrote: Scott, we already talked about this, but here's the patch for public review. The basic goal is to surface the native length, sort, push, and shift operators for JsArrays... I know you mentioned that IE6's push may be slower than indexed extension, and thus a candidate for deferred binding, but I wanted to get a basic implementation in first. There should be only checkstyle changes from what we discussed (though that obviously doesn't help the rest GWTC). I also added some checkstyle fixes to JavaScriptObject, introduced by my c5082. --~--~-~--~~~---~--~~ http://groups.google.com/group/Google-Web-Toolkit-Contributors -~--~~~~--~~--~--~---
