[gwt-contrib] Re: Update the missing plugin page to stop taunting Safari 5.1 users, and (issue1536803)
lgtm. http://gwt-code-reviews.appspot.com/1536803/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Fixes a bug in TypeOracle for computing information about single JSO impls. (issue1373803)
lgtm http://gwt-code-reviews.appspot.com/1373803/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Change the DevModeOptions page to use the xs linker due to recent Chrome extension permissions c... (issue1356803)
lgtm and the horse I rode in on. http://gwt-code-reviews.appspot.com/1356803/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Cache Document.get for DevMode. Kills thousands of JSNI calls. (issue1338806)
lgtm
http://gwt-code-reviews.appspot.com/1338806/diff/1/2
File user/src/com/google/gwt/dom/client/Document.java (right):
http://gwt-code-reviews.appspot.com/1338806/diff/1/2#newcode51
user/src/com/google/gwt/dom/client/Document.java:51: private static
native Document nativeGet() /*-{
I this method is out of sort order.
http://gwt-code-reviews.appspot.com/1338806/show
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Cache Document.get for DevMode. Kills thousands of JSNI calls. (issue1338806)
http://gwt-code-reviews.appspot.com/1338806/diff/1/2
File user/src/com/google/gwt/dom/client/Document.java (right):
http://gwt-code-reviews.appspot.com/1338806/diff/1/2#newcode51
user/src/com/google/gwt/dom/client/Document.java:51: private static
native Document nativeGet() /*-{
Oh right, it's static. cool.
http://gwt-code-reviews.appspot.com/1338806/show
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Removes a Java 1.6-ism. (issue1304801)
lgtm. thanks. http://gwt-code-reviews.appspot.com/1304801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Fixes a bug in JavaScriptObject support in DevMode that prevents dispatching (issue1287801)
Reviewers: bobv, scottb, Description: Fixes a bug in JavaScriptObject support in DevMode that prevents dispatching through an interface to a method defined in a super-interface. The change also includes a new test to serve as a regression test. Please review this at http://gwt-code-reviews.appspot.com/1287801/show Affected files: M dev/core/src/com/google/gwt/dev/shell/rewrite/RewriteSingleJsoImplDispatches.java M user/test/com/google/gwt/dev/jjs/test/singlejso/TypeHierarchyTest.java -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Fixes a bug in JavaScriptObject support in DevMode that prevents dispatching (issue1287801)
http://gwt-code-reviews.appspot.com/1287801/diff/1/3 File user/test/com/google/gwt/dev/jjs/test/singlejso/TypeHierarchyTest.java (right): http://gwt-code-reviews.appspot.com/1287801/diff/1/3#newcode205 user/test/com/google/gwt/dev/jjs/test/singlejso/TypeHierarchyTest.java:205: Element element = JsElement.create(); On 2011/01/13 22:04:29, cromwellian wrote: Should you also test this in conjunction with non-JSO implementers, e.g. PureJavaElement implements Element? To be honest, I don't know what our JSO tests currently cover. My intent was to put my case in as a regression and try to follow up with some more robust testing strategies. Do you anticipate a problem that is not covered with our current tests? http://gwt-code-reviews.appspot.com/1287801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Fixes a bug in JavaScriptObject support in DevMode that prevents dispatching (issue1287801)
No, it's a good point. I'm realizing we probably need some more tests covering a lot fo the patterns that are possible after Joel's change to loosen some of the interface restrictions. On 2011/01/13 22:32:43, cromwellian wrote: On Thu, Jan 13, 2011 at 2:24 PM, mailto:knor...@google.com wrote: To be honest, I don't know what our JSO tests currently cover. My intent was to put my case in as a regression and try to follow up with some more robust testing strategies. Do you anticipate a problem that is not covered with our current tests? Well, the reason I suggested it is because you changed the writeTrampoline() function which appears to affect non-JSO Java implementations, but your test case only tests the change you made to the one that lives on JavaScriptObject$ -Ray \ http://gwt-code-reviews.appspot.com/1287801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors http://gwt-code-reviews.appspot.com/1287801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Fixes a bug in JavaScriptObject support in DevMode that prevents dispatching (issue1287801)
http://gwt-code-reviews.appspot.com/1287801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Fixes a bug in JavaScriptObject support in DevMode that prevents dispatching (issue1287801)
http://gwt-code-reviews.appspot.com/1287801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Fixes a bug in JavaScriptObject support in DevMode that prevents dispatching (issue1287801)
Thanks Ray, I didn't actually change the logic in writeTrampoline in the last patch, but I realized that an identical change there actually fixes this problem for the Java objects as well. Can you give it another quick look? http://gwt-code-reviews.appspot.com/1287801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Fixes two DevMode issues: (issue1140801)
http://gwt-code-reviews.appspot.com/1140801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Fixes two DevMode issues: (issue1140801)
http://gwt-code-reviews.appspot.com/1140801/diff/6001/7002 File dev/core/src/com/google/gwt/core/ext/typeinfo/TypeOracle.java (right): http://gwt-code-reviews.appspot.com/1140801/diff/6001/7002#newcode687 dev/core/src/com/google/gwt/core/ext/typeinfo/TypeOracle.java:687: JMethod found = cls.findMethod(meth.getName(), meth.getParameterTypes()); On 2010/11/23 18:11:48, scottb wrote: Should this be comparing based on erased types of both? That's a great question. I don't know without investigating. I'll poke around soon. http://gwt-code-reviews.appspot.com/1140801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Fixes two DevMode issues: (issue1140801)
http://gwt-code-reviews.appspot.com/1140801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Add a permissions model to the Chrome NPAPI plugin. (issue1084801)
On 2010/11/15 19:53:51, conroy wrote: lgtm http://gwt-code-reviews.appspot.com/1084801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Adds overflow-x and overflow-y to Style since they have at least partial (issue1100801)
Reviewers: jgw,
Description:
Adds overflow-x and overflow-y to Style since they have at least partial
support on all browsers.
Review by: j...@google.com
Please review this at http://gwt-code-reviews.appspot.com/1100801/show
Affected files:
M user/src/com/google/gwt/dom/client/Style.java
Index: user/src/com/google/gwt/dom/client/Style.java
===
--- user/src/com/google/gwt/dom/client/Style.java (revision 9203)
+++ user/src/com/google/gwt/dom/client/Style.java (working copy)
@@ -663,6 +663,8 @@
private static final String STYLE_PADDING_BOTTOM = paddingBottom;
private static final String STYLE_PADDING = padding;
private static final String STYLE_OVERFLOW = overflow;
+ private static final String STYLE_OVERFLOW_X = overflow-x;
+ private static final String STYLE_OVERFLOW_Y = overflow-y;
private static final String STYLE_OPACITY = opacity;
private static final String STYLE_MARGIN_TOP = marginTop;
private static final String STYLE_MARGIN_RIGHT = marginRight;
@@ -878,6 +880,20 @@
}
/**
+ * Clears the overflow-x CSS property.
+ */
+ public final void clearOverflowX() {
+clearProperty(STYLE_OVERFLOW_X);
+ }
+
+ /**
+ * Clears the overflow-y CSS property.
+ */
+ public final void clearOverflowY() {
+clearProperty(STYLE_OVERFLOW_Y);
+ }
+
+ /**
* Clear the padding css property.
*/
public final void clearPadding() {
@@ -1120,6 +1136,20 @@
*/
public final String getOverflow() {
return getProperty(STYLE_OVERFLOW);
+ }
+
+ /**
+ * Gets the overflow-x CSS property.
+ */
+ public final String getOverflowX() {
+return getProperty(STYLE_OVERFLOW_X);
+ }
+
+ /**
+ * Gets the overflow-y CSS property.
+ */
+ public final String getOverflowY() {
+return getProperty(STYLE_OVERFLOW_Y);
}
/**
@@ -1380,6 +1410,20 @@
*/
public final void setOverflow(Overflow value) {
setProperty(STYLE_OVERFLOW, value.getCssName());
+ }
+
+ /**
+ * Sets the overflow-x CSS property.
+ */
+ public final void setOverflowX(Overflow value) {
+setProperty(STYLE_OVERFLOW_X, value.getCssName());
+ }
+
+ /**
+ * Sets the overflow-y CSS property.
+ */
+ public final void setOverflowY(Overflow value) {
+setProperty(STYLE_OVERFLOW_Y, value.getCssName());
}
/**
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Add a permissions model to the Chrome NPAPI plugin. (issue1084801)
http://gwt-code-reviews.appspot.com/1084801/diff/6001/7006 File plugins/npapi/DevModeOptions/src/com/google/gwt/devmodeoptions/DevModeOptions.gwt.xml (right): http://gwt-code-reviews.appspot.com/1084801/diff/6001/7006#newcode5 plugins/npapi/DevModeOptions/src/com/google/gwt/devmodeoptions/DevModeOptions.gwt.xml:5: inherits name='com.google.gwt.dom.DOM' / User should automatically include DOM Core. http://gwt-code-reviews.appspot.com/1084801/diff/6001/7006#newcode12 plugins/npapi/DevModeOptions/src/com/google/gwt/devmodeoptions/DevModeOptions.gwt.xml:12: inherits name='com.google.gwt.user.theme.standard.Standard'/ Are you using the Standard styles? If not, you can remove this. http://gwt-code-reviews.appspot.com/1084801/diff/6001/7006#newcode13 plugins/npapi/DevModeOptions/src/com/google/gwt/devmodeoptions/DevModeOptions.gwt.xml:13: !-- inherits name='com.google.gwt.user.theme.chrome.Chrome'/ -- Should be able to remove these comments. http://gwt-code-reviews.appspot.com/1084801/diff/6001/7007 File plugins/npapi/DevModeOptions/src/com/google/gwt/devmodeoptions/client/DevModeOptions.java (right): http://gwt-code-reviews.appspot.com/1084801/diff/6001/7007#newcode36 plugins/npapi/DevModeOptions/src/com/google/gwt/devmodeoptions/client/DevModeOptions.java:36: StyleInjector.inject(bundle.css().getText(), true); Minor thing, but if you inject this in onModuleLoad the compiler will not have to defensively call the static initializer on method invocations. http://gwt-code-reviews.appspot.com/1084801/diff/6001/7013 File plugins/npapi/DevModeOptions/src/com/google/gwt/devmodeoptions/client/resources/DevModeOptions.css (right): http://gwt-code-reviews.appspot.com/1084801/diff/6001/7013#newcode1 plugins/npapi/DevModeOptions/src/com/google/gwt/devmodeoptions/client/resources/DevModeOptions.css:1: @def TEXTWIDTH 30em; Throw a copyright up here ^ http://gwt-code-reviews.appspot.com/1084801/diff/6001/7013#newcode3 plugins/npapi/DevModeOptions/src/com/google/gwt/devmodeoptions/client/resources/DevModeOptions.css:3: font-weight: bold; I think your eclipse setup is putting either tabs or too many spaces for the css indentation. http://gwt-code-reviews.appspot.com/1084801/diff/6001/7013#newcode17 plugins/npapi/DevModeOptions/src/com/google/gwt/devmodeoptions/client/resources/DevModeOptions.css:17: color: IndianRed; IndianRed! OMG, you're so unix! http://gwt-code-reviews.appspot.com/1084801/diff/6001/7015 File plugins/npapi/DevModeOptions/war/WEB-INF/web.xml (right): http://gwt-code-reviews.appspot.com/1084801/diff/6001/7015#newcode7 plugins/npapi/DevModeOptions/war/WEB-INF/web.xml:7: !-- TODO: Add servlet tags for each servlet here. -- Use officially sanctioned TODO style: TODO(conroy): http://gwt-code-reviews.appspot.com/1084801/diff/6001/7016 File plugins/npapi/Makefile (left): http://gwt-code-reviews.appspot.com/1084801/diff/6001/7016#oldcode127 plugins/npapi/Makefile:127: ($(foreach src,$(SRCS),$(DEPEND)) true) Makefile Wow, I don't even know what this does. Next time I have Makefile questions, I'm coming to you. http://gwt-code-reviews.appspot.com/1084801/diff/6001/7018 File plugins/npapi/ScriptableInstance.cpp (right): http://gwt-code-reviews.appspot.com/1084801/diff/6001/7018#newcode270 plugins/npapi/ScriptableInstance.cpp:270: NPN_GetProperty(getNPP(), window, locationID, locationVariant.addressForReturn()); You should use NPN_GetValue with NPNVWindowNPObject to get the window object and avoid using NPN_GetProperty. Technically, the window property is const, but there shouldn't be any reason to rely on the property when we can get the object we need directly from the context. http://gwt-code-reviews.appspot.com/1084801/diff/6001/7018#newcode338 plugins/npapi/ScriptableInstance.cpp:338: strncpy(out, retStr.c_str(), retStr.size()); c_str is null terminated, so you should be able to safely copy size() + 1. http://gwt-code-reviews.appspot.com/1084801/diff/6001/7024 File plugins/npapi/prebuilt/gwt-dev-plugin/options.html (right): http://gwt-code-reviews.appspot.com/1084801/diff/6001/7024#newcode1 plugins/npapi/prebuilt/gwt-dev-plugin/options.html:1: !doctype html Do you even use this page? http://gwt-code-reviews.appspot.com/1084801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Add a permissions model to the Chrome NPAPI plugin. (issue1084801)
Oh, do you have a screenshot of the UI elements? On 2010/11/08 19:27:30, knorton wrote: http://gwt-code-reviews.appspot.com/1084801/diff/6001/7006 File plugins/npapi/DevModeOptions/src/com/google/gwt/devmodeoptions/DevModeOptions.gwt.xml (right): http://gwt-code-reviews.appspot.com/1084801/diff/6001/7006#newcode5 plugins/npapi/DevModeOptions/src/com/google/gwt/devmodeoptions/DevModeOptions.gwt.xml:5: inherits name='com.google.gwt.dom.DOM' / User should automatically include DOM Core. http://gwt-code-reviews.appspot.com/1084801/diff/6001/7006#newcode12 plugins/npapi/DevModeOptions/src/com/google/gwt/devmodeoptions/DevModeOptions.gwt.xml:12: inherits name='com.google.gwt.user.theme.standard.Standard'/ Are you using the Standard styles? If not, you can remove this. http://gwt-code-reviews.appspot.com/1084801/diff/6001/7006#newcode13 plugins/npapi/DevModeOptions/src/com/google/gwt/devmodeoptions/DevModeOptions.gwt.xml:13: !-- inherits name='com.google.gwt.user.theme.chrome.Chrome'/ -- Should be able to remove these comments. http://gwt-code-reviews.appspot.com/1084801/diff/6001/7007 File plugins/npapi/DevModeOptions/src/com/google/gwt/devmodeoptions/client/DevModeOptions.java (right): http://gwt-code-reviews.appspot.com/1084801/diff/6001/7007#newcode36 plugins/npapi/DevModeOptions/src/com/google/gwt/devmodeoptions/client/DevModeOptions.java:36: StyleInjector.inject(bundle.css().getText(), true); Minor thing, but if you inject this in onModuleLoad the compiler will not have to defensively call the static initializer on method invocations. http://gwt-code-reviews.appspot.com/1084801/diff/6001/7013 File plugins/npapi/DevModeOptions/src/com/google/gwt/devmodeoptions/client/resources/DevModeOptions.css (right): http://gwt-code-reviews.appspot.com/1084801/diff/6001/7013#newcode1 plugins/npapi/DevModeOptions/src/com/google/gwt/devmodeoptions/client/resources/DevModeOptions.css:1: @def TEXTWIDTH 30em; Throw a copyright up here ^ http://gwt-code-reviews.appspot.com/1084801/diff/6001/7013#newcode3 plugins/npapi/DevModeOptions/src/com/google/gwt/devmodeoptions/client/resources/DevModeOptions.css:3: font-weight: bold; I think your eclipse setup is putting either tabs or too many spaces for the css indentation. http://gwt-code-reviews.appspot.com/1084801/diff/6001/7013#newcode17 plugins/npapi/DevModeOptions/src/com/google/gwt/devmodeoptions/client/resources/DevModeOptions.css:17: color: IndianRed; IndianRed! OMG, you're so unix! http://gwt-code-reviews.appspot.com/1084801/diff/6001/7015 File plugins/npapi/DevModeOptions/war/WEB-INF/web.xml (right): http://gwt-code-reviews.appspot.com/1084801/diff/6001/7015#newcode7 plugins/npapi/DevModeOptions/war/WEB-INF/web.xml:7: !-- TODO: Add servlet tags for each servlet here. -- Use officially sanctioned TODO style: TODO(conroy): http://gwt-code-reviews.appspot.com/1084801/diff/6001/7016 File plugins/npapi/Makefile (left): http://gwt-code-reviews.appspot.com/1084801/diff/6001/7016#oldcode127 plugins/npapi/Makefile:127: ($(foreach src,$(SRCS),$(DEPEND)) true) Makefile Wow, I don't even know what this does. Next time I have Makefile questions, I'm coming to you. http://gwt-code-reviews.appspot.com/1084801/diff/6001/7018 File plugins/npapi/ScriptableInstance.cpp (right): http://gwt-code-reviews.appspot.com/1084801/diff/6001/7018#newcode270 plugins/npapi/ScriptableInstance.cpp:270: NPN_GetProperty(getNPP(), window, locationID, locationVariant.addressForReturn()); You should use NPN_GetValue with NPNVWindowNPObject to get the window object and avoid using NPN_GetProperty. Technically, the window property is const, but there shouldn't be any reason to rely on the property when we can get the object we need directly from the context. http://gwt-code-reviews.appspot.com/1084801/diff/6001/7018#newcode338 plugins/npapi/ScriptableInstance.cpp:338: strncpy(out, retStr.c_str(), retStr.size()); c_str is null terminated, so you should be able to safely copy size() + 1. http://gwt-code-reviews.appspot.com/1084801/diff/6001/7024 File plugins/npapi/prebuilt/gwt-dev-plugin/options.html (right): http://gwt-code-reviews.appspot.com/1084801/diff/6001/7024#newcode1 plugins/npapi/prebuilt/gwt-dev-plugin/options.html:1: !doctype html Do you even use this page? http://gwt-code-reviews.appspot.com/1084801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Update the npapi plugin to support OSX. (issue1036801)
LGTM2 On 2010/10/20 18:54:20, jat wrote: Ok. Be sure and check in the compiled libraries in prebuilt and an updated CRX at the same time. http://gwt-code-reviews.appspot.com/1036801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: GWT Development shell no longer cuts off the Launch Default Browser and Copy to Clipboard (issue758801)
Possible to throw a screenshot comparison up on http://imgur.com/? On 2010/09/15 19:16:00, jat wrote: On Wed, Sep 15, 2010 at 3:10 PM, mailto:con...@google.com wrote: fred, i'd love to see this go in. LGTM. My objection to it as written revolves around wasting vertical space when the URL fits fine. -- John A. Tamplin Software Engineer (GWT), Google http://gwt-code-reviews.appspot.com/758801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Ignore negative times after normalization (issue825801)
lgtm http://gwt-code-reviews.appspot.com/825801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Adds a hack to pre-initialize the Java Graphics2D library. (issue822801)
This method used to be called by the compiler because of some interesting SWT/AWT interaction on mac. It might be worth looking to see where it is called these days: http://www.google.com/codesearch/p?hl=en#A1edwVHBClQ/dev/core/src/com/google/gwt/dev/BootStrapPlatform.java%20package:http://google-web-toolkit%5C.googlecode%5C.comsa=Ncd=1ct=rcl=38 On 2010/08/31 21:47:43, zundel wrote: I went over to my Mac and ran a compile under GPE and it worked fine (I seemed to get a measurable speedup too.) I consolidated the thread logic and removed the static member, but left the Thread subclass defined by itself just not to clutter the precompile() method. http://gwt-code-reviews.appspot.com/822801/diff/1/2 File dev/core/src/com/google/gwt/dev/Precompile.java (right): http://gwt-code-reviews.appspot.com/822801/diff/1/2#newcode390 dev/core/src/com/google/gwt/dev/Precompile.java:390: createGraphicsEvent.end(); On 2010/08/31 21:11:48, scottb wrote: This looks fine, but my only comment is whether or not this can be boiled down to something more general that would get the same effect? e.g. GraphicsEnvironment.getLocalGraphicsEnvironment(). GraphicsEnvironment.getLocalGraphicsEnvironment() works. Updated. http://gwt-code-reviews.appspot.com/822801/diff/1/2#newcode401 dev/core/src/com/google/gwt/dev/Precompile.java:401: private static Thread graphicsInitThread = new GraphicsInitThread(); On 2010/08/31 21:13:05, scottb wrote: Oh yeah.. any particular reason to make this a field? Should just be a local var. I'd almost argue for making the Thread a local class, too. Originally I had a graphicsInitThread.join() in a particular place. I decided I didn't need that, so I removed it. http://gwt-code-reviews.appspot.com/822801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Enables AppStats for bikeshed. (issue477801)
Reviewers: jgw,
Description:
Enables AppStats for bikeshed.
Please review this at http://gwt-code-reviews.appspot.com/477801/show
Affected files:
M /bikeshed/war/WEB-INF/web.xml
Index: /bikeshed/war/WEB-INF/web.xml
===
--- /bikeshed/war/WEB-INF/web.xml (revision 7614)
+++ /bikeshed/war/WEB-INF/web.xml (working copy)
@@ -47,6 +47,37 @@
url-pattern/cookbook/tree/url-pattern
/servlet-mapping
+ !-- AppStats --
+ servlet
+servlet-nameappstats/servlet-name
+
servlet-classcom.google.appengine.tools.appstats.AppstatsServlet/servlet-class
+init-param
+ param-namerequireAdminAuthentication/param-name
+ param-valuefalse/param-value
+/init-param
+ /servlet
+ servlet-mapping
+servlet-nameappstats/servlet-name
+url-pattern/appstats/*/url-pattern
+ /servlet-mapping
+ filter
+filter-nameappstats/filter-name
+
filter-classcom.google.appengine.tools.appstats.AppstatsFilter/filter-class
+init-param
+ param-namelogMessage/param-name
+ param-valueAppstats available:
/appstats/details?time={ID}/param-value
+/init-param
+init-param
+ param-namebasePath/param-name
+ param-value/appstats//param-value
+/init-param
+ /filter
+ filter-mapping
+filter-nameappstats/filter-name
+url-pattern/*/url-pattern
+ /filter-mapping
+
+
!-- Require login. --
security-constraint
web-resource-collection
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Enables AppStats for bikeshed. (issue477801)
I have no way of testing this ... but I think it's correct. http://gwt-code-reviews.appspot.com/477801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Use getBoundingClientRect for Safari4/Chrome.
Reviewers: jgw, Description: This patch uses getBoundingClientRect on modern WebKit browsers for getAbsoluteTop and getAbsoluteLeft. Sadly, we still support Safari3 so I had to do this via runtime check (so we can use the old crazy impl for Safari3). However, this is still well worth the change for Safari4 and Chrome. Note: The diff looks crazy because I moved two giant JSNI methods around. It's actually not that crazy. Please review this at http://gwt-code-reviews.appspot.com/148802 Affected files: M user/src/com/google/gwt/dom/client/DOMImplSafari.java M user/test/com/google/gwt/user/client/ui/DOMTest.java -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Upgrade for get/setInnerText in DOMImplSafari.
Reviewers: jgw, jaimeyap,
Description:
Upgrade WebKit's DOM library to use textContent instead of falling back
to the slow versions of setInnerText and getInnerText.
Please review this at http://gwt-code-reviews.appspot.com/143803
Affected files:
M user/src/com/google/gwt/dom/client/DOMImplSafari.java
Index: user/src/com/google/gwt/dom/client/DOMImplSafari.java
diff --git
a/google3/third_party/java/gwt/source/svn/trunk/user/src/com/google/gwt/dom/client/DOMImplSafari.java
b/google3/third_party/java/gwt/source/svn/trunk/user/src/com/google/gwt/dom/client/DOMImplSafari.java
index
c6a5d2cd0a813b9767b8de0495762f826f12a7d7..24ec4e331c3abc08c4d7b3ddffbe1839cf21a528
100644
---
a/google3/third_party/java/gwt/source/svn/trunk/user/src/com/google/gwt/dom/client/DOMImplSafari.java
+++
b/google3/third_party/java/gwt/source/svn/trunk/user/src/com/google/gwt/dom/client/DOMImplSafari.java
@@ -168,6 +168,18 @@ class DOMImplSafari extends DOMImplStandard {
return top;
}-*/;
+ /*
+ * textContent is used over innerText for two reasons:
+ * 1 - It is consistent with DOMImplMozilla. textContent
+ * does not convert br's to new lines in WebKit.
+ * 2 - textContent is faster on retreival because WebKit
+ * does not recalculate styles as it does for innerText.
+ */
+ @Override
+ public native String getInnerText(Element node) /*-{
+return node.textContent;
+ }-*/;
+
@Override
public int getScrollLeft(Document doc) {
// Safari always applies document scrolling to the body element, even
in
@@ -215,9 +227,9 @@ class DOMImplSafari extends DOMImplStandard {
* elements, because their descendent elements are only one level deep.
*/
@Override
- public native void selectClear(SelectElement select) /*-{
-select.innerText = '';
- }-*/;
+ public void selectClear(SelectElement select) {
+select.setInnerText();
+ }
@Override
public native int selectGetLength(SelectElement select) /*-{
@@ -234,6 +246,14 @@ class DOMImplSafari extends DOMImplStandard {
select.removeChild(select.children[index]);
}-*/;
+ /*
+ * See getInnerText for why textContent is used instead of innerText.
+ */
+ @Override
+ public native void setInnerText(Element elem, String text) /*-{
+elem.textContent = text || '';
+ }-*/;
+
@Override
public void setScrollLeft(Document doc, int left) {
// Safari always applies document scrolling to the body element, even
in
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: FormPanel iframe name collisions
http://gwt-code-reviews.appspot.com/125801/diff/1/4 File user/src/com/google/gwt/user/client/ui/FormPanel.java (right): http://gwt-code-reviews.appspot.com/125801/diff/1/4#newcode298 Line 298: /** Rather than polluting $wnd even more, why not make the id: FormPanel_ + GWT.getModuleName() + (++formId) http://gwt-code-reviews.appspot.com/125801 -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Update release notes for 1.7.1 release
lgtm. http://gwt-code-reviews.appspot.com/66807 --~--~-~--~~~---~--~~ http://groups.google.com/group/Google-Web-Toolkit-Contributors -~--~~~~--~~--~--~---
[gwt-contrib] Snow Leopard 32-bit checking fix (1.7 based)
Reviewers: cromwellian,
Message:
Ray,
Can you confirm that this patch fixes the Snow Leopard issue. It's the
same patch as before with the typo fixed and the error message slightly
expanded.
Please review this at http://gwt-code-reviews.appspot.com/64805
Affected files:
M dev/mac/src/com/google/gwt/dev/BootStrapPlatform.java
Index: dev/mac/src/com/google/gwt/dev/BootStrapPlatform.java
diff --git a/dev/mac/src/com/google/gwt/dev/BootStrapPlatform.java
b/dev/mac/src/com/google/gwt/dev/BootStrapPlatform.java
index
164a4a6afbda075cb2c71695ee1b72f99f26c65d..e1c70573fec7098757477143e7087b0a23068080
100644
--- a/dev/mac/src/com/google/gwt/dev/BootStrapPlatform.java
+++ b/dev/mac/src/com/google/gwt/dev/BootStrapPlatform.java
@@ -69,8 +69,10 @@ public class BootStrapPlatform {
* The following check must be made before attempting to initialize
Safari,
* or we'll fail with an less-than-helpful UnsatisfiedLinkError.
*/
-if (!isJava5()) {
- System.err.println(You must use a Java 1.5 runtime to use GWT
Hosted Mode on Mac OS X.);
+if (!is32Bit()) {
+ System.err.println(You must use a 32-bit runtime to use GWT Hosted
Mode on Mac OS X.);
+ System.err.println( Leopard: Use the Java 1.5 runtime.);
+ System.err.println( Snow Leopard: Use the Java 1.6 runtime and add
-d32);
System.exit(-1);
}
@@ -111,11 +113,10 @@ public class BootStrapPlatform {
}
/**
- * Determine if we're using the Java 1.5 runtime, since the 1.6 runtime
is
- * 64-bit.
+ * Determine if we're using a 32 bit runtime.
*/
- private static boolean isJava5() {
-return System.getProperty(java.version).startsWith(1.5);
+ private static boolean is32Bit() {
+return 32.equals(System.getProperty(sun.arch.data.model));
}
/**
--~--~-~--~~~---~--~~
http://groups.google.com/group/Google-Web-Toolkit-Contributors
-~--~~~~--~~--~--~---
[gwt-contrib] Re: Snow Leopard 32-bit checking fix (1.7 based)
In the rare cases it fails, the user will just get what they get by default now. Seems like we should go ahead and do it for all the platforms. I'll add the checking to HostedModeBase, but have it call back into BootStrapPlatform for the error message so we can keep the OS X specific hints. http://gwt-code-reviews.appspot.com/64805 --~--~-~--~~~---~--~~ http://groups.google.com/group/Google-Web-Toolkit-Contributors -~--~~~~--~~--~--~---
[gwt-contrib] Re: Snow Leopard 32-bit checking fix (1.7 based)
Sure, does sun.arch.data.model work everywhere? /kel On 2009/09/03 19:17:16, jat wrote: On 2009/09/03 19:07:53, knorton wrote: Ray, Can you confirm that this patch fixes the Snow Leopard issue. It's the same patch as before with the typo fixed and the error message slightly expanded. Since we have the 32-bit issue on other platforms, should we do this in common code instead? http://gwt-code-reviews.appspot.com/64805 --~--~-~--~~~---~--~~ http://groups.google.com/group/Google-Web-Toolkit-Contributors -~--~~~~--~~--~--~---
[gwt-contrib] Re: Snow Leopard 32-bit checking fix (1.7 based)
Doing it all platforms style. I also confirmed that this does the right thing on Leopard when you run both the 1.5 and 1.6 vms. http://gwt-code-reviews.appspot.com/64805 --~--~-~--~~~---~--~~ http://groups.google.com/group/Google-Web-Toolkit-Contributors -~--~~~~--~~--~--~---
[gwt-contrib] Re: Fix handling of troubleshooting iframe, support gwt.codesvr query param
lgtm, w/ just one random comment. http://gwt-code-reviews.appspot.com/61805/diff/1/2 File dev/core/src/com/google/gwt/core/ext/linker/impl/hosted.html (right): http://gwt-code-reviews.appspot.com/61805/diff/1/2#newcode276 Line 276: // look for the old query parameter if we don't find the new one This looks correct to me, but I do think the logic in this file could be a lot more manageable. For instance, it seems like the straight line could could just read: $hosted = getCodeSvrQueryParam() || getLegacyHostedQueryParam(); Much of the logic now is in the straight line execution and not well encapsulated in functions. I'm not necessarily suggesting that you get it all organized before this commit, but I do think it would be good to start cleaning this up in preparation for maintaining it effectively long-term. http://gwt-code-reviews.appspot.com/61805 --~--~-~--~~~---~--~~ http://groups.google.com/group/Google-Web-Toolkit-Contributors -~--~~~~--~~--~--~---
[gwt-contrib] Re: noserver should be, well, -noserver
lgtm. http://gwt-code-reviews.appspot.com/47821 --~--~-~--~~~---~--~~ http://groups.google.com/group/Google-Web-Toolkit-Contributors -~--~~~~--~~--~--~---
[gwt-contrib] Re: Fixes OOPHM on IE using window.name instead of an expando; Fixes lightweight metrics.
lgtm. http://gwt-code-reviews.appspot.com/33840 --~--~-~--~~~---~--~~ http://groups.google.com/group/Google-Web-Toolkit-Contributors -~--~~~~--~~--~--~---
[gwt-contrib] Re: Fixes OOPHM on IE using window.name instead of an expando; Fixes lightweight metrics.
Is there a mail or something that describes the problem this fixes? I
don't actually understand the motivation for some of these changes.
http://gwt-code-reviews.appspot.com/33840/diff/1003/4
File dev/core/src/com/google/gwt/core/ext/linker/impl/hosted.html
(right):
http://gwt-code-reviews.appspot.com/33840/diff/1003/4#newcode13
Line 13: if (!moduleFuncName || !$wnd[moduleFuncName]) {
Mentioned this about my bookmarklet linker: If your module's name is
'console', it won't load.
http://gwt-code-reviews.appspot.com/33840/diff/1003/5
File dev/core/src/com/google/gwt/core/linker/IFrameTemplate.js (right):
http://gwt-code-reviews.appspot.com/33840/diff/1003/5#newcode325
Line 325:
document.getElementsByTagName('head')[0].appendChild(scriptFrame);
iframe in the head? We had that at one time and were eager to get rid of
it. Do we really need to put it in the head. Moz will likely transplant
the iframe to the body which can get weird.
http://gwt-code-reviews.appspot.com/33840/diff/1003/5#newcode327
Line 327: // Expose the module function via an expando on the iframe's
window.
I think this comment is out of date 'name' isn't an expando.
http://gwt-code-reviews.appspot.com/33840
--~--~-~--~~~---~--~~
http://groups.google.com/group/Google-Web-Toolkit-Contributors
-~--~~~~--~~--~--~---
[gwt-contrib] Re: Fix for issue 3649 (parallel script loading in iframe liner)
LGTM++ A few new spacing issues showed up. It could be tabs. http://gwt-code-reviews.appspot.com/33808/diff/1005/1007 File dev/core/src/com/google/gwt/core/linker/IFrameTemplate.js (right): http://gwt-code-reviews.appspot.com/33808/diff/1005/1007#newcode47 Line 47: // The frame that will contain the compiled script (created in There's some weird extra spacing here. http://gwt-code-reviews.appspot.com/33808/diff/1005/1007#newcode322 Line 322: scriptFrame.id = '__MODULE_NAME__'; More weird spacing. http://gwt-code-reviews.appspot.com/33808/diff/1005/1007#newcode359 Line 359: // Mark this module's script as done loading and (possibly) start the Is there extra space at the end of this line? http://gwt-code-reviews.appspot.com/33808 --~--~-~--~~~---~--~~ http://groups.google.com/group/Google-Web-Toolkit-Contributors -~--~~~~--~~--~--~---
[gwt-contrib] Re: Fix for issue 3649 (parallel script loading in iframe liner)
lgtm - and I confirmed parallel on ff3 on os x. http://gwt-code-reviews.appspot.com/33808/diff/1/3 File dev/core/src/com/google/gwt/core/linker/IFrameTemplate.js (left): http://gwt-code-reviews.appspot.com/33808/diff/1/3#oldcode284 Line 284: iframe.id = __MODULE_NAME__; Why was this here? I thought we used this, but it was lost in the update. http://gwt-code-reviews.appspot.com/33808/diff/1/3 File dev/core/src/com/google/gwt/core/linker/IFrameTemplate.js (right): http://gwt-code-reviews.appspot.com/33808/diff/1/3#newcode90 Line 90: var frameWnd = scriptFrame.contentWindow; Where is the var for scriptFrame? http://gwt-code-reviews.appspot.com/33808/diff/1/3#newcode318 Line 318: scriptFrame.style.position = 'absolute' Why did you not just set cssText? http://gwt-code-reviews.appspot.com/33808/diff/1/3#newcode357 Line 357: // Mark this module's script as done loading and (possibly) start the module. 80 char. http://gwt-code-reviews.appspot.com/33808 --~--~-~--~~~---~--~~ http://groups.google.com/group/Google-Web-Toolkit-Contributors -~--~~~~--~~--~--~---
[gwt-contrib] Re: Fix for issue 3649 (parallel script loading in iframe liner)
Found one thing: Expanded the host page w/ 10,000 p tags. Turned on 28.8 throttling and managed to get Showcase's UI injected about 2/3 of the way down the page amongst the p tags. I would've expected it to be the last element. http://gwt-code-reviews.appspot.com/33808 --~--~-~--~~~---~--~~ http://groups.google.com/group/Google-Web-Toolkit-Contributors -~--~~~~--~~--~--~---
[gwt-contrib] Adds best practice XSS safety to DynaTable.
Reviewers: jlabanca,
Description:
No exploit here, but it's generally bad to trust that server error
messages will be free of user supplied data. In fact, it's an error
we've been known to make:
http://code.google.com/p/google-web-toolkit/issues/detail?id=3637
In this case, there is no reason to handle the server error message as
HTML.
Please review this at http://gwt-code-reviews.appspot.com/33811
Affected files:
samples/dynatable/src/com/google/gwt/sample/dynatable/client/DynaTableWidget.java
Index:
samples/dynatable/src/com/google/gwt/sample/dynatable/client/DynaTableWidget.java
===
---
samples/dynatable/src/com/google/gwt/sample/dynatable/client/DynaTableWidget.java
(revision 4142)
+++
samples/dynatable/src/com/google/gwt/sample/dynatable/client/DynaTableWidget.java
(working copy)
@@ -1,5 +1,5 @@
/*
- * Copyright 2007 Google Inc.
+ * Copyright 2009 Google Inc.
*
* Licensed under the Apache License, Version 2.0 (the License); you may
not
* use this file except in compliance with the License. You may obtain a
copy of
@@ -60,9 +60,13 @@ public class DynaTableWidget extends Composite {
hide();
}
-public void setBody(String html) {
+public void setBodyHtml(String html) {
body.setHTML(html);
}
+
+public void setBodyText(String text) {
+ body.setText(text);
+}
}
private class NavBar extends Composite implements ClickHandler {
@@ -160,10 +164,12 @@ public class DynaTableWidget extends Composite {
}
if (caught instanceof InvocationException) {
errorDialog.setText(An RPC server could not be reached);
-errorDialog.setBody(NO_CONNECTION_MESSAGE);
+errorDialog.setBodyHtml(NO_CONNECTION_MESSAGE);
} else {
errorDialog.setText(Unexcepted Error processing remote call);
-errorDialog.setBody(caught.getMessage());
+// Some times error messages have user input in them, so we play
+// it safe and set this message as text instead of HTML.
+errorDialog.setBodyText(caught.getMessage());
}
errorDialog.center();
}
--~--~-~--~~~---~--~~
http://groups.google.com/group/Google-Web-Toolkit-Contributors
-~--~~~~--~~--~--~---
[gwt-contrib] Re: New StyleSheetLoader class
http://gwt-code-reviews.appspot.com/13802/diff/1/3
File src/com/google/gwt/gen2/styleloader/client/StyleSheetLoader.java
(right):
http://gwt-code-reviews.appspot.com/13802/diff/1/3#newcode37
Line 37: * {...@link StyleSheetLoader} creates a reference element on the
page that will be
Forgive the intrusion. I'm not sure what constraints you're working
with, but why are you not better off loading the css through xhr and
creating a style element in the head? The downsides are you can't do
cross-domain loading of styles and that relative urls are resolved from
a different base url. But given that you already have to encode
something in the stylesheet (the reference style) the tradeoff seems
similar. Also, watching for a particular element to be non-zero seems
pretty fragile. A style like div { height: 400px; } would give you a
false positive on loading.
http://gwt-code-reviews.appspot.com/13802
--~--~-~--~~~---~--~~
http://groups.google.com/group/Google-Web-Toolkit-Contributors
-~--~~~~--~~--~--~---
[gwt-contrib] ImmutableResources: Properly find package relative resources.
Reviewers: bobv,
Description:
The strategy for finding resources in ResourceGeneratorUtil limits
package relative resources to files contained directly in that package.
This patch make it possible to use arbitrary paths relative to the
package (i.e. resources/background.png).
Please review this at http://gwt-code-reviews.appspot.com/803
Affected files:
src/com/google/gwt/libideas/resources/ext/ResourceGeneratorUtil.java
Index: src/com/google/gwt/libideas/resources/ext/ResourceGeneratorUtil.java
===
--- src/com/google/gwt/libideas/resources/ext/ResourceGeneratorUtil.java
(revision 1273)
+++ src/com/google/gwt/libideas/resources/ext/ResourceGeneratorUtil.java
(working copy)
@@ -20,6 +20,7 @@
import com.google.gwt.core.ext.TreeLogger;
import com.google.gwt.core.ext.UnableToCompleteException;
import com.google.gwt.core.ext.typeinfo.JMethod;
+import com.google.gwt.core.ext.typeinfo.JPackage;
import
com.google.gwt.libideas.resources.client.ImmutableResourceBundle.Resource;
import
com.google.gwt.libideas.resources.client.ImmutableResourceBundle.Transform;
import com.google.gwt.libideas.resources.rebind.Transformer;
@@ -216,17 +217,16 @@
boolean error = false;
int tagIndex = 0;
for (String resource : resources) {
+ // Try to find the resource relative to the package.
+ URL resourceURL = tryFindResource(classLoader,
getPathRelativeToPackage(
+ method.getEnclosingType().getPackage(), resource), locale);
- // Make sure the name is either absolute or package-relative.
- if (resource.indexOf(/) == -1) {
-String pkgName = method.getEnclosingType().getPackage().getName();
-
-// This construction handles the default package correctly, too.
-resource = pkgName.replace('.', '/') + / + resource;
+ // If we didn't find the resource relative to the package, assume it
is
+ // absolute.
+ if (resourceURL == null) {
+resourceURL = tryFindResource(classLoader, resource, locale);
}
-
- URL resourceURL = tryFindResource(classLoader, resource, locale);
-
+
if (resourceURL == null) {
logger.log(TreeLogger.ERROR, Resource + resource
+ not found on classpath. Is the name specified as
@@ -253,6 +253,17 @@
}
/**
+ * Converts a package relative path into an absolute path.
+ *
+ * @param pkg the package
+ * @param path a path relative to the package
+ * @return an absolute path
+ */
+ private static String getPathRelativeToPackage(JPackage pkg, String
path) {
+return pkg.getName().replace('.', '/') + '/' + path;
+ }
+
+ /**
* This performs the locale lookup function for a given resource name.
*
* @param classLoader the ClassLoader to use to load the resources
--~--~-~--~~~---~--~~
http://groups.google.com/group/Google-Web-Toolkit-Contributors
-~--~~~~--~~--~--~---
[gwt-contrib] Re: Breaks dom.DOM dependency on user.UserAgent
Thanks for looking at this Thomas, Maybe UserAgent should just go into a path that has no client source associated with it. That would provide fine grain inheritance. But before we do do that, would it be reasonable in your uses to just inherit dom.Dom? For all my uses this seemed reasonable. This still means that UserAgent is not independently inheritable, but that is a big issue that we have all over the place. We've done an extremely poor job of separating those modules that are setup to be inherited and those that just group some deferred binding rules. In fact, most of the modules in User cannot be inherited by themselves. To be honest, I wish we would start creating larger .gwt.xml files and make each one that exists inheritable. Doing that would mean that I would get rid of UserAgent.gwt.xml altogether and move its contents into dom.DOM.gwt.xml. (or either create useragent.UserAgent.gwt.xml) So, I'm not opposed to making useragent.UserAgent. But I would like to try to just make UserAgent be a part of DOM if that is at all feasible. http://gwt-code-reviews.appspot.com/401 --~--~-~--~~~---~--~~ http://groups.google.com/group/Google-Web-Toolkit-Contributors -~--~~~~--~~--~--~---
[gwt-contrib] Re: Breaks dom.DOM dependency on user.UserAgent
I'm going to add useragent.UserAgent and update a new patch. /kel On 2008/12/03 12:50:52, knorton wrote: Thanks for looking at this Thomas, Maybe UserAgent should just go into a path that has no client source associated with it. That would provide fine grain inheritance. But before we do do that, would it be reasonable in your uses to just inherit dom.Dom? For all my uses this seemed reasonable. This still means that UserAgent is not independently inheritable, but that is a big issue that we have all over the place. We've done an extremely poor job of separating those modules that are setup to be inherited and those that just group some deferred binding rules. In fact, most of the modules in User cannot be inherited by themselves. To be honest, I wish we would start creating larger .gwt.xml files and make each one that exists inheritable. Doing that would mean that I would get rid of UserAgent.gwt.xml altogether and move its contents into dom.DOM.gwt.xml. (or either create useragent.UserAgent.gwt.xml) So, I'm not opposed to making useragent.UserAgent. But I would like to try to just make UserAgent be a part of DOM if that is at all feasible. http://gwt-code-reviews.appspot.com/401 --~--~-~--~~~---~--~~ http://groups.google.com/group/Google-Web-Toolkit-Contributors -~--~~~~--~~--~--~---
[gwt-contrib] Breaks dom.DOM dependency on user.UserAgent
Reviewers: jgw, Description: One of the goals of the new Dom library was proper modularity so that it would be possible to write apps and libraries that do not depend on User.gwt.xml. That's not currently possible due to UserAgent. This patch moves UserAgent.gwt.xml into the dom module and creates a bridge module in User so as not to break anything. Please review this at http://gwt-code-reviews.appspot.com/401 Affected files: user/src/com/google/gwt/dom/DOM.gwt.xml user/src/com/google/gwt/dom/UserAgent.gwt.xml user/src/com/google/gwt/user/UserAgent.gwt.xml --~--~-~--~~~---~--~~ http://groups.google.com/group/Google-Web-Toolkit-Contributors -~--~~~~--~~--~--~---
