[gwt-contrib] Re: Add RequestFactory validator implemented as an annotation processor. (issue1467804)
I don't know why, but it outputs an error twice (this is the only
missing ProxyFor annotation I have, so I can't tell whether it's
specific to that interface or more general):
myapp-shared/src/main/java/xxx/shared/breadcrumb/BreadCrumbItemProxy.java:14:
A proxy must be annotated with ProxyFor, ProxyForName, or JsonRpcProxy
public interface BreadCrumbItemProxyT extends EnumT FieldEnumT
extends BaseEmbeddedValueProxy {
^
myapp-shared/src/main/java/xxx/shared/breadcrumb/BreadCrumbItemProxy.java:14:
A proxy must be annotated with ProxyFor, ProxyForName, or JsonRpcProxy
public interface BreadCrumbItemProxyT extends EnumT FieldEnumT
extends BaseEmbeddedValueProxy {
^
BTW, this is a case I could push the extends down to subinterfaces
(having 4 proxies implementing the same mixin, rather than 4
interfaces extending the same proxy bae interface)
===
Another error should IMO be a warning:
myapp-shared/src/main/java/xxx/shared/LockInfoProxy.java:10: Could not
find domain method similar to xxx.AbstractLockableEntity
findAbstractLockableEntity(java.lang.String)
public interface LockInfoProxy extends EntityProxy, LockInfo {
^
This is expected, we only send LockInfoProxy down from the server to the
client, so we don't need the findXxx. In comparison, the lack of a
zero-arg constructor is only a warning:
myapp-shared/src/main/java/xxx/shared/breadcrumb/DossierBreadCrumbItemProxy.java:13:
warning: The domain type xxx.model.breadcrumb.DossierBreadCrumbItem has
no default constructor. Calling
RequestContext.create(DossierBreadCrumbItemProxy.class) will cause a
server error
It would be easy to workaround (use a Locator or put a dummy findXxx
method in the domain class) but still, I think it's a bug to flag this
as an error (it's admittedly a strong warning, as you could very
easily fall in the trap, much more than a missing zero-arg constructor,
but I don't think it's an error that should break the build).
Finally, I have a small issue with generics, but I believe it's my
fault: a ListSubFooProxy getItems() in the proxy interface, but a
List? extends BaseFooProxy getItems() in the domain class, and
RfValidator generates an error that it cannot find the domain method.
http://gwt-code-reviews.appspot.com/1467804/diff/6052/user/src/com/google/web/bindery/requestfactory/apt/DomainChecker.java
File
user/src/com/google/web/bindery/requestfactory/apt/DomainChecker.java
(right):
http://gwt-code-reviews.appspot.com/1467804/diff/6052/user/src/com/google/web/bindery/requestfactory/apt/DomainChecker.java#newcode340
user/src/com/google/web/bindery/requestfactory/apt/DomainChecker.java:340:
state.warn(warnTo, Cannot validate method (%s.%s) because the domain
mapping for the
On 2011/06/30 15:16:48, bobv wrote:
This is more verbose, but it will unambiguously identify the unchecked
method in
question.
This doesn't print what one would expect:
myapp-shared/src/main/java/xxx/shared/BaseEmbeddedProxy.java:16:
warning: Cannot validate method
(xxx.shared.xxx.xxx.XxxProxy.()com.atolcd.gertrude.production.shared.EmbeddedProxyId)
because the domain mapping for the return type
(xxx.shared.EmbeddedProxyId) could not be resolved to a domain type
Add @SuppressWarnings(requestfactory) to dismiss.
EmbeddedProxyId getId();
^
There's no mention of 'getId' in the message.
Note that I changed the way I launch the apt (calling 'javac -proc:only'
from the command line, rather tha using a Maven plugin) and the output
is different, and actually references the exact location of the warning;
so in the end, I don't think this change is necessary (sorry)
It appears I wasn't clear on my previous report though, and this
output format makes it even clearer: this warning is output once for
each interface extending BaseEmbeddedProxy, each time for the exact same
location (notice the beginning of the line, it points to
BaseEmbeddedProxy.java:16, where the getId method is defined).
Further more, the EmbeddedProxyId cannot be resolved because of this
earlier warning:
myapp-shared/src/main/java/xxx/shared/EmbeddedProxyId.java:12: warning:
Cannot fully validate proxy since type xxx.server.EmbeddedId is not
available
Add @SuppressWarnings(requestfactory) to dismiss.
public interface EmbeddedProxyId extends ValueProxy {
^
because the xxx.server.EmbeddedId is in the myapp-server project, which
depends on myapp-shared and therefore is not available at the time we
compile myapp-shared.
I don't mind having the warning on the getId method because of a
previous warning on EmbeddedProxyId, but there should IMO be only one,
not hundreds of them! (I actually have 52 such warnings exactly)
http://gwt-code-reviews.appspot.com/1467804/diff/6052/user/src/com/google/web/bindery/requestfactory/apt/TransportableTypeVisitor.java
File
user/src/com/google/web/bindery/requestfactory/apt/TransportableTypeVisitor.java
(right):
[gwt-contrib] Re: Implements UiBinder rendering for Cells. (issue1466809)
Was curious to see what it'd look like; finally have a few comments.
http://gwt-code-reviews.appspot.com/1466809/diff/1/user/src/com/google/gwt/uibinder/client/UiRenderer.java
File user/src/com/google/gwt/uibinder/client/UiRenderer.java (right):
http://gwt-code-reviews.appspot.com/1466809/diff/1/user/src/com/google/gwt/uibinder/client/UiRenderer.java#newcode32
user/src/com/google/gwt/uibinder/client/UiRenderer.java:32: * @param
cell {@link com.google.gwt.cell.client.Cell Cell} that will receive the
event
Why isn't T declared as T extends Cell? then?
http://gwt-code-reviews.appspot.com/1466809/diff/1/user/src/com/google/gwt/uibinder/rebind/UiBinderWriter.java
File user/src/com/google/gwt/uibinder/rebind/UiBinderWriter.java
(right):
http://gwt-code-reviews.appspot.com/1466809/diff/1/user/src/com/google/gwt/uibinder/rebind/UiBinderWriter.java#newcode1229
user/src/com/google/gwt/uibinder/rebind/UiBinderWriter.java:1229: return
firstLetter.toLowerCase() + name.substring(4);
Beware of the turkish locale!
You should use Introspector.decapitalize here
http://download.oracle.com/javase/6/docs/api/java/beans/Introspector.html#decapitalize(java.lang.String)
Or use Character.toLowerCase rather than String.toLowerCase.
http://gwt-code-reviews.appspot.com/1466809/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Implements UiBinder rendering for Cells. (issue1466809)
Was curious to see what it'd look like; finally have a few comments. http://gwt-code-reviews.appspot.com/1466809/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: This patch substantially reduces the overhead of Java types in the output by minimizing vtable s... (issue1447821)
LGTM (w/minor cleanup + a suggestion)
I like the management of class literal rescues much better now!
http://gwt-code-reviews.appspot.com/1447821/diff/15001/dev/core/src/com/google/gwt/dev/jjs/impl/ControlFlowAnalyzer.java
File dev/core/src/com/google/gwt/dev/jjs/impl/ControlFlowAnalyzer.java
(right):
http://gwt-code-reviews.appspot.com/1447821/diff/15001/dev/core/src/com/google/gwt/dev/jjs/impl/ControlFlowAnalyzer.java#newcode93
dev/core/src/com/google/gwt/dev/jjs/impl/ControlFlowAnalyzer.java:93:
whitespace
http://gwt-code-reviews.appspot.com/1447821/diff/15001/dev/core/src/com/google/gwt/dev/jjs/impl/ControlFlowAnalyzer.java#newcode600
dev/core/src/com/google/gwt/dev/jjs/impl/ControlFlowAnalyzer.java:600:
instantiatedTypes.add(type);
Suggest combining all this into 1 method call to something like
(maybeRescueClassLiteral(type)), and then mRCL contains logic to see
if classLiteralsToBeRescuedIfGetClassLive is null or not, to decide
whether to rescue or mark for rescue.
http://gwt-code-reviews.appspot.com/1447821/diff/15001/dev/core/src/com/google/gwt/dev/jjs/impl/ControlFlowAnalyzer.java#newcode763
dev/core/src/com/google/gwt/dev/jjs/impl/ControlFlowAnalyzer.java:763:
suggest (see above) changing this to maybeRescueClassLiteral(type), and
have it check whether classLiteralsToBeRescuedIfGetClassLive is null or
not. This way, the logic to decide whether to rescue immediately or
deferred is separated from the logic for whether getClass is live, etc.
http://gwt-code-reviews.appspot.com/1447821/diff/15001/dev/core/src/com/google/gwt/dev/jjs/impl/ControlFlowAnalyzer.java#newcode769
dev/core/src/com/google/gwt/dev/jjs/impl/ControlFlowAnalyzer.java:769:
if (classLiteralsToBeRescuedIfGetClassIsLive != null) {
is re-entrant the right term (I usually think of it wrt to concurrency)?
http://gwt-code-reviews.appspot.com/1447821/diff/15001/dev/core/src/com/google/gwt/dev/jjs/impl/ControlFlowAnalyzer.java#newcode870
dev/core/src/com/google/gwt/dev/jjs/impl/ControlFlowAnalyzer.java:870:
whitespace
http://gwt-code-reviews.appspot.com/1447821/diff/15001/dev/core/src/com/google/gwt/dev/jjs/impl/ControlFlowAnalyzer.java#newcode917
dev/core/src/com/google/gwt/dev/jjs/impl/ControlFlowAnalyzer.java:917:
/**
C
http://gwt-code-reviews.appspot.com/1447821/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Implements UiBinder rendering for Cells. (issue1466809)
http://gwt-code-reviews.appspot.com/1466809/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Implements UiBinder rendering for Cells. (issue1466809)
Thanks Thomas!.
http://gwt-code-reviews.appspot.com/1466809/diff/1/user/src/com/google/gwt/uibinder/client/UiRenderer.java
File user/src/com/google/gwt/uibinder/client/UiRenderer.java (right):
http://gwt-code-reviews.appspot.com/1466809/diff/1/user/src/com/google/gwt/uibinder/client/UiRenderer.java#newcode32
user/src/com/google/gwt/uibinder/client/UiRenderer.java:32: * @param
cell {@link com.google.gwt.cell.client.Cell Cell} that will receive the
event
On 2011/07/01 10:18:30, tbroyer wrote:
Why isn't T declared as T extends Cell? then?
In the end we figured it would be good to let people render for purposes
other than Cell widgets. The documentation (mistakenly) still gives the
impression that this is exclusively Cell related, though.
http://gwt-code-reviews.appspot.com/1466809/diff/1/user/src/com/google/gwt/uibinder/rebind/UiBinderWriter.java
File user/src/com/google/gwt/uibinder/rebind/UiBinderWriter.java
(right):
http://gwt-code-reviews.appspot.com/1466809/diff/1/user/src/com/google/gwt/uibinder/rebind/UiBinderWriter.java#newcode1229
user/src/com/google/gwt/uibinder/rebind/UiBinderWriter.java:1229: return
firstLetter.toLowerCase() + name.substring(4);
On 2011/07/01 10:18:30, tbroyer wrote:
Beware of the turkish locale!
You should use Introspector.decapitalize here
http://download.oracle.com/javase/6/docs/api/java/beans/Introspector.html#decapitalize%28java.lang.String)
Or use Character.toLowerCase rather than String.toLowerCase.
Thanks! I was not aware of this.
http://gwt-code-reviews.appspot.com/1466809/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Implements UiBinder rendering for Cells. (issue1466809)
http://gwt-code-reviews.appspot.com/1466809/diff/1/tools/api-checker/config/gwt23_24userApi.conf
File tools/api-checker/config/gwt23_24userApi.conf (right):
http://gwt-code-reviews.appspot.com/1466809/diff/1/tools/api-checker/config/gwt23_24userApi.conf#newcode58
tools/api-checker/config/gwt23_24userApi.conf:58:
:user/src/com/google/gwt/uibinder/client/UiRendererUtils.java\
This doesn't match the other lines. It shouldn't be necessary to exclude
new classes from the excludedFiles_old list.
http://gwt-code-reviews.appspot.com/1466809/diff/1/user/src/com/google/gwt/uibinder/client/UiRendererUtils.java
File user/src/com/google/gwt/uibinder/client/UiRendererUtils.java
(right):
http://gwt-code-reviews.appspot.com/1466809/diff/1/user/src/com/google/gwt/uibinder/client/UiRendererUtils.java#newcode25
user/src/com/google/gwt/uibinder/client/UiRendererUtils.java:25: public
class UiRendererUtils {
If this is an impl class, you should make it package protected or at
least append Impl to the class name so nobody uses it.
http://gwt-code-reviews.appspot.com/1466809/diff/1/user/src/com/google/gwt/uibinder/client/UiRendererUtils.java#newcode47
user/src/com/google/gwt/uibinder/client/UiRendererUtils.java:47: return
null;
Should this throw an IllegalArgumentException? If root is null, it
indicates that an invalid parent was passed into the method.
http://gwt-code-reviews.appspot.com/1466809/diff/1/user/src/com/google/gwt/uibinder/client/UiRendererUtils.java#newcode55
user/src/com/google/gwt/uibinder/client/UiRendererUtils.java:55: Element
elementById = Document.get().getElementById(renderedId);
You can move the assertion below this line, and only run it if
elementById is null. That way, you can throw a Runtime exception in
prod mode, but you only walk up the DOM in the error case.
if (elementById == null) {
if (!isAttachedToDom(root)) {
throw new RuntimeException(UiRendered element is not attached to
DOM while getting \ + fieldName + \);
} else {
throw new IllegalArgumentException(fieldName not found within
rendered element);
}
}
http://gwt-code-reviews.appspot.com/1466809/diff/1/user/src/com/google/gwt/uibinder/client/UiRendererUtils.java#newcode88
user/src/com/google/gwt/uibinder/client/UiRendererUtils.java:88: :
Parent Element of previously rendered element contains more than one
child;
This assertion should only be executed if ret is the first child of the
parent. If the parent itself is the rendered Element, you don't need
the check.
http://gwt-code-reviews.appspot.com/1466809/diff/1/user/src/com/google/gwt/uibinder/client/UiRendererUtils.java#newcode117
user/src/com/google/gwt/uibinder/client/UiRendererUtils.java:117: public
static SafeHtml stampUiRendererAttribute(SafeHtml safeHtml, String
attributeName,
Can you JavaDoc explaining what this is doing and what it returns.
http://gwt-code-reviews.appspot.com/1466809/diff/1/user/src/com/google/gwt/uibinder/client/UiRendererUtils.java#newcode120
user/src/com/google/gwt/uibinder/client/UiRendererUtils.java:120: int
endOfFirstTag = html.indexOf('');
What if the SafeHtml does not contain any HTML, or if it is self closing
(ends in /):
div id=placeholder /
If UiBinder controls the inputs and can guarantee this doesn't happen,
you should note that. An assertion in dev mode would be good too.
http://gwt-code-reviews.appspot.com/1466809/diff/1/user/src/com/google/gwt/uibinder/client/UiRendererUtils.java#newcode121
user/src/com/google/gwt/uibinder/client/UiRendererUtils.java:121: html =
html.substring(0, endOfFirstTag) + + attributeName + =' +
attributeValue + '
We generally use double quotes instead of quotes. I'm not sure if it
matters a lot.
http://gwt-code-reviews.appspot.com/1466809/diff/1/user/src/com/google/gwt/uibinder/client/UiRendererUtils.java#newcode123
user/src/com/google/gwt/uibinder/client/UiRendererUtils.java:123: return
new SafeHtmlBuilder().appendHtmlConstant(html).toSafeHtml();
This is simpler:
return SafeHtmlUtils.fromTrustedString(html);
http://gwt-code-reviews.appspot.com/1466809/diff/1/user/src/com/google/gwt/uibinder/rebind/UiBinderWriter.java
File user/src/com/google/gwt/uibinder/rebind/UiBinderWriter.java
(right):
http://gwt-code-reviews.appspot.com/1466809/diff/1/user/src/com/google/gwt/uibinder/rebind/UiBinderWriter.java#newcode1732
user/src/com/google/gwt/uibinder/rebind/UiBinderWriter.java:1732:
ListJMethod getters = findGetterNames(owner);
What happens if there are other methods in the interface that aren't
supported by UiRenderer? Do we rely on javac to trigger a compiler
exception?
http://gwt-code-reviews.appspot.com/1466809/diff/1/user/src/com/google/gwt/uibinder/rebind/UiBinderWriter.java#newcode1739
user/src/com/google/gwt/uibinder/rebind/UiBinderWriter.java:1739: String
elementParameter = getter.getParameters()[0].getName();
We should check that there is exactly 1 parameter, and its type is
assignable from Element.
http://gwt-code-reviews.appspot.com/1466809/
--
[gwt-contrib] Re: Add RequestFactory validator implemented as an annotation processor. (issue1467804)
Regarding the generics issue, I agree that the checker is behaving
correctly: the ListSubFooProxy would be converted to a ListSubFoo
when being checked against the domain type. A List? extends BaseFoo
can't be assigned to the type ListSubFoo since the list might contain
OtherFoo elements.
Since we've mainly be talking about edge-case details of the
implementation and it sounds like the checker mostly works with your
code base, I'm going to go ahead and check the code in with some
error-reporting fixes to make it less spammy when compiling with javac.
The findFoo() error is changed to a warning, although I'm thinking that
it might be better for the novice user if it's an error. For the
advanced user, an annotation processor option could be used to switch
that from an error to a warning.
I'll be on vacation the early part of next week and will be studiously
avoiding the computer. Thanks for your feedback, it's been very
helpful.
http://gwt-code-reviews.appspot.com/1467804/diff/6052/user/src/com/google/web/bindery/requestfactory/apt/DomainChecker.java
File
user/src/com/google/web/bindery/requestfactory/apt/DomainChecker.java
(right):
http://gwt-code-reviews.appspot.com/1467804/diff/6052/user/src/com/google/web/bindery/requestfactory/apt/DomainChecker.java#newcode340
user/src/com/google/web/bindery/requestfactory/apt/DomainChecker.java:340:
state.warn(warnTo, Cannot validate method (%s.%s) because the domain
mapping for the
On 2011/07/01 09:19:16, tbroyer wrote:
On 2011/06/30 15:16:48, bobv wrote:
This is more verbose, but it will unambiguously identify the
unchecked method
in
question.
This doesn't print what one would expect:
Another place where Eclipse and javac differ :-)
I don't mind having the warning on the getId method because of a
previous
warning on EmbeddedProxyId, but there should IMO be only one, not
hundreds of
them! (I actually have 52 such warnings exactly)
The duplicate messages are due to the way client interfaces are checked.
Each proxy or context type is examined individually as a flattened list
of all methods exposed by that type. If proxyA and proxyB both inherit
some interface X, the methods in X will be checked multiple times.
Admittedly this is more work than strictly necessary, but de-duplicating
the effort would complicate the checker code. Instead I've added some
code to the State object to prevent duplicate messages from being
emitted.
http://gwt-code-reviews.appspot.com/1467804/diff/6052/user/src/com/google/web/bindery/requestfactory/apt/TransportableTypeVisitor.java
File
user/src/com/google/web/bindery/requestfactory/apt/TransportableTypeVisitor.java
(right):
http://gwt-code-reviews.appspot.com/1467804/diff/6052/user/src/com/google/web/bindery/requestfactory/apt/TransportableTypeVisitor.java#newcode102
user/src/com/google/web/bindery/requestfactory/apt/TransportableTypeVisitor.java:102:
return state.types.erasure(t).accept(this, state);
On 2011/07/01 09:19:16, tbroyer wrote:
Have you tried with an intersection type? i.e. something like T
extends
MyInterface EntityProxy
I don't have that in my code base, and haven't tried it, but judging
from the
code of Eclipse's TypeVariableImpl#getUpperBound, it might very well
trigger the
bug too, in which case erasure() probably wouldn't be an appropriare
workaround.
I was thinking about looping on the Types#directSupertypes until one
is detected
as a transportable type:
for (TypeMirror type : state.types.directSupertypes(t)) {
if (type.accept(this, state)) {
return true;
}
}
return false;
that should work, because the expected type of getUpperBound is a
DeclaredType,
so visitDeclared should have been called, and it only uses
isAssignable checks
(so isAssignable(MyInterfaceEntityProxy, EntityProxy) would be
equivalent to
isAssignable(MyInterface,EntityProxy)||
isAssignable(EntityProxy,EntityProxy).
In the case of T extends EnumT, I guess it'd work the same.
I don't think that intersection types make sense in the most-derived
RequestFactory interfaces. Enums are the only concrete types that you
can specify in a RequestFactory interface. The implementations of all
other return types (proxies, etc) will necessarily be implemented by the
RequestFactory generator. The extra mix-in interfaces in the
intersection type probably won't be implemented by whatever generated,
concrete type is being returned. The only way that would work is if the
concrete type was generated from an interface that already implemented
the mix-in.
http://gwt-code-reviews.appspot.com/1467804/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Add RequestFactory validator implemented as an annotation processor. (issue1467804)
On Fri, Jul 1, 2011 at 5:52 PM, b...@google.com wrote: Regarding the generics issue, I agree that the checker is behaving correctly: the ListSubFooProxy would be converted to a ListSubFoo when being checked against the domain type. A List? extends BaseFoo can't be assigned to the type ListSubFoo since the list might contain OtherFoo elements. Since we've mainly be talking about edge-case details of the implementation and it sounds like the checker mostly works with your code base, I'm going to go ahead and check the code in with some error-reporting fixes to make it less spammy when compiling with javac. The findFoo() error is changed to a warning, although I'm thinking that it might be better for the novice user if it's an error. For the advanced user, an annotation processor option could be used to switch that from an error to a warning. I totally agree: go ahead, check it in! Better than an option would be distinct @SuppressWarnings values (such as a @SuppressWarnings(requestfactory.noFinder)), but I don't know how they work, even less if that's possible at all. And if you feel better with an error, no problem, I'll add a locator with a throwing find() and create() to make sure they don't go client-to-server. I'm so happy I could remove my horrendous hacks in ServiceLayerDecorators once RFIV will be removed! I'll be on vacation the early part of next week and will be studiously avoiding the computer. Thanks for your feedback, it's been very helpful. You're welcome. You made (and still makes) such great work with RF and editors (and everything else) that I had to do it in return as a thank you. Have great vacations! -- Thomas Broyer /tɔ.ma.bʁwa.je/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Updating npapi plugin to remove gwtId from Jso objects (idenity fix), (issue1469803)
Reviewers: jat, conroy, Description: Updating npapi plugin to remove gwtId from Jso objects (idenity fix), adds a second map in LocalObjects. Please review this at http://gwt-code-reviews.appspot.com/1469803/ Affected files: M plugins/npapi/LocalObjectTable.h M plugins/npapi/ScriptableInstance.cpp M plugins/npapi/ScriptableInstance.h M plugins/npapi/prebuilt/gwt-dev-plugin/manifest.json M user/test/com/google/gwt/core/CoreSuite.java A user/test/com/google/gwt/core/client/JsIdentityTest.java -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Updating npapi plugin to remove gwtId from Jso objects (idenity fix), (issue1469803)
http://gwt-code-reviews.appspot.com/1469803/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Updating npapi plugin to remove gwtId from Jso objects (idenity fix), (issue1469803)
On 2011/07/01 18:28:32, codefu wrote: We can't rip out the ID hack quite yet-- we'd have to wait until the fixes roll out to stable users. We need to add some version checking of chrome to decide if it's safe to do this. http://gwt-code-reviews.appspot.com/1469803/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Updating npapi plugin to remove gwtId from Jso objects (idenity fix), (issue1469803)
On 2011/07/01 18:31:43, conroy wrote: On 2011/07/01 18:28:32, codefu wrote: We can't rip out the ID hack quite yet-- we'd have to wait until the fixes roll out to stable users. We need to add some version checking of chrome to decide if it's safe to do this. Is windows still on Chrome12? Chrome13 has Kelly's identity fixes in it. http://gwt-code-reviews.appspot.com/1469803/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Updating npapi plugin to remove gwtId from Jso objects (idenity fix), (issue1469803)
http://gwt-code-reviews.appspot.com/1469803/diff/1/plugins/npapi/LocalObjectTable.h
File plugins/npapi/LocalObjectTable.h (right):
http://gwt-code-reviews.appspot.com/1469803/diff/1/plugins/npapi/LocalObjectTable.h#newcode100
plugins/npapi/LocalObjectTable.h:100: NPObject* get(int id) {
Given the new method added, please change this to something like
getById.
http://gwt-code-reviews.appspot.com/1469803/diff/1/plugins/npapi/LocalObjectTable.h#newcode109
plugins/npapi/LocalObjectTable.h:109: int get(NPObject* jsObject) {
Please use a more descriptive name, such as getObjectId since there is
now collision with the above.
http://gwt-code-reviews.appspot.com/1469803/diff/1/plugins/npapi/ScriptableInstance.cpp
File plugins/npapi/ScriptableInstance.cpp (right):
http://gwt-code-reviews.appspot.com/1469803/diff/1/plugins/npapi/ScriptableInstance.cpp#newcode434
plugins/npapi/ScriptableInstance.cpp:434: int id =
localObjects.get(obj);
So how does this fix the identity problem? The reason this was added
was to avoid the identity problem, since the same JS object passed to
the plugin twice will get different NPObject wrappers. Has that been
fixed now?
http://gwt-code-reviews.appspot.com/1469803/diff/1/user/test/com/google/gwt/core/client/JsIdentityTest.java
File user/test/com/google/gwt/core/client/JsIdentityTest.java (right):
http://gwt-code-reviews.appspot.com/1469803/diff/1/user/test/com/google/gwt/core/client/JsIdentityTest.java#newcode2
user/test/com/google/gwt/core/client/JsIdentityTest.java:2: * Copyright
2008 Google Inc.
2011
http://gwt-code-reviews.appspot.com/1469803/diff/1/user/test/com/google/gwt/core/client/JsIdentityTest.java#newcode24
user/test/com/google/gwt/core/client/JsIdentityTest.java:24: * @author
cod...@google.com (John McDole)
We don't use @author tags in GWT, and the first line should have a
period. Did you run checkstyle or have Eclipse configured to use it?
http://gwt-code-reviews.appspot.com/1469803/diff/1/user/test/com/google/gwt/core/client/JsIdentityTest.java#newcode34
user/test/com/google/gwt/core/client/JsIdentityTest.java:34: return
com.google.gwt.core.Core;
I'm not sure I follow what all these tests are looking for.
The most basic tests are:
testJavaToJs() { obj=...; assertTrue(jsID(obj, obj)); }
native jsID(a, b) { return a === b; }
testJsToJava() { initJsObj(); a=getJsObj(); b=getJsObj(); assertSame(a,
b); }
native initJsObj() { obj = {}; }
native getJsObj() { return obj; }
Having additional tests that store objects and return them later are
fine, but it is hard for me to tell these cases are covered from the
test and I would prefer a more direct test for them.
http://gwt-code-reviews.appspot.com/1469803/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Updating npapi plugin to remove gwtId from Jso objects (idenity fix), (issue1469803)
http://gwt-code-reviews.appspot.com/1469803/diff/1/plugins/npapi/LocalObjectTable.h
File plugins/npapi/LocalObjectTable.h (right):
http://gwt-code-reviews.appspot.com/1469803/diff/1/plugins/npapi/LocalObjectTable.h#newcode100
plugins/npapi/LocalObjectTable.h:100: NPObject* get(int id) {
On 2011/07/01 18:38:17, jat wrote:
Given the new method added, please change this to something like
getById.
Done.
http://gwt-code-reviews.appspot.com/1469803/diff/1/plugins/npapi/LocalObjectTable.h#newcode109
plugins/npapi/LocalObjectTable.h:109: int get(NPObject* jsObject) {
On 2011/07/01 18:38:17, jat wrote:
Please use a more descriptive name, such as getObjectId since there is
now
collision with the above.
Done.
http://gwt-code-reviews.appspot.com/1469803/diff/1/plugins/npapi/ScriptableInstance.cpp
File plugins/npapi/ScriptableInstance.cpp (right):
http://gwt-code-reviews.appspot.com/1469803/diff/1/plugins/npapi/ScriptableInstance.cpp#newcode434
plugins/npapi/ScriptableInstance.cpp:434: int id =
localObjects.get(obj);
On 2011/07/01 18:38:17, jat wrote:
So how does this fix the identity problem? The reason this was added
was to
avoid the identity problem, since the same JS object passed to the
plugin twice
will get different NPObject wrappers. Has that been fixed now?
Kelly's patch to chrome is not present in Chrome13 builds. Running the
GWT tests shows these passing with Chrome13+NewPlugin and failing in
Chrome12+NewPlugin
http://gwt-code-reviews.appspot.com/1469803/diff/1/user/test/com/google/gwt/core/client/JsIdentityTest.java
File user/test/com/google/gwt/core/client/JsIdentityTest.java (right):
http://gwt-code-reviews.appspot.com/1469803/diff/1/user/test/com/google/gwt/core/client/JsIdentityTest.java#newcode2
user/test/com/google/gwt/core/client/JsIdentityTest.java:2: * Copyright
2008 Google Inc.
On 2011/07/01 18:38:17, jat wrote:
2011
Done.
http://gwt-code-reviews.appspot.com/1469803/diff/1/user/test/com/google/gwt/core/client/JsIdentityTest.java#newcode24
user/test/com/google/gwt/core/client/JsIdentityTest.java:24: * @author
cod...@google.com (John McDole)
On 2011/07/01 18:38:17, jat wrote:
We don't use @author tags in GWT, and the first line should have a
period. Did
you run checkstyle or have Eclipse configured to use it?
Ran the code formatter on the whole file and created it in Eclipse. I
did not run checktyle, but will.
http://gwt-code-reviews.appspot.com/1469803/diff/1/user/test/com/google/gwt/core/client/JsIdentityTest.java#newcode34
user/test/com/google/gwt/core/client/JsIdentityTest.java:34: return
com.google.gwt.core.Core;
On 2011/07/01 18:38:17, jat wrote:
I'm not sure I follow what all these tests are looking for.
The most basic tests are:
testJavaToJs() { obj=...; assertTrue(jsID(obj, obj)); }
native jsID(a, b) { return a === b; }
testJsToJava() { initJsObj(); a=getJsObj(); b=getJsObj();
assertSame(a, b); }
native initJsObj() { obj = {}; }
native getJsObj() { return obj; }
Having additional tests that store objects and return them later are
fine, but
it is hard for me to tell these cases are covered from the test and I
would
prefer a more direct test for them.
The .contains() test was suggested by another team that was running into
that problem.
testJsoJavaComparison() does your testJstoJava().
testJavaObjectStorage() is similar enough to the first test, just using
== and === in JavaScript instead of indexOf(). It was a simple test
suggested by someone else.
The last test, testJavaArrayArray(), was something I was experiencing
working with scheduled tasks leaking. I can certainly remove it.
http://gwt-code-reviews.appspot.com/1469803/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Updating npapi plugin to remove gwtId from Jso objects (idenity fix), (issue1469803)
http://gwt-code-reviews.appspot.com/1469803/diff/1/plugins/npapi/ScriptableInstance.cpp File plugins/npapi/ScriptableInstance.cpp (right): http://gwt-code-reviews.appspot.com/1469803/diff/1/plugins/npapi/ScriptableInstance.cpp#newcode434 plugins/npapi/ScriptableInstance.cpp:434: int id = localObjects.get(obj); On 2011/07/01 18:50:17, codefu wrote: On 2011/07/01 18:38:17, jat wrote: So how does this fix the identity problem? The reason this was added was to avoid the identity problem, since the same JS object passed to the plugin twice will get different NPObject wrappers. Has that been fixed now? Kelly's patch to chrome is not present in Chrome13 builds. Running the GWT tests shows these passing with Chrome13+NewPlugin and failing in Chrome12+NewPlugin Did you mean is now present? If not, I don't understand the statement. The old plugin should be passing for the Java-JS direction, right? How many people are upgraded to Chrome13? Can we make it so it works on both, by using different plugins based on the Chrome version? If not, can we make it so the new plugin would not get installed on older Chromes by setting a minimum version? We would not want people running older Chromes to get updated to something that works less well. http://gwt-code-reviews.appspot.com/1469803/diff/1/user/test/com/google/gwt/core/client/JsIdentityTest.java File user/test/com/google/gwt/core/client/JsIdentityTest.java (right): http://gwt-code-reviews.appspot.com/1469803/diff/1/user/test/com/google/gwt/core/client/JsIdentityTest.java#newcode34 user/test/com/google/gwt/core/client/JsIdentityTest.java:34: return com.google.gwt.core.Core; On 2011/07/01 18:50:17, codefu wrote: The last test, testJavaArrayArray(), was something I was experiencing working with scheduled tasks leaking. I can certainly remove it. No need to remove a test if it is useful. Please add a test like my first example using JS === to make sure the same Java object passed twice is in fact the same object seen by JS -- you can keep the testjavaObjectStorage test. http://gwt-code-reviews.appspot.com/1469803/diff/1/user/test/com/google/gwt/core/client/JsIdentityTest.java#newcode88 user/test/com/google/gwt/core/client/JsIdentityTest.java:88: assertTrue(obj1 == obj2); Use assertSame here instead. http://gwt-code-reviews.appspot.com/1469803/diff/1/user/test/com/google/gwt/core/client/JsIdentityTest.java#newcode108 user/test/com/google/gwt/core/client/JsIdentityTest.java:108: assertTrue(id2 == get2); Use assertSame. http://gwt-code-reviews.appspot.com/1469803/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] [google-web-toolkit] r10418 committed - Update pre-built requestfactory-apt.jar to r10417.
Revision: 10418 Author: gwt.mirror...@gmail.com Date: Fri Jul 1 12:22:05 2011 Log: Update pre-built requestfactory-apt.jar to r10417. http://code.google.com/p/google-web-toolkit/source/detail?r=10418 Modified: /tools/lib/requestfactory/requestfactory-apt.jar === --- /tools/lib/requestfactory/requestfactory-apt.jar Fri Jun 17 13:08:11 2011 +++ /tools/lib/requestfactory/requestfactory-apt.jar Fri Jul 1 12:22:05 2011 Binary file, no diff available. -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Updating npapi plugin to remove gwtId from Jso objects (idenity fix), (issue1469803)
http://gwt-code-reviews.appspot.com/1469803/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Updating npapi plugin to remove gwtId from Jso objects (idenity fix), (issue1469803)
http://gwt-code-reviews.appspot.com/1469803/diff/1/plugins/npapi/ScriptableInstance.cpp File plugins/npapi/ScriptableInstance.cpp (right): http://gwt-code-reviews.appspot.com/1469803/diff/1/plugins/npapi/ScriptableInstance.cpp#newcode434 plugins/npapi/ScriptableInstance.cpp:434: int id = localObjects.get(obj); On 2011/07/01 19:00:48, jat wrote: On 2011/07/01 18:50:17, codefu wrote: On 2011/07/01 18:38:17, jat wrote: So how does this fix the identity problem? The reason this was added was to avoid the identity problem, since the same JS object passed to the plugin twice will get different NPObject wrappers. Has that been fixed now? Kelly's patch to chrome is not present in Chrome13 builds. Running the GWT tests shows these passing with Chrome13+NewPlugin and failing in Chrome12+NewPlugin Did you mean is now present? If not, I don't understand the statement. The old plugin should be passing for the Java-JS direction, right? How many people are upgraded to Chrome13? Can we make it so it works on both, by using different plugins based on the Chrome version? If not, can we make it so the new plugin would not get installed on older Chromes by setting a minimum version? We would not want people running older Chromes to get updated to something that works less well. Sorry, is now present in Chrome 13 builds according to the engineer responsible for committing the patch. I will check on the requirements for supporting older versions. http://gwt-code-reviews.appspot.com/1469803/diff/1/user/test/com/google/gwt/core/client/JsIdentityTest.java File user/test/com/google/gwt/core/client/JsIdentityTest.java (right): http://gwt-code-reviews.appspot.com/1469803/diff/1/user/test/com/google/gwt/core/client/JsIdentityTest.java#newcode34 user/test/com/google/gwt/core/client/JsIdentityTest.java:34: return com.google.gwt.core.Core; On 2011/07/01 19:00:48, jat wrote: On 2011/07/01 18:50:17, codefu wrote: The last test, testJavaArrayArray(), was something I was experiencing working with scheduled tasks leaking. I can certainly remove it. No need to remove a test if it is useful. Please add a test like my first example using JS === to make sure the same Java object passed twice is in fact the same object seen by JS -- you can keep the testjavaObjectStorage test. Easy enough to add! http://gwt-code-reviews.appspot.com/1469803/diff/1/user/test/com/google/gwt/core/client/JsIdentityTest.java#newcode88 user/test/com/google/gwt/core/client/JsIdentityTest.java:88: assertTrue(obj1 == obj2); On 2011/07/01 19:00:48, jat wrote: Use assertSame here instead. Done. http://gwt-code-reviews.appspot.com/1469803/diff/1/user/test/com/google/gwt/core/client/JsIdentityTest.java#newcode108 user/test/com/google/gwt/core/client/JsIdentityTest.java:108: assertTrue(id2 == get2); On 2011/07/01 19:00:48, jat wrote: Use assertSame. Done. http://gwt-code-reviews.appspot.com/1469803/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Remove JSNI paths from RPC deserialization. AKA: Make RPC screaming fast in DevMode. (issue1470802)
LGTM http://gwt-code-reviews.appspot.com/1470802/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Remove JSNI paths from RPC deserialization. AKA: Make RPC screaming fast in DevMode. (issue1470802)
lgtm2 http://gwt-code-reviews.appspot.com/1470802/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: This patch substantially reduces the overhead of Java types in the output by minimizing vtable s... (issue1447821)
I made the fixes and submitted it. Since most people are headed out of
the office for the holiday, I need to get it in early to see if I need
to revert it before everyone is gone. :)
http://gwt-code-reviews.appspot.com/1447821/diff/15001/dev/core/src/com/google/gwt/dev/jjs/impl/ControlFlowAnalyzer.java
File dev/core/src/com/google/gwt/dev/jjs/impl/ControlFlowAnalyzer.java
(right):
http://gwt-code-reviews.appspot.com/1447821/diff/15001/dev/core/src/com/google/gwt/dev/jjs/impl/ControlFlowAnalyzer.java#newcode93
dev/core/src/com/google/gwt/dev/jjs/impl/ControlFlowAnalyzer.java:93:
On 2011/07/01 10:24:38, jbrosenberg wrote:
whitespace
Done.
http://gwt-code-reviews.appspot.com/1447821/diff/15001/dev/core/src/com/google/gwt/dev/jjs/impl/ControlFlowAnalyzer.java#newcode600
dev/core/src/com/google/gwt/dev/jjs/impl/ControlFlowAnalyzer.java:600:
instantiatedTypes.add(type);
On 2011/07/01 10:24:38, jbrosenberg wrote:
Suggest combining all this into 1 method call to something like
(maybeRescueClassLiteral(type)), and then mRCL contains logic to see
if
classLiteralsToBeRescuedIfGetClassLive is null or not, to decide
whether to
rescue or mark for rescue.
Done.
http://gwt-code-reviews.appspot.com/1447821/diff/15001/dev/core/src/com/google/gwt/dev/jjs/impl/ControlFlowAnalyzer.java#newcode769
dev/core/src/com/google/gwt/dev/jjs/impl/ControlFlowAnalyzer.java:769:
if (classLiteralsToBeRescuedIfGetClassIsLive != null) {
Not sure, but rescueClassLiteral can end up invoking this function
again, and you'll get a concurrent modification exception if you don't
clone, and if you do, you end up with extra work.
On 2011/07/01 10:24:38, jbrosenberg wrote:
is re-entrant the right term (I usually think of it wrt to
concurrency)?
http://gwt-code-reviews.appspot.com/1447821/diff/15001/dev/core/src/com/google/gwt/dev/jjs/impl/ControlFlowAnalyzer.java#newcode870
dev/core/src/com/google/gwt/dev/jjs/impl/ControlFlowAnalyzer.java:870:
On 2011/07/01 10:24:38, jbrosenberg wrote:
whitespace
Done.
http://gwt-code-reviews.appspot.com/1447821/diff/15001/dev/core/src/com/google/gwt/dev/jjs/impl/ControlFlowAnalyzer.java#newcode917
dev/core/src/com/google/gwt/dev/jjs/impl/ControlFlowAnalyzer.java:917:
/**
On 2011/07/01 10:24:38, jbrosenberg wrote:
C
IDE has bizarre bug where it was randomly inserting these.
http://gwt-code-reviews.appspot.com/1447821/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Implements UiBinder rendering for Cells. (issue1466809)
I had a few nits below but I felt a bit out of context. Is there an
overview somewhere of what you guys are trying to accomplish? What
should rendering for Cells look like?
http://gwt-code-reviews.appspot.com/1466809/diff/1/user/src/com/google/gwt/uibinder/client/UiRenderer.java
File user/src/com/google/gwt/uibinder/client/UiRenderer.java (right):
http://gwt-code-reviews.appspot.com/1466809/diff/1/user/src/com/google/gwt/uibinder/client/UiRenderer.java#newcode32
user/src/com/google/gwt/uibinder/client/UiRenderer.java:32: * @param
cell {@link com.google.gwt.cell.client.Cell Cell} that will receive the
event
You should add a blank line before @param
http://gwt-code-reviews.appspot.com/1466809/diff/1/user/src/com/google/gwt/uibinder/client/UiRendererUtils.java
File user/src/com/google/gwt/uibinder/client/UiRendererUtils.java
(right):
http://gwt-code-reviews.appspot.com/1466809/diff/1/user/src/com/google/gwt/uibinder/client/UiRendererUtils.java#newcode38
user/src/com/google/gwt/uibinder/client/UiRendererUtils.java:38: *
Retrieves a specific element within a previously rendered elements.
/s/elements./element./
http://gwt-code-reviews.appspot.com/1466809/diff/1/user/src/com/google/gwt/uibinder/client/UiRendererUtils.java#newcode41
user/src/com/google/gwt/uibinder/client/UiRendererUtils.java:41: *
@param fieldName name of the field to retrieve
you forgo the javadoc for attribute (I was actually curious about that
one :-) )
http://gwt-code-reviews.appspot.com/1466809/diff/1/user/src/com/google/gwt/uibinder/client/UiRendererUtils.java#newcode45
user/src/com/google/gwt/uibinder/client/UiRendererUtils.java:45: Element
root = findRootElement(parent, attribute);
Do you really need the root element? Couldn't you just check if parent
is attached?
http://gwt-code-reviews.appspot.com/1466809/diff/1/user/src/com/google/gwt/uibinder/client/UiRendererUtils.java#newcode87
user/src/com/google/gwt/uibinder/client/UiRendererUtils.java:87: assert
isRenderedElementSingleChild(ret)//
Are the trailing // on purpose?
http://gwt-code-reviews.appspot.com/1466809/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Implements UiBinder rendering for Cells. (issue1466809)
On 2011/07/01 20:59:37, rdcastro wrote: I had a few nits below but I felt a bit out of context. Is there an overview somewhere of what you guys are trying to accomplish? What should rendering for Cells look like? Yes, we have a design doc... which I forgot to share. Here is the link, comments welcome: https://docs.google.com/a/google.com/document/pub?id=1a-_8IdBrBmWCnhV6rnQVmzXB_bXQ9JQ4ya-k1_s1_sA http://gwt-code-reviews.appspot.com/1466809/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Implements UiBinder rendering for Cells. (issue1466809)
https://docs.google.com/a/google.com/document/pub?id=1a-_8IdBrBmWCnhV6rnQVmzXB_bXQ9JQ4ya-k1_s1_sA Is it okay to make that public? http://gwt-code-reviews.appspot.com/1466809/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Implements UiBinder rendering for Cells. (issue1466809)
Thanks for sharing, I'll read it tomorrow and try and come up with more interesting suggestions :-) On Fri, Jul 1, 2011 at 6:14 PM, rchan...@google.com wrote: On 2011/07/01 20:59:37, rdcastro wrote: I had a few nits below but I felt a bit out of context. Is there an overview somewhere of what you guys are trying to accomplish? What should rendering for Cells look like? Yes, we have a design doc... which I forgot to share. Here is the link, comments welcome: https://docs.google.com/a/google.com/document/pub?id=1a-_8IdBrBmWCnhV6rnQVmzXB_bXQ9JQ4ya-k1_s1_sA http://gwt-code-reviews.appspot.com/1466809/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Implements UiBinder rendering for Cells. (issue1466809)
On 2011/07/01 21:19:25, stephenh wrote: https://docs.google.com/a/google.com/document/pub?id=1a-_8IdBrBmWCnhV6rnQVmzXB_bXQ9JQ4ya-k1_s1_sA Is it okay to make that public? I think it is OK. We usually publish them in the Wiki, but Docs is getting so good that I wanted to try it this way. http://gwt-code-reviews.appspot.com/1466809/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: This patch substantially reduces the overhead of Java types in the output by minimizing vtable s... (issue1447821)
Oops, there's a problem, I need one more cycle before I can commit.
Will send patch tonite. :(
On Fri, Jul 1, 2011 at 1:43 PM, cromwell...@google.com wrote:
I made the fixes and submitted it. Since most people are headed out of
the office for the holiday, I need to get it in early to see if I need
to revert it before everyone is gone. :)
http://gwt-code-reviews.appspot.com/1447821/diff/15001/dev/core/src/com/google/gwt/dev/jjs/impl/ControlFlowAnalyzer.java
File dev/core/src/com/google/gwt/dev/jjs/impl/ControlFlowAnalyzer.java
(right):
http://gwt-code-reviews.appspot.com/1447821/diff/15001/dev/core/src/com/google/gwt/dev/jjs/impl/ControlFlowAnalyzer.java#newcode93
dev/core/src/com/google/gwt/dev/jjs/impl/ControlFlowAnalyzer.java:93:
On 2011/07/01 10:24:38, jbrosenberg wrote:
whitespace
Done.
http://gwt-code-reviews.appspot.com/1447821/diff/15001/dev/core/src/com/google/gwt/dev/jjs/impl/ControlFlowAnalyzer.java#newcode600
dev/core/src/com/google/gwt/dev/jjs/impl/ControlFlowAnalyzer.java:600:
instantiatedTypes.add(type);
On 2011/07/01 10:24:38, jbrosenberg wrote:
Suggest combining all this into 1 method call to something like
(maybeRescueClassLiteral(type)), and then mRCL contains logic to see
if
classLiteralsToBeRescuedIfGetClassLive is null or not, to decide
whether to
rescue or mark for rescue.
Done.
http://gwt-code-reviews.appspot.com/1447821/diff/15001/dev/core/src/com/google/gwt/dev/jjs/impl/ControlFlowAnalyzer.java#newcode769
dev/core/src/com/google/gwt/dev/jjs/impl/ControlFlowAnalyzer.java:769:
if (classLiteralsToBeRescuedIfGetClassIsLive != null) {
Not sure, but rescueClassLiteral can end up invoking this function
again, and you'll get a concurrent modification exception if you don't
clone, and if you do, you end up with extra work.
On 2011/07/01 10:24:38, jbrosenberg wrote:
is re-entrant the right term (I usually think of it wrt to
concurrency)?
http://gwt-code-reviews.appspot.com/1447821/diff/15001/dev/core/src/com/google/gwt/dev/jjs/impl/ControlFlowAnalyzer.java#newcode870
dev/core/src/com/google/gwt/dev/jjs/impl/ControlFlowAnalyzer.java:870:
On 2011/07/01 10:24:38, jbrosenberg wrote:
whitespace
Done.
http://gwt-code-reviews.appspot.com/1447821/diff/15001/dev/core/src/com/google/gwt/dev/jjs/impl/ControlFlowAnalyzer.java#newcode917
dev/core/src/com/google/gwt/dev/jjs/impl/ControlFlowAnalyzer.java:917:
/**
On 2011/07/01 10:24:38, jbrosenberg wrote:
C
IDE has bizarre bug where it was randomly inserting these.
http://gwt-code-reviews.appspot.com/1447821/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] UnifyAst correctly handles polymorphic overrides with mixed default/public access. (issue1470803)
Reviewers: zundel, jbrosenberg, Message: Hey guys, Please review. This fixes the front-end issue withe GwtAstBuilder, bringing it up to parity with GenerateJavaAST. However, there *is* in fact a bug in the backend. See: http://gwt-code-reviews.appspot.com/1470803/ But I want to go ahead and get this fix in, the bug has been around forever, it would seem. Description: GWT AST now tracks method access. This is used by UnifyAst to correctly compute overrides. Before, UnifyAst would allow a public method to override a default-access method in a different package, which is illegal. Please review this at http://gwt-code-reviews.appspot.com/1470803/ Affected files: A dev/core/src/com/google/gwt/dev/jjs/ast/AccessModifiers.java M dev/core/src/com/google/gwt/dev/jjs/ast/JConstructor.java M dev/core/src/com/google/gwt/dev/jjs/ast/JMethod.java M dev/core/src/com/google/gwt/dev/jjs/ast/JProgram.java M dev/core/src/com/google/gwt/dev/jjs/impl/BuildTypeMap.java M dev/core/src/com/google/gwt/dev/jjs/impl/GenerateJavaAST.java M dev/core/src/com/google/gwt/dev/jjs/impl/GwtAstBuilder.java M dev/core/src/com/google/gwt/dev/jjs/impl/JsoDevirtualizer.java M dev/core/src/com/google/gwt/dev/jjs/impl/MakeCallsStatic.java M dev/core/src/com/google/gwt/dev/jjs/impl/ReferenceMapper.java M dev/core/src/com/google/gwt/dev/jjs/impl/ResolveRebinds.java M dev/core/src/com/google/gwt/dev/jjs/impl/ToStringGenerationVisitor.java M dev/core/src/com/google/gwt/dev/jjs/impl/UnifyAst.java M dev/core/test/com/google/gwt/dev/jjs/impl/PrunerTest.java -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: This patch substantially reduces the overhead of Java types in the output by minimizing vtable s... (issue1447821)
http://gwt-code-reviews.appspot.com/1447821/diff/15001/dev/core/src/com/google/gwt/dev/jjs/impl/ControlFlowAnalyzer.java
File dev/core/src/com/google/gwt/dev/jjs/impl/ControlFlowAnalyzer.java
(right):
http://gwt-code-reviews.appspot.com/1447821/diff/15001/dev/core/src/com/google/gwt/dev/jjs/impl/ControlFlowAnalyzer.java#newcode769
dev/core/src/com/google/gwt/dev/jjs/impl/ControlFlowAnalyzer.java:769:
if (classLiteralsToBeRescuedIfGetClassIsLive != null) {
Ah, so you're saying it could re-enter recursively...
http://gwt-code-reviews.appspot.com/1447821/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
