http://gwt-code-reviews.appspot.com/1355805/diff/6003/user/src/com/google/gwt/user/client/ui/DeckPanel.java
File user/src/com/google/gwt/user/client/ui/DeckPanel.java (right):
http://gwt-code-reviews.appspot.com/1355805/diff/6003/user/src/com/google/gwt/user/client/ui/DeckPanel.java#newcode108
LGTM
http://gwt-code-reviews.appspot.com/1401803/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
LGTM
http://gwt-code-reviews.appspot.com/1404801/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
LGTM with nits about which comments are helpful.
http://gwt-code-reviews.appspot.com/1399803/diff/1/user/super/com/google/gwt/junit/translatable/junit/framework/TestSuite.java
File
user/super/com/google/gwt/junit/translatable/junit/framework/TestSuite.java
(right):
LGTM, now without nits.
http://gwt-code-reviews.appspot.com/1399803/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
http://gwt-code-reviews.appspot.com/1383804/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
Reviewers: scottb,
Description:
Moving UnitTestTreeLogger from dev/core/src to dev/core/tests, so
gwt-dev
doesn't have an unnecessary dependency on junit.
Please review this at http://gwt-code-reviews.appspot.com/1383804/
Affected files:
D
LGTM, too. Shoulda caught that in my change, sorry.
On 2011/03/09 15:26:32, pdr wrote:
On 2011/03/09 15:23:11, jlabanca wrote:
LGTM
http://gwt-code-reviews.appspot.com/1377803/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
http://gwt-code-reviews.appspot.com/1374801/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
http://gwt-code-reviews.appspot.com/1374801/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
Reviewers: jat,
Description:
Switching to the junit4 jars, although the @annotation stuff isn't
going to work.
Review by: j...@google.com
Please review this at http://gwt-code-reviews.appspot.com/1374801/
Affected files:
M build-tools/ant-gwt/build.xml
M common.ant.xml
M dev/build.xml
Yep, the tools update just happened at r9794.
http://gwt-code-reviews.appspot.com/1374801/diff/1/user/test/org/hibernate/jsr303/tck/util/TckTestSuiteWrapper.java
File user/test/org/hibernate/jsr303/tck/util/TckTestSuiteWrapper.java
(left):
Lgtm.
http://gwt-code-reviews.appspot.com/953801/show
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
Reviewers: jlabanca,
Description:
Fixing the javadoc build breaks. One problem is that the doc/build.xml
has drifted stale against user/build.xml's classpath (we should unify
them, but the goal here is to fix the break); another is citing
@examples that don't (yet) exist.
Please review this
http://gwt-code-reviews.appspot.com/953801/diff/1/5
File
user/src/com/google/gwt/requestfactory/client/impl/messages/JsonResults.java
(right):
http://gwt-code-reviews.appspot.com/953801/diff/1/5#newcode28
user/src/com/google/gwt/requestfactory/client/impl/messages/JsonResults.java:28:
// javac
Assuming we like the concept, I have two issues noted here.
At the larger level, people should be doing this today in e.g. build.xml
as a wrapper, which also works... I'm not sure whether I'd be more
annoyed to fail on existing dirt, or to have something I'd tried to
preserve be overwritten.
LGTM
http://gwt-code-reviews.appspot.com/917801/show
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
LGTM
http://gwt-code-reviews.appspot.com/916801/show
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
One checkstyle nit, but the larger question is whether we think anyone
else is using dumpSignatures... I certainly hope not, and can't think of
a good one (even when it was made!), but having released with it, I'm
nervous about suddenly neutering it like this.
LGTM.
http://gwt-code-reviews.appspot.com/805801/show
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
Reviewers: cramsdale,
Description:
Not compile-building expenses sample, just collecting its files
into the distro.
Review by: cramsd...@google.com
Please review this at http://gwt-code-reviews.appspot.com/794802/show
Affected files:
M samples/build.xml
M samples/common.ant.xml
A
http://gwt-code-reviews.appspot.com/794802/show
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
http://gwt-code-reviews.appspot.com/794802/show
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
Reviewers: unnurg,
Description:
Allow merging of modules with the same prefix, for example if you have a
directory of widgets that are separated into multiple modules (but in
the
same java package), or if you have src/test roots that each have modules
for the same package. The existing exclude
So, is that a LGTM as-is, a request to move the now-separate classes
into static nested ones, or a hold for more discussion?
On 2010/07/12 21:19:29, scottb wrote:
Actually, I may have jumped the gun on that. Making it static nested
will
have an impact on command line callers who are
http://gwt-code-reviews.appspot.com/686801/diff/1/3
File user/src/com/google/gwt/i18n/tools/ArgHandlerValueChooser.java
(right):
http://gwt-code-reviews.appspot.com/686801/diff/1/3#newcode49
user/src/com/google/gwt/i18n/tools/ArgHandlerValueChooser.java:49:
private Class? extends Localizable
Reviewers: jat,
Description:
Refactoring to one top level class per .java file, since some
tools don't understand more than that.
Review by: jat
Please review this at http://gwt-code-reviews.appspot.com/686801/show
Affected files:
M dev/core/super/com/google/gwt/lang/LongLibBase.java
A
I have mixed feelings... if bikeshed is null now, do we think we
will/won't need such a workspace in the future? I thought something
like that was being considered as the incubator replacement model, for
example.
I suppose we can turn it back on at will, but it seems to me that having
build.xml
Sorry to get to this a bit late...
I've got a couple of concerns:
1. regarding especially the user.compile use of javax validation,
does that suggest we should embed that library in gwt-dev.jar
with all of our other runtime dependencies? (I don't love that
practice, but it has
LGTM, for a slightly loose definition of good. ;)
Thanks.
http://gwt-code-reviews.appspot.com/652802/show
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
Reviewers: Lex,
Description:
Correcting the output names for SoycTest.java
Review by: sp...@google.com
Please review this at http://gwt-code-reviews.appspot.com/442801/show
Affected files:
M dev/core/test/com/google/gwt/dev/SoycTest.java
Index:
Reviewers: fabbott,
Description:
Test patch.
Please review this at http://gwt-code-reviews.appspot.com/420801/show
Affected files:
M /bikeshed/README
Index: /bikeshed/README
===
--- /bikeshed/README(revision 7979
LGTM, too
http://gwt-code-reviews.appspot.com/319801/show
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
To unsubscribe, reply using remove me as the subject.
The biggie is MutableArray.java:60
http://gwt-code-reviews.appspot.com/291801/diff/4001/5003
File
bikeshed/src/com/google/gwt/collections/ImmutableArrayEmptyImpl.java
(right):
http://gwt-code-reviews.appspot.com/291801/diff/4001/5003#newcode27
LGTM, through the checkstyle patch #5.
Given we're in bikeshed anyway, I'd submit and do a new issue/review for
your next increment
http://gwt-code-reviews.appspot.com/232801/show
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
To unsubscribe from this group, send email
LGTM
http://gwt-code-reviews.appspot.com/232801/show
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
To unsubscribe from this group, send email to
google-web-toolkit-contributors+unsubscribegooglegroups.com or reply to this email with
the words REMOVE ME as the subject.
Looks almost good to me; I think you're non-invariant (but maybe
correct) if you remove the last element, so that needs a test, and you
may have been better off with getModuleName() returning null, depending
on what you're trying to test in this increment.
Can you edit the issue, to cc
Comments as noted; also, what semantics do we want for equality?
(Identity is certainly cheapest, and probably good, just wanted to make
an explicit choice there).
I think you're buggy if your first element is null, and I'm not entirely
sure the singleElem special case is worth the time-cost of
LGTM.
In build.xml, I don't think you need to add -ea explicitly to devmode
tests. Not wrong, but they're already on.
A bigger problem, but mine not yours, is that rietveld is (wrongly)
basing this on http://google-web-toolkit, not on incubator, so the
side-by-sides are all wrong. (I assume
LGTM
http://gwt-code-reviews.appspot.com/153819
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
Reviewers: Lex,
Description:
turns out that the insistence on concreteType.isStatic() prevents
top-level concrete types, because it's false for top-level types (where
explicit static is forbidden). Scott and Lex are nervous about making
isStatic() true for top-level types, but we can just test
LGTM.
http://gwt-code-reviews.appspot.com/128803
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
Reviewers: jlabanca,
Description:
This fixes the ServletMappingSuite buildbreak.
The issue appears to be that a pathelement location=/ is taken, much
like a :: substring in -classpath, to mean this directory rather than
nothing: it puts gwt-root/user onto the classpath. And, when the
LGTM. Sorry for the delay on it.
http://gwt-code-reviews.appspot.com/93804
--~--~-~--~~~---~--~~
http://groups.google.com/group/Google-Web-Toolkit-Contributors
-~--~~~~--~~--~--~---
LGTM, with one whitespace nit. Pity ant doesn't have a built-in
$ant.target property, though.
http://gwt-code-reviews.appspot.com/91810/diff/1/3
File user/build.xml (right):
http://gwt-code-reviews.appspot.com/91810/diff/1/3#newcode380
Line 380: test.cases=test.dev.htmlunit.tests
I don't
lgtm
http://gwt-code-reviews.appspot.com/93805
--~--~-~--~~~---~--~~
http://groups.google.com/group/Google-Web-Toolkit-Contributors
-~--~~~~--~~--~--~---
Reviewers: scottb,
Description:
Two issues here, both arguably ant bugs. Although the scripts have
correct perms, and zip creates a non-executable copy unless you
explicitly set fileperm in the zipfileset. That gets you a correct
build/dist/gwt-X.X.X.zip, and unzip will do the right thing with
Reviewers: ,
Description:
Two issues here, both arguably ant bugs. Although the scripts have
correct perms, and zip creates a non-executable copy unless you
explicitly set fileperm in the zipfileset. That gets you a correct
build/dist/gwt-X.X.X.zip, and unzip will do the right thing with it...
Reviewers: scottb,
Description:
Now that we've admitted htmlunit isn't quite ready, but all other tests
require odd property assignments, people should be warned that they
maybe didn't run the testing they thought they did.
I thought about getting fancy and testing the -D's and making a more
Reviewers: jlabanca,
Description:
let it compile!
Please review this at http://gwt-code-reviews.appspot.com/87803
Affected files:
common.ant.xml
Index: common.ant.xml
===
--- common.ant.xml (revision 1733)
+++
http://gwt-code-reviews.appspot.com/83806/diff/1/2
File dev/build.xml (right):
http://gwt-code-reviews.appspot.com/83806/diff/1/2#newcode212
Line 212: filename
name=com/google/gwt/dev/shell/remoteui/RemoteMessageProto.java
negate=yes /
perhaps too late, but should we generalize this to e.g.
Reviewers: Ray Ryan,
Description:
It's pretty ungainly, and hurts both pick-and-choose test execution and
any attempts to parallelize test suites. (The divisions here are pretty
arbitrary, though; I just forked of DeRPC and the couple biggest time
contributors into their own suites.)
Please
Reviewers: Ray Ryan,
Please review this at http://gwt-code-reviews.appspot.com/78807
Affected files:
build.xml
Index: build.xml
===
--- build.xml (revision 6346)
+++ build.xml (working copy)
@@ -108,11 +108,8 @@
LGTM. I'm a bit hesitant about die == UnableToCompleteException in
TreeLogger in general, but have no issues with it here.
(As an example, I'm not sure that's the right exception to throw for an
issue early in starting hosted mode shell, although it's maybe not
clearly the wrong exception
Does there exist a CompileStrategyTest? (hint.)
http://gwt-code-reviews.appspot.com/74803/diff/1/3
File user/src/com/google/gwt/junit/BatchingStrategy.java (right):
http://gwt-code-reviews.appspot.com/74803/diff/1/3#newcode36
Line 36: private boolean isSingleTestOnly;
is this pattern better
Reviewers: amitmanjhi, jat,
Description:
I see both sides of the fail/permit argument here, but we're seeing a
lot of flake to be penalizing everyone for it. Can we agree to run the
tests, but make them non-blocking?
The admitted danger is that we never fix the problem, and so never run
the
One race condition. In practice, it'd come up only if you had two or
more builds at once (which I at least do fairly often), AND if they
chose similar seed random numbers (which should be very unlikely)... but
it's easy to close.
http://gwt-code-reviews.appspot.com/61816/diff/1/2
File
I see removal of code from the old location here; shouldn't I also see
addition at the new? ('Cause I don't.)
http://gwt-code-reviews.appspot.com/56811/diff/1/10
File dev/common.ant.xml (right):
http://gwt-code-reviews.appspot.com/56811/diff/1/10#newcode33
Line 33: fileset
LGTM.
On 2009/07/16 23:52:23, Ray Ryan wrote:
http://gwt-code-reviews.appspot.com/47820
--~--~-~--~~~---~--~~
http://groups.google.com/group/Google-Web-Toolkit-Contributors
-~--~~~~--~~--~--~---
On 2009/07/15 17:16:37, jat wrote:
LGTM.
I only reviewed the two requested files -- let me know if I should
review
anything else.
http://gwt-code-reviews.appspot.com/47817/diff/1/9
File user/test/com/google/gwt/dev/jjs/test/CoverageTest.java (right):
Reviewers: Lex, jat,
Description:
Minor (but significant!) typo from r5094... Passes on my XP box, at
least.
Please review this at http://gwt-code-reviews.appspot.com/47821
Affected files:
user/build.xml
Index: user/build.xml
Reviewers: Ray Ryan, jat,
Description:
CoverageTest.java and HandlerManagerTest.java need new review. The
other files are welcome to be re-reviewed, but are by flin and reviewed
by me already.
The change in CoverageTest is to do a more correct epsilon-compare of
floating point; apparently IE's
Reviewers: Ray Ryan,
Description:
As long as it's ant day... this is a performance tweak. Right now, when
we test in the native-code directories (dev and jni), we nominally test
all three operating systems, which can result in triplicate builds of
alldeps.jar. Since we, in fact, can only test
Reviewers: amitmanjhi,
Description:
the old form got confused by a refJar path of
'C:\GWT\tools\api-checker\reference\gwt-dev-modified.jar:C:\GWT\tools\api-checker\reference\gwt-user-modified.jar'
because splitting on colons was wrong. Worse, ant thought that it had
succeeded because an
Since Lex and I are push/pull'ing you on the flag name, I'd leave it the
way you have it unless a consensus otherwise emerges. But the
when-to-enable flag is wrong, and I think you've moved from a
too-expansive to a too-restricted spec!
http://gwt-code-reviews.appspot.com/47802/diff/2003/2009
Reviewers: bobv,
Description:
This is build-timeout surgery... I have some suggestions for cleanup
later.
The real problem is that in r5557 I caused target test to
call-subproject into to top-level servlet for subtarget test... and
servlet depends on user, which uses gwt.ant (sensitive to
Reviewers: scottb,
Description:
Shaves 10min off a dist-one build vs dist (aka old build).
Please review this at http://gwt-code-reviews.appspot.com/37803
Affected files:
build.xml
--~--~-~--~~~---~--~~
http://gwt-code-reviews.appspot.com/37803/diff/1/2
File build.xml (right):
http://gwt-code-reviews.appspot.com/37803/diff/1/2#newcode21
Line 21: target name=dist-one depends=buildonly, tools, samples,
doc description=Make this platform's distributions
On 2009/06/11 17:41:56, zundel wrote:
Now
Reviewers: scottb,
Description:
This change saves me about 4 minutes on a work-to-do build. The idea is
that, if the default isn't good for people, they can set a local
override in GWT_ROOT/local.ant.properties.
Please review this at http://gwt-code-reviews.appspot.com/36802
Affected files:
On 2009/06/05 20:09:49, fabbott wrote:
Ping for comment.
Bruce had asked about additional possible use cases for the property
provider template thing. The abstract answer is any case where a
permuted property depends on a non-permuting one... these are
hypothetical, and off-the-cuff so
Reviewers: jgw,
Description:
Joel, I'm seeing my hosted mode tests fail as noted without this
one-liner. It seems that node node.specified returns undefined,
triggering a HostedModeException from JsValueGlue.get(), without
something like this == test.
I haven't checked any speed impact of the
Reviewers: bobv,
Description:
I'm getting almost-consistent local faults in hosted and web mode (XP,
ie8) with a half-second timeout. Moving up to a full second, which
seems to reliably work
Please review this at http://gwt-code-reviews.appspot.com/33846
Affected files:
Reviewers: jat, scottb,
Description:
This allows binding the default locale to a user-specified locale, so
that requests for which locale negotiation fails end up being an actual
locale with actual currency and number formatting specification.
To achieve this, we also allow property providers
Reviewers: scottb,
http://gwt-code-reviews.appspot.com/33837/diff/1/2
File common.ant.xml (right):
http://gwt-code-reviews.appspot.com/33837/diff/1/2#newcode145
Line 145: jar destfile=${project.lib} update=true duplicate=fail
index=true filesonly=true /
On 2009/06/01 17:56:26, scottb wrote:
I
74 matches
Mail list logo
