[gwt-contrib] [google-web-toolkit] r9254 committed - Fixes issue http://code.google.com/p/google-web-toolkit/issues/detail?...
Revision: 9254
Author: gwt.mirror...@gmail.com
Date: Thu Nov 18 23:14:02 2010
Log: Fixes issue
http://code.google.com/p/google-web-toolkit/issues/detail?id=5578
Restores the domain class upcasting behavior of RequestFactoryServlet,
which is relied upon by our UserInformation class.
Also fixes the fact that UserInformation never provided a finder
method, which we're now less forgiving of. Really, though,
UserInformation should be a value object, not a proxy at all. It acts
the way it does to hack around our lack of such things.
Also introduces unit tests to ensure that UserInformation and
LoggingService keep working.
Review at http://gwt-code-reviews.appspot.com/1098801
Review by: robertvaw...@google.com
http://code.google.com/p/google-web-toolkit/source/detail?r=9254
Modified:
/trunk/user/src/com/google/gwt/requestfactory/server/Logging.java
/trunk/user/src/com/google/gwt/requestfactory/server/RequestFactoryInterfaceValidator.java
/trunk/user/src/com/google/gwt/requestfactory/server/UserInformation.java
/trunk/user/src/com/google/gwt/requestfactory/shared/LoggingRequest.java
/trunk/user/test/com/google/gwt/requestfactory/client/RequestFactoryTest.java
/trunk/user/test/com/google/gwt/requestfactory/server/SimpleFoo.java
/trunk/user/test/com/google/gwt/requestfactory/shared/SimpleFooRequest.java
/trunk/user/test/com/google/gwt/requestfactory/shared/SimpleRequestFactory.java
===
--- /trunk/user/src/com/google/gwt/requestfactory/server/Logging.java Fri
Nov 5 04:25:19 2010
+++ /trunk/user/src/com/google/gwt/requestfactory/server/Logging.java Thu
Nov 18 23:14:02 2010
@@ -21,6 +21,8 @@
import com.google.gwt.logging.server.StackTraceDeobfuscator;
import com.google.gwt.user.client.rpc.RpcRequestBuilder;
+import javax.servlet.http.HttpServletRequest;
+
/**
* Server side object that handles log messages sent by
* {...@link com.google.gwt.requestfactory.client.RequestFactoryLogHandler}.
@@ -33,15 +35,22 @@
/**
* Logs a message.
*
- * @param logRecordJson a log record in JSON format.
+ * @param serializedLogRecordString a json serialized LogRecord, as
provided by
+ * {...@link
com.google.gwt.logging.client.JsonLogRecordClientUtil.logRecordAsJsonObject(LogRecord)}
* @throws RemoteLoggingException if logging fails
*/
public static void logMessage(String logRecordJson)
throws RemoteLoggingException {
-// if the header does not exist, we pass null, which is handled
gracefully
-// by the deobfuscation code.
-String strongName =
RequestFactoryServlet.getThreadLocalRequest().getHeader(
-RpcRequestBuilder.STRONG_NAME_HEADER);
+/*
+ * if the header does not exist, we pass null, which is handled
gracefully
+ * by the deobfuscation code.
+ */
+HttpServletRequest threadLocalRequest =
RequestFactoryServlet.getThreadLocalRequest();
+String strongName = null;
+if (threadLocalRequest != null) {
+ // can be null during tests
+ threadLocalRequest.getHeader(RpcRequestBuilder.STRONG_NAME_HEADER);
+}
RemoteLoggingServiceUtil.logOnServer(logRecordJson, strongName,
deobfuscator, null);
}
===
---
/trunk/user/src/com/google/gwt/requestfactory/server/RequestFactoryInterfaceValidator.java
Thu Nov 18 13:15:58 2010
+++
/trunk/user/src/com/google/gwt/requestfactory/server/RequestFactoryInterfaceValidator.java
Thu Nov 18 23:14:02 2010
@@ -326,7 +326,7 @@
if (!seen.add(name)) {
return;
}
- if (!"java/lang/Object".equals(superName)) {
+ if (!objectType.getInternalName().equals(superName)) {
RequestFactoryInterfaceValidator.this.visit(logger, superName,
this);
}
if (interfaces != null) {
@@ -384,7 +384,7 @@
private class SupertypeCollector extends EmptyVisitor {
private final ErrorContext logger;
private final Set seen = new HashSet();
-private final List supertypes = new ArrayList();
+private final List supers = new ArrayList();
public SupertypeCollector(ErrorContext logger) {
this.logger = logger;
@@ -393,7 +393,7 @@
public List exec(Type type) {
RequestFactoryInterfaceValidator.this.visit(logger,
type.getInternalName(), this);
- return supertypes;
+ return supers;
}
@Override
@@ -402,8 +402,8 @@
if (!seen.add(name)) {
return;
}
- supertypes.add(Type.getObjectType(name));
- if (!"java/lang/Object".equals(name)) {
+ supers.add(Type.getObjectType(name));
+ if (!objectType.getInternalName().equals(name)) {
RequestFactoryInterfaceValidator.this.visit(logger, superName,
this);
}
if (interfaces != null) {
@@ -787,22 +787,46 @@
String clientTypeBinaryName) {
Type key =
Type.getObjectType(BinaryName.toInternalName(domainTypeBinaryName));
List found = domainToClientType.get(key);
+
+/
[gwt-contrib] Re: Fix Alignment Attribute Parsing for HasAlignment (issue1121801)
LGTM http://gwt-code-reviews.appspot.com/1121801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Fix Alignment Attribute Parsing for HasAlignment (issue1121801)
http://gwt-code-reviews.appspot.com/1121801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Public: Improve reporting of TCK failures (issue1117801)
http://gwt-code-reviews.appspot.com/1117801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Public: Improve reporting of TCK failures (issue1117801)
http://gwt-code-reviews.appspot.com/1117801/diff/1/21
File
samples/validationtck/test/com/google/gwt/sample/validationtck/validation/ValidatePropertyTest.java
(right):
http://gwt-code-reviews.appspot.com/1117801/diff/1/21#newcode38
samples/validationtck/test/com/google/gwt/sample/validationtck/validation/ValidatePropertyTest.java:38:
try {
On 2010/11/18 16:56:04, rchandia wrote:
Are you sure you need to wrap the test in a try/catch? Usually such
tests
internally wrap the call with their own try/catch's and exceptions
escaping the
test are genuine errors/failures.
Yes, TestNG annotates the expected exceptions, but I have to catch
them.
I added a comment.
http://gwt-code-reviews.appspot.com/1117801/show
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Allow EntityProxy, ValueProxy, or any simple value type to be used as an entity's id and version... (issue1127801)
Reviewers: rchandia, rjrjr, Message: This should be the last big change to the RequestFactory wire format and data-processing code for 2.1.1. The next change will be the ServiceeLayer API cleanup to let end-users have more control. Then expect a bugfix / cleanup patch beyond that. Description: Allow EntityProxy, ValueProxy, or any simple value type to be used as an entity's id and version values. Allow RequestFactory to work with inner classes to make self-contained tests easier to write. Resolves issue 5368. Patch by: bobv Review by: rchandia, rjrjr Please review this at http://gwt-code-reviews.appspot.com/1127801/show Affected files: M user/src/com/google/gwt/autobean/rebind/AutoBeanFactoryGenerator.java M user/src/com/google/gwt/autobean/server/AutoBeanFactoryMagic.java M user/src/com/google/gwt/autobean/shared/AutoBeanCodex.java M user/src/com/google/gwt/editor/rebind/model/ModelUtils.java M user/src/com/google/gwt/requestfactory/rebind/RequestFactoryGenerator.java M user/src/com/google/gwt/requestfactory/rebind/model/EntityProxyModel.java M user/src/com/google/gwt/requestfactory/rebind/model/RequestFactoryModel.java M user/src/com/google/gwt/requestfactory/server/ReflectiveServiceLayer.java M user/src/com/google/gwt/requestfactory/server/RequestFactoryInterfaceValidator.java M user/src/com/google/gwt/requestfactory/server/RequestState.java M user/src/com/google/gwt/requestfactory/server/Resolver.java M user/src/com/google/gwt/requestfactory/server/SimpleRequestProcessor.java M user/src/com/google/gwt/requestfactory/shared/impl/AbstractRequestContext.java M user/src/com/google/gwt/requestfactory/shared/impl/AbstractRequestFactory.java M user/src/com/google/gwt/requestfactory/shared/impl/Constants.java M user/src/com/google/gwt/requestfactory/shared/impl/IdFactory.java A user/src/com/google/gwt/requestfactory/shared/impl/IdUtil.java M user/src/com/google/gwt/requestfactory/shared/impl/MessageFactoryHolder.java M user/src/com/google/gwt/requestfactory/shared/impl/SimpleProxyId.java M user/src/com/google/gwt/requestfactory/shared/messages/EntityCodex.java M user/src/com/google/gwt/requestfactory/shared/messages/IdMessage.java D user/src/com/google/gwt/requestfactory/shared/messages/IdUtil.java M user/src/com/google/gwt/requestfactory/shared/messages/MessageFactory.java M user/src/com/google/gwt/requestfactory/shared/messages/VersionedMessage.java M user/test/com/google/gwt/requestfactory/RequestFactoryJreSuite.java M user/test/com/google/gwt/requestfactory/RequestFactorySuite.java M user/test/com/google/gwt/requestfactory/client/RequestFactoryTest.java A user/test/com/google/gwt/requestfactory/server/ComplexKeysJreTest.java M user/test/com/google/gwt/requestfactory/server/FindServiceJreTest.java M user/test/com/google/gwt/requestfactory/server/RequestFactoryInterfaceValidatorTest.java M user/test/com/google/gwt/requestfactory/server/RequestFactoryJreTest.java A user/test/com/google/gwt/requestfactory/shared/ComplexKeysTest.java -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] [google-web-toolkit] r9253 committed - Edited wiki page RequestFactory_2_1_1 through web user interface.
Revision: 9253 Author: b...@google.com Date: Thu Nov 18 16:56:53 2010 Log: Edited wiki page RequestFactory_2_1_1 through web user interface. http://code.google.com/p/google-web-toolkit/source/detail?r=9253 Modified: /wiki/RequestFactory_2_1_1.wiki === --- /wiki/RequestFactory_2_1_1.wiki Mon Nov 15 13:43:22 2010 +++ /wiki/RequestFactory_2_1_1.wiki Thu Nov 18 16:56:53 2010 @@ -11,10 +11,6 @@ * TODO: Allow return types of `EntityProxyId`. * Multiple methods may be invoked in a single `RequestContext` before `fire()` is called. * (Issue 5549) Support boolean is/hasFoo() properties - -= What's in review = - -http://gwt-code-reviews.appspot.com/1108802: * (Issue 5522, issue 5357) Value types / Embedded objects * A user-defined value object must extend the empty ValueProxy interface and declare a @ProxyFor annotation. * ValueProxy instances are never sparse and will implement equals() and hashCode() based on the values in the proxy. @@ -30,6 +26,10 @@ * If you don't want the destructive operation, don't use value objects, or use the to-be-written service helper / Locator to cook up your own id scheme. * Since we currently support Date (and it is mutable) the RF client-side code will use a subclass of Date that can be frozen to ensure that the owning EntityProxy must be edited. += What's in review = + + * (Issue 5368) Remove integer version constraint + = What's coming = * (Issue 5111) Extracting a `ServiceHelper` for `ReflectiveServiceLayer` for a simple API to interface with existing systems @@ -38,7 +38,6 @@ * Support for custom validation strategies. * (Issue 5550) Refactor the Codex types to make them injectable. * (Issue 5523) Client-side caching of requests and EntityProxy persistence - * (Issue 5368) Remove integer version constraint = Add to 2.1.1 smoke testing = * The server components of RequestFactory have been completely rewritten. -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Fix Alignment Attribute Parsing for HasAlignment (issue1121801)
C'mon, give me a little credit; I wouldn't suggest a change without having
tested to prove that it works:) I changed HorizontalPanel to implement
HasHorizontalAlignment and HasVerticalAlignment instead of implementing
HasAlignment. The parsers generate in the correct order and the resulting
code passes all tests with this change (yes, it fails with the existing
code).
Are you sure this feature is being exercised by the current widget set
without issue?
--Stephanie
On Thu, Nov 18, 2010 at 5:24 PM, Ray Ryan wrote:
>
>
> On Thu, Nov 18, 2010 at 11:59 AM, wrote:
>
>>
>> http://gwt-code-reviews.appspot.com/1121801/diff/1/2
>> File
>> user/src/com/google/gwt/uibinder/elementparsers/HasAlignmentParser.java
>> (right):
>>
>> http://gwt-code-reviews.appspot.com/1121801/diff/1/2#newcode34
>>
>> user/src/com/google/gwt/uibinder/elementparsers/HasAlignmentParser.java:34:
>> public class HasAlignmentParser implements ElementParser {
>> On 2010/11/18 19:45:35, rjrjr wrote:
>>
>>> On 2010/11/18 19:34:50, sbrubaker wrote:
>>> > On 2010/11/18 18:22:17, rjrjr wrote:
>>> > > No reason for these to be inner classes.
>>> >
>>> > These classes were inlined per your comment at
>>> > http://gwt-code-reviews.appspot.com/1109801/diff/1/5#newcode1000,
>>>
>>
>> Yes, but they're not as inlined as they sensibly could be.
>>>
>>
>> > but on further
>>> > thought I think it makes more sense to keep them as separate classes
>>>
>> and
>>
>>> > register these parsers as well. This solves the alignment issue for
>>>
>> any class
>>
>>> > that implements HasHorizontalAlignment or HasVerticalAlignment, and
>>>
>> there's no
>>
>>> > harm done to classes that implement HasAlignment (since the relevant
>>> attributes
>>> > are consumed by the first parser pass).
>>>
>>
>> My comment there suggested doing that iff it would actually solve any
>>>
>> problems.
>>
>>> The fact that you had to create HasAlignment to solve the actual bug
>>>
>> at hand
>>
>>> made me wonder.
>>>
>>
>> What will be fixed by the existence of HasHorizontalAlignment and
>>> HasVerticalAlignment?
>>>
>>
>> For any class that implements HasHorizontalAlignment or
>> HasVerticalAlignment rather than HasAlignment, the horizontalAlignment
>> and verticalAlignment tags are effectively useless, for the same reason
>> as HasAlignment. Registering these parsers will fix the parse order for
>> classes that implement the Has*Alignment classes directly.
>>
>>
>> http://gwt-code-reviews.appspot.com/1121801/show
>>
>
> We have lots of widgets that implement HasHorizontalAlignment directly. All
> I'm asking is if you have confirmed that they are broken now, and fixed by
> this parser. It doesn't look like it to me. (M, empiricism.)
>
> I take your point that any future panel that imitates the nasty
> order-dependency of the three HasAlignment panels would be fixed by this.
> But there aren't any that I can see, and I really dislike speculative
> coding.
>
> rjrjr
>
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: RR : First pass at implementing ValueProxy support. (issue1108802)
Thanks for the review, will submit with ValueProxy validation strategy
previously discussed.
http://gwt-code-reviews.appspot.com/1108802/diff/1/20
File
user/src/com/google/gwt/requestfactory/server/RequestFactoryInterfaceValidator.java
(right):
http://gwt-code-reviews.appspot.com/1108802/diff/1/20#newcode1239
user/src/com/google/gwt/requestfactory/server/RequestFactoryInterfaceValidator.java:1239:
* Examine the arguments ond return value of a method and check any
On 2010/11/17 22:23:02, rchandia wrote:
ond -> and
Done.
http://gwt-code-reviews.appspot.com/1108802/diff/1/20#newcode1252
user/src/com/google/gwt/requestfactory/server/RequestFactoryInterfaceValidator.java:1252:
// Check EntityProxy args ond return types against the domain
On 2010/11/17 22:23:02, rchandia wrote:
ond -> and
Done.
http://gwt-code-reviews.appspot.com/1108802/diff/1/22
File user/src/com/google/gwt/requestfactory/server/Resolver.java
(right):
http://gwt-code-reviews.appspot.com/1108802/diff/1/22#newcode91
user/src/com/google/gwt/requestfactory/server/Resolver.java:91: * Used
to map the objecting being resolved and its API slice to the
On 2010/11/17 22:23:02, rchandia wrote:
objecting -> object
Done.
http://gwt-code-reviews.appspot.com/1108802/diff/1/22#newcode185
user/src/com/google/gwt/requestfactory/server/Resolver.java:185: * Given
a method a domain object, return a value that can be encoded by the
On 2010/11/17 22:23:02, rchandia wrote:
Fix javadoc
Done.
http://gwt-code-reviews.appspot.com/1108802/diff/1/22#newcode203
user/src/com/google/gwt/requestfactory/server/Resolver.java:203: *
@param the client object to resolve
On 2010/11/17 22:23:02, rchandia wrote:
missing param name
Done.
http://gwt-code-reviews.appspot.com/1108802/diff/1/27
File user/src/com/google/gwt/requestfactory/shared/RequestContext.java
(left):
http://gwt-code-reviews.appspot.com/1108802/diff/1/27#oldcode28
user/src/com/google/gwt/requestfactory/shared/RequestContext.java:28: *
@return an {...@link EntityProxy} instance of type T
On 2010/11/17 22:23:02, rchandia wrote:
No longer returns an EntityProxy
Done.
http://gwt-code-reviews.appspot.com/1108802/diff/1/29
File user/src/com/google/gwt/requestfactory/shared/Violation.java
(right):
http://gwt-code-reviews.appspot.com/1108802/diff/1/29#newcode46
user/src/com/google/gwt/requestfactory/shared/Violation.java:46: *
method will return a ValueProxy that contains the invalid values.
Changed this API a little so that you can always retrieve the BaseProxy
associated with the violation so it will work the same way for all proxy
types.
http://gwt-code-reviews.appspot.com/1108802/diff/1/30
File
user/src/com/google/gwt/requestfactory/shared/impl/AbstractRequestContext.java
(left):
http://gwt-code-reviews.appspot.com/1108802/diff/1/30#oldcode385
user/src/com/google/gwt/requestfactory/shared/impl/AbstractRequestContext.java:385:
* Resolves an IdMessage into an EntityProxyId.
On 2010/11/17 22:23:02, rchandia wrote:
SimpleEntityProxyId
Done.
http://gwt-code-reviews.appspot.com/1108802/diff/1/35
File
user/src/com/google/gwt/requestfactory/shared/impl/EntityProxyCategory.java
(right):
http://gwt-code-reviews.appspot.com/1108802/diff/1/35#newcode31
user/src/com/google/gwt/requestfactory/shared/impl/EntityProxyCategory.java:31:
* ValueProxies are equal if they are from the same RequestContext and
their
This was a bad copy-and-paste of the javadoc.
http://gwt-code-reviews.appspot.com/1108802/diff/1/41
File
user/src/com/google/gwt/requestfactory/shared/impl/SimpleProxyId.java
(right):
http://gwt-code-reviews.appspot.com/1108802/diff/1/41#newcode22
user/src/com/google/gwt/requestfactory/shared/impl/SimpleProxyId.java:22:
* The base implementation of id objects in the RequestFactory system.
This
On 2010/11/17 22:23:02, rchandia wrote:
This...
Done.
http://gwt-code-reviews.appspot.com/1108802/show
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Add a permissions model to the Chrome NPAPI plugin. (issue1084801)
fabio, can you respond to the windows questions? http://gwt-code-reviews.appspot.com/1084801/diff/57001/58001 File plugins/npapi/DevModeOptions/.classpath (right): http://gwt-code-reviews.appspot.com/1084801/diff/57001/58001#newcode3 plugins/npapi/DevModeOptions/.classpath:3: On 2010/11/18 22:02:02, jat wrote: Shouldn't this use a linked resource relate to GWT_ROOT, and the eclipse files be under $GWT_ROOT/eclipse? yeah, i'll fix this. http://gwt-code-reviews.appspot.com/1084801/diff/57001/58012 File plugins/npapi/DevModeOptions/src/com/google/gwt/devmodeoptions/client/LocalStorage.java (right): http://gwt-code-reviews.appspot.com/1084801/diff/57001/58012#newcode56 plugins/npapi/DevModeOptions/src/com/google/gwt/devmodeoptions/client/LocalStorage.java:56: return this.setItem(key, JSON.stringify(dataObject)); On 2010/11/18 22:02:02, jat wrote: Why return for a void method? because I blindly copied from speedtracer which also does thisfixed locally. http://gwt-code-reviews.appspot.com/1084801/diff/57001/58024 File plugins/npapi/main.cpp (right): http://gwt-code-reviews.appspot.com/1084801/diff/57001/58024#newcode26 plugins/npapi/main.cpp:26: DisableThreadLibraryCalls(hModule); On 2010/11/18 22:02:02, jat wrote: Spacing. fixed locally. http://gwt-code-reviews.appspot.com/1084801/diff/57001/58024#newcode242 plugins/npapi/main.cpp:242: return 0 ; On 2010/11/18 22:02:02, jat wrote: IIUC, this means we silently ignore any events rather than report them as unhandled -- is that right? Chrome calls NPP_HandleEvent in its paint loop on OSX as a workaround for the fact that OSX doesn't support child windows. This is theoretically our opportunity to draw on screen for OSX, but we're not doing that. According to the API, if we aren't handling the event we should return 0. http://gwt-code-reviews.appspot.com/1084801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Adding CellPreviewEvents to Cell Widgets to preview all events that are fired to Cells. This all... (issue1126801)
http://gwt-code-reviews.appspot.com/1126801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Fix Alignment Attribute Parsing for HasAlignment (issue1121801)
On Thu, Nov 18, 2010 at 11:59 AM, wrote:
>
> http://gwt-code-reviews.appspot.com/1121801/diff/1/2
> File
> user/src/com/google/gwt/uibinder/elementparsers/HasAlignmentParser.java
> (right):
>
> http://gwt-code-reviews.appspot.com/1121801/diff/1/2#newcode34
> user/src/com/google/gwt/uibinder/elementparsers/HasAlignmentParser.java:34:
> public class HasAlignmentParser implements ElementParser {
> On 2010/11/18 19:45:35, rjrjr wrote:
>
>> On 2010/11/18 19:34:50, sbrubaker wrote:
>> > On 2010/11/18 18:22:17, rjrjr wrote:
>> > > No reason for these to be inner classes.
>> >
>> > These classes were inlined per your comment at
>> > http://gwt-code-reviews.appspot.com/1109801/diff/1/5#newcode1000,
>>
>
> Yes, but they're not as inlined as they sensibly could be.
>>
>
> > but on further
>> > thought I think it makes more sense to keep them as separate classes
>>
> and
>
>> > register these parsers as well. This solves the alignment issue for
>>
> any class
>
>> > that implements HasHorizontalAlignment or HasVerticalAlignment, and
>>
> there's no
>
>> > harm done to classes that implement HasAlignment (since the relevant
>> attributes
>> > are consumed by the first parser pass).
>>
>
> My comment there suggested doing that iff it would actually solve any
>>
> problems.
>
>> The fact that you had to create HasAlignment to solve the actual bug
>>
> at hand
>
>> made me wonder.
>>
>
> What will be fixed by the existence of HasHorizontalAlignment and
>> HasVerticalAlignment?
>>
>
> For any class that implements HasHorizontalAlignment or
> HasVerticalAlignment rather than HasAlignment, the horizontalAlignment
> and verticalAlignment tags are effectively useless, for the same reason
> as HasAlignment. Registering these parsers will fix the parse order for
> classes that implement the Has*Alignment classes directly.
>
>
> http://gwt-code-reviews.appspot.com/1121801/show
>
We have lots of widgets that implement HasHorizontalAlignment directly. All
I'm asking is if you have confirmed that they are broken now, and fixed by
this parser. It doesn't look like it to me. (M, empiricism.)
I take your point that any future panel that imitates the nasty
order-dependency of the three HasAlignment panels would be fixed by this.
But there aren't any that I can see, and I really dislike speculative
coding.
rjrjr
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Add a permissions model to the Chrome NPAPI plugin. (issue1084801)
LGTM with nits. http://gwt-code-reviews.appspot.com/1084801/diff/57001/58001 File plugins/npapi/DevModeOptions/.classpath (right): http://gwt-code-reviews.appspot.com/1084801/diff/57001/58001#newcode3 plugins/npapi/DevModeOptions/.classpath:3: Shouldn't this use a linked resource relate to GWT_ROOT, and the eclipse files be under $GWT_ROOT/eclipse? http://gwt-code-reviews.appspot.com/1084801/diff/57001/58012 File plugins/npapi/DevModeOptions/src/com/google/gwt/devmodeoptions/client/LocalStorage.java (right): http://gwt-code-reviews.appspot.com/1084801/diff/57001/58012#newcode56 plugins/npapi/DevModeOptions/src/com/google/gwt/devmodeoptions/client/LocalStorage.java:56: return this.setItem(key, JSON.stringify(dataObject)); Why return for a void method? http://gwt-code-reviews.appspot.com/1084801/diff/57001/58022 File plugins/npapi/VisualStudio/npapi-plugin.sln (right): http://gwt-code-reviews.appspot.com/1084801/diff/57001/58022#newcode3 plugins/npapi/VisualStudio/npapi-plugin.sln:3: # Visual Studio 2008 Are we making a conscious decision to drop support for VS2005? http://gwt-code-reviews.appspot.com/1084801/diff/57001/58023 File plugins/npapi/VisualStudio/npapi-plugin.vcproj (right): http://gwt-code-reviews.appspot.com/1084801/diff/57001/58023#newcode74 plugins/npapi/VisualStudio/npapi-plugin.vcproj:74: DataExecutionPrevention="0" Why not DEP if we are switching to VS2008? http://gwt-code-reviews.appspot.com/1084801/diff/57001/58024 File plugins/npapi/main.cpp (right): http://gwt-code-reviews.appspot.com/1084801/diff/57001/58024#newcode26 plugins/npapi/main.cpp:26: DisableThreadLibraryCalls(hModule); Spacing. http://gwt-code-reviews.appspot.com/1084801/diff/57001/58024#newcode242 plugins/npapi/main.cpp:242: return 0 ; IIUC, this means we silently ignore any events rather than report them as unhandled -- is that right? http://gwt-code-reviews.appspot.com/1084801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: First-pass for adding HTML5's Canvas. (issue1082801)
LGTM as long as you fix the nits below and disable the failing tests in FF. http://gwt-code-reviews.appspot.com/1082801/diff/78001/79009 File user/src/com/google/gwt/canvas/dom/client/CssColor.java (right): http://gwt-code-reviews.appspot.com/1082801/diff/78001/79009#newcode27 user/src/com/google/gwt/canvas/dom/client/CssColor.java:27: * To handle devmode we must wrap JSO strings in an array. Therefore, when in devmode, CssColor is Wrap in a paragraph http://gwt-code-reviews.appspot.com/1082801/diff/78001/79009#newcode51 user/src/com/google/gwt/canvas/dom/client/CssColor.java:51: * wrap the String in an array if necessary. Move this comment inline into the method. You already mention the array thing in the class JavaDoc. http://gwt-code-reviews.appspot.com/1082801/diff/78001/79009#newcode66 user/src/com/google/gwt/canvas/dom/client/CssColor.java:66: * unwrap the String if necessary. Move this comment inline into the method. You already mention the array thing in the class JavaDoc. http://gwt-code-reviews.appspot.com/1082801/diff/78001/79010 File user/src/com/google/gwt/canvas/dom/client/FillStrokeStyle.java (right): http://gwt-code-reviews.appspot.com/1082801/diff/78001/79010#newcode39 user/src/com/google/gwt/canvas/dom/client/FillStrokeStyle.java:39: * check its type when in devmode (when isScript == false.) @see CssColor Move this comment inline into the method. Its just an implementation detail. http://gwt-code-reviews.appspot.com/1082801/diff/78001/79014 File user/src/com/google/gwt/dom/client/Document.java (right): http://gwt-code-reviews.appspot.com/1082801/diff/78001/79014#newcode17 user/src/com/google/gwt/dom/client/Document.java:17: extra newline http://gwt-code-reviews.appspot.com/1082801/diff/78001/79016 File user/src/com/google/gwt/user/client/IsSupported.java (right): http://gwt-code-reviews.appspot.com/1082801/diff/78001/79016#newcode23 user/src/com/google/gwt/user/client/IsSupported.java:23: * "boolean isSupported()" that checks whether the canvas method is supported at runtime. Use boolean isSupported() instead of wrapping in quotes. Replace "canvas" with "feature". http://gwt-code-reviews.appspot.com/1082801/diff/78001/79019 File user/test/com/google/gwt/canvas/dom/client/Context2dTest.java (right): http://gwt-code-reviews.appspot.com/1082801/diff/78001/79019#newcode261 user/test/com/google/gwt/canvas/dom/client/Context2dTest.java:261: ImageData verifyPx = context.getImageData(10, 10, 1, 1); // will fail on FF3.0 Why does this fail on FF3? In any case, we test on FF3, so you'll have to do a runtime check and skip this test for FF3. http://gwt-code-reviews.appspot.com/1082801/diff/78001/79019#newcode271 user/test/com/google/gwt/canvas/dom/client/Context2dTest.java:271: // (testing that values are clamped to 0..255 failed due to FF3.5 not implementing it correctly) This is the kind of thing we should handle. In the setter(), go through com.google.gwt.dom.client.DOMImpl and manually clamp the range in Mozilla. http://gwt-code-reviews.appspot.com/1082801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] [google-web-toolkit] r9251 committed - Expenses sample pom.xml. Fixes Issues:...
Revision: 9251 Author: rchan...@google.com Date: Thu Nov 18 10:24:21 2010 Log: Expenses sample pom.xml. Fixes Issues: 5438: Expenses sample pom.xml points to outdated Roo version 5497: Expenses sample cannot find jdo2-api-2.3-ec.jar Review at http://gwt-code-reviews.appspot.com/1124801 http://code.google.com/p/google-web-toolkit/source/detail?r=9251 Modified: /trunk/samples/expenses/pom.xml === --- /trunk/samples/expenses/pom.xml Tue Oct 19 16:22:52 2010 +++ /trunk/samples/expenses/pom.xml Thu Nov 18 10:24:21 2010 @@ -8,7 +8,7 @@ expenses 2.1.0 - 1.1.0.M2 + 1.1.0.RELEASE 3.0.3.RELEASE 1.6.1 1.3.7 @@ -17,14 +17,6 @@ 1.1.5 - - google-maven-snapshot-repository - Google Maven Snapshot Repository - https://oss.sonatype.org/content/repositories/google-snapshots/ - - true - - spring-maven-release Spring Maven Release Repository @@ -610,6 +602,15 @@ datanucleus-enhancer 1.1.4 + + +javax.jdo +jdo2-api +2.3-ec +runtime + -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Adding CellPreviewEvents to Cell Widgets to preview all events that are fired to Cells. This all... (issue1126801)
Reviewers: pdr, Description: Adding CellPreviewEvents to Cell Widgets to preview all events that are fired to Cells. This allows us to add DefaultSelectionEventManager, which adds shift/ctrl selection support to the Cell Widgets. Please review this at http://gwt-code-reviews.appspot.com/1126801/show Affected files: M samples/showcase/src/com/google/gwt/sample/showcase/client/content/cell/ContactTreeViewModel.java M samples/showcase/src/com/google/gwt/sample/showcase/client/content/cell/CwCellTable.java M user/src/com/google/gwt/cell/client/CheckboxCell.java M user/src/com/google/gwt/user/cellview/client/AbstractHasData.java M user/src/com/google/gwt/user/cellview/client/CellBrowser.java M user/src/com/google/gwt/user/cellview/client/CellList.java M user/src/com/google/gwt/user/cellview/client/CellTable.java M user/src/com/google/gwt/user/cellview/client/CellTree.java M user/src/com/google/gwt/user/cellview/client/CellTreeNodeView.java M user/src/com/google/gwt/user/cellview/client/HasDataPresenter.java A user/src/com/google/gwt/view/client/CellPreviewEvent.java A user/src/com/google/gwt/view/client/DefaultSelectionEventManager.java A user/src/com/google/gwt/view/client/HasCellPreviewHandlers.java M user/src/com/google/gwt/view/client/HasData.java M user/src/com/google/gwt/view/client/TreeViewModel.java M user/test/com/google/gwt/cell/client/CheckboxCellTest.java M user/test/com/google/gwt/user/cellview/client/HasDataPresenterTest.java M user/test/com/google/gwt/view/ViewSuite.java M user/test/com/google/gwt/view/client/DefaultNodeInfoTest.java A user/test/com/google/gwt/view/client/DefaultSelectionEventManagerTest.java M user/test/com/google/gwt/view/client/MockHasData.java -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Expenses sample pom.xml. Fixes Issues: (issue1124801)
LGTM http://gwt-code-reviews.appspot.com/1124801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Expenses sample pom.xml. Fixes Issues: (issue1124801)
http://gwt-code-reviews.appspot.com/1124801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Fix Alignment Attribute Parsing for HasAlignment (issue1121801)
http://gwt-code-reviews.appspot.com/1121801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Expenses sample pom.xml. Fixes Issues: (issue1124801)
Reviewers: cromwellian, drfibonacci, Description: Expenses sample pom.xml. Fixes Issues: 5438: Expenses sample pom.xml points to outdated Roo version 5497: Expenses sample cannot find jdo2-api-2.3-ec.jar Please review this at http://gwt-code-reviews.appspot.com/1124801/show Affected files: M samples/expenses/pom.xml Index: samples/expenses/pom.xml === --- samples/expenses/pom.xml(revision 9249) +++ samples/expenses/pom.xml(working copy) @@ -8,7 +8,7 @@ expenses 2.1.0 - 1.1.0.M2 + 1.1.0.RELEASE 3.0.3.RELEASE 1.6.1 1.3.7 @@ -610,6 +610,15 @@ datanucleus-enhancer 1.1.4 + + +javax.jdo +jdo2-api +2.3-ec +runtime + -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: First-pass for adding HTML5's Canvas. (issue1082801)
http://gwt-code-reviews.appspot.com/1082801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Fix Alignment Attribute Parsing for HasAlignment (issue1121801)
http://gwt-code-reviews.appspot.com/1121801/diff/1/2
File
user/src/com/google/gwt/uibinder/elementparsers/HasAlignmentParser.java
(right):
http://gwt-code-reviews.appspot.com/1121801/diff/1/2#newcode34
user/src/com/google/gwt/uibinder/elementparsers/HasAlignmentParser.java:34:
public class HasAlignmentParser implements ElementParser {
On 2010/11/18 19:45:35, rjrjr wrote:
On 2010/11/18 19:34:50, sbrubaker wrote:
> On 2010/11/18 18:22:17, rjrjr wrote:
> > No reason for these to be inner classes.
>
> These classes were inlined per your comment at
> http://gwt-code-reviews.appspot.com/1109801/diff/1/5#newcode1000,
Yes, but they're not as inlined as they sensibly could be.
> but on further
> thought I think it makes more sense to keep them as separate classes
and
> register these parsers as well. This solves the alignment issue for
any class
> that implements HasHorizontalAlignment or HasVerticalAlignment, and
there's no
> harm done to classes that implement HasAlignment (since the relevant
attributes
> are consumed by the first parser pass).
My comment there suggested doing that iff it would actually solve any
problems.
The fact that you had to create HasAlignment to solve the actual bug
at hand
made me wonder.
What will be fixed by the existence of HasHorizontalAlignment and
HasVerticalAlignment?
For any class that implements HasHorizontalAlignment or
HasVerticalAlignment rather than HasAlignment, the horizontalAlignment
and verticalAlignment tags are effectively useless, for the same reason
as HasAlignment. Registering these parsers will fix the parse order for
classes that implement the Has*Alignment classes directly.
http://gwt-code-reviews.appspot.com/1121801/show
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Issues 5479, 5507 and 5571 (and a few other editor-related tweaks) (issue1099801)
http://gwt-code-reviews.appspot.com/1099801/diff/26001/27026
File
user/test/com/google/gwt/uibinder/elementparsers/NumberLabelParserTest.java
(right):
http://gwt-code-reviews.appspot.com/1099801/diff/26001/27026#newcode145
user/test/com/google/gwt/uibinder/elementparsers/NumberLabelParserTest.java:145:
b.append("");
On 2010/11/18 10:17:55, tbroyer wrote:
I agree. Do you want me to create another review with this change?
(and update
this one rely on it; or rather include the NumberFormat change right
into this
review?)
Let's get this landed and do that as a separate patch (and add
DATE_DEFAULT, TIME_DEFAULT, and DATE_TIME_DEFAULT to
DateTimeFormat.PredefinedFormat as well).
There's a difference with DateLabelParser though, in that
PredefinedFormat.CURRENCY would still have to be special-cased in the
NumberLabelParser, at least when used with a currency code.
A currency code can be supplied in other cases as well, so we already
have to deal with those.
http://gwt-code-reviews.appspot.com/1099801/show
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Issues 5479, 5507 and 5571 (and a few other editor-related tweaks) (issue1099801)
On Thu, Nov 18, 2010 at 2:35 PM, wrote: > @jat, I'll be on vacation from tomorrow through Thanksgiving. Will you > be able to land this for Thomas, and get it on to the 2.1.1 branch while > I'm gone? Sure. -- John A. Tamplin Software Engineer (GWT), Google -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Fix Alignment Attribute Parsing for HasAlignment (issue1121801)
http://gwt-code-reviews.appspot.com/1121801/diff/1/2
File
user/src/com/google/gwt/uibinder/elementparsers/HasAlignmentParser.java
(right):
http://gwt-code-reviews.appspot.com/1121801/diff/1/2#newcode34
user/src/com/google/gwt/uibinder/elementparsers/HasAlignmentParser.java:34:
public class HasAlignmentParser implements ElementParser {
On 2010/11/18 19:34:50, sbrubaker wrote:
On 2010/11/18 18:22:17, rjrjr wrote:
> No reason for these to be inner classes.
These classes were inlined per your comment at
http://gwt-code-reviews.appspot.com/1109801/diff/1/5#newcode1000,
Yes, but they're not as inlined as they sensibly could be.
but on further
thought I think it makes more sense to keep them as separate classes
and
register these parsers as well. This solves the alignment issue for
any class
that implements HasHorizontalAlignment or HasVerticalAlignment, and
there's no
harm done to classes that implement HasAlignment (since the relevant
attributes
are consumed by the first parser pass).
My comment there suggested doing that iff it would actually solve any
problems. The fact that you had to create HasAlignment to solve the
actual bug at hand made me wonder.
What will be fixed by the existence of HasHorizontalAlignment and
HasVerticalAlignment?
http://gwt-code-reviews.appspot.com/1121801/diff/1/2#newcode37
user/src/com/google/gwt/uibinder/elementparsers/HasAlignmentParser.java:37:
* Parses widgets that inherit from {...@link
com.google.gwt.user.client.ui.HasHorizontalAlignment}.
On 2010/11/18 19:34:50, sbrubaker wrote:
On 2010/11/18 18:22:17, rjrjr wrote:
> A lot of long lines. Please check your auto format settings.
Ran this file through checkstyle again and not seeing any issues.
Checkstyle doesn't enforce line length. None the less, GWT code is
expected to wrap at 80 characters. Please use our standard autoformat
settings, per trunk/eclipse/README.txt
http://gwt-code-reviews.appspot.com/1121801/show
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Issues 5479, 5507 and 5571 (and a few other editor-related tweaks) (issue1099801)
The general direction of the patch looks great to me.
@jat, I'll be on vacation from tomorrow through Thanksgiving. Will you
be able to land this for Thomas, and get it on to the 2.1.1 branch while
I'm gone?
http://gwt-code-reviews.appspot.com/1099801/diff/26001/27004
File
user/src/com/google/gwt/uibinder/elementparsers/DateLabelParser.java
(right):
http://gwt-code-reviews.appspot.com/1099801/diff/26001/27004#newcode36
user/src/com/google/gwt/uibinder/elementparsers/DateLabelParser.java:36:
static final String NO_TIMEZONE_WITHOUT_SPECIFIED_FORMAT = "May not
specify a time zone if no format is given.";
Sounds good, and no need to mess with the other tests.
On 2010/11/18 10:17:55, tbroyer wrote:
On 2010/11/18 05:46:19, rjrjr wrote:
> Why are these not private?
Testing.
(I believe I copied this from another class but can find it back; but
clearly
other classes could benefit from it: LayoutPanelParserTest for
instance could
use the constants from LayoutPanelParser instead of duplicating the
string
values)
http://gwt-code-reviews.appspot.com/1099801/diff/26001/27009
File user/src/com/google/gwt/user/client/ui/DateLabel.java (right):
http://gwt-code-reviews.appspot.com/1099801/diff/26001/27009#newcode33
user/src/com/google/gwt/user/client/ui/DateLabel.java:33: public
DateLabel(DateTimeFormat format) {
I see. Yes, if you could punch up the javadoc, something like:
Extends ValueLabel for convenience when dealing with dates and
DateTimeFormat, especially in UiBinder templates. (Note that this class
does not accept renderers. To do so use ValueLabel directly.)
The use of DateTimeFormatRenderer really is an implementation detail,
shouldn't be metioned in the doc.
On 2010/11/18 10:17:55, tbroyer wrote:
On 2010/11/18 05:46:19, rjrjr wrote:
> This is bad, how can I give this thing a renderer?
My reasoning was that if you do have a renderer, you don't need a
DateLabel,
just use a ValueLabel instead. Maybe it should just be
documented?
http://gwt-code-reviews.appspot.com/1099801/diff/26001/27013
File user/src/com/google/gwt/user/client/ui/NumberLabel.java (right):
http://gwt-code-reviews.appspot.com/1099801/diff/26001/27013#newcode32
user/src/com/google/gwt/user/client/ui/NumberLabel.java:32: public
NumberLabel(NumberFormat format) {
On 2010/11/18 10:17:55, tbroyer wrote:
On 2010/11/18 05:46:19, rjrjr wrote:
> Again, should be Renderer
Same rationale as for DateLabel: if you have a Renderer at
hand, just
use a ValueLabel, the whole point of DateLabel and NumberLabel
is to tie
them to specific renderers.
Quibble: the whole point is to make it convenient to deal w/them and
format objects, especially from mark up. The renderer is just an
implementation detail.
http://gwt-code-reviews.appspot.com/1099801/diff/26001/27014
File user/src/com/google/gwt/user/client/ui/SimpleCheckBox.java (right):
http://gwt-code-reviews.appspot.com/1099801/diff/26001/27014#newcode235
user/src/com/google/gwt/user/client/ui/SimpleCheckBox.java:235: //
overrides in RadioButton
HasValue really is the right thing.
I'd hold off on setName, and make editor compatibility the goal for this
patch. Surely we can get CheckBox to extend SimpleCheckBox down the
road.
On 2010/11/18 10:17:55, tbroyer wrote:
On 2010/11/18 05:46:19, rjrjr wrote:
> RadioButton doesn't override this, copy paste error.
>
> I don't see SimpleRadioButton in this patch, shouldn't I?
Ah oops! Maybe we should just stick with TakesValue instead of
HasValue then, it
would make things much simpler! ;-)
(I also note that SimpleCheckBox/SimpleRadioButton don't have a
setName like
CheckBox/RadioButton)
http://gwt-code-reviews.appspot.com/1099801/diff/26001/27025
File
user/test/com/google/gwt/uibinder/elementparsers/DateLabelParserTest.java
(right):
http://gwt-code-reviews.appspot.com/1099801/diff/26001/27025#newcode230
user/test/com/google/gwt/uibinder/elementparsers/DateLabelParserTest.java:230:
public void testChokeOnUnknownPredefinedFormat() throws SAXException {
On 2010/11/18 10:17:55, tbroyer wrote:
On 2010/11/15 22:14:37, jat wrote:
> Should there be a test that shows predefinedFormat working and
having the
right
> output?
Added a testHappyWithPredefinedFormat and testHappyWithTimeZoneOffset.
(I wonder whether there should be explicit tests for other things,
given that
they're implicitly tested in other tests; for instance, timezone is
tested in
testHappyWithSubclassWithDateTimeFormatAndTimeZoneConstructor, and
format is
tested in all testHappy* tests except testHappyWithNoFormat and
testHappyWithPredefinedFormat)
I think not.
http://gwt-code-reviews.appspot.com/1099801/diff/26001/27026
File
user/test/com/google/gwt/uibinder/elementparsers/NumberLabelParserTest.java
(right):
http://gwt-code-reviews.appspot.com/1099801/diff/26001/27026#newcode145
user/test/com/google/gwt/uibinder/elementparsers/NumberLabelParserTest.java:145:
b.append("");
On 2010/11/18 10:17:55, tbroyer wrote:
On 2010/11/15 22:14:37, jat wrote
[gwt-contrib] Re: Fix Alignment Attribute Parsing for HasAlignment (issue1121801)
http://gwt-code-reviews.appspot.com/1121801/diff/1/2
File
user/src/com/google/gwt/uibinder/elementparsers/HasAlignmentParser.java
(right):
http://gwt-code-reviews.appspot.com/1121801/diff/1/2#newcode34
user/src/com/google/gwt/uibinder/elementparsers/HasAlignmentParser.java:34:
public class HasAlignmentParser implements ElementParser {
On 2010/11/18 18:22:17, rjrjr wrote:
No reason for these to be inner classes.
These classes were inlined per your comment at
http://gwt-code-reviews.appspot.com/1109801/diff/1/5#newcode1000, but on
further thought I think it makes more sense to keep them as separate
classes and register these parsers as well. This solves the alignment
issue for any class that implements HasHorizontalAlignment or
HasVerticalAlignment, and there's no harm done to classes that implement
HasAlignment (since the relevant attributes are consumed by the first
parser pass).
http://gwt-code-reviews.appspot.com/1121801/diff/1/2#newcode37
user/src/com/google/gwt/uibinder/elementparsers/HasAlignmentParser.java:37:
* Parses widgets that inherit from {...@link
com.google.gwt.user.client.ui.HasHorizontalAlignment}.
On 2010/11/18 18:22:17, rjrjr wrote:
A lot of long lines. Please check your auto format settings.
Ran this file through checkstyle again and not seeing any issues.
http://gwt-code-reviews.appspot.com/1121801/show
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Makes part of the Compiler Report (SOYC) smaller by (issue1123801)
Reviewers: kathrin, Description: Makes part of the Compiler Report (SOYC) smaller by replacing a flat HTML output with output that is generated with JavaScript from a dictionary of strings. This decreases the size of the dependency reports by a factor of 5 and the overall report by a factor of 4. There is a difference in the time to display for a report of a large app when running in Chrome for what was a 5MB report. (It takes longer to build the report using javascript) Please review this at http://gwt-code-reviews.appspot.com/1123801/show Affected files: M dev/core/src/com/google/gwt/soyc/MakeTopLevelHtmlForPerm.java -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] [google-web-toolkit] r9250 committed - Minor little fix in DefaultSuggestionDisplay...
Revision: 9250
Author: porte...@google.com
Date: Thu Nov 18 07:58:46 2010
Log: Minor little fix in DefaultSuggestionDisplay
Review at http://gwt-code-reviews.appspot.com/1122801
Review by: jlaba...@google.com
http://code.google.com/p/google-web-toolkit/source/detail?r=9250
Modified:
/trunk/user/src/com/google/gwt/user/client/ui/SuggestBox.java
===
--- /trunk/user/src/com/google/gwt/user/client/ui/SuggestBox.java Wed Oct
27 07:59:52 2010
+++ /trunk/user/src/com/google/gwt/user/client/ui/SuggestBox.java Thu Nov
18 07:58:46 2010
@@ -678,11 +678,11 @@
private final TextBoxBase box;
private final Callback callback = new Callback() {
public void onSuggestionsReady(Request request, Response response) {
+ display.setMoreSuggestions(response.hasMoreSuggestions(),
+ response.getMoreSuggestionsCount());
display.showSuggestions(SuggestBox.this, response.getSuggestions(),
oracle.isDisplayStringHTML(), isAutoSelectEnabled(),
suggestionCallback);
- display.setMoreSuggestions(response.hasMoreSuggestions(),
- response.getMoreSuggestionsCount());
}
};
private final SuggestionCallback suggestionCallback = new
SuggestionCallback() {
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Minor little fix in DefaultSuggestionDisplay (issue1122801)
LGTM http://gwt-code-reviews.appspot.com/1122801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Fix Alignment Attribute Parsing for HasAlignment (issue1121801)
http://gwt-code-reviews.appspot.com/1121801/diff/1/2
File
user/src/com/google/gwt/uibinder/elementparsers/HasAlignmentParser.java
(right):
http://gwt-code-reviews.appspot.com/1121801/diff/1/2#newcode34
user/src/com/google/gwt/uibinder/elementparsers/HasAlignmentParser.java:34:
public class HasAlignmentParser implements ElementParser {
No reason for these to be inner classes.
http://gwt-code-reviews.appspot.com/1121801/diff/1/2#newcode37
user/src/com/google/gwt/uibinder/elementparsers/HasAlignmentParser.java:37:
* Parses widgets that inherit from {...@link
com.google.gwt.user.client.ui.HasHorizontalAlignment}.
A lot of long lines. Please check your auto format settings.
http://gwt-code-reviews.appspot.com/1121801/show
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Public: Improve reporting of TCK failures (issue1117801)
http://gwt-code-reviews.appspot.com/1117801/diff/1/21
File
samples/validationtck/test/com/google/gwt/sample/validationtck/validation/ValidatePropertyTest.java
(right):
http://gwt-code-reviews.appspot.com/1117801/diff/1/21#newcode38
samples/validationtck/test/com/google/gwt/sample/validationtck/validation/ValidatePropertyTest.java:38:
try {
Are you sure you need to wrap the test in a try/catch? Usually such
tests internally wrap the call with their own try/catch's and exceptions
escaping the test are genuine errors/failures.
http://gwt-code-reviews.appspot.com/1117801/show
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Fix Alignment Attribute Parsing for HasAlignment (issue1117802)
Reviewers: rjrjr, Description: Fix Alignment Attribute Parsing for HasAlignment Review by: rj...@google.com Please review this at http://gwt-code-reviews.appspot.com/1117802/show Affected files: A user/src/com/google/gwt/uibinder/elementparsers/HasAlignmentParser.java M user/src/com/google/gwt/uibinder/rebind/UiBinderWriter.java A user/test/com/google/gwt/uibinder/elementparsers/HasAlignmentParserTest.java M user/test/com/google/gwt/uibinder/test/client/UiBinderTest.java M user/test/com/google/gwt/uibinder/test/client/WidgetBasedUi.java M user/test/com/google/gwt/uibinder/test/client/WidgetBasedUi.ui.xml -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Fix Alignment Attribute Parsing for HasAlignment (issue1121801)
Reviewers: rjrjr, Description: Fix Alignment Attribute Parsing for HasAlignment Review by: rj...@google.com Please review this at http://gwt-code-reviews.appspot.com/1121801/show Affected files: A user/src/com/google/gwt/uibinder/elementparsers/HasAlignmentParser.java M user/src/com/google/gwt/uibinder/rebind/UiBinderWriter.java A user/test/com/google/gwt/uibinder/elementparsers/HasAlignmentParserTest.java M user/test/com/google/gwt/uibinder/test/client/UiBinderTest.java M user/test/com/google/gwt/uibinder/test/client/WidgetBasedUi.java M user/test/com/google/gwt/uibinder/test/client/WidgetBasedUi.ui.xml -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: IE devmode plugin: 64 bits support end-to-end, build fixes & cleanup, other polishing items. (issue1116801)
http://gwt-code-reviews.appspot.com/1116801/diff/1/6 File plugins/ie/installer/build.cmd (right): http://gwt-code-reviews.appspot.com/1116801/diff/1/6#newcode8 plugins/ie/installer/build.cmd:8: echo IMPORTANT: Make sure "%~dp0oophm.wsx" is checked out and writable! Rather than just blindly warning about this, why not test and error if the files aren't writable? Also, s/prebuild/prebuilt http://gwt-code-reviews.appspot.com/1116801/diff/1/11 File plugins/ie/installer/wix/candle.exe.config (right): http://gwt-code-reviews.appspot.com/1116801/diff/1/11#newcode3 plugins/ie/installer/wix/candle.exe.config:3: Copyright (c) Microsoft Corporation. All rights reserved. does this msft copyright need to be here? if anything, shouldn't all these new files be getting the standard GWT copyright notice? http://gwt-code-reviews.appspot.com/1116801/diff/1/15 File plugins/ie/oophm/oophm/dllmain.cpp (right): http://gwt-code-reviews.appspot.com/1116801/diff/1/15#newcode36 plugins/ie/oophm/oophm/dllmain.cpp:36: AllowDialog::setHInstance(hInstance); indentation looks off here http://gwt-code-reviews.appspot.com/1116801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Add a permissions model to the Chrome NPAPI plugin. (issue1084801)
On 2010/11/15 19:53:51, conroy wrote: lgtm http://gwt-code-reviews.appspot.com/1084801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Fixes issue http://code.google.com/p/google-web-toolkit/issues/detail?id=5578 (issue1098801)
LGTM http://gwt-code-reviews.appspot.com/1098801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Issues 5479, 5507 and 5571 (and a few other editor-related tweaks) (issue1099801)
http://gwt-code-reviews.appspot.com/1099801/diff/26001/27004
File
user/src/com/google/gwt/uibinder/elementparsers/DateLabelParser.java
(right):
http://gwt-code-reviews.appspot.com/1099801/diff/26001/27004#newcode36
user/src/com/google/gwt/uibinder/elementparsers/DateLabelParser.java:36:
static final String NO_TIMEZONE_WITHOUT_SPECIFIED_FORMAT = "May not
specify a time zone if no format is given.";
On 2010/11/18 05:46:19, rjrjr wrote:
Why are these not private?
Testing.
(I believe I copied this from another class but can find it back; but
clearly other classes could benefit from it: LayoutPanelParserTest for
instance could use the constants from LayoutPanelParser instead of
duplicating the string values)
http://gwt-code-reviews.appspot.com/1099801/diff/26001/27005
File
user/src/com/google/gwt/uibinder/elementparsers/NumberLabelParser.java
(right):
http://gwt-code-reviews.appspot.com/1099801/diff/26001/27005#newcode70
user/src/com/google/gwt/uibinder/elementparsers/NumberLabelParser.java:70:
writer.getOracle().findType(CurrencyData.class.getCanonicalName()));
On 2010/11/15 22:14:37, jat wrote:
How do you get a CurrencyData instance from the ui.xml file?
using a field reference:
This was only added for completeness.
http://gwt-code-reviews.appspot.com/1099801/diff/26001/27009
File user/src/com/google/gwt/user/client/ui/DateLabel.java (right):
http://gwt-code-reviews.appspot.com/1099801/diff/26001/27009#newcode33
user/src/com/google/gwt/user/client/ui/DateLabel.java:33: public
DateLabel(DateTimeFormat format) {
On 2010/11/18 05:46:19, rjrjr wrote:
This is bad, how can I give this thing a renderer?
My reasoning was that if you do have a renderer, you don't need a
DateLabel, just use a ValueLabel instead. Maybe it should just be
documented?
http://gwt-code-reviews.appspot.com/1099801/diff/26001/27012
File user/src/com/google/gwt/user/client/ui/LabelBase.java (right):
http://gwt-code-reviews.appspot.com/1099801/diff/26001/27012#newcode83
user/src/com/google/gwt/user/client/ui/LabelBase.java:83: *
{...@inheritdoc}
On 2010/11/18 05:46:19, rjrjr wrote:
We don't seem to do the inheritdoc thing
It was in Label.
http://code.google.com/p/google-web-toolkit/source/browse/trunk/user/src/com/google/gwt/user/client/ui/Label.java?r=9187#319
I just used Eclipse's "extract superclass" tool.
I've removed it.
http://gwt-code-reviews.appspot.com/1099801/diff/26001/27013
File user/src/com/google/gwt/user/client/ui/NumberLabel.java (right):
http://gwt-code-reviews.appspot.com/1099801/diff/26001/27013#newcode32
user/src/com/google/gwt/user/client/ui/NumberLabel.java:32: public
NumberLabel(NumberFormat format) {
On 2010/11/18 05:46:19, rjrjr wrote:
Again, should be Renderer
Same rationale as for DateLabel: if you have a Renderer at hand,
just use a ValueLabel, the whole point of DateLabel and
NumberLabel is to tie them to specific renderers.
http://gwt-code-reviews.appspot.com/1099801/diff/26001/27014
File user/src/com/google/gwt/user/client/ui/SimpleCheckBox.java (right):
http://gwt-code-reviews.appspot.com/1099801/diff/26001/27014#newcode235
user/src/com/google/gwt/user/client/ui/SimpleCheckBox.java:235: //
overrides in RadioButton
On 2010/11/18 05:46:19, rjrjr wrote:
RadioButton doesn't override this, copy paste error.
I don't see SimpleRadioButton in this patch, shouldn't I?
Ah oops! Maybe we should just stick with TakesValue instead of HasValue
then, it would make things much simpler! ;-)
(I also note that SimpleCheckBox/SimpleRadioButton don't have a setName
like CheckBox/RadioButton)
http://gwt-code-reviews.appspot.com/1099801/diff/26001/27025
File
user/test/com/google/gwt/uibinder/elementparsers/DateLabelParserTest.java
(right):
http://gwt-code-reviews.appspot.com/1099801/diff/26001/27025#newcode230
user/test/com/google/gwt/uibinder/elementparsers/DateLabelParserTest.java:230:
public void testChokeOnUnknownPredefinedFormat() throws SAXException {
On 2010/11/15 22:14:37, jat wrote:
Should there be a test that shows predefinedFormat working and having
the right
output?
Added a testHappyWithPredefinedFormat and testHappyWithTimeZoneOffset.
(I wonder whether there should be explicit tests for other things, given
that they're implicitly tested in other tests; for instance, timezone is
tested in testHappyWithSubclassWithDateTimeFormatAndTimeZoneConstructor,
and format is tested in all testHappy* tests except
testHappyWithNoFormat and testHappyWithPredefinedFormat)
http://gwt-code-reviews.appspot.com/1099801/diff/26001/27026
File
user/test/com/google/gwt/uibinder/elementparsers/NumberLabelParserTest.java
(right):
http://gwt-code-reviews.appspot.com/1099801/diff/26001/27026#newcode145
user/test/com/google/gwt/uibinder/elementparsers/NumberLabelParserTest.java:145:
b.append("");
On 2010/11/15 22:14:37, jat wrote:
I like the parallelism with DateTimeFormat, and we probably should
make a
similar change to it that includes a PredefinedFormat enum.
I agree. Do you want me to create ano
Re: [gwt-contrib] Re: GWT 2.1 / gwt-incubator tables
a customer is a broad concept :-) On Wed, Nov 17, 2010 at 10:04 PM, Stephen Haberman wrote: > >> I complained about the same subject when 2.1 was released. After 2 >> years of waiting very little functionality in the new table was >> released. I can understand the motivation of the GWT team, but they do >> not seem to understand their customers very well. > > Wow, tough crowd. I would have reserved the term "customers" for > commercial software. :-) > > - Stephen > > -- > http://groups.google.com/group/Google-Web-Toolkit-Contributors -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
