http://gwt-code-reviews.appspot.com/1374802/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
http://gwt-code-reviews.appspot.com/1374802/diff/1/user/src/com/google/gwt/junit/JUnitShell.java
File user/src/com/google/gwt/junit/JUnitShell.java (right):
http://gwt-code-reviews.appspot.com/1374802/diff/1/user/src/com/google/gwt/junit/JUnitShell.java#newcode163
http://gwt-code-reviews.appspot.com/1384801/diff/3003/user/src/com/google/gwt/safecss/shared/SafeCssProperties.java
File user/src/com/google/gwt/safecss/shared/SafeCssProperties.java
(right):
Seems basically fine. I'm just wondering if we're missing anything.
http://gwt-code-reviews.appspot.com/1384801/diff/3003/user/test/com/google/gwt/safehtml/client/SafeHtmlTemplatesTest.java
File user/test/com/google/gwt/safehtml/client/SafeHtmlTemplatesTest.java
(right):
/client/SafeHtmlTemplatesTest.java#newcode80
user/test/com/google/gwt/safehtml/client/SafeHtmlTemplatesTest.java:80:
public void testTemplateWithCssAttribute() {
On 2011/03/14 22:01:18, jlabanca wrote:
On 2011/03/14 18:25:38, skybrian wrote:
On 2011/03/14 17:03:18, jlabanca wrote:
The character
http://gwt-code-reviews.appspot.com/1380806/diff/25001/user/src/com/google/gwt/safehtml/shared/SafeUri.java
File user/src/com/google/gwt/safehtml/shared/SafeUri.java (right):
http://gwt-code-reviews.appspot.com/1380806/diff/25001/user/src/com/google/gwt/safehtml/shared/SafeUri.java#newcode37
http://gwt-code-reviews.appspot.com/1380806/diff/25001/user/src/com/google/gwt/safehtml/shared/UriUtils.java
File user/src/com/google/gwt/safehtml/shared/UriUtils.java (right):
On 2011/06/09 21:20:52, jhumphries wrote:
But with this, startOption() (for example) jumps on a tangent,
building
something else -- with no way to return to building the original
element.
In a previous API I liked, we actually constructed HTML entirely
vertically using methods that returned
Hmm... it seems fairly reasonable, except for the part about having to
write an enormous amount of code to cover all of HTML. (If it weren't
client-side code, I'd go with a more concise API and do more checking at
runtime.)
I wonder if we should generate the code? (Not in a GWT generator, but as
http://gwt-code-reviews.appspot.com/1454808/diff/4001/user/src/com/google/gwt/safecss/shared/SafeStylesBuilder.java
File user/src/com/google/gwt/safecss/shared/SafeStylesBuilder.java
(right):
LGTM
http://gwt-code-reviews.appspot.com/1454808/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
I haven't looked at every class, but this should be enough for now.
http://gwt-code-reviews.appspot.com/1455802/diff/5002/user/src/com/google/gwt/dom/builder/client/DomBuilderFactory.java
File user/src/com/google/gwt/dom/builder/client/DomBuilderFactory.java
(right):
(Actually looked at everything this time.)
http://gwt-code-reviews.appspot.com/1455802/diff/5002/user/src/com/google/gwt/dom/builder/shared/HtmlBuilderImpl.java
File user/src/com/google/gwt/dom/builder/shared/HtmlBuilderImpl.java
(right):
http://gwt-code-reviews.appspot.com/1455802/diff/11003/user/src/com/google/gwt/dom/builder/client/DomStylesBuilder.java
File user/src/com/google/gwt/dom/builder/client/DomStylesBuilder.java
(right):
LGTM
http://gwt-code-reviews.appspot.com/1483804/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
http://gwt-code-reviews.appspot.com/1499808/diff/1/samples/showcase/src/com/google/gwt/sample/showcase/client/content/cell/CwCustomDataGrid.java
File
samples/showcase/src/com/google/gwt/sample/showcase/client/content/cell/CwCustomDataGrid.java
(right):
http://gwt-code-reviews.appspot.com/1526803/diff/1/user/src/com/google/web/bindery/event/shared/testing/CountingEventBus.java
File
user/src/com/google/web/bindery/event/shared/testing/CountingEventBus.java
(right):
http://gwt-code-reviews.appspot.com/1499808/diff/14004/samples/showcase/src/com/google/gwt/sample/showcase/client/content/cell/CwCustomDataGrid.java
File
samples/showcase/src/com/google/gwt/sample/showcase/client/content/cell/CwCustomDataGrid.java
(right):
http://gwt-code-reviews.appspot.com/1526803/diff/3/user/src/com/google/web/bindery/event/shared/testing/CountingEventBus.java
File
user/src/com/google/web/bindery/event/shared/testing/CountingEventBus.java
(right):
Makes sense. A few nitpicks.
http://gwt-code-reviews.appspot.com/1529805/diff/1/user/src/com/google/gwt/junit/JUnitShell.java
File user/src/com/google/gwt/junit/JUnitShell.java (right):
On 2011/08/25 01:56:26, stephenh wrote:
Thanks for the review. Agreed on all the nits and updated.
Sorry about the line length--I haven't gotten checkstyle installed in
Eclipse,
nor have auto-format turned on because it seems like a lot of files
(like the
ones in this review) haven't been
http://gwt-code-reviews.appspot.com/1529805/diff/3002/user/src/com/google/gwt/junit/RunStyleHtmlUnit.java
File user/src/com/google/gwt/junit/RunStyleHtmlUnit.java (right):
http://gwt-code-reviews.appspot.com/1529805/diff/3002/user/src/com/google/gwt/junit/RunStyleHtmlUnit.java#newcode184
Looks good. I need to look into how to actually commit this.
http://gwt-code-reviews.appspot.com/1529805/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
LGTM
http://gwt-code-reviews.appspot.com/1526803/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
Looks like it will work. Raised some issues that might be addressed in
another CL.
http://gwt-code-reviews.appspot.com/1533804/diff/1/user/src/com/google/gwt/user/cellview/client/AbstractCellTable.java
File user/src/com/google/gwt/user/cellview/client/AbstractCellTable.java
(right):
http://gwt-code-reviews.appspot.com/1533804/diff/6001/user/src/com/google/gwt/user/cellview/client/AbstractCellTable.java
File user/src/com/google/gwt/user/cellview/client/AbstractCellTable.java
(right):
/user/src/com/google/gwt/user/cellview/client/FooterBuilder.java#newcode61
user/src/com/google/gwt/user/cellview/client/FooterBuilder.java:61: *
does not contain a header. The {@link Header} will be used to respond to
On 2011/08/30 14:28:15, jlabanca wrote:
On 2011/08/29 17:47:59, skybrian wrote:
I'm
LGTM
http://gwt-code-reviews.appspot.com/1542803/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
Seems reasonable, except that I lack context and don't know what you're
actually trying to do. Is there a design doc somewhere?
http://gwt-code-reviews.appspot.com/1499811/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
LGTM
http://gwt-code-reviews.appspot.com/1561804/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
Reviewers: rjrjr,
Description:
Make client-side JUnit 3 classes available without GWTTestCase.
Review by: rj...@google.com
Please review this at http://gwt-code-reviews.appspot.com/1564803/
Affected files:
M user/src/com/google/gwt/junit/JUnit.gwt.xml
A
Reviewers: rjrjr,
Description:
Change the superclass of the translatable version of
junit.framework.AssertionFailedError to match the JVM version,
for consistency when catching java.lang.AssertionError in
testing tools.
Fixes issue 6863.
Please review this at
The design seems a bit opaque - I'm having trouble following what the
invariants are and what's going on with the concurrency. Can you explain
it a bit more?
http://gwt-code-reviews.appspot.com/1490801/diff/11002/dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java
File
Reviewers: tobyr,
Description:
Switch to internal implementation of StringInterner to avoid class
loader
leaks when reloading the page in dev mode. (Not tuned.)
Please review this at http://gwt-code-reviews.appspot.com/1578804/
Affected files:
M
http://gwt-code-reviews.appspot.com/1578804/diff/1/dev/core/src/com/google/gwt/dev/util/StringInterner.java
File dev/core/src/com/google/gwt/dev/util/StringInterner.java (right):
Surprisingly, using a ThreadLocal is the slowest so far. Going back to
using shards.
http://gwt-code-reviews.appspot.com/1578804/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
LGTM
http://gwt-code-reviews.appspot.com/1578811/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
http://gwt-code-reviews.appspot.com/1578810/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
Reviewers: cromwellian,
Description:
Changes to support experimental development mode work:
- Adds a hook to CrossSiteIframeLinker that a bookmarklet can use to
change the URL loaded for a module.
- Introduces the ResourceLoader interface, which allows the compiler's
classpath to be modified
http://gwt-code-reviews.appspot.com/1594803/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
LGTM
http://gwt-code-reviews.appspot.com/1598803/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
LGTM
http://gwt-code-reviews.appspot.com/1604804/diff/1/user/src/com/google/gwt/junit/FakeCssMaker.java
File user/src/com/google/gwt/junit/FakeCssMaker.java (right):
http://gwt-code-reviews.appspot.com/1604804/diff/1/user/src/com/google/gwt/junit/FakeCssMaker.java#newcode2
http://gwt-code-reviews.appspot.com/1601805/diff/1/user/src/com/google/web/bindery/autobean/shared/ValueCodex.java
File user/src/com/google/web/bindery/autobean/shared/ValueCodex.java
(right):
LGTM
http://gwt-code-reviews.appspot.com/1606804/diff/2002/user/src/com/google/gwt/safehtml/shared/SafeHtmlHostedModeUtils.java
File
user/src/com/google/gwt/safehtml/shared/SafeHtmlHostedModeUtils.java
(right):
LGTM
http://gwt-code-reviews.appspot.com/1606804/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
http://gwt-code-reviews.appspot.com/1614803/diff/1/user/src/com/google/gwt/user/client/DOM.java
File user/src/com/google/gwt/user/client/DOM.java (right):
http://gwt-code-reviews.appspot.com/1614803/diff/1/user/src/com/google/gwt/user/client/DOM.java#newcode1192
Sorry about the delay. The functionality looks fine but I will quibble
with the API a little.
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
LGTM
http://gwt-code-reviews.appspot.com/1655803/diff/1/user/src/com/google/gwt/user/cellview/client/CellWidget.java
File user/src/com/google/gwt/user/cellview/client/CellWidget.java
(right):
LGTM
http://gwt-code-reviews.appspot.com/1656803/diff/1/user/src/com/google/gwt/view/client/ListDataProvider.java
File user/src/com/google/gwt/view/client/ListDataProvider.java (right):
LGTM
http://gwt-code-reviews.appspot.com/1666803/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
http://gwt-code-reviews.appspot.com/1601805/diff/2004/user/src/com/google/web/bindery/autobean/shared/ValueCodex.java
File user/src/com/google/web/bindery/autobean/shared/ValueCodex.java
(right):
Hi, I think it's sufficient to test the happy path for the range that
currently works and defer testing error conditions for some other time.
The point is to make sure we don't accidentally regress to a smaller
range and break an app that starts relying on the fix that you've added.
http://gwt-code-reviews.appspot.com/1601805/diff/11002/user/src/com/google/web/bindery/autobean/shared/ValueCodex.java
File user/src/com/google/web/bindery/autobean/shared/ValueCodex.java
(right):
Okay, thanks for the explanation. The thing about consistency is that
there are so many things to choose from :-)
Since BigDecimal / BigInteger didn't work before, I think we can wait on
those. Backward compatibility when expanding the range is an issue but
I'm not sure that changing types is
Um, help? Even after studying it for a while, I'm finding the
RequestFactory implementation to be rather opaque.
https://gwt-code-reviews.appspot.com/1660803/diff/1/user/src/com/google/web/bindery/requestfactory/server/SimpleRequestProcessor.java
File
LGTM
http://gwt-code-reviews.appspot.com/1707803/diff/1/user/src/com/google/gwt/view/client/MultiSelectionModel.java
File user/src/com/google/gwt/view/client/MultiSelectionModel.java
(right):
LGTM
http://gwt-code-reviews.appspot.com/1709803/diff/1/user/src/com/google/gwt/dom/client/Style.java
File user/src/com/google/gwt/dom/client/Style.java (right):
http://gwt-code-reviews.appspot.com/1709803/diff/1/user/src/com/google/gwt/dom/client/Style.java#newcode1764
https://gwt-code-reviews.appspot.com/1660803/diff/1/user/src/com/google/web/bindery/requestfactory/server/SimpleRequestProcessor.java
File
user/src/com/google/web/bindery/requestfactory/server/SimpleRequestProcessor.java
(right):
Looks good to me.
(Now to figure out how to actually commit it.)
https://gwt-code-reviews.appspot.com/1660803/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
LGTM and I'll be committing this soon.
http://gwt-code-reviews.appspot.com/1601805/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
On 2012/05/11 22:20:05, skybrian wrote:
Looks good to me.
(Now to figure out how to actually commit it.)
By the way, back in the original issue, someone claims that the patch
doesn't fix their performance problem. I don't think there's any reason
not to commit this, but there might be more
LGTM
http://gwt-code-reviews.appspot.com/1709803/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
I tried reviewing this but I'm missing some things. When can an
EntityProxy have a null RequestContext? Aren't all entities created from
a RequestContext?
LGTM
https://gwt-code-reviews.appspot.com/1664803/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
This may be a naive question, but how are immutable entities supposed to
work with the RequestContext lifecycle?
I think the more normal approach is something like this:
- fetch an entity using a RequestContext
- start editing it
- commit it back, using the same RequestContext
Alternately you
Okay, so immutable here just means not editable, aka not checked out
for edit, in a source control sense.
http://gwt-code-reviews.appspot.com/1601806/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
More naive questions:
It seems like instead of having EntityProxy.equals() work in two
different ways, we should inline AutoBeanUtils.diff() and simplify it to
do exactly what we want for this case, without relying on
Object.equals() if possible? We don't care what the diffs are and can
return
https://gwt-code-reviews.appspot.com/1674804/diff/1/user/src/com/google/gwt/place/rebind/PlaceHistoryGeneratorContext.java
File
user/src/com/google/gwt/place/rebind/PlaceHistoryGeneratorContext.java
(right):
LGTM
http://gwt-code-reviews.appspot.com/1707803/diff/6001/user/src/com/google/gwt/view/client/OrderedMultiSelectionModel.java
File user/src/com/google/gwt/view/client/OrderedMultiSelectionModel.java
(right):
Oops, I had started an email about the revert but apparently never sent
it. Sorry about that!
I might need some background about the editor framework...
https://gwt-code-reviews.appspot.com/1664803/diff/4002/user/src/com/google/gwt/editor/client/adapters/ListEditor.java
File
Okay, yeah, a TODO is fine as it looks kind of difficult. I see now that
we are calling ValueProxy.equals() which calls deepEquals(). Worse, when
AutoBeanUtils.diff() is called with a ValueProxy, it looks like we will
be doing a deep equality check twice. (For the fast check and again
for each
LGTM
http://gwt-code-reviews.appspot.com/1707804/diff/1/user/src/com/google/gwt/cell/client/ButtonCellBase.java
File user/src/com/google/gwt/cell/client/ButtonCellBase.java (right):
I've read about Places and PlaceTokenizers and I'm a bit confused. It
looks like the generated PlaceHistoryMapper does a bunch of instanceof
checks on a passed-in Place to figure out which tokenizer to use to
create the token. So if we have multiple PlaceTokenizers with the same
Place type
http://gwt-code-reviews.appspot.com/1622803/diff/6001/user/src/com/google/web/bindery/requestfactory/shared/DefaultProxyStore.java
File
user/src/com/google/web/bindery/requestfactory/shared/DefaultProxyStore.java
(right):
LGTM
http://gwt-code-reviews.appspot.com/1712804/diff/1/samples/showcase/src/com/google/gwt/sample/showcase/client/content/lists/CwTree.java
File
samples/showcase/src/com/google/gwt/sample/showcase/client/content/lists/CwTree.java
(right):
/bindery/requestfactory/server/SimpleBar.java#newcode95
user/test/com/google/web/bindery/requestfactory/server/SimpleBar.java:95:
public static SimpleBar returnFirst(ListSimpleBar list) {
On 2012/05/24 00:28:11, tbroyer wrote:
On 2012/05/23 18:05:24, skybrian wrote:
It's weird that this method isn't just
LGTM. (Some optional nitpicks.)
It seems a little odd that you'd want to do this. I'd expect that the
subtype (Manager or Intern) would have additional fields to edit, so you
wouldn't be able to just reuse the editor for the parent type. But it
should still be allowed.
On 2012/05/26 23:09:03, tbroyer wrote:
(the use OperationMessage here can already be seen as a hack, kind
of, because all we need is a MapString,Splittable for the storage,
and a String for the version; so using another field in some diverted
way is not really a problem I think)
Oh yuck. Yeah,
On 2012/05/26 15:20:15, tbroyer wrote:
On 2012/05/25 19:35:47, skybrian wrote:
Having two levels of class nesting is rather confusing. Could you
move the
Driver and other 4 inner classes up a level?
Done.
Do you think the Intern/Manager/Department should be moved to
top-level classes
I got this feedback from someone who requested a rollback but decided to
work around it instead. I wonder how widespread this sort of thing is?
Our use of the editor/driver stuff assumed the editors would be
rebuilt when driver.edit(T) was invoked. This isn't true with this CL,
as the point of
LGTM (assuming tests still pass)
http://gwt-code-reviews.appspot.com/1601806/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
Reviewers: cromwellian,
Description:
Move Super Dev Mode to the open source repository.
Includes a simple demo target that works with the Hello sample app.
I've also made some linker options configurable, as necessary to make
this work.
Review by: cromwell...@google.com
Please review this at
http://gwt-code-reviews.appspot.com/1727804/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
LGTM. Thanks!
https://gwt-code-reviews.appspot.com/1734803/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
Moving org.json into gwt-dev makes sense because the compiler actually
uses it. I'd like to see it repackaged, but perhaps we can wait until
someone complains?
I'm not sure about validation. Many people don't use it and I sorta
think GWT's validation support ought to be split out of gwt-user
http://gwt-code-reviews.appspot.com/1727803/diff/4003/user/src/com/google/gwt/resources/css/RtlVisitor.java
File user/src/com/google/gwt/resources/css/RtlVisitor.java (right):
http://gwt-code-reviews.appspot.com/1727803/diff/4003/user/src/com/google/gwt/resources/css/RtlVisitor.java#newcode46
http://gwt-code-reviews.appspot.com/1727807/diff/8001/user/src/com/google/gwt/editor/client/impl/DelegateMap.java
File user/src/com/google/gwt/editor/client/impl/DelegateMap.java
(right):
http://gwt-code-reviews.appspot.com/1727807/diff/8001/user/src/com/google/gwt/editor/client/impl/DelegateMap.java
File user/src/com/google/gwt/editor/client/impl/DelegateMap.java
(right):
I don't understand this code, but I wonder if there is any way to write
a smoke test for soyc?
http://gwt-code-reviews.appspot.com/1726803/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
http://gwt-code-reviews.appspot.com/1727807/diff/7/user/src/com/google/gwt/editor/client/impl/SimpleViolation.java
File user/src/com/google/gwt/editor/client/impl/SimpleViolation.java
(right):
LGTM
https://gwt-code-reviews.appspot.com/1729805/diff/1/user/test/com/google/web/bindery/requestfactory/apt/EntityProxyCheckDomainMapping.java
File
user/test/com/google/web/bindery/requestfactory/apt/EntityProxyCheckDomainMapping.java
(right):
http://gwt-code-reviews.appspot.com/1735804/diff/1/user/src/com/google/gwt/resources/css/SubstitutionReplacer.java
File user/src/com/google/gwt/resources/css/SubstitutionReplacer.java
(right):
I mostly looked over the build process. (Since this is experimental, I
suppose it can all be done later.)
http://gwt-code-reviews.appspot.com/1728806/diff/1/elemental/META-INF/MANIFEST.MF
File elemental/META-INF/MANIFEST.MF (right):
Okay, seems fine for now. I'm going to commit this.
https://gwt-code-reviews.appspot.com/1731804/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
LGTM. I can understand the code now and I'm basically okay with it; the
rest is nitpicks.
http://gwt-code-reviews.appspot.com/1727807/diff/15003/user/src/com/google/gwt/editor/client/impl/SimpleViolation.java
File user/src/com/google/gwt/editor/client/impl/SimpleViolation.java
(right):
I'm getting failing tests because JSON is gone from gwt-user.jar and the
requestfactory jars. I could add the dependency internal to google, but
I think we might still be trying to do too much at once - this is
looking more like churn than an actual improvement.
To fix the compiler and close
Reviewers: cromwellian,
Description:
Include json-1.5.jar in gwt-dev.jar. JSON is needed by the Closure
compiler and also to generate source maps.
Fixes issue 7397
Please review this at http://gwt-code-reviews.appspot.com/1732804/
Affected files:
M dev/build.xml
M
LGTM. I'll submit this.
On 2012/06/14 07:32:40, tbroyer wrote:
because gwt-user
has no declared dependency on gwt-dev (this might be a mistake, or
not)
I'm guessing this is because we don't want GWT user code to have
dependencies on classes that should only be used in the compiler.
LGTM
http://gwt-code-reviews.appspot.com/1731806/diff/1/dev/core/test/com/google/gwt/dev/jjs/impl/EnumOrdinalizerTest.java
File dev/core/test/com/google/gwt/dev/jjs/impl/EnumOrdinalizerTest.java
(right):
https://gwt-code-reviews.appspot.com/1727808/diff/8001/build.xml
File build.xml (right):
https://gwt-code-reviews.appspot.com/1727808/diff/8001/build.xml#newcode41
build.xml:41: call-subproject subproject=elemental subtarget=build
/
I asked Ray about this. Elemental is experimental and its
1 - 100 of 238 matches
Mail list logo
