[gwt-contrib] Re: Fix for ScrollImplTrident leak (issue1601803)
LGTM http://gwt-code-reviews.appspot.com/1601803/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Issue 6331: Let AutoBean serialize dates as numbers when possible. (issue1601805)
http://gwt-code-reviews.appspot.com/1601805/diff/1/user/src/com/google/web/bindery/autobean/shared/ValueCodex.java
File user/src/com/google/web/bindery/autobean/shared/ValueCodex.java
(right):
http://gwt-code-reviews.appspot.com/1601805/diff/1/user/src/com/google/web/bindery/autobean/shared/ValueCodex.java#newcode368
user/src/com/google/web/bindery/autobean/shared/ValueCodex.java:368: if
(clazz.isEnum()) {
On 2011/12/06 16:29:24, tbroyer wrote:
(I'll try to do that testing right now though, so we can have numbers
on which
to base our decision).
I did my homework:
https://docs.google.com/spreadsheet/ccc?key=0Agcd-Zsy2T-YdGtvV2tOS1c0VlBYV0t6Vl9HTmlOa2c
Note that it's by no mean a scientific experiment (machine has more than
15 days of uptime, I had Chrome running at the same time –though I
didn't touch the machine while the benchmarks run–).
I'm not sure my tests are OK too (are you supposed to do loops in your
benchmarks? or the Benchmark machinery does that for you?)
Anyway, as I interpret it, there's an overall gain in removing the test,
except for the case where you want to encode an enum value for which
getClass().isEnum()==false (MyEnum.BAR in the test).
Based on that, I kept the change in. Let me know if you want me to
revert it.
http://gwt-code-reviews.appspot.com/1601805/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Issue 6331: Let AutoBean serialize dates as numbers when possible. (issue1601805)
OK, two more runs of the benchmark (using ant benchmark -Dgwt.benchmark.testcase.includes=**/ValueCodexBenchmark.class) scrambled the results (I also slightly modified one of the test, but that's another story). I decided to keep the change in nevertheless (based on the 80/20 rule and the fact that the fastest code is the one that never runs). Also, I forgot to mention in my previous message that there are graphs in separate sheets. https://docs.google.com/spreadsheet/ccc?key=0Agcd-Zsy2T-YdGtvV2tOS1c0VlBYV0t6Vl9HTmlOa2c The benchmark test is included in the latest patch set, as well as a fix for issue 6636 (accept numeric values for longs in addition to dates), removal of the serialization change for dates, and non-regression tests in AutoBeanCodexTest. http://gwt-code-reviews.appspot.com/1601805/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Adding jscomp to the classpath for gwt user and dev projects. (issue1606803)
committed as r10779 http://gwt-code-reviews.appspot.com/1606803/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Document a bug in maven-gae-plugin that prevents gae:unpack goal in mobilewebapp from running if... (issue1607803)
Reviewers: drfibonacci, Description: Document a bug in maven-gae-plugin that prevents gae:unpack goal in mobilewebapp from running if gae.home is set in ~/.m2/settings.xml Please review this at http://gwt-code-reviews.appspot.com/1607803/ Affected files: M samples/mobilewebapp/README-MAVEN.txt Index: samples/mobilewebapp/README-MAVEN.txt === --- samples/mobilewebapp/README-MAVEN.txt (revision 10779) +++ samples/mobilewebapp/README-MAVEN.txt (working copy) @@ -13,7 +13,8 @@ You can now browse the project in Eclipse. -To launch your web app in GWT development mode +To launch your web app in GWT development mode (see note below if you +have gae.home set in settings.xml): Go to the Run menu item and select Run - Run as - Web Application. @@ -47,7 +48,7 @@ 1.6 JDK. Maven uses the supplied 'pom.xml' file which describes exactly how to build your project. This file has been tested to work against Maven 2.2.1. The following assumes 'mvn' is on your command -line path. +line path. Also, see note below if you have gae.home set in settings.xml. To run development mode use the Maven GWT Plugin. @@ -57,3 +58,17 @@ For a full listing of other goals, visit: http://mojo.codehaus.org/gwt-maven-plugin/plugin-info.html + +-- Important Note: + +The first time you get a new App Engine version from Maven +Central, you must unpack it into your local repo with +mvn gae:unpack. The mobilewebapp POM should do this automatically as it +includes the gae:unpack goal; however, if you have gae.home set in +your settings.xml, it won't work properly so you must run mvn +gae:unpack manually first. + +For more info, see this bug report for maven-gae-plugin: + +https://github.com/maven-gae-plugin/maven-gae-plugin/issues/8 + -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Document a bug in maven-gae-plugin that prevents gae:unpack goal in mobilewebapp from running if... (issue1607803)
http://gwt-code-reviews.appspot.com/1607803/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Fix leak in LayoutImplIE6 (issue1601804)
As I understand it, there are two cases where createStyleRuler is called, and attached as a __styleRuler expando: - in initParent, the expando then references a child element; I'm not sure this creates a leak, as there's no loop here. It could easily be changed to use an instance variable instead (just like relativeRuler in LayoutImpl) if it happened to be an issue. - in attachChild, the expando then references a sibling element. It's not clear to me whether it'd create a leak either. IMO (and without any testing), the leak in TabLayoutPanel is caused by the forgotten calls to onAttach and onDetach from DeckLayoutPanel's onLoad/onUnload. All widgets using Layout make those calls, except DeckLayoutPanel. Then, there's the leak introduced by LayoutImplIE8#setLayer, which is explicitly documented: // Potential leak: This is cleaned up in detach(). So all layout panels leak in IE (all versions) when they're never attached to the document. Finally, there might be another leak you didn't talk about: in fillParentImpl, because parent.onresize is set to a local function referencing 'elem', and 'elem' in turn references parent from its __resizeParent expando, so: elem -(__resizeParent)- parent -(onresize)- elem; not to mention the simple leak: elem.__resizeParent == elem.parentElement (or elem.parentElement.parentElement; it looks like the __container loop from the other bug, in ScrollImpl). That one could be overcome by doing the hackery about tagName.toLowerCase()=='body' twice: once in fillParentImpl to branch to the hookWindowResize code path; and then in resizeRelativeToParent: in other words, instead of computing the resize parent once and store it in the __resizeParent expando, compute it each time we want it. However, it's not clear to me when the parent.onresize code path could be reached: fillParentImpl is only called from fillParent, and fillParent is only called by RootLayoutPanel, which always attaches itself within RootPanel.get(), which is the document.body. LayoutImpl calls fillParent from attachChild, but LayoutImplIE6 overrides the method and doesn't call its super implementation (i.e. it totally replaces it). It once called it in IE7 –not IE6– though, so it might be a leftover from the change: r8339 for reference). http://gwt-code-reviews.appspot.com/1601804/diff/4001/user/src/com/google/gwt/layout/client/LayoutImplIE6.java File user/src/com/google/gwt/layout/client/LayoutImplIE6.java (right): http://gwt-code-reviews.appspot.com/1601804/diff/4001/user/src/com/google/gwt/layout/client/LayoutImplIE6.java#newcode201 user/src/com/google/gwt/layout/client/LayoutImplIE6.java:201: setPropertyElement(parent, __styleRuler, null); Wouldn't it break the widget if it's later re-attached? initParent is only called when the widget is created; you'd have to recreate a styleRuler in onAttach for re-attaching to work. http://gwt-code-reviews.appspot.com/1601804/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Add assert for null provided fields, fixes #7024 (issue1602805)
Ping: rdayal or scheglov On 2011/12/01 08:11:49, tbroyer wrote: LGTM http://gwt-code-reviews.appspot.com/1602805/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Change SafeHtmlHostedModeUtils.isCompleteHtml() to public. (issue1606804)
LGTM Just reorder the method alphabetically within the other public methods. In GWT, we sort by visibility first, then alphabetically. http://gwt-code-reviews.appspot.com/1606804/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Change SafeHtmlHostedModeUtils.isCompleteHtml() to public. (issue1606804)
LGTM http://gwt-code-reviews.appspot.com/1606804/diff/2002/user/src/com/google/gwt/safehtml/shared/SafeHtmlHostedModeUtils.java File user/src/com/google/gwt/safehtml/shared/SafeHtmlHostedModeUtils.java (right): http://gwt-code-reviews.appspot.com/1606804/diff/2002/user/src/com/google/gwt/safehtml/shared/SafeHtmlHostedModeUtils.java#newcode75 user/src/com/google/gwt/safehtml/shared/SafeHtmlHostedModeUtils.java:75: * For example, this check will pass for the following strings: It seems like the first half of the JavaDoc for this method should be moved to the other one, now that it's public? For maybeCheckCompleteHtml(), we can just document the conditions under which it will do the check. http://gwt-code-reviews.appspot.com/1606804/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Document a bug in maven-gae-plugin that prevents gae:unpack goal in mobilewebapp from running if... (issue1607803)
http://gwt-code-reviews.appspot.com/1607803/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Change SafeHtmlHostedModeUtils.isCompleteHtml() to public. (issue1606804)
On 2011/12/07 19:38:28, mdempsky wrote: LGTM. http://gwt-code-reviews.appspot.com/1606804/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Change SafeHtmlHostedModeUtils.isCompleteHtml() to public. (issue1606804)
LGTM http://gwt-code-reviews.appspot.com/1606804/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Introduce a new method onPreviewColumnSortEvent to Header. An AbstractCellTable checks this meth... (issue1605804)
LGTM http://gwt-code-reviews.appspot.com/1605804/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Just wondering about the protocol for code commits.....
I have tacitly presumed that a LGTM from the reviewers is kinda mandatory for a commit to be made. However, I have just noticed a commit made today without a explicit LGTM. http://code.google.com/p/google-web-toolkit/source/detail?r=10780 http://gwt-code-reviews.appspot.com/1506802 -- Karthik Reddy https://plus.google.com/103243388366746199136 -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
Re: [gwt-contrib] Just wondering about the protocol for code commits.....
On Wed, Dec 7, 2011 at 3:58 PM, karthik reddy karthik.ele...@gmail.comwrote: I have tacitly presumed that a LGTM from the reviewers is kinda mandatory for a commit to be made. However, I have just noticed a commit made today without a explicit LGTM. Since GWT code is committed to an internal system which also requires an LGTM, jlabanca approved it there and just didn't put the LGTM on the public review. -- John A. Tamplin Software Engineer (GWT), Google -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Add assert for null provided fields, fixes #7024 (issue1602805)
LGTM. http://gwt-code-reviews.appspot.com/1602805/diff/1/user/test/com/google/gwt/uibinder/test/client/UiProvidedNullTest.java File user/test/com/google/gwt/uibinder/test/client/UiProvidedNullTest.java (right): http://gwt-code-reviews.appspot.com/1602805/diff/1/user/test/com/google/gwt/uibinder/test/client/UiProvidedNullTest.java#newcode2 user/test/com/google/gwt/uibinder/test/client/UiProvidedNullTest.java:2: * Copyright 2009 Google Inc. update year http://gwt-code-reviews.appspot.com/1602805/diff/1/user/test/com/google/gwt/uibinder/test/client/UiProvidedNullTest.java#newcode21 user/test/com/google/gwt/uibinder/test/client/UiProvidedNullTest.java:21: * Test UiFields(provided = true) give meaningful errors when a field is not initialized. Mention that this test is only relevant when assertions are turned on. http://gwt-code-reviews.appspot.com/1602805/diff/1/user/test/com/google/gwt/uibinder/test/client/UiProvidedNullUi.java File user/test/com/google/gwt/uibinder/test/client/UiProvidedNullUi.java (right): http://gwt-code-reviews.appspot.com/1602805/diff/1/user/test/com/google/gwt/uibinder/test/client/UiProvidedNullUi.java#newcode2 user/test/com/google/gwt/uibinder/test/client/UiProvidedNullUi.java:2: * Copyright 2008 Google Inc. update year http://gwt-code-reviews.appspot.com/1602805/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
Re: [gwt-contrib] Just wondering about the protocol for code commits.....
I see. Thanks for clarifying, John. -- Karthik Reddy https://plus.google.com/103243388366746199136 -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
Re: [gwt-contrib] Just wondering about the protocol for code commits.....
Note also that sometimes we have to rollback or patch something in an emergency that has broken important internal apps, so we will often rollback or quickfix these without the regular delay of going through the public issue tracker, but hopefully these are rare instances. -Ray On Wed, Dec 7, 2011 at 1:08 PM, John Tamplin j...@google.com wrote: On Wed, Dec 7, 2011 at 3:58 PM, karthik reddy karthik.ele...@gmail.com wrote: I have tacitly presumed that a LGTM from the reviewers is kinda mandatory for a commit to be made. However, I have just noticed a commit made today without a explicit LGTM. Since GWT code is committed to an internal system which also requires an LGTM, jlabanca approved it there and just didn't put the LGTM on the public review. -- John A. Tamplin Software Engineer (GWT), Google -- http://groups.google.com/group/Google-Web-Toolkit-Contributors -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Document a bug in maven-gae-plugin that prevents gae:unpack goal in mobilewebapp from running if... (issue1607803)
http://gwt-code-reviews.appspot.com/1607803/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Document a bug in maven-gae-plugin that prevents gae:unpack goal in mobilewebapp from running if... (issue1607803)
lgtm http://gwt-code-reviews.appspot.com/1607803/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Adding jscomp to the classpath for gwt user and dev projects. (issue1606803)
http://gwt-code-reviews.appspot.com/1606803/diff/1/eclipse/dev/.classpath File eclipse/dev/.classpath (right): http://gwt-code-reviews.appspot.com/1606803/diff/1/eclipse/dev/.classpath#newcode43 eclipse/dev/.classpath:43: classpathentry kind=var path=GWT_TOOLS/lib/jscomp/sourcemap-rebased.jar/ I just updated to trunk 10781 and also needed to add: classpathentry kind=var path=GWT_TOOLS/lib/jscomp/r1649/compiler-rebased.jar/ http://gwt-code-reviews.appspot.com/1606803/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
Re: [gwt-contrib] Re: Fix leak in LayoutImplIE6 (issue1601804)
- in initParent, the expando then references a child element; I'm not sure this creates a leak, as there's no loop here. If I don't clear the __styleRuler on the parent, doing just this: LayoutPanel p = new LayoutPanel(); RootPanel.get().add(p); RootPanel.get().remove(p); Leaks. The loop is: parent - __styleRuler expando - styleRuler - parent (E.g. after calling parent.appendChild(styleRuler), child gets an an implicit pointer back to its parent, e.g. for child.parentNode.) (I can also stop the leak by breaking the loop at child - parent by removing the parent.appendChild(styleRuler) line.) I'm still thinking about the rest. - Stephen -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
