[gwt-contrib] Re: Resubmitting r9970 again (again) - updating Animation to use the native requestAnimationFrame, w... (issue1446812)
I only gave it a glance but here are a few comments:
- while I like the public requestAnimationFrame API, I don't like the
static methods
- I don't quite like the ImplMozilla/ImplWebkit extends ImplTimer
pattern
See details inline.
http://gwt-code-reviews.appspot.com/1446812/diff/1/user/src/com/google/gwt/animation/Animation.gwt.xml
File user/src/com/google/gwt/animation/Animation.gwt.xml (right):
http://gwt-code-reviews.appspot.com/1446812/diff/1/user/src/com/google/gwt/animation/Animation.gwt.xml#newcode22
user/src/com/google/gwt/animation/Animation.gwt.xml:22: inherits
name=com.google.gwt.user.UserAgent/
Those are implicitly inherited from the c.g.g.user.User module.
+ inheriting UserAgent without User cause all sorts warnings, see
http://code.google.com/p/google-web-toolkit/issues/detail?id=2815
http://gwt-code-reviews.appspot.com/1446812/diff/1/user/src/com/google/gwt/animation/client/Animation.java
File user/src/com/google/gwt/animation/client/Animation.java (right):
http://gwt-code-reviews.appspot.com/1446812/diff/1/user/src/com/google/gwt/animation/client/Animation.java#newcode56
user/src/com/google/gwt/animation/client/Animation.java:56: public
static void cancelAnimationFrame(AnimationRequestId requestId) {
How about a cancel() method on AnimationRequestId instead?
http://gwt-code-reviews.appspot.com/1446812/diff/1/user/src/com/google/gwt/animation/client/Animation.java#newcode82
user/src/com/google/gwt/animation/client/Animation.java:82: public
static AnimationRequestId requestAnimationFrame(AnimationCallback
callback) {
The issue with static methods is that you cannot mock them. When doing
the previous patch, my idea rather was to extend Scheduler with a
scheduleAnimationFrame or something like that (but without the
cancel bit then, as there's no such notion of cancellation in
Scheduler). I didn't go as far as implementing it as I though it'd be
better done in a distinct CL.
http://gwt-code-reviews.appspot.com/1446812/diff/1/user/src/com/google/gwt/animation/client/Animation.java#newcode105
user/src/com/google/gwt/animation/client/Animation.java:105: private
static AnimationImpl impl() {
Is there anything in Animation that wouldn't use the impl and justify
lazy-initializing it? or is to allow injecting a mock AnimationImpl
using reflection in unit tests? (looks bad; I like the Scheduler.get() /
SchedulerImpl.INSTANCE pattern much better)
http://gwt-code-reviews.appspot.com/1446812/diff/1/user/src/com/google/gwt/animation/client/AnimationImplMozilla.java
File user/src/com/google/gwt/animation/client/AnimationImplMozilla.java
(right):
http://gwt-code-reviews.appspot.com/1446812/diff/1/user/src/com/google/gwt/animation/client/AnimationImplMozilla.java#newcode57
user/src/com/google/gwt/animation/client/AnimationImplMozilla.java:57:
if (!isSupported) {
Can't there be a better pattern that would use either
AnimationImplMozilla (mozRequestAnimationFrame) or AnimationImplTimer so
that isSupported is only evaluated once at startup/first use?
Maybe something like a hasNativeSupport() method that Animation#impl()
would call, and if it returns false it switches to an explicit new
AnimationImplTimer() ?
(and those ifs in ImplMozilla and ImplWebkit would simply turn into
asserts: if the ImplMozilla/ImplWebkit is used, it must be that there is
native support for requestAnimationFrame)
http://gwt-code-reviews.appspot.com/1446812/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Resubmitting r9970 again (again) - updating Animation to use the native requestAnimationFrame, w... (issue1446812)
http://gwt-code-reviews.appspot.com/1446812/diff/1/user/src/com/google/gwt/animation/Animation.gwt.xml
File user/src/com/google/gwt/animation/Animation.gwt.xml (right):
http://gwt-code-reviews.appspot.com/1446812/diff/1/user/src/com/google/gwt/animation/Animation.gwt.xml#newcode22
user/src/com/google/gwt/animation/Animation.gwt.xml:22: inherits
name=com.google.gwt.user.UserAgent/
On 2011/05/26 13:07:58, tbroyer wrote:
Those are implicitly inherited from the c.g.g.user.User module.
+ inheriting UserAgent without User cause all sorts warnings, see
http://code.google.com/p/google-web-toolkit/issues/detail?id=2815
Done.
http://gwt-code-reviews.appspot.com/1446812/diff/1/user/src/com/google/gwt/animation/client/Animation.java
File user/src/com/google/gwt/animation/client/Animation.java (right):
http://gwt-code-reviews.appspot.com/1446812/diff/1/user/src/com/google/gwt/animation/client/Animation.java#newcode56
user/src/com/google/gwt/animation/client/Animation.java:56: public
static void cancelAnimationFrame(AnimationRequestId requestId) {
That works for me. Should we rename AnimationRequestId to
AnimationHandle?
On 2011/05/26 13:07:58, tbroyer wrote:
How about a cancel() method on AnimationRequestId instead?
http://gwt-code-reviews.appspot.com/1446812/diff/1/user/src/com/google/gwt/animation/client/Animation.java#newcode82
user/src/com/google/gwt/animation/client/Animation.java:82: public
static AnimationRequestId requestAnimationFrame(AnimationCallback
callback) {
I think the method will be used like a Scheduler as well, but some
people objected to actually putting animation methods in the Scheduler
class. What about creating a
com.google.gwt.animation.client.AnimationScheduler class and move these
methods there (as instance methods)?
On 2011/05/26 13:07:58, tbroyer wrote:
The issue with static methods is that you cannot mock them. When doing
the
previous patch, my idea rather was to extend Scheduler with a
scheduleAnimationFrame or something like that (but without the
cancel bit
then, as there's no such notion of cancellation in Scheduler). I
didn't go as
far as implementing it as I though it'd be better done in a distinct
CL.
http://gwt-code-reviews.appspot.com/1446812/diff/1/user/src/com/google/gwt/animation/client/Animation.java#newcode105
user/src/com/google/gwt/animation/client/Animation.java:105: private
static AnimationImpl impl() {
Agreed. See AnimationScheduler suggestion above.
On 2011/05/26 13:07:58, tbroyer wrote:
Is there anything in Animation that wouldn't use the impl and justify
lazy-initializing it? or is to allow injecting a mock AnimationImpl
using
reflection in unit tests? (looks bad; I like the Scheduler.get() /
SchedulerImpl.INSTANCE pattern much better)
http://gwt-code-reviews.appspot.com/1446812/diff/1/user/src/com/google/gwt/animation/client/AnimationImplMozilla.java
File user/src/com/google/gwt/animation/client/AnimationImplMozilla.java
(right):
http://gwt-code-reviews.appspot.com/1446812/diff/1/user/src/com/google/gwt/animation/client/AnimationImplMozilla.java#newcode57
user/src/com/google/gwt/animation/client/AnimationImplMozilla.java:57:
if (!isSupported) {
On 2011/05/26 13:07:58, tbroyer wrote:
Can't there be a better pattern that would use either
AnimationImplMozilla
(mozRequestAnimationFrame) or AnimationImplTimer so that isSupported
is only
evaluated once at startup/first use?
Maybe something like a hasNativeSupport() method that Animation#impl()
would
call, and if it returns false it switches to an explicit new
AnimationImplTimer() ?
(and those ifs in ImplMozilla and ImplWebkit would simply turn into
asserts: if
the ImplMozilla/ImplWebkit is used, it must be that there is native
support for
requestAnimationFrame)
I don't like mixing GWT.create() with new, especially if
AnimationImplTimer uses a Timer that needs to be mocked as well for
non-browser testing.
Alternatively, AnimationImplMozilla can have an inner class NativeImpl
that implements AnimationImpl. In the constructor of
AnimationImplMozilla, we check isSupported(), then set an instance field
impl = new AnimationImplTimer() or to new NativeImpl().
class AnimationImplMozilla implements AnimationImpl }
AnimationImpl impl;
AnimationImplMozilla() {
impl = isSupported() ? new NativeImpl() : new AnimationImplTimer();
}
requestAnimationFrame() {
return impl.requestAnimationFrame();
}
}
That way, AnimationImplMozilla does not need to extend
AnimationImplTimer, there is only one check in the constructor, and we
do not have any new calls in Animation.
http://gwt-code-reviews.appspot.com/1446812/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Resubmitting r9970 again (again) - updating Animation to use the native requestAnimationFrame, w... (issue1446812)
http://gwt-code-reviews.appspot.com/1446812/diff/1/user/src/com/google/gwt/animation/client/Animation.java
File user/src/com/google/gwt/animation/client/Animation.java (right):
http://gwt-code-reviews.appspot.com/1446812/diff/1/user/src/com/google/gwt/animation/client/Animation.java#newcode56
user/src/com/google/gwt/animation/client/Animation.java:56: public
static void cancelAnimationFrame(AnimationRequestId requestId) {
On 2011/05/26 14:15:47, jlabanca wrote:
That works for me. Should we rename AnimationRequestId to
AnimationHandle?
+1
http://gwt-code-reviews.appspot.com/1446812/diff/1/user/src/com/google/gwt/animation/client/Animation.java#newcode82
user/src/com/google/gwt/animation/client/Animation.java:82: public
static AnimationRequestId requestAnimationFrame(AnimationCallback
callback) {
On 2011/05/26 14:15:47, jlabanca wrote:
I think the method will be used like a Scheduler as well, but some
people
objected to actually putting animation methods in the Scheduler class.
What
about creating a com.google.gwt.animation.client.AnimationScheduler
class and
move these methods there (as instance methods)?
+1, using the same pattern as Scheduler with a static get() method
http://gwt-code-reviews.appspot.com/1446812/diff/1/user/src/com/google/gwt/animation/client/AnimationImplMozilla.java
File user/src/com/google/gwt/animation/client/AnimationImplMozilla.java
(right):
http://gwt-code-reviews.appspot.com/1446812/diff/1/user/src/com/google/gwt/animation/client/AnimationImplMozilla.java#newcode57
user/src/com/google/gwt/animation/client/AnimationImplMozilla.java:57:
if (!isSupported) {
On 2011/05/26 14:15:47, jlabanca wrote:
I don't like mixing GWT.create() with new, especially if
AnimationImplTimer
uses a Timer that needs to be mocked as well for non-browser testing.
You could GWT.create(AnimationImplTimer.class) if you prefer ;-)
But there's already a similar pattern in FocusImpl where, if the
GWT.create()d class is a FocusImplStandard (instanceof), then it news
a FocusImpl.
I think the idea is not to allow usage with GWTMockUtilities.disarm(),
but rather to use a DI pattern where, outside tests, you fill the value
using Scheduler.get().
(In most cases, code using Scheduler would fail with NPEs if you use it
with GWTMockUtilities.disarm(), and I'd support doing the same for
AnimationScheduler).
Alternatively, AnimationImplMozilla can have an inner class NativeImpl
that
implements AnimationImpl. In the constructor of AnimationImplMozilla,
we check
isSupported(), then set an instance field impl = new
AnimationImplTimer() or
to new NativeImpl().
class AnimationImplMozilla implements AnimationImpl }
AnimationImpl impl;
AnimationImplMozilla() {
impl = isSupported() ? new NativeImpl() : new
AnimationImplTimer();
}
requestAnimationFrame() {
return impl.requestAnimationFrame();
}
}
That way, AnimationImplMozilla does not need to extend
AnimationImplTimer, there
is only one check in the constructor, and we do not have any new
calls in
Animation.
But that introduces yet another indirection.
Honestly, I have absolutely no problem mixing GWT.create() with new as
in FocusImpl, provided this is done in an impl class as well:
public abstract class AnimationScheduler {
public AnimationScheduler get() {
// This is the only use of AnimationSchedulerImpl in
AnimationScheduler, so it's safe as long as you don't call get()
outside of a browser context; in unit tests, you'd provide a mock
AnimationScheduler and never touch AnimationSchedulerImpl
return AnimationSchedulerImpl.INSTANCE;
}
...
}
class AnimationSchedulerImpl extends AnimationScheduler {
static final AnimationScheduler INSTANCE;
static {
AnimationSchedulerImpl impl =
GWT.create(AnimationSchedulerImpl.class);
// if impl==null, use null (would be because of
GWTMockUtilities.disarm(), we don't want to new an
AnimationSchedulerImpl in this case)
INSTANCE = (impl == null || impl.isSupported()) ? impl : new
AnimationSchedulerImpl();
}
protected abstract boolean isSupported();
}
or something like that.
Of course YMMV, and I'd be OK with whatever the team prefers.
http://gwt-code-reviews.appspot.com/1446812/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Bugfixes in ControlFlowAnalyzer (issue1443807)
http://gwt-code-reviews.appspot.com/1443807/diff/2001/dev/core/src/com/google/gwt/dev/jjs/impl/ControlFlowAnalyzer.java
File dev/core/src/com/google/gwt/dev/jjs/impl/ControlFlowAnalyzer.java
(left):
http://gwt-code-reviews.appspot.com/1443807/diff/2001/dev/core/src/com/google/gwt/dev/jjs/impl/ControlFlowAnalyzer.java#oldcode231
dev/core/src/com/google/gwt/dev/jjs/impl/ControlFlowAnalyzer.java:231: }
This loop has the same comment as the one in visit(JInterfaceType, ...)
but written differently. Since refereces is always false, if
isInstantiated is always false, this rescue becomes a no-op. We could
either surround this with an if test as below, or just predicate rescue
(JReferenceType, isReferenced, isInstantiated with:
if (type == null || (!isReferenced !isInstantiated)) { return; }
http://gwt-code-reviews.appspot.com/1443807/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Broken editor type conversions in GWT trunk r10227
TextBox is for editing String (it's a IsEditorLeafValueEditorString), use an IntegerBox for Integers (IsEditorLeafValueEditorInteger). (and this group is for contributors to GWT, use http://groups.google.com/group/google-web-toolkit for support questions) -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Changed method dependencies report to show method code sizes. Call stacks are now a dropdown act... (issue1451803)
Reviewers: zundel, Description: Changed method dependencies report to show method code sizes. Call stacks are now a dropdown action under each method. Please review this at http://gwt-code-reviews.appspot.com/1451803/ Affected files: M dev/core/src/com/google/gwt/dev/jjs/JavaToJavaScriptCompiler.java M dev/core/src/com/google/gwt/dev/jjs/impl/JsAbstractTextTransformer.java M dev/core/src/com/google/gwt/dev/jjs/impl/JsFunctionClusterer.java M dev/core/src/com/google/gwt/dev/jjs/impl/JsIEBlockTextTransformer.java M dev/core/src/com/google/gwt/dev/js/JsReportGenerationVisitor.java M dev/core/src/com/google/gwt/dev/js/JsSourceGenerationVisitorWithSizeBreakdown.java M dev/core/src/com/google/gwt/soyc/MakeTopLevelHtmlForPerm.java M dev/core/src/com/google/gwt/soyc/SizeBreakdown.java M dev/core/src/com/google/gwt/soyc/SoycDashboard.java M dev/core/src/com/google/gwt/soyc/StaticResources.java M dev/core/src/com/google/gwt/soyc/resources/soyc.css M eclipse/dev/.checkstyle M eclipse/samples/DynaTable/.classpath M eclipse/samples/DynaTable/.project M eclipse/user/.project M samples/dynatable/src/com/google/gwt/sample/dynatable/DynaTable.gwt.xml -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Changed method dependencies report to show method code sizes. Call stacks are now a dropdown act... (issue1451803)
A NOOO On 2011/05/26 15:08:30, dconnelly wrote: http://gwt-code-reviews.appspot.com/1451803/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Отг: Re: Broken editor type conversions in GWT trunk r10227
Ah my mistake. Sorry about that post, but I wasn't sure where the problem is with my usage or some GWT issue. Thats why I posted it on both groups. -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Misc GWT compiler bugfixes and cleanups (issue1452802)
Reviewers: jbrosenberg, zundel,
http://gwt-code-reviews.appspot.com/1452802/diff/1/dev/core/src/com/google/gwt/dev/jjs/ast/JGwtCreate.java
File dev/core/src/com/google/gwt/dev/jjs/ast/JGwtCreate.java (right):
http://gwt-code-reviews.appspot.com/1452802/diff/1/dev/core/src/com/google/gwt/dev/jjs/ast/JGwtCreate.java#newcode40
dev/core/src/com/google/gwt/dev/jjs/ast/JGwtCreate.java:40: if (ctor
instanceof JConstructor) {
All methods have their params frozen as soon as construction is done.
Probably it would be better to simply require params to be provided at
construction time instead of making it stateful, but that's how it was
written.
The reason to check for original params is that we're looking for a
specific signature. If some non-default constructor theoretically had
its params removed prior to this, we wouldn't want to call the wrong
one.
Practically speaking, it doesn't matter since opts don't run before
GWT.creates() are replaced, but this is more correct.
http://gwt-code-reviews.appspot.com/1452802/diff/1/dev/core/src/com/google/gwt/dev/jjs/impl/ImplementClassLiteralsAsFields.java
File
dev/core/src/com/google/gwt/dev/jjs/impl/ImplementClassLiteralsAsFields.java
(right):
http://gwt-code-reviews.appspot.com/1452802/diff/1/dev/core/src/com/google/gwt/dev/jjs/impl/ImplementClassLiteralsAsFields.java#newcode220
dev/core/src/com/google/gwt/dev/jjs/impl/ImplementClassLiteralsAsFields.java:220:
for (JMethod methodIt : enumType.getMethods()) {
Probably so. Will fix, or maybe just compare method signature.
http://gwt-code-reviews.appspot.com/1452802/diff/1/dev/core/src/com/google/gwt/dev/jjs/impl/ReferenceMapper.java
File dev/core/src/com/google/gwt/dev/jjs/impl/ReferenceMapper.java
(right):
http://gwt-code-reviews.appspot.com/1452802/diff/1/dev/core/src/com/google/gwt/dev/jjs/impl/ReferenceMapper.java#newcode157
dev/core/src/com/google/gwt/dev/jjs/impl/ReferenceMapper.java:157:
JMethod clinit =
I *think* that the currently-being-compiled-unit will want to encode a
reference from its own clinit to its super clinit, which will generally
be an external type from another unit. There needs to be a placeholder
to target so that it can be resolved later. I suppose it could be
written such that GwtAstBuilder does not insert the super clinit call
and leaves it to UnifyAst (the code I'm working on) to do so later.
But even if it didn't make a practical difference, there's a hard-baked
assumption in lots of places that method 0 of any class is always the
clinit. This isn't necessarily a great formulation, there's probably
better ways we could have modeled it, but there you have it. To not
have a placeholder method would risk introducing subtle bugs.
Please review this at http://gwt-code-reviews.appspot.com/1452802/
Affected files:
M dev/core/src/com/google/gwt/dev/jjs/ast/JClassType.java
M dev/core/src/com/google/gwt/dev/jjs/ast/JConstructor.java
M dev/core/src/com/google/gwt/dev/jjs/ast/JField.java
M dev/core/src/com/google/gwt/dev/jjs/ast/JFieldRef.java
M dev/core/src/com/google/gwt/dev/jjs/ast/JGwtCreate.java
M dev/core/src/com/google/gwt/dev/jjs/ast/JInterfaceType.java
M dev/core/src/com/google/gwt/dev/jjs/ast/JMethod.java
M dev/core/src/com/google/gwt/dev/jjs/ast/JStringLiteral.java
M dev/core/src/com/google/gwt/dev/jjs/impl/BuildTypeMap.java
M dev/core/src/com/google/gwt/dev/jjs/impl/GwtAstBuilder.java
M
dev/core/src/com/google/gwt/dev/jjs/impl/ImplementClassLiteralsAsFields.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/util/collect/Lists.java
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Adding new DataGrid widget. DataGrid is a variation of CellTable that supports a fixed header a... (issue1450805)
committed as r10228
http://gwt-code-reviews.appspot.com/1450805/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/1450805/diff/1/user/src/com/google/gwt/user/cellview/client/AbstractCellTable.java#newcode290
user/src/com/google/gwt/user/cellview/client/AbstractCellTable.java:290:
// IE doesn't support innerHtml on a TableSection or Table element, so
we
The comment is misleading. Its important to know if anyone touches the
code, but generating a new table makes sense anyway. Setting innerHtml
on a Table or TableSection seems more fragile than just wrapping it with
a table tag, regardless of the browser.
On 2011/05/25 23:16:42, rchandia wrote:
If this is due to IE behavior, shouldn't this strategy go in
ImplTrident, with a
more refined replacement strategy used here? Or may be explain why
generating
the whole table is not as bad as it sounds.
http://gwt-code-reviews.appspot.com/1450805/diff/1/user/src/com/google/gwt/user/cellview/client/AbstractCellTable.java#newcode1722
user/src/com/google/gwt/user/cellview/client/AbstractCellTable.java:1722:
private void updateDependsOnSelection() {
On 2011/05/25 23:16:42, rchandia wrote:
The name of this method is now a misnomer as it updates a lot more
than just
dependsOnSelection. Same for the javadoc.
Done.
Renamed to coalesceCellProperties() and updated javadoc.
http://gwt-code-reviews.appspot.com/1450805/diff/1/user/src/com/google/gwt/user/cellview/client/CellTable.java
File user/src/com/google/gwt/user/cellview/client/CellTable.java
(right):
http://gwt-code-reviews.appspot.com/1450805/diff/1/user/src/com/google/gwt/user/cellview/client/CellTable.java#newcode669
user/src/com/google/gwt/user/cellview/client/CellTable.java:669:
super.setColumnWidth(column, width);
I wanted to update the JavaDoc comments. The additional comment doesn't
apply to DataGrid because it always uses fixed layout. I added a note
explaining why the methods are overridden.
On 2011/05/25 23:16:42, rchandia wrote:
Are these overriden methods necessary? I'd imagine the standard
virtual
dispatch in Java would handle this.
http://gwt-code-reviews.appspot.com/1450805/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Misc GWT compiler bugfixes and cleanups (issue1452802)
LGTM http://gwt-code-reviews.appspot.com/1452802/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Misc GWT compiler bugfixes and cleanups (issue1452802)
http://gwt-code-reviews.appspot.com/1452802/diff/1/dev/core/src/com/google/gwt/dev/jjs/impl/ReferenceMapper.java File dev/core/src/com/google/gwt/dev/jjs/impl/ReferenceMapper.java (right): http://gwt-code-reviews.appspot.com/1452802/diff/1/dev/core/src/com/google/gwt/dev/jjs/impl/ReferenceMapper.java#newcode157 dev/core/src/com/google/gwt/dev/jjs/impl/ReferenceMapper.java:157: JMethod clinit = I'm playing catchup: this part I don't understand. Why is this not just bloat on this class? Is this to satisfy referencing from subclasses? - if so, at the time they are resolved, won't there be a real $clinit() http://gwt-code-reviews.appspot.com/1452802/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: AutoboxUtils cleanup (issue1443808)
http://gwt-code-reviews.appspot.com/1443808/diff/1/dev/core/src/com/google/gwt/dev/jjs/impl/AutoboxUtils.java
File dev/core/src/com/google/gwt/dev/jjs/impl/AutoboxUtils.java (right):
http://gwt-code-reviews.appspot.com/1443808/diff/1/dev/core/src/com/google/gwt/dev/jjs/impl/AutoboxUtils.java#newcode47
dev/core/src/com/google/gwt/dev/jjs/impl/AutoboxUtils.java:47: for
(JMethod method : wrapperType.getMethods()) {
move initialization of boxSig unboxSig before the start of the for
loop.
http://gwt-code-reviews.appspot.com/1443808/diff/1/dev/core/src/com/google/gwt/dev/jjs/impl/AutoboxUtils.java#newcode97
dev/core/src/com/google/gwt/dev/jjs/impl/AutoboxUtils.java:97:
JMethodCall argMethodCall = (JMethodCall) arg;
How do we know the only possible methodCall that could be passed here
will be an unboxMethod()? Is that the only methodCall type that can be
the lhs of a binaray assignment (e.g. looking at the call-site in
FixAssignmentToUnbox).
It looks like there are other call-sites with less guaranteed
semantics...
http://gwt-code-reviews.appspot.com/1443808/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: AutoboxUtils cleanup (issue1443808)
http://gwt-code-reviews.appspot.com/1443808/diff/1/dev/core/src/com/google/gwt/dev/jjs/impl/AutoboxUtils.java
File dev/core/src/com/google/gwt/dev/jjs/impl/AutoboxUtils.java (right):
http://gwt-code-reviews.appspot.com/1443808/diff/1/dev/core/src/com/google/gwt/dev/jjs/impl/AutoboxUtils.java#newcode47
dev/core/src/com/google/gwt/dev/jjs/impl/AutoboxUtils.java:47: for
(JMethod method : wrapperType.getMethods()) {
On 2011/05/26 15:45:00, jbrosenberg wrote:
move initialization of boxSig unboxSig before the start of the for
loop.
Done. Whoops!!
http://gwt-code-reviews.appspot.com/1443808/diff/1/dev/core/src/com/google/gwt/dev/jjs/impl/AutoboxUtils.java#newcode97
dev/core/src/com/google/gwt/dev/jjs/impl/AutoboxUtils.java:97:
JMethodCall argMethodCall = (JMethodCall) arg;
Yeah, you cannot assign into a method call. Code like foo() = 3 is an
error, so is foo()++. The only time it should ever happen is exactly
this case that this pass fixes. So it really should just be an
assertion.
http://gwt-code-reviews.appspot.com/1443808/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: A mechanical refactoring of the Precompile options as prep for further cleanup. (issue1452804)
Specifically, I used Eclipse refactoring to move Precompile.PrecompileOptions, Precompile.PrecompileOptionsImpl, Precompile.ArgProcessor, and GraphicsInitThread to top level classes. I updated one or two comments, and added @Override tags where eclipse highlighted them as being needed. http://gwt-code-reviews.appspot.com/1452804/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: A mechanical refactoring of the Precompile options as prep for further cleanup. (issue1452804)
LGTM http://gwt-code-reviews.appspot.com/1452804/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Changed method dependencies report to show method code sizes. Call stacks are now a dropdown act... (issue1443809)
Reviewers: zundel, Description: Changed method dependencies report to show method code sizes. Call stacks are now a dropdown action under each method. Review by: zun...@google.com Please review this at http://gwt-code-reviews.appspot.com/1443809/ Affected files: M dev/core/src/com/google/gwt/soyc/MakeTopLevelHtmlForPerm.java M dev/core/src/com/google/gwt/soyc/SizeBreakdown.java M dev/core/src/com/google/gwt/soyc/SoycDashboard.java M dev/core/src/com/google/gwt/soyc/StaticResources.java M dev/core/src/com/google/gwt/soyc/resources/soyc.css -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Add BatchedRequestScope utility class to aggregate all requests made within a single tick of the... (issue1449804)
LGTM http://gwt-code-reviews.appspot.com/1449804/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Add RequestContext.find() to support chained requests. (issue1448806)
LGTM
http://gwt-code-reviews.appspot.com/1448806/diff/1/user/src/com/google/web/bindery/requestfactory/shared/RequestContext.java
File
user/src/com/google/web/bindery/requestfactory/shared/RequestContext.java
(right):
http://gwt-code-reviews.appspot.com/1448806/diff/1/user/src/com/google/web/bindery/requestfactory/shared/RequestContext.java#newcode19
user/src/com/google/web/bindery/requestfactory/shared/RequestContext.java:19:
* The base interface for RequestFactory service endpoints.
Add disclaimer explaining that this interface (and the others) are
normally implemented by generated code, and are subject to incompatible
updates?
And should log an item on the issue tracker with the appropriate
breaking change label.
http://gwt-code-reviews.appspot.com/1448806/diff/1/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://gwt-code-reviews.appspot.com/1448806/diff/1/user/src/com/google/web/bindery/requestfactory/shared/impl/AbstractRequestContext.java#newcode528
user/src/com/google/web/bindery/requestfactory/shared/impl/AbstractRequestContext.java:528:
new Object[] {proxyId}, propertyRefs, proxyId.getProxyClass(), null);
Ha! Do you have other sneaky literals like this lying around?
http://gwt-code-reviews.appspot.com/1448806/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Changed method dependencies report to show method code sizes. Call stacks are now a dropdown act... (issue1443809)
And, as you pointed out, there is no need for the class name headers to
be clickable - could you remove that non-functioning clicking?
http://gwt-code-reviews.appspot.com/1443809/diff/1/dev/core/src/com/google/gwt/soyc/MakeTopLevelHtmlForPerm.java
File dev/core/src/com/google/gwt/soyc/MakeTopLevelHtmlForPerm.java
(right):
http://gwt-code-reviews.appspot.com/1443809/diff/1/dev/core/src/com/google/gwt/soyc/MakeTopLevelHtmlForPerm.java#newcode209
dev/core/src/com/google/gwt/soyc/MakeTopLevelHtmlForPerm.java:209:
outFile.print(b1( + nameArray + ,);
Rename b1() and b2() to be one character names in order to save space in
the generated output.
http://gwt-code-reviews.appspot.com/1443809/diff/1/dev/core/src/com/google/gwt/soyc/MakeTopLevelHtmlForPerm.java#newcode279
dev/core/src/com/google/gwt/soyc/MakeTopLevelHtmlForPerm.java:279:
outFile.println(\open\ : \images/play-g16-down.png\,);
I assume these files will be checked in with the patch.
http://gwt-code-reviews.appspot.com/1443809/diff/1/dev/core/src/com/google/gwt/soyc/MakeTopLevelHtmlForPerm.java#newcode327
dev/core/src/com/google/gwt/soyc/MakeTopLevelHtmlForPerm.java:327:
outFile.println(document.write(\img onclick='swapShowHide(
+
fix the size of the icon when the element is created to avoid re-layout
once the image resource is loaded.
http://gwt-code-reviews.appspot.com/1443809/diff/1/dev/core/src/com/google/gwt/soyc/SizeBreakdown.java
File dev/core/src/com/google/gwt/soyc/SizeBreakdown.java (right):
http://gwt-code-reviews.appspot.com/1443809/diff/1/dev/core/src/com/google/gwt/soyc/SizeBreakdown.java#newcode27
dev/core/src/com/google/gwt/soyc/SizeBreakdown.java:27: public
MapString, Integer methodToSize = new HashMapString, Integer();
this and most of the other fields should probably be private final and
use accessors, but I won't make you go through and do all that.
http://gwt-code-reviews.appspot.com/1443809/diff/1/dev/core/src/com/google/gwt/soyc/resources/soyc.css
File dev/core/src/com/google/gwt/soyc/resources/soyc.css (right):
http://gwt-code-reviews.appspot.com/1443809/diff/1/dev/core/src/com/google/gwt/soyc/resources/soyc.css#newcode208
dev/core/src/com/google/gwt/soyc/resources/soyc.css:208:
.soyc-method-size-table {
? needed
http://gwt-code-reviews.appspot.com/1443809/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Creating a default Locator for RequestFactoryServlet
@Bobv Thanks for committing the previous change, and I've got one more change that will make my, and probably a bunch of other people's, lives easier. By being able to setup a DefaultLocator, it would stop me from having to copy/paste @ServiceName(value=com.my.service.MyService * locator=com.my.locator.SpringLocator*) in every RequstContext. The cleanest way I can think to implement this is by adding an additional constructor param and requiring the user extend RequestFactoryServlet like they already do with ServiceLayerDecorator The other option I considered was allowing them to specify a default Locator in web.xml, but that either becomes a much bigger patch or involves a static variable in RequestFactoryServlet being referenced by LocatorServiceLayer. What do you think? -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
Re: [gwt-contrib] Dollar sign and binary types
2011/5/26 Grzegorz Kossakowski grzegorz.kossakow...@gmail.com:
2011/5/26 Eric Ayers zun...@google.com:
Hi again,
Can you point out any of places where you saw this assumption? The
last time I was mucking around with binary type names I was told not
to assume that $ could not appear in source names, so it might be
unintentional.
Hi Eric,
The problematic place for me is ReplaceBindings.java, lines 154-155:
// Rebinds are always on a source type name.
String reqType = type.getName().replace('$', '.');
Any comment on that one? Working-around it is possible but quite
involved. I'd love to know if I need some solid work-around or just
hack to keep me going because this will be removed upstream.
--
Grzegorz Kossakowski
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Creating a default Locator for RequestFactoryServlet
How about simply using a ServiceLayerDecorator that overrides resolveLocator, delegating to super.resolveLocator and, if it returns null then return the default locator instead? -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Changed method dependencies report to show method code sizes. Call stacks are now a dropdown act... (issue1443809)
http://gwt-code-reviews.appspot.com/1443809/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
Re: [gwt-contrib] Dollar sign and binary types
On Thu, May 26, 2011 at 2:03 PM, Grzegorz Kossakowski
grzegorz.kossakow...@gmail.com wrote:
The problematic place for me is ReplaceBindings.java, lines 154-155:
// Rebinds are always on a source type name.
String reqType = type.getName().replace('$', '.');
Any comment on that one? Working-around it is possible but quite
involved. I'd love to know if I need some solid work-around or just
hack to keep me going because this will be removed upstream.
There is a common place for these in c.g.g.dev.util.Name, that provides all
the conversions between different name types. So, this should be:
String reqType = Name.BinaryName.toSourceName(type.getName())
However, that doesn't solve your problem.
The main problem is that once you get to a binary class name as a string,
you can no longer distinguish these cases. You would have to have package,
enclosing class, and class broken out separately in order to do anything
more useful with this, or at least have an oracle of possible classes you
can look up (though that would still fail if A.B and A$B were both present).
JReferenceType just takes a single string, so you can't get that
information.
However, I think in this case there is no need to replace $, since despite
the lack of documentation I believe type.getName() is actually a source name
(we have been really sloppy with the 3 types of names - a year or two ago I
started to rewrite it all with type-safe wrappers but that was rejected
because it dramatically increased memory usage in the compiler). For
example, if you look at ReferenceMapper.createType, it takes a JDT
ReferenceBinding.compoundName and creates the names by simply joining the
components with dots. My guess is if you simply remove the .replace from
that line, it will work fine (and in fact is removing a bug). I wouldn't
get your hopes up that that is the last time you will run into a problem,
because anyplace that calls Name.isSourceName, for example, will fail on a
class name with an embedded $.
--
John A. Tamplin
Software Engineer (GWT), Google
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
Re: [gwt-contrib] Re: Creating a default Locator for RequestFactoryServlet
On Thu, May 26, 2011 at 2:23 PM, Thomas Broyer t.bro...@gmail.com wrote: How about simply using a ServiceLayerDecorator that overrides resolveLocator, delegating to super.resolveLocator and, if it returns null then return the default locator instead? If it were in a separate annotation, you could simply define your own interface with whatever defaults you like and then extend that interface instead of RequestContext. We use this pattern a lot for i18n Messages to get company/project-wide defaults. -- John A. Tamplin Software Engineer (GWT), Google -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Change the wrapElement API to receive the id and a parent element. I also ported some of the boo... (issue1446811)
http://gwt-code-reviews.appspot.com/1446811/diff/1/user/src/com/google/gwt/user/client/ui/IsRenderable.java
File user/src/com/google/gwt/user/client/ui/IsRenderable.java (right):
http://gwt-code-reviews.appspot.com/1446811/diff/1/user/src/com/google/gwt/user/client/ui/IsRenderable.java#newcode29
user/src/com/google/gwt/user/client/ui/IsRenderable.java:29: public
interface IsRenderable extends SafeHtmlRendererString {
Let's drop the extends SafeHtmlRenderer.
Should document that it's up to the implementation to choose how to use
the id (unless we pass in a helper, see below). Perhaps change its name
to token to drive home the point.
http://gwt-code-reviews.appspot.com/1446811/diff/1/user/src/com/google/gwt/user/client/ui/IsRenderable.java#newcode33
user/src/com/google/gwt/user/client/ui/IsRenderable.java:33: * the DOM
tree. The id is assumed to be the same passed in at render time.
I think it's time to implement a helper class, to be used for both
rendering and
lookup. If we don't have one soon retrofitting will be a nightmare.
Document
here that the receiver cannot assume that the element is attached to the
dom,
and @see to the helper.
The helper could be something like:
SafeHtml stampElement(SafeHtml mustBeOpeningTag, String token) {
String tag = mustBeOpeningTag().asString();
assert tag.endsWith();
return SafeHtmlUtil.whatever(tag.substring(tag.length()-1) + ,
id=' +
token + '));
}
Element find(String token, Element parentElement) {
ensureAttached(parentElement);
return Document.get().getElementById(token);
}
And I think the thing to do is pass an instance of the helper into the
render() and wrap() calls. The alternative is to make some hacky static
lookup scheme, but I think we'll regret that pretty quickly.
You buying it?
http://gwt-code-reviews.appspot.com/1446811/diff/1/user/src/com/google/gwt/user/client/ui/RenderablePanel.java
File user/src/com/google/gwt/user/client/ui/RenderablePanel.java
(right):
http://gwt-code-reviews.appspot.com/1446811/diff/1/user/src/com/google/gwt/user/client/ui/RenderablePanel.java#newcode61
user/src/com/google/gwt/user/client/ui/RenderablePanel.java:61: BUILT,
RENDERED, WRAPPED, DONE
I'm not liking the phases. I think we can simplify.
http://gwt-code-reviews.appspot.com/1446811/diff/1/user/src/com/google/gwt/user/client/ui/RenderablePanel.java#newcode65
user/src/com/google/gwt/user/client/ui/RenderablePanel.java:65: //
single callback.
Time to take care of this. But that said, I don't like our plan from the
other day. :-)
We earlier talked about having binder generate subclasses of
RenderablePanel that override no-op default implementations, but I'm
having second thoughts about that. My rational was that it avoids
unnecessary class definitions, but that's silly — a subclass of
RenderablePanel is a new class just like a command object is — and it
will make it trickier to allow people to use their own subclasses of
RenderablePanel (which they will do, trust me).
Instead, let's introduce a null command object and use it here, so that
you can always call the thing without having to think abou it:
public Command wrapInitializationCallback = NullCommand.INSTANCE;
public Command detachedInitializationCallback = NullCommand.INSTANCE;
That said, I think you're right to dislike the magical phase check in
getElement(). Can you go ahead and rework this to give the wrap callback
an argument? So this becomes:
public WrapCallback wrapInitializationCallback = WrapCallback.INSTANCE;
public Command detachedInitializationCallback = NullCommand.INSTANCE;
But *that* said, see the comment on line 182. Maybe we dont need these
commands at all.
http://gwt-code-reviews.appspot.com/1446811/diff/1/user/src/com/google/gwt/user/client/ui/RenderablePanel.java#newcode70
user/src/com/google/gwt/user/client/ui/RenderablePanel.java:70:
protected SafeHtml html = null;
I dislike protected fields. Can you methods around these? Should also
ensure that the code here uses the methods rather than poking the fields
directly, in case subclasses get tricky.
http://gwt-code-reviews.appspot.com/1446811/diff/1/user/src/com/google/gwt/user/client/ui/RenderablePanel.java#newcode72
user/src/com/google/gwt/user/client/ui/RenderablePanel.java:72: private
String styleName = null;
TODO need a more general mechanism for caching attributes while
unwrapped
http://gwt-code-reviews.appspot.com/1446811/diff/1/user/src/com/google/gwt/user/client/ui/RenderablePanel.java#newcode182
user/src/com/google/gwt/user/client/ui/RenderablePanel.java:182: public
void performDetachedInitialization() {
If we go ahead and add the helper object to the wrap and render calls,
can it be in charge of accumulating the callbacks as well, and get these
command objects out of here?
http://gwt-code-reviews.appspot.com/1446811/diff/1/user/src/com/google/gwt/user/client/ui/RenderablePanel.java#newcode230
user/src/com/google/gwt/user/client/ui/RenderablePanel.java:230: if
(currentPhase != Phase.RENDERED) {
You're forcing the
[gwt-contrib] Re: Change the wrapElement API to receive the id and a parent element. I also ported some of the boo... (issue1446811)
http://gwt-code-reviews.appspot.com/1446811/diff/1/user/src/com/google/gwt/user/client/ui/IsRenderable.java
File user/src/com/google/gwt/user/client/ui/IsRenderable.java (right):
http://gwt-code-reviews.appspot.com/1446811/diff/1/user/src/com/google/gwt/user/client/ui/IsRenderable.java#newcode33
user/src/com/google/gwt/user/client/ui/IsRenderable.java:33: * the DOM
tree. The id is assumed to be the same passed in at render time.
Re-reading this, I see I shifted my stance in midstream. Let me shift if
further.
To be clear, I think we should not put control of the
document.getElementById() call in the hands of the individual panels,
and instead we should give them a helper object.
And now that I think about it, if we're giving them the helper then we
don't need to give them the id at all. It can be built into the helper.
And we can go back to giving them just the element they stamped.
interface Stamper {
/** Error to call more than once */
SafeHtml stampRenderedOpenTag(SafeHtml mustBeOpeningTag);
}
interface IsRenderable {
void render(SafeHtmlBuilder builder, Stamper stamper);
/** Provides the element that was stamped, not some ancestor */
void wrapElement(Element);
}
Note that it's entirely up to them to choose what element they stamp.
There is no requirement that it's the root of their rendered product,
and no requirement that the renderer produce a single dom element.
http://gwt-code-reviews.appspot.com/1446811/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Changed method dependencies report to show method code sizes. Call stacks are now a dropdown act... (issue1443809)
LGTM. Updated report looks great! http://gwt-code-reviews.appspot.com/1443809/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
Re: [gwt-contrib] Dollar sign and binary types
Unfortunately, I tried removing the replace('$',.) and it failed
miserably. Looking at it in the debugger, those really are binary names.
e.g.:
com.google.gwt.core.client.impl.AsyncFragmentLoader$LoadingStrategy
On Thu, May 26, 2011 at 2:24 PM, John Tamplin j...@google.com wrote:
On Thu, May 26, 2011 at 2:03 PM, Grzegorz Kossakowski
grzegorz.kossakow...@gmail.com wrote:
The problematic place for me is ReplaceBindings.java, lines 154-155:
// Rebinds are always on a source type name.
String reqType = type.getName().replace('$', '.');
Any comment on that one? Working-around it is possible but quite
involved. I'd love to know if I need some solid work-around or just
hack to keep me going because this will be removed upstream.
There is a common place for these in c.g.g.dev.util.Name, that provides
all the conversions between different name types. So, this should be:
String reqType = Name.BinaryName.toSourceName(type.getName())
However, that doesn't solve your problem.
The main problem is that once you get to a binary class name as a string,
you can no longer distinguish these cases. You would have to have package,
enclosing class, and class broken out separately in order to do anything
more useful with this, or at least have an oracle of possible classes you
can look up (though that would still fail if A.B and A$B were both present).
JReferenceType just takes a single string, so you can't get that
information.
However, I think in this case there is no need to replace $, since despite
the lack of documentation I believe type.getName() is actually a source name
(we have been really sloppy with the 3 types of names - a year or two ago I
started to rewrite it all with type-safe wrappers but that was rejected
because it dramatically increased memory usage in the compiler). For
example, if you look at ReferenceMapper.createType, it takes a JDT
ReferenceBinding.compoundName and creates the names by simply joining the
components with dots. My guess is if you simply remove the .replace from
that line, it will work fine (and in fact is removing a bug). I wouldn't
get your hopes up that that is the last time you will run into a problem,
because anyplace that calls Name.isSourceName, for example, will fail on a
class name with an embedded $.
--
John A. Tamplin
Software Engineer (GWT), Google
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
--
Eric Z. Ayers
Google Web Toolkit, Atlanta, GA USA
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
Re: [gwt-contrib] Activities Places
Thanks!! 2011/5/25 A. Stevko andy.ste...@gmail.com Thanks. I like it. It makes clearer some of the relationships. On Wed, May 25, 2011 at 11:43 AM, danieldietrich cafeb...@googlemail.comwrote: Hi, I've drawn an informal map about Activities Places - perhaps it is helpful for someone... Greetings from Kiel/Germany - Daniel (No warranty is given about correctness/completeness) -- http://groups.google.com/group/Google-Web-Toolkit-Contributors -- -- A. Stevko === If everything seems under control, you're just not going fast enough. M. Andretti -- http://groups.google.com/group/Google-Web-Toolkit-Contributors -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Change the wrapElement API to receive the id and a parent element. I also ported some of the boo... (issue1446811)
For the record: Rafa talked some sense into me offline. The Stamper notion really isn't practical given the limitations of SafeHtmlTemplates, so I'm backing off of most of this craziness. Instead we'll just delete the extends SafeHtmlRenderer thing. That will also allow backing away from the phase stuff, since we won't yet shift responsibility for the getElementById call. Instead, when this is done Rafa will look into extracting a superclass out of UIObject that is able to cache calls made to setStyleName, setVisible, setWidget, etc., before setElement has been called; maybe apply them during getElement(); but mainly provide access to them for smart subclasses to use in their render methods. (Eventually this new superclass, RenderableObject, should itself implement IsRenderable, but not in the first pass.) With that in hand we should be able to make move RenderableComposite back into Composite, and RenderablePanel back into HTMLPanel. WOOT! http://gwt-code-reviews.appspot.com/1446811/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Change the wrapElement API to receive the id and a parent element. I also ported some of the boo... (issue1446811)
http://gwt-code-reviews.appspot.com/1446811/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Re-implement runAsync to improve code size. (issue1442807)
Hey guys, I fixed some bugs and optimized a few things. Looking to resubmit this. http://gwt-code-reviews.appspot.com/1442807/diff/5033/user/src/com/google/gwt/core/client/impl/AsyncFragmentLoader.java File user/src/com/google/gwt/core/client/impl/AsyncFragmentLoader.java (right): http://gwt-code-reviews.appspot.com/1442807/diff/5033/user/src/com/google/gwt/core/client/impl/AsyncFragmentLoader.java#newcode306 user/src/com/google/gwt/core/client/impl/AsyncFragmentLoader.java:306: private final Object[][] allCallbacks; This actually generates less code. http://gwt-code-reviews.appspot.com/1442807/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Reformatting in advance of forthcoming patch (issue1446813)
Reviewers: zundel, Description: Reformatting in advance of forthcoming patch Please review this at http://gwt-code-reviews.appspot.com/1446813/ Affected files: M dev/core/src/com/google/gwt/core/ext/GeneratorContext.java M dev/core/src/com/google/gwt/core/ext/GeneratorContextExtWrapper.java M dev/core/src/com/google/gwt/dev/javac/StandardGeneratorContext.java M dev/core/src/com/google/gwt/dev/shell/StandardRebindOracle.java M user/src/com/google/gwt/i18n/rebind/CachedGeneratorContext.java M user/test/com/google/gwt/user/rebind/rpc/SerializableTypeOracleBuilderTest.java -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Reformatting in advance of forthcoming patch (issue1446813)
LGTM http://gwt-code-reviews.appspot.com/1446813/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Removes CompileModule.java, retaining the unit test that depends on it. (issue1449806)
Reviewers: scottb, jbrosenberg, Description: Removes CompileModule.java, retaining the unit test that depends on it. Please review this at http://gwt-code-reviews.appspot.com/1449806/ Affected files: D dev/core/src/com/google/gwt/dev/CompileModule.java A dev/core/src/com/google/gwt/dev/GwtAstBuilderUtil.java M user/test/com/google/gwt/dev/jjs/GwtAstBuilderTest.java -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Re-implement runAsync to improve code size. (issue1442807)
LGTM http://gwt-code-reviews.appspot.com/1442807/diff/5033/user/src/com/google/gwt/core/client/impl/AsyncFragmentLoader.java File user/src/com/google/gwt/core/client/impl/AsyncFragmentLoader.java (right): http://gwt-code-reviews.appspot.com/1442807/diff/5033/user/src/com/google/gwt/core/client/impl/AsyncFragmentLoader.java#newcode306 user/src/com/google/gwt/core/client/impl/AsyncFragmentLoader.java:306: private final Object[][] allCallbacks; On 2011/05/26 21:11:51, scottb wrote: This actually generates less code. I'll bite. Why does it generate less code? http://gwt-code-reviews.appspot.com/1442807/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Resubmitting r9970 again (again) - updating Animation to use the native requestAnimationFrame, w... (issue1446812)
http://gwt-code-reviews.appspot.com/1446812/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Adds ability to query the generator context whether a rebind rule exists for a given type (issue1450806)
Reviewers: scottb, zundel, Description: Adds ability to query the generator context whether a rebind rule exists for a given type Please review this at http://gwt-code-reviews.appspot.com/1450806/ Affected files: M dev/core/src/com/google/gwt/core/ext/GeneratorContext.java M dev/core/src/com/google/gwt/core/ext/GeneratorContextExtWrapper.java M dev/core/src/com/google/gwt/dev/javac/StandardGeneratorContext.java A dev/core/src/com/google/gwt/dev/javac/rebind/RebindRuleResolver.java M dev/core/src/com/google/gwt/dev/shell/StandardRebindOracle.java M user/src/com/google/gwt/i18n/rebind/CachedGeneratorContext.java M user/test/com/google/gwt/user/rebind/rpc/SerializableTypeOracleBuilderTest.java -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Resubmitting r9970 again (again) - updating Animation to use the native requestAnimationFrame, w... (issue1446812)
http://gwt-code-reviews.appspot.com/1446812/diff/1/user/src/com/google/gwt/animation/client/AnimationImplMozilla.java
File user/src/com/google/gwt/animation/client/AnimationImplMozilla.java
(right):
http://gwt-code-reviews.appspot.com/1446812/diff/1/user/src/com/google/gwt/animation/client/AnimationImplMozilla.java#newcode57
user/src/com/google/gwt/animation/client/AnimationImplMozilla.java:57:
if (!isSupported) {
On 2011/05/26 14:50:36, tbroyer wrote:
On 2011/05/26 14:15:47, jlabanca wrote:
I don't like mixing GWT.create() with new, especially if
AnimationImplTimer
uses a Timer that needs to be mocked as well for non-browser
testing.
You could GWT.create(AnimationImplTimer.class) if you prefer ;-)
But there's already a similar pattern in FocusImpl where, if the
GWT.create()d
class is a FocusImplStandard (instanceof), then it news a FocusImpl.
I think the idea is not to allow usage with GWTMockUtilities.disarm(),
but
rather to use a DI pattern where, outside tests, you fill the value
using
Scheduler.get().
(In most cases, code using Scheduler would fail with NPEs if you use
it with
GWTMockUtilities.disarm(), and I'd support doing the same for
AnimationScheduler).
Alternatively, AnimationImplMozilla can have an inner class
NativeImpl that
implements AnimationImpl. In the constructor of
AnimationImplMozilla, we
check
isSupported(), then set an instance field impl = new
AnimationImplTimer() or
to new NativeImpl().
class AnimationImplMozilla implements AnimationImpl }
AnimationImpl impl;
AnimationImplMozilla() {
impl = isSupported() ? new NativeImpl() : new
AnimationImplTimer();
}
requestAnimationFrame() {
return impl.requestAnimationFrame();
}
}
That way, AnimationImplMozilla does not need to extend
AnimationImplTimer,
there
is only one check in the constructor, and we do not have any new
calls in
Animation.
But that introduces yet another indirection.
Honestly, I have absolutely no problem mixing GWT.create() with new as
in
FocusImpl, provided this is done in an impl class as well:
public abstract class AnimationScheduler {
public AnimationScheduler get() {
// This is the only use of AnimationSchedulerImpl in
AnimationScheduler,
so it's safe as long as you don't call get() outside of a browser
context;
in unit tests, you'd provide a mock AnimationScheduler and never touch
AnimationSchedulerImpl
return AnimationSchedulerImpl.INSTANCE;
}
...
}
class AnimationSchedulerImpl extends AnimationScheduler {
static final AnimationScheduler INSTANCE;
static {
AnimationSchedulerImpl impl =
GWT.create(AnimationSchedulerImpl.class);
// if impl==null, use null (would be because of
GWTMockUtilities.disarm(),
we don't want to new an AnimationSchedulerImpl in this case)
INSTANCE = (impl == null || impl.isSupported()) ? impl : new
AnimationSchedulerImpl();
}
protected abstract boolean isSupported();
}
or something like that.
Of course YMMV, and I'd be OK with whatever the team prefers.
Done.
http://gwt-code-reviews.appspot.com/1446812/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Adds ability to query the generator context whether a rebind rule exists for a given type (issue1450806)
http://gwt-code-reviews.appspot.com/1450806/diff/1/dev/core/src/com/google/gwt/dev/shell/StandardRebindOracle.java File dev/core/src/com/google/gwt/dev/shell/StandardRebindOracle.java (left): http://gwt-code-reviews.appspot.com/1450806/diff/1/dev/core/src/com/google/gwt/dev/shell/StandardRebindOracle.java#oldcode52 dev/core/src/com/google/gwt/dev/shell/StandardRebindOracle.java:52: These 2 data structures appear to be relics from an older version of this class, I can't see how it's possible for them to retain state between successive calls to Rebinder.rebind() http://gwt-code-reviews.appspot.com/1450806/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Removes CompileModule.java, retaining the unit test that depends on it. (issue1449806)
LGTM http://gwt-code-reviews.appspot.com/1449806/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: RequestFactoryServlet
Here's the blog post and sample code. It's not Spring-y, but it's Guice-y! http://wanderingcanadian.posterous.com/guiced-up-gwt-requestfactory You are right Andrés. I've noticed the same behavior. Not sure if it's a feature or an issue though. Either way, it's something useful to know! -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] [google-web-toolkit] r10229 committed - Misc GWT compiler bugfixes and cleanups....
Revision: 10229
Author: sco...@google.com
Date: Thu May 26 06:00:55 2011
Log: Misc GWT compiler bugfixes and cleanups.
http://gwt-code-reviews.appspot.com/1452802/
http://code.google.com/p/google-web-toolkit/source/detail?r=10229
Modified:
/trunk/dev/core/src/com/google/gwt/dev/jjs/ast/JClassType.java
/trunk/dev/core/src/com/google/gwt/dev/jjs/ast/JConstructor.java
/trunk/dev/core/src/com/google/gwt/dev/jjs/ast/JField.java
/trunk/dev/core/src/com/google/gwt/dev/jjs/ast/JFieldRef.java
/trunk/dev/core/src/com/google/gwt/dev/jjs/ast/JGwtCreate.java
/trunk/dev/core/src/com/google/gwt/dev/jjs/ast/JInterfaceType.java
/trunk/dev/core/src/com/google/gwt/dev/jjs/ast/JMethod.java
/trunk/dev/core/src/com/google/gwt/dev/jjs/ast/JStringLiteral.java
/trunk/dev/core/src/com/google/gwt/dev/jjs/impl/BuildTypeMap.java
/trunk/dev/core/src/com/google/gwt/dev/jjs/impl/GwtAstBuilder.java
/trunk/dev/core/src/com/google/gwt/dev/jjs/impl/ImplementClassLiteralsAsFields.java
/trunk/dev/core/src/com/google/gwt/dev/jjs/impl/MakeCallsStatic.java
/trunk/dev/core/src/com/google/gwt/dev/jjs/impl/ReferenceMapper.java
/trunk/dev/core/src/com/google/gwt/dev/util/collect/Lists.java
===
--- /trunk/dev/core/src/com/google/gwt/dev/jjs/ast/JClassType.java Tue Apr
19 10:10:18 2011
+++ /trunk/dev/core/src/com/google/gwt/dev/jjs/ast/JClassType.java Thu May
26 06:00:55 2011
@@ -33,9 +33,7 @@
}
private Object readResolve() {
- JClassType result = new JClassType(SourceOrigin.UNKNOWN, name,
false, false);
- result.setExternal(true);
- return result;
+ return new JClassType(name);
}
}
@@ -48,6 +46,15 @@
this.isAbstract = isAbstract;
this.isFinal = isFinal;
}
+
+ /**
+ * Construct a bare-bones deserialized external class.
+ */
+ private JClassType(String name) {
+super(SourceOrigin.UNKNOWN, name);
+isAbstract = false;
+setExternal(true);
+ }
@Override
public String getClassLiteralFactoryMethod() {
===
--- /trunk/dev/core/src/com/google/gwt/dev/jjs/ast/JConstructor.java Tue
Apr 19 10:10:18 2011
+++ /trunk/dev/core/src/com/google/gwt/dev/jjs/ast/JConstructor.java Thu
May 26 06:00:55 2011
@@ -128,7 +128,7 @@
@Override
protected Object writeReplace() {
-if (getEnclosingType() != null getEnclosingType().isExternal()) {
+if (isExternal()) {
return new ExternalSerializedForm(this);
} else {
return this;
===
--- /trunk/dev/core/src/com/google/gwt/dev/jjs/ast/JField.java Fri May 13
08:44:36 2011
+++ /trunk/dev/core/src/com/google/gwt/dev/jjs/ast/JField.java Thu May 26
06:00:55 2011
@@ -122,6 +122,10 @@
public boolean isCompileTimeConstant() {
return isCompileTimeConstant;
}
+
+ public boolean isExternal() {
+return getEnclosingType() != null getEnclosingType().isExternal();
+ }
public boolean isStatic() {
return isStatic;
@@ -162,7 +166,7 @@
}
protected Object writeReplace() {
-if (enclosingType != null enclosingType.isExternal()) {
+if (isExternal()) {
return new ExternalSerializedForm(this);
} else if (this == NULL_FIELD) {
return ExternalSerializedNullField.INSTANCE;
===
--- /trunk/dev/core/src/com/google/gwt/dev/jjs/ast/JFieldRef.java Tue Apr
19 10:10:18 2011
+++ /trunk/dev/core/src/com/google/gwt/dev/jjs/ast/JFieldRef.java Thu May
26 06:00:55 2011
@@ -27,11 +27,6 @@
*/
private final JDeclaredType enclosingType;
- /**
- * The referenced field.
- */
- private final JField field;
-
/**
* This can only be null if the referenced field is static.
*/
@@ -54,7 +49,6 @@
assert (instance != null || field.isStatic());
assert (enclosingType != null);
this.instance = instance;
-this.field = field;
this.enclosingType = enclosingType;
this.overriddenType = overriddenType;
}
@@ -64,7 +58,7 @@
}
public JField getField() {
-return field;
+return (JField) getTarget();
}
public JExpression getInstance() {
@@ -80,6 +74,7 @@
}
public boolean hasClinit() {
+JField field = getField();
// A cross-class reference to a static, non constant field forces
clinit
if (!field.isStatic()) {
return false;
===
--- /trunk/dev/core/src/com/google/gwt/dev/jjs/ast/JGwtCreate.java Wed Apr
27 14:46:52 2011
+++ /trunk/dev/core/src/com/google/gwt/dev/jjs/ast/JGwtCreate.java Thu May
26 06:00:55 2011
@@ -38,7 +38,7 @@
JConstructor noArgCtor = null;
for (JMethod ctor : classType.getMethods()) {
if (ctor instanceof JConstructor) {
-if (ctor.getParams().size() == 0) {
+if (ctor.getOriginalParamTypes().size() == 0) {
noArgCtor = (JConstructor) ctor;
break;
}
===
---
[gwt-contrib] [google-web-toolkit] r10231 committed - Changed method dependencies report to show method code sizes. Call sta...
Revision: 10231
Author: dconne...@google.com
Date: Thu May 26 09:07:47 2011
Log: Changed method dependencies report to show method code sizes.
Call stacks are now a dropdown action under each method.
Review at http://gwt-code-reviews.appspot.com/1443809
Review by: zun...@google.com
http://code.google.com/p/google-web-toolkit/source/detail?r=10231
Added:
/trunk/dev/core/src/com/google/gwt/soyc/resources/images/play-g16-down.png
/trunk/dev/core/src/com/google/gwt/soyc/resources/images/play-g16.png
Modified:
/trunk/dev/core/src/com/google/gwt/soyc/MakeTopLevelHtmlForPerm.java
/trunk/dev/core/src/com/google/gwt/soyc/SizeBreakdown.java
/trunk/dev/core/src/com/google/gwt/soyc/SoycDashboard.java
/trunk/dev/core/src/com/google/gwt/soyc/StaticResources.java
/trunk/dev/core/src/com/google/gwt/soyc/resources/soyc.css
===
--- /dev/null
+++
/trunk/dev/core/src/com/google/gwt/soyc/resources/images/play-g16-down.png
Thu May 26 09:07:47 2011
Binary file, no diff available.
===
--- /dev/null
+++ /trunk/dev/core/src/com/google/gwt/soyc/resources/images/play-g16.png
Thu May 26 09:07:47 2011
Binary file, no diff available.
===
--- /trunk/dev/core/src/com/google/gwt/soyc/MakeTopLevelHtmlForPerm.java
Wed May 18 12:28:03 2011
+++ /trunk/dev/core/src/com/google/gwt/soyc/MakeTopLevelHtmlForPerm.java
Thu May 26 09:07:47 2011
@@ -37,6 +37,7 @@
import java.util.LinkedHashMap;
import java.util.List;
import java.util.Map;
+import java.util.Map.Entry;
import java.util.Set;
import java.util.SortedSet;
import java.util.TreeMap;
@@ -260,6 +261,23 @@
+ globalInformation.getSplitPointToLocation().get(sp) + ',);
}
outFile.println( ];);
+
+ // object/dictionary containing method sizes
+ outFile.println( var methodSizes = {);
+ for (SizeBreakdown breakdown :
globalInformation.allSizeBreakdowns()) {
+for (EntryString, Integer methodEntry :
breakdown.methodToSize.entrySet()) {
+ String methodSignature = methodEntry.getKey();
+ String method = methodSignature.substring(0,
methodSignature.indexOf(());
+ outFile.println(\ + method + \ : +
methodEntry.getValue() + ,);
+}
+ }
+ outFile.println(};);
+
+ // dropdown button image srcs
+ outFile.println( var images = {);
+ outFile.println(\closed\ : \images/play-g16.png\,);
+ outFile.println(\open\ : \images/play-g16-down.png\,);
+ outFile.println( };);
// TODO(zundel): Most of this below is just inserting a fixed string
into the code. It would
// be easier to read and maintain if we could store the fixed part
in a flat file. Use some
@@ -270,27 +288,68 @@
outFile.println( function a(packageRef, classRef) {);
outFile.println(var className = internedStrings[packageRef] +
\.\ +
+ internedStrings[classRef];);
- outFile.println(document.write(\a name='\ + className +
\'\););
- outFile.println(document.write(\h3
class='soyc-class-header'Class: \ + className +
- + \/a/h3\););
+ outFile.println(document.write(\div class='main'\););
+ outFile.println(document.write(\table
class='soyc-table'\););
+ outFile.println(document.write(\thead\););
+ outFile.println(document.write(\tha
class='soyc-class-name'
+ + name='\ + className + \'
+ + Class: \ + className + \/a/th\););
+ outFile.println(document.write(\th
class='soyc-numerical-col-header'Size
+ + span class='soyc-th-units'(bytes)/span/th\););
+ outFile.println(document.write(\/thead\););
outFile.println( });
-
+
+ outFile.println( function swapShowHide(elementName) {);
+ outFile.println(hp = document.getElementById(elementName););
+ outFile.println(arrow = document.getElementById(\dropdown-\ +
elementName););
+ outFile.println(if (hp.style.display !== \none\
hp.style.display
+ + !== \inline\) {);
+ outFile.println( hp.style.display = \inline\;);
+ outFile.println( arrow.src = images[\open\];);
+ outFile.println(} else if (hp.style.display === \none\) {);
+ outFile.println( hp.style.display = \inline\;);
+ outFile.println( arrow.src = images[\open\];);
+ outFile.println(} else {);
+ outFile.println( hp.style.display = \none\;);
+ outFile.println( arrow.src = images[\closed\];);
+ outFile.println(});
+ outFile.println( });
+
// function to print a single dependency in the methodDependencies
report
// see printDependency()
outFile.println( function b(c, deps) {);
- outFile.println(document.write(\div class='main'
- + a class='toggle soyc-call-stack-link'
onclick='toggle.call(this)'\););
-
