Generally, looks good, but wouldn't mind hearing your comments to my
questions before the submit.
https://gwt-code-reviews.appspot.com/1646803/diff/1/user/src/com/google/web/bindery/requestfactory/server/Resolver.java
File user/src/com/google/web/bindery/requestfactory/server/Resolver.java
(righ
Generally looks good, just a couple of suggestions. Let's go one more
round on this.
http://gwt-code-reviews.appspot.com/1601806/diff/1015/user/src/com/google/web/bindery/requestfactory/shared/impl/AbstractRequestContext.java
File
user/src/com/google/web/bindery/requestfactory/shared/impl/Abstra
Ping. Brian, can you take a final look at this?
http://gwt-code-reviews.appspot.com/1601805/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
On 2012/04/06 18:44:50, rdayal wrote:
LGTM.
Submitted.
http://gwt-code-reviews.appspot.com/1615805/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
Looks like this one is not going to be accepted.
@portersi: If you could close this issue, that would be great.
http://gwt-code-reviews.appspot.com/1503806/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
LGTM
http://gwt-code-reviews.appspot.com/1614806/diff/1/user/src/com/google/gwt/dom/client/StyleInjector.java
File user/src/com/google/gwt/dom/client/StyleInjector.java (right):
http://gwt-code-reviews.appspot.com/1614806/diff/1/user/src/com/google/gwt/dom/client/StyleInjector.java#newcode219
u
@cromwellian: Can you take another look so we can get this in?
http://gwt-code-reviews.appspot.com/1618807/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
Great, thanks!
http://gwt-code-reviews.appspot.com/1602805/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
Rodrigo, has this been submitted?
http://gwt-code-reviews.appspot.com/1602805/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
Looks good to me, but I'd like John to take a look.
http://gwt-code-reviews.appspot.com/1573803/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
LGTM.
http://gwt-code-reviews.appspot.com/1580804/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
On 2012/04/05 21:55:42, rdayal wrote:
LGTM.
Submitted.
http://gwt-code-reviews.appspot.com/1580804/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
Submitted.
http://gwt-code-reviews.appspot.com/1582803/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
On 2012/04/05 18:59:13, rdayal wrote:
Jason, can you look at this one more time? Sorry about the delay. Had to
make a couple of changes:
1) You can't map null values in a ConcurrentHashMap. Bad suggestion on
my part.
2) It's a change in behavior to pass null into readContent. Chang
LGTM.
http://gwt-code-reviews.appspot.com/1615805/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
On 2012/04/06 15:00:31, rdayal wrote:
Thanks!
On Fri Apr 06 10:52:30 GMT-400 2012, <mailto:jlaba...@google.com>
wrote:
> LGTM
>
>
http://gwt-code-reviews.appspot.com/1573803/%3Chttps://www.google.com/url?sa=D&q=http://gwt-code-reviews.appspot.com/1573803/>
>
On 2011/12/11 18:32:26, Jim Douglas wrote:
LGTM.
submitted.
http://gwt-code-reviews.appspot.com/1610804/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
On 2012/04/06 18:30:56, rdayal wrote:
On 2011/12/17 07:00:31, stephenh wrote:
LGTM.
Submitted.
http://gwt-code-reviews.appspot.com/1618805/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
http://gwt-code-reviews.appspot.com/1632803/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
On 2011/12/17 07:00:31, stephenh wrote:
LGTM.
http://gwt-code-reviews.appspot.com/1618805/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
LGTM.
http://gwt-code-reviews.appspot.com/1679803/diff/1/user/src/com/google/gwt/resources/css/ast/CssFontFace.java
File user/src/com/google/gwt/resources/css/ast/CssFontFace.java (right):
http://gwt-code-reviews.appspot.com/1679803/diff/1/user/src/com/google/gwt/resources/css/ast/CssFontFace.j
Ping. Can we get this review wrapped up? I'd like to push this into GWT
2.5 (or close the issue completely, if not).
http://gwt-code-reviews.appspot.com/1513803/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
Ping. Is this patch dead, or do we still want to get this in?
http://gwt-code-reviews.appspot.com/1490801/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
On 2012/04/05 19:27:55, Jason Terk wrote:
LGTM
Thanks, submitted.
http://gwt-code-reviews.appspot.com/1632803/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
LGTM, with a couple of nits.
http://gwt-code-reviews.appspot.com/1629803/diff/1/user/src/com/google/gwt/uibinder/rebind/GetEscapedInnerTextVisitor.java
File
user/src/com/google/gwt/uibinder/rebind/GetEscapedInnerTextVisitor.java
(right):
http://gwt-code-reviews.appspot.com/1629803/diff/1/user/s
http://gwt-code-reviews.appspot.com/1632803/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
On 2012/01/30 22:31:09, rdayal wrote:
http://gwt-code-reviews.appspot.com/1632803/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
http://gwt-code-reviews.appspot.com/1632803/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
Reviewers: Jason Terk, tbroyer_gmail.com, skybrian_gmail.com,
Description:
Avoid Java bottleneck by using explicit Charset for byte[] <-> String
conversions.
Repost of Rietveld issue 158803.
Patch By: Jason Terk
Please review this at http://gwt-code-reviews.appspot.com/1632803/
Affected files
LGTM.
http://gwt-code-reviews.appspot.com/1588803/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
http://gwt-code-reviews.appspot.com/1628803/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
Reviewers: tbroyer,
Description:
When encoding the payload, make sure that we're using the JSONified
version of the object with non-collection encodes; otherwise we'll end
up encoding a String as [Object] in web mode.
Also remove the toString() on JsonSplittable; the proper implementation
of toS
http://gwt-code-reviews.appspot.com/1609804/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
Based on Thomas' comments, I'm inclined to leave it, but I agree that
unit tests need to be put in place. Those will be in a forthcoming CL.
http://gwt-code-reviews.appspot.com/1618806/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
On 2011/12/20 22:31:52, rdayal wrote:
On 2011/12/20 20:21:45, skybrian wrote:
>
http://gwt-code-reviews.appspot.com/1618806/diff/1003/user/src/com/google/web/bindery/requestfactory/shared/impl/AbstractRequestContext.java
> File
>
user/src/com/google/web/bindery/requestfactory/sh
On 2011/12/20 20:21:45, skybrian wrote:
http://gwt-code-reviews.appspot.com/1618806/diff/1003/user/src/com/google/web/bindery/requestfactory/shared/impl/AbstractRequestContext.java
File
user/src/com/google/web/bindery/requestfactory/shared/impl/AbstractRequestContext.java
(right):
http://g
(to be clear, the patch with test will be part of a separate issue).
http://gwt-code-reviews.appspot.com/1618806/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
Thanks very much for the review! Your point is taken on tests; I'm
currently in the process of writing some rudimentary tests for JSON-RPC
in RequestFactory.
As you mention, these tests will not be integration-level tests at
first, as there is no RF server side in the testing infrastructure (as
y
http://gwt-code-reviews.appspot.com/1618806/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
http://gwt-code-reviews.appspot.com/1618806/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
Reviewers: tbroyer, jasonhall, Jeff Larsen,
Description:
Properly encode request parameters that use Collections when in JSON-RPC
mode.
Please review this at http://gwt-code-reviews.appspot.com/1618806/
Affected files:
M
user/src/com/google/web/bindery/requestfactory/shared/impl/AbstractRe
Thanks for doing this!
A few comments.
http://gwt-code-reviews.appspot.com/1588803/diff/2001/src/com/google/gwt/user/server/rpc/RPCServletUtils.java
File src/com/google/gwt/user/server/rpc/RPCServletUtils.java (right):
http://gwt-code-reviews.appspot.com/1588803/diff/2001/src/com/google/gwt/us
Reviewers: rice, Daniel Kurka,
Description:
Fix issue 6834.
Repost of Rietveld issue 1563803.
Patch by: Daniel Kurka
Please review this at http://gwt-code-reviews.appspot.com/1609804/
Affected files:
M user/src/com/google/gwt/geolocation/client/Geolocation.java
Index: user/src/com/google/
Hi Daniel,
Did you sign the GWT CLA? I was about to submit your patch, but I
noticed that you're not on the list..
Rajeev
http://gwt-code-reviews.appspot.com/1563803/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
Reviewers: johnlenz_google.com,
Description:
Add Closure compiler jar to the classpath.
Please review this at http://gwt-code-reviews.appspot.com/1610803/
Affected files:
M eclipse/dev/.classpath
Index: eclipse/dev/.classpath
Reviewers: unnurg,
Description:
Avoid bottlenecks by using ConcurrentHashMap instead of a synchronized
IdentityHashMap.
Repost from Rietveld issue 1582804
Thanks to Jason Terk for the original patch!
Please review this at http://gwt-code-reviews.appspot.com/1609803/
Affected files:
M user/sr
LGTM.
http://gwt-code-reviews.appspot.com/1602805/diff/1/user/test/com/google/gwt/uibinder/test/client/UiProvidedNullTest.java
File
user/test/com/google/gwt/uibinder/test/client/UiProvidedNullTest.java
(right):
http://gwt-code-reviews.appspot.com/1602805/diff/1/user/test/com/google/gwt/uibinder
Brian, can you review this CL?
http://gwt-code-reviews.appspot.com/1601805/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
LGTM. Thanks for doing this!
http://gwt-code-reviews.appspot.com/1586803/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
LGTM
http://gwt-code-reviews.appspot.com/1578810/diff/3001/dev/core/test/com/google/gwt/dev/util/StringInternerTest.java
File dev/core/test/com/google/gwt/dev/util/StringInternerTest.java
(right):
http://gwt-code-reviews.appspot.com/1578810/diff/3001/dev/core/test/com/google/gwt/dev/util/String
http://gwt-code-reviews.appspot.com/1582803/diff/1/user/src/com/google/gwt/user/client/ui/FormPanel.java
File user/src/com/google/gwt/user/client/ui/FormPanel.java (right):
http://gwt-code-reviews.appspot.com/1582803/diff/1/user/src/com/google/gwt/user/client/ui/FormPanel.java#newcode138
user/sr
http://gwt-code-reviews.appspot.com/1578810/diff/1/dev/core/test/com/google/gwt/dev/util/StringInternerTest.java
File dev/core/test/com/google/gwt/dev/util/StringInternerTest.java
(right):
http://gwt-code-reviews.appspot.com/1578810/diff/1/dev/core/test/com/google/gwt/dev/util/StringInternerTest
LGTM.
http://gwt-code-reviews.appspot.com/1578809/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
LGTM.
http://gwt-code-reviews.appspot.com/1578806/diff/1/user/src/com/google/gwt/uibinder/rebind/UiBinderGenerator.java
File user/src/com/google/gwt/uibinder/rebind/UiBinderGenerator.java
(right):
http://gwt-code-reviews.appspot.com/1578806/diff/1/user/src/com/google/gwt/uibinder/rebind/UiBind
LGTM.
http://gwt-code-reviews.appspot.com/1529804/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
LGTM.
http://gwt-code-reviews.appspot.com/1453808/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
http://gwt-code-reviews.appspot.com/1453808/diff/1/dev/core/src/com/google/gwt/core/ext/debug/JsoEval.java
File dev/core/src/com/google/gwt/core/ext/debug/JsoEval.java (right):
http://gwt-code-reviews.appspot.com/1453808/diff/1/dev/core/src/com/google/gwt/core/ext/debug/JsoEval.java#newcode47
de
Thanks for looking into this.
A couple of questions/comments before I give the LGTM..
http://gwt-code-reviews.appspot.com/1224801/diff/1/2
File user/src/com/google/gwt/junit/client/GWTTestCase.java (right):
http://gwt-code-reviews.appspot.com/1224801/diff/1/2#newcode445
user/src/com/google/gwt
http://gwt-code-reviews.appspot.com/1149801/show
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
Thanks for the review; updated as per your comments.
http://gwt-code-reviews.appspot.com/1149801/diff/1/3
File dev/core/src/com/google/gwt/dev/shell/remoteui/RemoteUI.java
(right):
http://gwt-code-reviews.appspot.com/1149801/diff/1/3#newcode139
dev/core/src/com/google/gwt/dev/shell/remoteui/Rem
I think that jat is right - if you have a builder defined on your
project that Eclipse does not know about, it will throw an exception.
For now, let's go with this update to the README. I think, as a future
step, we should add the natures and builders to the gwt-user and gwt-dev
project, and make
Reviewers: scottb,
Description:
Added error handling for exceptions that can occur when logging messages
are sent over the wire from DevMode to GPE.
Review by: sco...@google.com
Please review this at http://gwt-code-reviews.appspot.com/1149801/show
Affected files:
M dev/core/src/com/google/gw
Looks good to me.
I wonder why we haven't taken the step of adding the GWT Nature to the
gwt-user project's .eclipse file...
http://gwt-code-reviews.appspot.com/966802/show
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
LGTM - I'm guessing that the appropriate classes have now been merged
into gwt-user proper?
Side note: The plugin code can now be modified so that it doesn't have
special handling for the bikeshed jar (i.e. putting it at the top of the
container).
http://gwt-code-reviews.appspot.com/665801/show
f/1/2
File
dev/core/src/com/google/gwt/dev/shell/remoteui/MessageTransport.java
(right):
http://gwt-code-reviews.appspot.com/550801/diff/1/2#newcode240
dev/core/src/com/google/gwt/dev/shell/remoteui/MessageTransport.java:240:
class FutureTaskExtension extends FutureTask {
On 2010/06/10 23:06:48, r
I like the cleanup you've done here - however, there are a few things
that I'd like you to take a look at before I give the LGTM. Let's go for
one more round on this one; we can discuss this stuff f2f if it's
easier.
Side note: When these changes to MessageTransport land, we should update
GPE so
LGTM.
http://gwt-code-reviews.appspot.com/541801/show
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
LGTM. Though, I wonder if a better solution would be delegating the
validation of web.xml to the SCL, as opposed to doing it in GWT.
http://gwt-code-reviews.appspot.com/371801/show
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
LGTM.
http://gwt-code-reviews.appspot.com/323801/show
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
LGTM.
http://gwt-code-reviews.appspot.com/323801/show
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
To unsubscribe, reply using "remove me" as the subject.
I think this still needs some work. I'd suggest that we meet up and
discuss the general threading pattern here; I think there could be a few
pitfalls, so let's discuss the way that we want this class to work, and
then re-work the threading/synchronization around that.
http://gwt-code-reviews.app
Reviewers: kjin,
Description:
Fix flaky MessageTransport test (again).
It is possible that an IOException can occur in this test. Also, get rid
of a redundant test.
Review by: k...@google.com
Please review this at http://gwt-code-reviews.appspot.com/250802/show
Affected files:
M
dev/core/
Reviewers: kjin,
Description:
Fix flaxy MessageTransport test.
Review by: k...@google.com
Please review this at http://gwt-code-reviews.appspot.com/234801
Affected files:
M
dev/core/test/com/google/gwt/dev/shell/remoteui/MessageTransportTest.java
Index:
dev/core/test/com/google/gwt/dev
ping
http://gwt-code-reviews.appspot.com/111808
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
LGTM.
http://gwt-code-reviews.appspot.com/134805
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
, rdayal wrote:
> Why is this necessary? Won't this cause the termination of the
DevMode
process?
Why would it terminate anything? The effect of this is to tell the UI
you are
no longer interested in RestartServer events, so presumably the UI
won't show
the button. If we fail
http://gwt-code-reviews.appspot.com/130806/diff/1/2
File dev/core/src/com/google/gwt/core/ext/ServletContainerLauncher.java
(right):
http://gwt-code-reviews.appspot.com/130806/diff/1/2#newcode70
Line 70: public boolean processArguments(TreeLogger logger, String
arguments) {
Another way to do thi
LGTM.
http://gwt-code-reviews.appspot.com/124803
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
LGTM with a nit - I only reviewed the JettyLauncher change.
http://gwt-code-reviews.appspot.com/120801/diff/1/4
File dev/core/src/com/google/gwt/dev/shell/jetty/JettyLauncher.java
(right):
http://gwt-code-reviews.appspot.com/120801/diff/1/4#newcode479
Line 479: private TreeLogger.Type baseLogLev
LGTM, with a few nits. Also, I didn't understand why you changed a bunch
of things in SwingUI to run on the UI thread. What was the specific
reason for this?
http://gwt-code-reviews.appspot.com/111809/diff/1/6
File dev/core/src/com/google/gwt/dev/SwingUI.java (right):
http://gwt-code-reviews.app
Reviewers: mmendez,
Description:
Add a delay to make sure the stream is closed before executing the
request.
Please review this at http://gwt-code-reviews.appspot.com/111808
Affected files:
M
dev/core/test/com/google/gwt/dev/shell/remoteui/MessageTransportTest.java
Index:
dev/core/test/
Thanks for the review.
Committed as tr...@r7150 and cherry-picked into releases/2.0 at r7151.
http://gwt-code-reviews.appspot.com/111807
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
Thanks for the review!
http://gwt-code-reviews.appspot.com/111807/diff/1/5
File dev/core/src/com/google/gwt/dev/shell/remoteui/RemoteUI.java
(right):
http://gwt-code-reviews.appspot.com/111807/diff/1/5#newcode169
Line 169: public void setStartupUrls(Map urls) {
On 2009/11/24 05:14:07, mmendez wr
Reviewers: jat, mmendez,
Description:
Send startup URLs over the wire to GPE. The initialization code has now
been moved from RemoteUI.initialize to RemoteUI.setStartupURLs.
Please review this at http://gwt-code-reviews.appspot.com/111807
Affected files:
M dev/core/src/com/google/gwt/dev/DevM
Thanks for the review.
Committed as tr...@r7112 and cherry-picked into releases/2.0 at r7113.
http://gwt-code-reviews.appspot.com/111802
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
Committed as tr...@r7109 and cherry-picked into releases/2.0 at r7110.
http://gwt-code-reviews.appspot.com/112802
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
Ended up going with "Module xxx has been loaded." John, I agree with
your point, but I think those other messages should be changed to be
more specific.
http://gwt-code-reviews.appspot.com/112802
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
http://gwt-code-reviews.appspot.com/112802/diff/1/2
File dev/core/src/com/google/gwt/dev/shell/OophmSessionHandler.java
(right):
http://gwt-code-reviews.appspot.com/112802/diff/1/2#newcode186
Line 186: "Module " + moduleName + " loaded");
Lets go with "Module xxx has been started.". Truly, that i
Reviewers: mmendez, jat,
Description:
Add better handling of failure to connect to remote UI. Instead of
printing out the exception, we print out a user-friendly error message.
Please review this at http://gwt-code-reviews.appspot.com/111802
Affected files:
M dev/core/src/com/google/gwt/dev/D
Reviewers: jat, mmendez,
Description:
Adds an info message to indicate when a module has loaded successfully.
As I was writing the code, I noticed that the SwingUI actually has the
"loading module xxx" message in the call to getModuleLogger().
Technically, this logging information should be in OO
Thanks for the review. Committed as tr...@r7089 and cherry-picked into
releases/2.0 at r7090.
http://gwt-code-reviews.appspot.com/109801
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
Reviewers: kplatfoot, mmendez,
Message:
Tweaks to get restart server up and running. We now always indicate that
we support the RESTART_SERVER capability (checking it was problematic,
because it had not been registered at the time of capability exchange).
Fixed an issue in MessageTransport where
Committed as tr...@r7406 and cherry-picked into releases/2.0 at r7407.
http://gwt-code-reviews.appspot.com/103812
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
Thanks for the review.
http://gwt-code-reviews.appspot.com/103812/diff/2002/2003
File dev/core/src/com/google/gwt/dev/DevMode.java (right):
http://gwt-code-reviews.appspot.com/103812/diff/2002/2003#newcode359
Line 359: ((JettyLauncher) scl).setBaseLogLevel(getBaseLogLevelForUI());
On 2009/11/19
Thanks for the reviews guys! Updated patch based on both of your
comments. I've ditched the system property in favor of exposing a setter
on JettyLauncher for the base log level. While this is still not ideal,
it's less hacky than the previous solution.
http://gwt-code-reviews.appspot.com/103812
Updated this issue based on our face-to-face descriptions. Please
re-read the description and check out patch set #2.
http://gwt-code-reviews.appspot.com/103812
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
Reviewers: jat, mmendez,
Description:
Drop Request/Response Logging Levels down to TRACE.
Please review this at http://gwt-code-reviews.appspot.com/103812
Affected files:
M dev/core/src/com/google/gwt/dev/shell/jetty/JettyLauncher.java
Index: dev/core/src/com/google/gwt/dev/shell/jetty/Jett
LGTM.
http://gwt-code-reviews.appspot.com/104810
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
LGTM.
http://gwt-code-reviews.appspot.com/103810/diff/1/2
File dev/core/src/com/google/gwt/dev/DevModeBase.java (right):
http://gwt-code-reviews.appspot.com/103810/diff/1/2#newcode272
Line 272: return new String[] {CODE_SERVER_PORT_TAG, "9997"};
Could extract a constant for this, since you use i
Thanks for the review. Ran the unit tests and updates against the
plugin, everything checked out.
Committed into trunk at r6880.
Cherry-picked into releases/2.0 at r6881.
http://gwt-code-reviews.appspot.com/102813
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
101 - 200 of 255 matches
Mail list logo
