[gwt-contrib] Re: Fixing AbsolutePanel to set positioning of 'right' instead of 'left' for RTL languages. Without ... (issue1900803)
On 2013/04/17 17:21:49, goktug wrote: On 2013/04/17 05:14:32, yaminik wrote: Reviewers needed. I don't have much experience with RTL issues. At least this change will very likely break any previous code that was not assuming the different behavior for RTL in AbsolutePanel. I would love to hear what you guys think. You should get Aharon inside Google to look at it -- he wrote much of the Bidi support in GWT. http://gwt-code-reviews.appspot.com/1900803/ -- -- http://groups.google.com/group/Google-Web-Toolkit-Contributors --- You received this message because you are subscribed to the Google Groups Google Web Toolkit Contributors group. To unsubscribe from this group and stop receiving emails from it, send an email to google-web-toolkit-contributors+unsubscr...@googlegroups.com. For more options, visit https://groups.google.com/groups/opt_out.
[gwt-contrib] Re: Avoid using eval() (issue1882803)
LGTM with the suggested change. http://gwt-code-reviews.appspot.com/1882803/diff/1/user/src/com/google/gwt/i18n/client/TimeZoneInfo.java File user/src/com/google/gwt/i18n/client/TimeZoneInfo.java (right): http://gwt-code-reviews.appspot.com/1882803/diff/1/user/src/com/google/gwt/i18n/client/TimeZoneInfo.java#newcode45 user/src/com/google/gwt/i18n/client/TimeZoneInfo.java:45: return JSON.parse(json); +1 on JsonUtils.safeEval(), and you don't need the separate eval method any more -- just put it into buildTimeZoneData. http://gwt-code-reviews.appspot.com/1882803/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Devmode broken with elemental (#7481) (issue1801804)
http://gwt-code-reviews.appspot.com/1801804/diff/1/core/src/com/google/gwt/dev/shell/DispatchClassInfo.java
File core/src/com/google/gwt/dev/shell/DispatchClassInfo.java (right):
http://gwt-code-reviews.appspot.com/1801804/diff/1/core/src/com/google/gwt/dev/shell/DispatchClassInfo.java#newcode86
core/src/com/google/gwt/dev/shell/DispatchClassInfo.java:86: Integer id
= memberIdByMember.get(m);
On 2013/01/15 01:38:37, mdempsky wrote:
Is this a very heavily used function and/or does memberBy[Member]Id
get very
large normally? If not, would it be simpler to just do:
int id = memberById.indexOf(m);
if (id == -1) {
id = memberById.size();
memberById.add(m);
}
memberIdByName.put(StringInterner.get().intern(name), id);
I believe that was essentially the implementation I originally wrote,
and was changed as part of a modification for performance and memory
usage (which is where the StringInterner calls came in). At the time,
AdWords was having trouble running DevMode in a 32bit JVM.
I wasn't involved in that, but I presume there were lots of duplicate
strings which led to this being a win.
http://gwt-code-reviews.appspot.com/1801804/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Submenu may be drawn off the screen: Issue 3888 (issue1876803)
LGTM with formatting fix http://gwt-code-reviews.appspot.com/1876803/diff/1/user/src/com/google/gwt/user/client/ui/MenuBar.java File user/src/com/google/gwt/user/client/ui/MenuBar.java (right): http://gwt-code-reviews.appspot.com/1876803/diff/1/user/src/com/google/gwt/user/client/ui/MenuBar.java#newcode1247 user/src/com/google/gwt/user/client/ui/MenuBar.java:1247: int left; This block should only be indented 2 spaces, which would make the diffs show only what was really changed. http://gwt-code-reviews.appspot.com/1876803/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: PushButton constructors that take ImageResource (4714) (issue1833803)
LGTM http://gwt-code-reviews.appspot.com/1833803/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Added new public static method to Window.Location: reloadParameterMap. (issue1859804)
LGTM http://gwt-code-reviews.appspot.com/1859804/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Chrome plugin: don't use an iterator after the underlying entry has been freed. (issue1861803)
LGTM http://gwt-code-reviews.appspot.com/1861803/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Split ValidationTool.exec() into two methods so alternative (issue1859803)
LGTM
http://gwt-code-reviews.appspot.com/1859803/diff/2001/user/src/com/google/web/bindery/requestfactory/apt/ValidationTool.java
File
user/src/com/google/web/bindery/requestfactory/apt/ValidationTool.java
(right):
http://gwt-code-reviews.appspot.com/1859803/diff/2001/user/src/com/google/web/bindery/requestfactory/apt/ValidationTool.java#newcode224
user/src/com/google/web/bindery/requestfactory/apt/ValidationTool.java:224:
if (args.length 2) {
Should this be public so other tools have a way to run it in-process
without having to worry about the System.exit call?
http://gwt-code-reviews.appspot.com/1859803/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Replace instances of element.setInnerHTML(safeHtml.asString()) (issue1857803)
LGTM Can we deprecate Element.setInnerHTML in favor of setInnerSafeHtml? Ideally, we would be able to remove the unsafe versions before long. http://gwt-code-reviews.appspot.com/1857803/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Add java.lang.reflect.Type to GWT. (issue1855803)
http://gwt-code-reviews.appspot.com/1855803/diff/1003/user/super/com/google/gwt/emul/java/lang/reflect/Type.java File user/super/com/google/gwt/emul/java/lang/reflect/Type.java (right): http://gwt-code-reviews.appspot.com/1855803/diff/1003/user/super/com/google/gwt/emul/java/lang/reflect/Type.java#newcode19 user/super/com/google/gwt/emul/java/lang/reflect/Type.java:19: * Implemented only by java.lang.Class. If we actually include this, then I think this comment needs to be stronger, something like This interface exists for compatibility purposes only, and does not imply any reflection support. http://gwt-code-reviews.appspot.com/1855803/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Update the dev mode plugin for Firefox 16. Includes binaries for 32-bit and (issue1851804)
http://gwt-code-reviews.appspot.com/1851804/diff/3001/plugins/xpcom/JavaObject.cpp File plugins/xpcom/JavaObject.cpp (right): http://gwt-code-reviews.appspot.com/1851804/diff/3001/plugins/xpcom/JavaObject.cpp#newcode151 plugins/xpcom/JavaObject.cpp:151: return JavaObject::getProperty(ctx, obj.get(), id.get(), vp); On 2012/10/09 00:04:17, cromwellian wrote: Are they doing these kinds of breakages on purpose?! :) Actually, yes - they want to make it painful enough to write binary extensions that people stop doing it. http://gwt-code-reviews.appspot.com/1851804/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Fix safari on IOS6 caching problem (issue #7703) (issue1845803)
Given how much of the net is broken on iOS6 due to this (and hanging get breakage), I suspect they will fix it before a version of GWT with this change could be rolled out. http://gwt-code-reviews.appspot.com/1845803/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Fix the Chrome plugin to work on a Mac. (issue1844803)
LGTM
http://gwt-code-reviews.appspot.com/1844803/diff/1/plugins/npapi/Makefile
File plugins/npapi/Makefile (right):
http://gwt-code-reviews.appspot.com/1844803/diff/1/plugins/npapi/Makefile#newcode17
plugins/npapi/Makefile:17: ARCH=x86
I assume this means Chrome is always built as a 32-bit executable on
Mac?
http://gwt-code-reviews.appspot.com/1844803/diff/1/plugins/npapi/main.cpp
File plugins/npapi/main.cpp (right):
http://gwt-code-reviews.appspot.com/1844803/diff/1/plugins/npapi/main.cpp#newcode193
plugins/npapi/main.cpp:193: if (browser-getvalue(instance,
NPNVsupportsCoreGraphicsBool, supportsCoreGraphics) == NPERR_NO_ERROR
supportsCoreGraphics) {
Split the long lines here and below
http://gwt-code-reviews.appspot.com/1844803/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Upgraded the devmode Chrome extension to manifest version 2. (issue1840803)
LGTM, though I still wonder if you want -style PRETTY to be the default. http://gwt-code-reviews.appspot.com/1840803/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Upgraded the devmode Chrome extension to manifest version 2. (issue1840803)
LGTM http://gwt-code-reviews.appspot.com/1840803/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Upgraded the devmode Chrome extension to manifest version 2. (issue1840803)
http://gwt-code-reviews.appspot.com/1840803/diff/1/plugins/npapi/DevModeOptions/build.xml File plugins/npapi/DevModeOptions/build.xml (right): http://gwt-code-reviews.appspot.com/1840803/diff/1/plugins/npapi/DevModeOptions/build.xml#newcode4 plugins/npapi/DevModeOptions/build.xml:4: property name=gwt.args value=-style PRETTY / Did you mean for this to stay here? http://gwt-code-reviews.appspot.com/1840803/diff/1/plugins/npapi/prebuilt/gwt-dev-plugin/page_action.html File plugins/npapi/prebuilt/gwt-dev-plugin/page_action.html (right): http://gwt-code-reviews.appspot.com/1840803/diff/1/plugins/npapi/prebuilt/gwt-dev-plugin/page_action.html#newcode7 plugins/npapi/prebuilt/gwt-dev-plugin/page_action.html:7: script src=page_action.js/script Did you leave this file out? http://gwt-code-reviews.appspot.com/1840803/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Add a getter for XHR's responseType. (issue1830803)
https://gwt-code-reviews.appspot.com/1830803/diff/2003/user/src/com/google/gwt/xhr/client/XMLHttpRequest.java
File user/src/com/google/gwt/xhr/client/XMLHttpRequest.java (right):
https://gwt-code-reviews.appspot.com/1830803/diff/2003/user/src/com/google/gwt/xhr/client/XMLHttpRequest.java#newcode147
user/src/com/google/gwt/xhr/client/XMLHttpRequest.java:147: @Deprecated
Since this never appeared in any released version, didn't work until the
typo was fixed, and can't work on FF, I sugest simply removing this
rather than deprecating it.
https://gwt-code-reviews.appspot.com/1830803/diff/2003/user/src/com/google/gwt/xhr/client/XMLHttpRequest.java#newcode253
user/src/com/google/gwt/xhr/client/XMLHttpRequest.java:253: public final
native String getResponseType() /*-{
This should probably either be named getResponseTypeString or return a
ResponseType. Given where we are in the release process, I think we
should just do the simple thing now (getResponseTypeString) and add a
better one later (which will sill need a native getResponseTypeString,
though it wouldn't have to be public).
https://gwt-code-reviews.appspot.com/1830803/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Synchronize static Maps used for caching. (issue1829804)
LGTM after fixing for comment or deciding to disregard it.
https://gwt-code-reviews.appspot.com/1829804/diff/1/user/src/com/google/web/bindery/autobean/shared/impl/AutoBeanCodexImpl.java
File
user/src/com/google/web/bindery/autobean/shared/impl/AutoBeanCodexImpl.java
(right):
https://gwt-code-reviews.appspot.com/1829804/diff/1/user/src/com/google/web/bindery/autobean/shared/impl/AutoBeanCodexImpl.java#newcode518
user/src/com/google/web/bindery/autobean/shared/impl/AutoBeanCodexImpl.java:518:
synchronized (coderFor) {
Isn't it considered bad practice to synchronize on the object you are
protecting like this? I don't think it is a problem with HashMap, but I
would still prefer having explicit coderForLock and codersLock objects.
https://gwt-code-reviews.appspot.com/1829804/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Add a getter for XHR's responseType. (issue1830803)
LGTM Did you test it? It probably needs testing on IE6 if GWT still officially supports it, since I recall some weirdness about getting exceptions if you referenced properties that didn't exist on native objects that were exposed to JS. https://gwt-code-reviews.appspot.com/1830803/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Add a getter for XHR's responseType. (issue1830803)
On 2012/09/12 20:47:20, skybrian wrote: Hmm, I'm inclined to skip this patch for GWT 2.5. The typo fix at least makes it work on some browsers... If I understand Thomas' test results, the getter works properly everywhere -- it is the existing setter which fails on FF11+. So, this patch doesn't add anything broken, but you can 't actually use any non-default response type on FF+ even with what is in trunk now. I think we don't want to screw up the API just to deal with a non-compliant browser -- making a setter would require the callers to set it after the fact (and it seems like it would be a race condition if the reply comes back before you set it). Is there a FF bug for this? https://gwt-code-reviews.appspot.com/1830803/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Breaking Change: Use ValueBoxBaseString instead of TextBox in SuggestBox (issue 6492092)
BTW: you should create code reviews at
http://gwt-code-reviews.appspot.com/ instead.
There is a also a TODO to support SafeHtml here. I'm not entirely sure
what that means in this case (Doesn't TextBox already ensure the string
is uninterpreted? Does changing to a ValueBoxBase mean you might be
displaying HTML in a value?), but we should either make sure this change
moves us closer to that or remove the TODO.
https://codereview.appspot.com/6492092/diff/1/user/src/com/google/gwt/user/client/ui/SuggestBox.java
File user/src/com/google/gwt/user/client/ui/SuggestBox.java (right):
https://codereview.appspot.com/6492092/diff/1/user/src/com/google/gwt/user/client/ui/SuggestBox.java#newcode877
user/src/com/google/gwt/user/client/ui/SuggestBox.java:877: public
ValueBoxBaseString getTextBox() {
On 2012/09/10 10:42:04, t.broyer wrote:
How about adding a getValueBox() instead, and let this method throw a
ClassCastException in case 'box' is not a TextBoxBase?
(and deprecate it in favor of getValueBox)
That way, we wouldn't even break anyone's code.
True, but you leave a runtime landmine that may fail in production. I
think if you do that, you need to deprecate this method and retire it
pretty soon.
https://codereview.appspot.com/6492092/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Breaking Change: Use ValueBoxBaseString instead of TextBox in SuggestBox (issue 6492092)
LGTM https://codereview.appspot.com/6492092/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Breaking Change: Use ValueBoxBaseString instead of TextBox in SuggestBox (issue 6492092)
https://codereview.appspot.com/6492092/diff/5001/user/src/com/google/gwt/user/client/ui/SuggestBox.java File user/src/com/google/gwt/user/client/ui/SuggestBox.java (right): https://codereview.appspot.com/6492092/diff/5001/user/src/com/google/gwt/user/client/ui/SuggestBox.java#newcode878 user/src/com/google/gwt/user/client/ui/SuggestBox.java:878: * @deprecated in favour of getValueBox If there is going to be an official policy of removing deprecated items, should list a date or a release version where it will be removed here? https://codereview.appspot.com/6492092/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: xhr.responseType was misspelled as responsetype (lowercase t) (issue1820806)
LGTM I swear I remember this being fixed some time ago, but apparently not. We really need to get an updated HtmlUnit so we can have automated tests for some of these things. https://gwt-code-reviews.appspot.com/1820806/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: xhr.responseType was misspelled as responsetype (lowercase t) (issue1820806)
On 2012/09/09 23:35:48, tbroyer wrote: I don't know the status of HtmlUnit, but trying new versions should be made much easier with the move Maven. That being said, I skimmed HtmlUnit's Web site and it doesn't seem to support either TypedArrays or xhr.responseType :-( Yes, I meant make the modifications to HtmlUnit ourselves. I don't know if we ever switched to the pure upstream version, or if we still have local modifications to it -- I know we made extensive changes to get DevMode working in it originally. https://gwt-code-reviews.appspot.com/1820806/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: EditorDriver#setConstraintViolations used to thrown NPE if arg was null. (issue1826803)
https://gwt-code-reviews.appspot.com/1826803/diff/2001/user/src/com/google/web/bindery/requestfactory/gwt/client/impl/AbstractRequestFactoryEditorDriver.java File user/src/com/google/web/bindery/requestfactory/gwt/client/impl/AbstractRequestFactoryEditorDriver.java (right): https://gwt-code-reviews.appspot.com/1826803/diff/2001/user/src/com/google/web/bindery/requestfactory/gwt/client/impl/AbstractRequestFactoryEditorDriver.java#newcode199 user/src/com/google/web/bindery/requestfactory/gwt/client/impl/AbstractRequestFactoryEditorDriver.java:199: return doSetViolations(violations == null ? null : new ViolationIterable(violations)); Wouldn't it be much cleaner elsewhere to just set violations to an empty list here if it is null? https://gwt-code-reviews.appspot.com/1826803/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Use c.g.g.core.shared.GWT in shared code. (issue1818803)
LGTM, as far as it goes However, when you use GWT.create in non-client code, there is some setup you need to do - you have to create the ServerGwtBridge, set the deferred binding properties (either globally or per-thread), and you need to register any ClassInstantiators you need for the various GWT.create calls. Look at ServerGwtBridgeTest for some examples. I haven't looked at the usages in these files, but basically the only things GWT.create knows how to do out of the box right now on the server is to find the local-specific classes for subtypes of Localizable and to do new FooImpl() or new Foo() for GWT.create(Foo.class). Most likely, you will also need to create and register additional class instantiators for the GWT.create usages here, and then document what additional properties need to be setup for server-side usage. https://gwt-code-reviews.appspot.com/1818803/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Got the development mode plugin to compile for Firefox 15 using xulrunner-15.0b6 on 64-bit Linux. (issue1816803)
LGTM after testing. Suggest changing how JSVAL_IS_OBJECT is handled, but I don't feel strongly about it. http://gwt-code-reviews.appspot.com/1816803/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Got the development mode plugin to compile for Firefox 15 using xulrunner-15.0b6 on 64-bit Linux. (issue1816803)
Sorry, didn't notice my draft comment wasn't included in the reply
before.
http://gwt-code-reviews.appspot.com/1816803/diff/1/plugins/xpcom/FFSessionHandler.cpp
File plugins/xpcom/FFSessionHandler.cpp (right):
http://gwt-code-reviews.appspot.com/1816803/diff/1/plugins/xpcom/FFSessionHandler.cpp#newcode464
plugins/xpcom/FFSessionHandler.cpp:464: } else if
(JSVAL_IS_OBJECT(value)) {
In the past, I tried to deal with simple things like this in a header
file, like this:
#ifndef JSVAL_IS_OBJECT
#define JSVAL_IS_OBJECT(x) (!JSVAL_IS_PRIMITIVE(x))
#endif
to minimize the changes that have to go elsewhere.
http://gwt-code-reviews.appspot.com/1816803/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Add setComparator to allow custom sorting of candidates from search. (issue1807805)
On 2012/08/15 00:27:19, skybrian wrote: Looks good. I was wondering whether the sort would be slower, but I looked at the implementation and it's not using native sort() in any case. Let's wait until tomorrow to submit, to give people a chance to respond. LGTM Just adding a comparator shouldn't change whether it uses native sort, since the JS sort allows a comparator. I originally used a native sort + stability fixup pass, but it was a lot slower on IE than a merge sort which was already stable. This was before we had browser-specific implementations in the JRE (StringBuffer), so at some point it might be worth revisiting that and having the current implementation just for IE. Anyway, not really relevant to this change, since it would behave the same in either case. http://gwt-code-reviews.appspot.com/1807805/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Fixed DATE_MEDIUM format for IT which shouldn't have slashes. (issue1798805)
A couple of issues: 1) this isn't the right file -- the .properties files aren't used by GWT (these are kept around for now because some old code GWT.create'd them directly, but will soon be removed). The ones you want are at http://code.google.com/p/google-web-toolkit/source/browse/trunk/user/src/com/google/gwt/i18n/client/impl/cldr/DateTimeFormatInfoImpl_it.java and http://code.google.com/p/google-web-toolkit/source/browse/trunk/user/src/com/google/gwt/i18n/shared/impl/cldr/DateTimeFormatInfoImpl_it.java (they are duplicated now, will be fixed when I can get my shared i18n fix committed) 2) as Rajeev mentioned, these are created from CLDR data. In this case, from http://unicode.org/cldr/trac/browser/tags/release-21/common/main/it.xml#L1105 -- we really try to avoid making changes to CLDR data just in GWT. If you think the CLDR data is wrong, you need to submit the change there -- see http://cldr.unicode.org/index/survey-tool So, I do not support making this change. http://gwt-code-reviews.appspot.com/1798805/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Fixed DATE_MEDIUM format for IT which shouldn't have slashes. (issue1798805)
Btw, if you go to http://st.unicode.org/cldr-apps/survey?_=itx=Gregorian#x@14164b88b71705de you will see that the a format without slashes was considered and rejected, so you will probably need strong evidence (such as usage in newspapers, government documents, etc) to get them to change their mind. http://gwt-code-reviews.appspot.com/1798805/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Fix JSON escaping of unicode characters to work in JDK 7. (issue1796803)
LGTM http://gwt-code-reviews.appspot.com/1796803/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Firefox 14 DevMode Plugin (issue1792803)
LGTM http://gwt-code-reviews.appspot.com/1792803/diff/1/plugins/xpcom/JavaObject.h File plugins/xpcom/JavaObject.h (right): http://gwt-code-reviews.appspot.com/1792803/diff/1/plugins/xpcom/JavaObject.h#newcode55 plugins/xpcom/JavaObject.h:55: static void finalize(JSFreeOp* fop, JSObject* obj); On 2012/07/24 23:30:44, acleung wrote: On 2012/07/24 23:05:37, skybrian wrote: who calls this? From what I understand, Firefox (spidermonkey)'s garbage collector calls when it decided that object has no reference. Correct. http://gwt-code-reviews.appspot.com/1792803/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: IE8 32k data:url bugfix (issue1778803)
http://gwt-code-reviews.appspot.com/1778803/diff/1/user/src/com/google/gwt/resources/rebind/context/InlineClientBundleGeneratorIE8.java File user/src/com/google/gwt/resources/rebind/context/InlineClientBundleGeneratorIE8.java (right): http://gwt-code-reviews.appspot.com/1778803/diff/1/user/src/com/google/gwt/resources/rebind/context/InlineClientBundleGeneratorIE8.java#newcode35 user/src/com/google/gwt/resources/rebind/context/InlineClientBundleGeneratorIE8.java:35: InlineResourceContext inlineCtx = (InlineResourceContext) On 2012/07/14 09:10:49, tbroyer wrote: How about adding a constructor with an 'int' argument to InlineClientBundleGenerator and have that class call it with the IE8_MAX_ENCODED_SIZE constant? InlineClientBundleGenerator would then use the value passed in its constructor (or the default value of (215)-1 when the zero-arg constructor has been used) when creating the InlineResourceContext, and InlineClientBundleGeneratorIE8 wouldn't have to override createResourceContext. An alternative would be to use a deferred-binding property instead of a hard-coded constant, and use set-property ...when-property-is name=user.agent value=ie8 //set-property to override it for IE8 with a smaller value. I haven't checked but I suppose the property value could be extracted from the GeneratorContext. If you want to have an arbitrary value, it has to be a config property not a deferred-bound property. http://gwt-code-reviews.appspot.com/1778803/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Remove DOCTYPEs from all our modules. (issue1772803)
Why not just fix the URL (removing the tags and pointing at head) instead? I am not positive, but I believe there are internal tools that expect the DTD to be present. https://gwt-code-reviews.appspot.com/1772803/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Add/improve i18n shared APIs (issue1775803)
Note that only a small subset of generated files are included -- rietveld chokes on a large number of files, so only default, en, en_US, es, and ar are included as examples in shared/cldr. http://gwt-code-reviews.appspot.com/1775803/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Add/improve i18n shared APIs (issue1775803)
http://gwt-code-reviews.appspot.com/1775803/diff/1/dev/core/test/com/google/gwt/dev/javac/typemodel/test/package-info.java
File
dev/core/test/com/google/gwt/dev/javac/typemodel/test/package-info.java
(right):
http://gwt-code-reviews.appspot.com/1775803/diff/1/dev/core/test/com/google/gwt/dev/javac/typemodel/test/package-info.java#newcode20
dev/core/test/com/google/gwt/dev/javac/typemodel/test/package-info.java:20:
@TestAnnotation(value = Package)
Unrelated, please ignore.
http://gwt-code-reviews.appspot.com/1775803/diff/1/user/src/com/google/gwt/i18n/shared/NumberFormatFactory.java
File user/src/com/google/gwt/i18n/shared/NumberFormatFactory.java
(right):
http://gwt-code-reviews.appspot.com/1775803/diff/1/user/src/com/google/gwt/i18n/shared/NumberFormatFactory.java#newcode189
user/src/com/google/gwt/i18n/shared/NumberFormatFactory.java:189: void
setForcedLatinDigits(boolean useLatinDigits);
Oops, this needs to be removed -- this is now set when you create the
NumberFormat instance via NumberFormatFactory.
http://gwt-code-reviews.appspot.com/1775803/diff/1/user/src/com/google/gwt/text/client/DateTimeFormatRenderer.java
File user/src/com/google/gwt/text/client/DateTimeFormatRenderer.java
(right):
http://gwt-code-reviews.appspot.com/1775803/diff/1/user/src/com/google/gwt/text/client/DateTimeFormatRenderer.java#newcode19
user/src/com/google/gwt/text/client/DateTimeFormatRenderer.java:19:
import com.google.gwt.i18n.client.DateTimeFormat.PredefinedFormat;
These two files need some thought -- they probably need to use the
shared version and do something like
GWT.LocaleInfocreate(LocaleInfo.class).dateTimes().getFormat(...) but
that might change depending on how we decide the LocaleInfo injection
should work.
http://gwt-code-reviews.appspot.com/1775803/diff/1/user/test/com/google/gwt/i18n/client/DateTimeFormat_en_Test.java
File user/test/com/google/gwt/i18n/client/DateTimeFormat_en_Test.java
(right):
http://gwt-code-reviews.appspot.com/1775803/diff/1/user/test/com/google/gwt/i18n/client/DateTimeFormat_en_Test.java#newcode26
user/test/com/google/gwt/i18n/client/DateTimeFormat_en_Test.java:26: *
Tests formatting functionality in {@link
com.google.gwt.i18n.shared.DateTimeFormatImpl} for the
Bogus refactoring change, will be reverted.
http://gwt-code-reviews.appspot.com/1775803/diff/1/user/test/com/google/gwt/i18n/shared/DateTimeFormatTestBase.java
File user/test/com/google/gwt/i18n/shared/DateTimeFormatTestBase.java
(right):
http://gwt-code-reviews.appspot.com/1775803/diff/1/user/test/com/google/gwt/i18n/shared/DateTimeFormatTestBase.java#newcode67
user/test/com/google/gwt/i18n/shared/DateTimeFormatTestBase.java:67: //
public void testIso8601() {
Needs some work, depends on how we decide to inject LocaleInfo.
http://gwt-code-reviews.appspot.com/1775803/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Add/improve i18n shared APIs (issue1775803)
Reviewers: skybrian, Description: There is still a bit of work to do, but I think this is nearly done. I need to add more tests, and we need to work out exactly how to inject LocaleInfo instances in a way that the same code works properly on both the client and the server. Please review this at http://gwt-code-reviews.appspot.com/1775803/ Affected files: dev/core/test/com/google/gwt/dev/javac/typemodel/test/package-info.java eclipse/external/cldr-tools/.classpath eclipse/lang/.classpath eclipse/lang/.project eclipse/tools/cldr-import/.classpath eclipse/tools/cldr-import/GenerateGwtCldrData.launch tools/cldr-import/src/com/google/gwt/tools/cldr/CurrencyDataProcessor.java tools/cldr-import/src/com/google/gwt/tools/cldr/DateTimeFormatInfoProcessor.java tools/cldr-import/src/com/google/gwt/tools/cldr/GenerateGwtCldrData.java tools/cldr-import/src/com/google/gwt/tools/cldr/ListFormattingProcessor.java tools/cldr-import/src/com/google/gwt/tools/cldr/LocaleData.java tools/cldr-import/src/com/google/gwt/tools/cldr/LocaleInfoProcessor.java tools/cldr-import/src/com/google/gwt/tools/cldr/LocalizedNamesProcessor.java tools/cldr-import/src/com/google/gwt/tools/cldr/NumberConstantsProcessor.java tools/cldr-import/src/com/google/gwt/tools/cldr/Processor.java user/src/com/google/gwt/core/server/CldrInstantiator.java user/src/com/google/gwt/core/server/ServerGwtBridge.java user/src/com/google/gwt/i18n/client/CurrencyData.java user/src/com/google/gwt/i18n/client/DateTimeFormat.java user/src/com/google/gwt/i18n/client/LocalizedNames.java user/src/com/google/gwt/i18n/client/NumberFormat.java user/src/com/google/gwt/i18n/client/constants/NumberConstants.java user/src/com/google/gwt/i18n/rebind/CurrencyInfo.java user/src/com/google/gwt/i18n/rebind/CurrencyListGenerator.java user/src/com/google/gwt/i18n/rebind/CustomDateTimeFormatGenerator.java user/src/com/google/gwt/i18n/rebind/LocalizableLinkageCreator.java user/src/com/google/gwt/i18n/shared/CurrencyData.java user/src/com/google/gwt/i18n/shared/CurrencyList.java user/src/com/google/gwt/i18n/shared/DateTimeFormat.java user/src/com/google/gwt/i18n/shared/DateTimeFormatFactory.java user/src/com/google/gwt/i18n/shared/DefaultCurrencyData.java user/src/com/google/gwt/i18n/shared/DefaultDateTimeFormatInfo.java user/src/com/google/gwt/i18n/shared/DefaultListPatterns.java user/src/com/google/gwt/i18n/shared/DefaultLocaleInfo.java user/src/com/google/gwt/i18n/shared/DefaultLocalizedNames.java user/src/com/google/gwt/i18n/shared/DefaultLocalizedNamesBase.java user/src/com/google/gwt/i18n/shared/DefaultNumberConstants.java user/src/com/google/gwt/i18n/shared/ListPatterns.java user/src/com/google/gwt/i18n/shared/LocaleInfo.java user/src/com/google/gwt/i18n/shared/LocalizedNames.java user/src/com/google/gwt/i18n/shared/NumberConstants.java user/src/com/google/gwt/i18n/shared/NumberFormat.java user/src/com/google/gwt/i18n/shared/NumberFormatFactory.java user/src/com/google/gwt/i18n/shared/cldr/CurrencyListImpl.java user/src/com/google/gwt/i18n/shared/cldr/CurrencyListImpl_ar.java user/src/com/google/gwt/i18n/shared/cldr/CurrencyListImpl_en.java user/src/com/google/gwt/i18n/shared/cldr/CurrencyListImpl_en_US.java user/src/com/google/gwt/i18n/shared/cldr/CurrencyListImpl_es.java user/src/com/google/gwt/i18n/shared/cldr/DateTimeFormatInfoImpl.java user/src/com/google/gwt/i18n/shared/cldr/DateTimeFormatInfoImpl_ar.java user/src/com/google/gwt/i18n/shared/cldr/DateTimeFormatInfoImpl_en.java user/src/com/google/gwt/i18n/shared/cldr/DateTimeFormatInfoImpl_en_US.java user/src/com/google/gwt/i18n/shared/cldr/DateTimeFormatInfoImpl_es.java user/src/com/google/gwt/i18n/shared/cldr/ListPatternsImpl.java user/src/com/google/gwt/i18n/shared/cldr/ListPatternsImpl_ar.java user/src/com/google/gwt/i18n/shared/cldr/ListPatternsImpl_en.java user/src/com/google/gwt/i18n/shared/cldr/ListPatternsImpl_es.java user/src/com/google/gwt/i18n/shared/cldr/LocaleInfoImpl.java user/src/com/google/gwt/i18n/shared/cldr/LocaleInfoImpl_ar.java user/src/com/google/gwt/i18n/shared/cldr/LocaleInfoImpl_en.java user/src/com/google/gwt/i18n/shared/cldr/LocaleInfoImpl_en_US.java user/src/com/google/gwt/i18n/shared/cldr/LocaleInfoImpl_es.java user/src/com/google/gwt/i18n/shared/cldr/LocalizedNamesImpl.java user/src/com/google/gwt/i18n/shared/cldr/LocalizedNamesImpl_ar.java user/src/com/google/gwt/i18n/shared/cldr/LocalizedNamesImpl_en.java user/src/com/google/gwt/i18n/shared/cldr/LocalizedNamesImpl_es.java user/src/com/google/gwt/i18n/shared/cldr/NumberConstantsImpl.java user/src/com/google/gwt/i18n/shared/cldr/NumberConstantsImpl_ar.java user/src/com/google/gwt/i18n/shared/cldr/NumberConstantsImpl_es.java user/src/com/google/gwt/i18n/shared/impl/CurrencyDataHelper.java user/src/com/google/gwt/i18n/shared/impl/CurrencyDataImpl.java
[gwt-contrib] Re: Issue 7062: documentation uses @DefaultText instead of @DefaultMessage (issue1766803)
LGTM https://gwt-code-reviews.appspot.com/1766803/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Update to 1.6 for javac source/target. (issue1753803)
LGTM http://gwt-code-reviews.appspot.com/1753803/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Fix unicode escaping in JSON Util. (issue1754803)
LGTM http://gwt-code-reviews.appspot.com/1754803/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Move test Messages files from client to shared; was missed in r10061. This was causing I18NSuite... (issue1752803)
LGTM Thanks for tracking this down and fixing it. http://gwt-code-reviews.appspot.com/1752803/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Extracted constant strings to the constructor, that allow translation to be provided from the ou... (issue1739803)
LGTM http://gwt-code-reviews.appspot.com/1739803/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Changed Constants to Messages in CellTree. (issue1747804)
LGTM It isn't showing up properly here, but I assume it is the same as the internal change. http://gwt-code-reviews.appspot.com/1747804/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Shared i18n APIs (issue1743805)
Reviewers: skybrian,
http://gwt-code-reviews.appspot.com/1743805/diff/1/dev/core/test/com/google/gwt/dev/javac/typemodel/test/package-info.java
File
dev/core/test/com/google/gwt/dev/javac/typemodel/test/package-info.java
(right):
http://gwt-code-reviews.appspot.com/1743805/diff/1/dev/core/test/com/google/gwt/dev/javac/typemodel/test/package-info.java#newcode20
dev/core/test/com/google/gwt/dev/javac/typemodel/test/package-info.java:20:
@TestAnnotation(value = Package)
Unrelated, please ignore.
http://gwt-code-reviews.appspot.com/1743805/diff/1/user/src/com/google/gwt/i18n/shared/DateTimeFormat.java
File user/src/com/google/gwt/i18n/shared/DateTimeFormat.java (right):
http://gwt-code-reviews.appspot.com/1743805/diff/1/user/src/com/google/gwt/i18n/shared/DateTimeFormat.java#newcode1
user/src/com/google/gwt/i18n/shared/DateTimeFormat.java:1: package
com.google.gwt.i18n.shared;
Oops, lost file header.
It might be that we can't make this change at this point, but I would
prefer it to be consistent with the rest.
http://gwt-code-reviews.appspot.com/1743805/diff/1/user/src/com/google/gwt/i18n/shared/LocaleInfo.java
File user/src/com/google/gwt/i18n/shared/LocaleInfo.java (right):
http://gwt-code-reviews.appspot.com/1743805/diff/1/user/src/com/google/gwt/i18n/shared/LocaleInfo.java#newcode30
user/src/com/google/gwt/i18n/shared/LocaleInfo.java:30: public interface
LocaleInfo extends Localizable {
Still TBD exactly how to get an instance of LocaleInfo -- it could be
GWT.create'd, or via a Locales instance (which then means you need to
address how to get that instance).
Description:
This is not ready for a serious review -- mostly at this point I want
feedback on how the APIs fit together and would look in either shared
code, client-only code, or in server-only code.
See
https://groups.google.com/d/msg/google-web-toolkit-contributors/5eaMEP5qY6c/GiqLy1kLeLoJ
for some related discussions.
Please review this at http://gwt-code-reviews.appspot.com/1743805/
Affected files:
M
dev/core/test/com/google/gwt/dev/javac/typemodel/test/package-info.java
M eclipse/lang/.classpath
M eclipse/lang/.project
M user/src/com/google/gwt/i18n/client/CurrencyData.java
M user/src/com/google/gwt/i18n/client/DateTimeFormat.java
M user/src/com/google/gwt/i18n/client/LocalizedNames.java
M user/src/com/google/gwt/i18n/client/NumberFormat.java
M user/src/com/google/gwt/i18n/client/constants/NumberConstants.java
M
user/src/com/google/gwt/i18n/rebind/CustomDateTimeFormatGenerator.java
A user/src/com/google/gwt/i18n/shared/CurrencyData.java
A user/src/com/google/gwt/i18n/shared/CurrencyList.java
M user/src/com/google/gwt/i18n/shared/DateTimeFormat.java
A user/src/com/google/gwt/i18n/shared/DateTimeFormatFactory.java
A user/src/com/google/gwt/i18n/shared/DefaultCurrencyData.java
A user/src/com/google/gwt/i18n/shared/DefaultLocalizedNames.java
A user/src/com/google/gwt/i18n/shared/DefaultLocalizedNamesBase.java
A user/src/com/google/gwt/i18n/shared/DefaultNumberConstants.java
A user/src/com/google/gwt/i18n/shared/LocaleInfo.java
A user/src/com/google/gwt/i18n/shared/LocalizedNames.java
A user/src/com/google/gwt/i18n/shared/NumberConstants.java
A user/src/com/google/gwt/i18n/shared/NumberFormat.java
A user/src/com/google/gwt/i18n/shared/NumberFormatFactory.java
A user/src/com/google/gwt/i18n/shared/impl/CurrencyDataHelper.java
A user/src/com/google/gwt/i18n/shared/impl/CurrencyDataImpl.java
A user/src/com/google/gwt/i18n/shared/impl/CurrencyListImpl.java
A user/src/com/google/gwt/i18n/shared/impl/CurrencyListImplBase.java
A user/src/com/google/gwt/i18n/shared/impl/CurrencyListImpl_en.java
A
user/src/com/google/gwt/i18n/shared/impl/DateTimeFormatFactoryImpl.java
A user/src/com/google/gwt/i18n/shared/impl/DateTimeFormatImpl.java
A user/src/com/google/gwt/i18n/shared/impl/DateTimeFormatInfoImpl.java
A
user/src/com/google/gwt/i18n/shared/impl/DateTimeFormatInfoImpl_en.java
A user/src/com/google/gwt/i18n/shared/impl/LocaleInfoImpl.java
A user/src/com/google/gwt/i18n/shared/impl/LocaleInfoImplBase.java
A user/src/com/google/gwt/i18n/shared/impl/LocaleInfoImpl_en.java
A user/src/com/google/gwt/i18n/shared/impl/LocalizedNamesImpl.java
A user/src/com/google/gwt/i18n/shared/impl/LocalizedNamesImplBase.java
A user/src/com/google/gwt/i18n/shared/impl/LocalizedNamesImpl_en.java
A user/src/com/google/gwt/i18n/shared/impl/NumberConstantsImpl.java
A user/src/com/google/gwt/i18n/shared/impl/NumberConstantsImpl_en.java
A
user/src/com/google/gwt/i18n/shared/impl/NumberFormatFactoryImpl.java
A user/src/com/google/gwt/i18n/shared/impl/NumberFormatHelper.java
A user/src/com/google/gwt/i18n/shared/impl/NumberFormatImpl.java
M user/src/com/google/gwt/text/client/DateTimeFormatRenderer.java
M
[gwt-contrib] Re: Extracted constant strings to the constructor, that allow translation to be provided from the ou... (issue1739803)
On 2012/06/14 15:19:09, rkj wrote: I am a bit confused now. I follow the example: https://developers.google.com/web-toolkit/doc/latest/tutorial/i18n As I understand the GWT.create should create proper implementation based on properties file - why isn't that so? The issue is that they would need to integrate these messages with their translation system, but they don't have the hooks to do so. For example, there is no @Generate notation here, and no way to put one appropriate for whatever they are doing with translations. They can't specify MD5 keys or some other key that won't collide with what they have elsewhere in their app. It is possible to supply the translations anyway, but they will have to know about it themselves and do something different than every other message in the app. Problem with passing an instance of this is that it's practically an inner class of CellTree, so I would have to allow passing this interface to CellTree - is that expected? How does GWT handle more complicated widgets? AFAIK, there aren't any other widgets that need translations. Basically, either the widget should be self-contained, which means it has all the translations for all supported languages, or it needs to allow the caller to pass in the messages to use. You can leave a helper method without Messages that just GWT.creates it and hopes for the best (hopefully only used by people that aren't localizing their app). For example, the caller could create an empty extension of the Messages interface (which would need to be moved so it could be accessed) and add their own @Generate* annotations on it as needed to tie into their translation system, and then GWT.create it themselves when creating the widget. http://gwt-code-reviews.appspot.com/1739803/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Extracted constant strings to the constructor, that allow translation to be provided from the ou... (issue1739803)
http://gwt-code-reviews.appspot.com/1739803/diff/6002/user/src/com/google/gwt/user/cellview/client/CellTree.java
File user/src/com/google/gwt/user/cellview/client/CellTree.java (right):
http://gwt-code-reviews.appspot.com/1739803/diff/6002/user/src/com/google/gwt/user/cellview/client/CellTree.java#newcode571
user/src/com/google/gwt/user/cellview/client/CellTree.java:571: *
Construct a new {@link CellTree}.
Should warn that without extra manual steps by the caller, there will be
no translations of the text supplied by the CellTree itself.
http://gwt-code-reviews.appspot.com/1739803/diff/6002/user/src/com/google/gwt/user/cellview/client/CellTree.java#newcode589
user/src/com/google/gwt/user/cellview/client/CellTree.java:589: * @param
messages translation messages
Probably needs more explanation here, telling them they need to create
an empty extension of CellTreeMessages and add whatever annotations are
needed to hook into their translation system, then GWT.create their
interface to pass in here.
http://gwt-code-reviews.appspot.com/1739803/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Comment out an invalid test in TreeMapTest. (issue1735805)
On 2012/06/14 01:33:21, skybrian wrote: LGTM It seems like we should also change the web implementation to behave like JDK 7. But that can wait. Unless we are going to upgrade the rest of the JRE to match JDK 7, I disagree. More likely, this behavior should be considered undefined and allow either the current or new behaviors in the test. Still, I am fine with this as an expedient. http://gwt-code-reviews.appspot.com/1735805/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Extracted constant strings to the constructor, that allow translation to be provided from the ou... (issue1739803)
http://gwt-code-reviews.appspot.com/1739803/diff/8001/user/src/com/google/gwt/user/cellview/client/CellTreeNodeView.java
File user/src/com/google/gwt/user/cellview/client/CellTreeNodeView.java
(right):
http://gwt-code-reviews.appspot.com/1739803/diff/8001/user/src/com/google/gwt/user/cellview/client/CellTreeNodeView.java#newcode86
user/src/com/google/gwt/user/cellview/client/CellTreeNodeView.java:86: *
Constant for labeling the simple pager navigational {@link ImageButton}s
On 2012/06/14 03:38:37, jlabanca wrote:
Add a comment that the default Messages only provide English
translations.
So, if you want the users to supply their own translations, then you
need to have them pass an instance of this interface in rather than
calling GWT.create on it (that could work, but it will make integration
with their translation system much harder).
Another option would be to get this translated to all the languages GWT
supports, which Google might be willing to do, but it won't be fast (it
is possible similar strings already exist in Google products that could
be used easily).
http://gwt-code-reviews.appspot.com/1739803/diff/8001/user/src/com/google/gwt/user/cellview/client/CellTreeNodeView.java#newcode705
user/src/com/google/gwt/user/cellview/client/CellTreeNodeView.java:705:
private static final Messages MESSAGES =
GWT.Messagescreate(Messages.class);
On 2012/06/14 03:38:37, jlabanca wrote:
My GWT.create() foo is failing me. How does one go about adding
translations
again?
You shouldn't need to do anything if you aren't actually wanting to
supply other translations, as it will fall back to the ones in the
annotations.
http://gwt-code-reviews.appspot.com/1739803/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Use shared.GWT to avoid ClassNotFoundExceptions (issue1722803)
LGTM Thanks, this is one of the cases where server-side code was using code from c.g.g.*.client and it was working, but that is always going to be very dangerous. Over time, we should fix all these cases and make it so server-side code can't even see any client packages. http://gwt-code-reviews.appspot.com/1722803/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: ExternalTextResourceGenerator is locale/machine-dependent. (issue1716804)
LGTM https://gwt-code-reviews.appspot.com/1716804/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Add Integer.TYPE, etc. (issue1710804)
We will need a new config file for GWT 2.5, I don't know exactly where in the release process that goes, but probably sooner rather than later. http://gwt-code-reviews.appspot.com/1710804/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Cast return value from JsDate's various setter methods to be (issue1704804)
LGTM http://gwt-code-reviews.appspot.com/1704804/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Cast return value from JsDate's various setter methods to be (issue1704804)
I think D is actively wrong -- if the incorrect value is returned, then having it work in DevMode and get incorrect results when compiled to JS is the worst combination (just because it doesn't throw an exception doesn't mean the calculation works as intended). I agree that the ecmascript 5.1 spec specifies a return value, but 1.3 (which introduced the Date object) did not. I haven't done an evaluation of exactly what browsers have exactly what behavior, but somewhere between 17 and 19 Chrome changed from returning a Number to returning a Date object. If we can verify that all browsers/versions GWT supports (IE6+, FF3.6+, etc) always return either a Number or a Date object, then I am fine with the original change of a cast. If that isn't the case, then I still think either A or B is the proper fix, and would prefer A -- but it should be Bhaskar's call. http://gwt-code-reviews.appspot.com/1704804/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Cast return value from JsDate's various setter methods to be (issue1704804)
Are you sure this actually results in a usable and correct value? Since the spec doesn't say what is returned and browser implementations differ, my opinion is that we should do one of the following: 1) change the return values to void, breaking the API for those callers that actually use the return value. 2) add a return this.getTime() at the end of all of these setters The problem with #2 is that adds bloat to all callers who don't use the return value. In the case of #1, the docs didn't say what the return value is, so they were already treading on thin ice anyway. So, my preference would be #1. http://gwt-code-reviews.appspot.com/1704804/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Cast return value from JsDate's various setter methods to be (issue1704804)
Since structures have changed, I don't know if the old process would still apply anyway, so I suggest adding bjanakira...@google.com (the new manager of GWT) and let him decide. But I don't think casting to Number is the appropriate answer -- if we don't want to break the API, we should add the getTime call. http://gwt-code-reviews.appspot.com/1704804/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Cast return value from JsDate's various setter methods to be (issue1704804)
On 2012/05/10 22:24:02, wayneshu wrote: On 2012/05/10 22:22:32, wayneshu wrote: To avoid potential issues, I updated the change to return the value of getTime() Assuming we want to avoid breaking the API, then I think this change is fine. I still hate adding unnecessary bloat in every use of these setters (the compiler cannot remove arbitrary JS calls because it doesn't know if they have side effects or not), so my preference would be to just change them to void. Bhaskar can decide which way he wants to do it. http://gwt-code-reviews.appspot.com/1704804/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: CaptionPanel should use ScheduledCommand instead of Command (issue1690803)
LGTM http://gwt-code-reviews.appspot.com/1690803/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: FormPanel should use ScheduledCommand instead of Command (issue1691803)
LGTM http://gwt-code-reviews.appspot.com/1691803/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: SplitLayoutPanel should use ScheduledCommand instead of Command (issue1689803)
LGTM, I'll get this committed next week. http://gwt-code-reviews.appspot.com/1689803/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Incorrect doc for Style.Position (issue1688803)
LGTM, I'll get this committed next week. http://gwt-code-reviews.appspot.com/1688803/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Make GWT.create/etc usable on server. (issue1677803)
Thanks for the review.
http://gwt-code-reviews.appspot.com/1677803/diff/4001/user/src/com/google/gwt/core/server/ServerGwtBridge.java
File user/src/com/google/gwt/core/server/ServerGwtBridge.java (right):
http://gwt-code-reviews.appspot.com/1677803/diff/4001/user/src/com/google/gwt/core/server/ServerGwtBridge.java#newcode263
user/src/com/google/gwt/core/server/ServerGwtBridge.java:263: public
String getVersion() {
On 2012/04/18 21:45:01, cromwellian wrote:
Too bad com.google.gwt.dev.About is part of the gwt-dev package and
not shared.
:(
I thought about it, but we don't have an easy way to share classes
between gwt-dev and gwt-user. Plus, it might be confusing for shared
code -- do we consider the client or the server? I'm not sure this
should even really be here, but the API already had it.
http://gwt-code-reviews.appspot.com/1677803/diff/4001/user/src/com/google/gwt/core/server/ServerGwtBridge.java#newcode273
user/src/com/google/gwt/core/server/ServerGwtBridge.java:273: public
void log(String message, Throwable e) {
On 2012/04/18 21:45:01, cromwellian wrote:
Maybe Logger.getLogger(ServerGwtBridge.class).log() or allow the
ServerGwtBridge
to be configured with some logger callback interface so that
ServletContext.log(), java.u.logging, or any other desired logger can
be used.
I don't want to add a bunch, so I will just use j.u.logging.
http://gwt-code-reviews.appspot.com/1677803/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Move more classes from client to shared, so they can be used on the (issue1681803)
This isn't completely ready for review, as UiBinder still has a reference to the old DateTimeFormat.PredefinedFormat that I haven't tracked down yet. In particular, I am interested in feedback on API changes in c.g.g.i18n.shared as part of moving from client: - introducing Locales for information about multiple locales and the set of locales built for a GWT app (basically the static portion of LocaleInfo) - add a way to get a CurrencyList from LocaleInfo rather than having a static singleton in CurrencyList. - over time, any other things where we need to get an instance specific to the current locale, it will be added to LocaleInfo rather than having scattered singletons http://gwt-code-reviews.appspot.com/1681803/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Make GWT.create/etc usable on server. (issue1677803)
@scottb - now that you are back, would you care to review this again? Since you last looked at it, I changed the way server-side class instantiators are registered, added a way to register property values, and added a servlet testing the Localizable instantiator. http://gwt-code-reviews.appspot.com/1677803/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Add support for soft permutations to SingleScriptLinker. (issue1678803)
LGTM It seems like there should be some test for this to make sure it keeps working. Could you extend SingleScriptLinkerTest to make sure that it handles collapsing soft permutations? http://gwt-code-reviews.appspot.com/1678803/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Use one common implementation of UserAgentProperty when runtime (issue1680803)
LGTM http://gwt-code-reviews.appspot.com/1680803/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Add support for soft permutations to SingleScriptLinker. (issue1678803)
LGTM http://gwt-code-reviews.appspot.com/1678803/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Make GWT.create/etc usable on server. (issue1677803)
http://gwt-code-reviews.appspot.com/1677803/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Deprecating TreeItem.addItem/insertItem(String html) in favor of methods that take SafeHtml. Th... (issue1666803)
LGTM http://gwt-code-reviews.appspot.com/1666803/diff/1/user/test/com/google/gwt/user/client/ui/TreeTest.java File user/test/com/google/gwt/user/client/ui/TreeTest.java (right): http://gwt-code-reviews.appspot.com/1666803/diff/1/user/test/com/google/gwt/user/client/ui/TreeTest.java#newcode187 user/test/com/google/gwt/user/client/ui/TreeTest.java:187: // Normalize the html for ancient safari 3 Could this comment be clearer, like Safari 3 leaves in the HTML. http://gwt-code-reviews.appspot.com/1666803/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Introduced PagerFactory to allow easy swap of pagers for CellBrowser. (issue1659803)
http://gwt-code-reviews.appspot.com/1659803/diff/1/user/src/com/google/gwt/user/cellview/client/CellBrowser.java File user/src/com/google/gwt/user/cellview/client/CellBrowser.java (right): http://gwt-code-reviews.appspot.com/1659803/diff/1/user/src/com/google/gwt/user/cellview/client/CellBrowser.java#newcode69 user/src/com/google/gwt/user/cellview/client/CellBrowser.java:69: import javax.annotation.Nullable; On 2012/03/09 15:16:52, jlabanca wrote: Probably not if it isn't part of the standard Java library. @rkj - can you verify that the compiler doesn't warn/error about Nullable, and remove it if it does? @Nullable is not in core Java, and is in jsr305 (IIRC). We could add it, but it doesn't really add a lot of value for GWT unless we also altered the compiler to understand it. For annotations, we only need the class files on the classpath (and in fact the source for the jar isn't GWT-compatbile) if you want to add it. http://gwt-code-reviews.appspot.com/1659803/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Updates Missing Plugin Page (issue1641803)
LGTM http://gwt-code-reviews.appspot.com/1641803/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Custom Field Serializer for EnumMap (issue1634804)
There appears to be some problem in how this was created -- the side-by-side diffs don't work, and Publish+Mail Comments doesn't work. Basically: - you need tests - copyright and code style should match GWT standards - the limitation on non-empty maps seems too limiting. The GWT emulation can reach a full set of enum values via keySet, so that could be used on the client side to avoid this limitation - the server doesn't appear to read the exemplar value from the stream http://gwt-code-reviews.appspot.com/1634804/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Adding support for Typed Arrays (issue1621803)
I'm eagerly waiting for it ;-) Done - http://gwt-code-reviews.appspot.com/1626803/ http://gwt-code-reviews.appspot.com/1621803/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Adding support for Typed Arrays (issue1621803)
I think it is appropriate to keep the correct type on things, as using it on the server people will be unhappy using double for purely integral values. So, I think using long for uint32 is correct, but there should be docs explaining that it is slow and should be avoided in the client, and also have the methods taking/returning double. Is it worth doing to have all the separate isSupported? As someone using it, I would rather have one check that makes sure everything is available and be done with it -- that would mean either saying typed arrays aren't supported due to DataView missing, or else emulating it using the other views (the byte-order swapping for floats would be problematic there, I think). Likewise for Uint8ClampedArray on WebKit. Shared code should be able to instantiate arrays and views, so you need one factory that has a super-source version that instantiates the client-side version. One use case I have in mind is implementing binary protobuf support -- you don't want to have to generate code that is different between the client and server. For the Java/JS array aliasing/copying, enough places have needed this I think it needs to go into c.g.g.core.client with the other JsArray* classes, and everyone can just use that implementation rather than reinventing the wheel slightly differently. I am off today, but I hacked up a proof-of-concept of how to divide into into shared/client/server, and a quick ByteBuffers implementation -- I'll try and post it later today so you can look at it. http://gwt-code-reviews.appspot.com/1621803/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Fix this-window confusion in DevMode (issue1620805)
But the point is it is almost certainly an error to call a method on a null value, and catching it in DevMode would be better than replicating the behavior of prod mode in this case. http://gwt-code-reviews.appspot.com/1620805/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Fixes issue 4301: ensures all strings generated by GWT are JS string values. (issue1623803)
See my comments on the referenced bug, but basically I am not sure I agree with the change being made. That said, the cost to projects that don't care is pretty small, so I could be convinced. http://gwt-code-reviews.appspot.com/1623803/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Add History.replaceItem(String) method to replace the current history (issue1614809)
The idea and initial implementation looks good, but most of the complexity and implementation details are due to cross-browser issues. In particular, we left out functionality that we couldn't implement consistently across all browsers, and my recollection is that this was one of those cuts. It is possible that has changed now, or that the set of browsers that don't support it is small enough we can add it with documentation explaining when it can't be used, but it definitely needs extensive testing across the full range of browsers GWT supports. http://gwt-code-reviews.appspot.com/1614809/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Fix: __gwt_ObjectId is fetched in for-in loop in DevMode. (issue1592803)
Sorry, I completely forgot about this. LGTM http://gwt-code-reviews.appspot.com/1592803/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Add a FakeCssMaker for CSS resources similar to FakeMessagesMaker. (issue1604804)
http://gwt-code-reviews.appspot.com/1604804/diff/1/user/src/com/google/gwt/junit/FakeCssMaker.java
File user/src/com/google/gwt/junit/FakeCssMaker.java (right):
http://gwt-code-reviews.appspot.com/1604804/diff/1/user/src/com/google/gwt/junit/FakeCssMaker.java#newcode44
user/src/com/google/gwt/junit/FakeCssMaker.java:44:
FakeCssMaker.class.getClassLoader(), new Class[] {cssClass},
Is this the right classloader to use? FakeCssMaker is part of GWT,
while cssClass will typically be a user class.
I suggest using the thread context classloader instead.
http://gwt-code-reviews.appspot.com/1604804/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Add a FakeCssMaker for CSS resources similar to FakeMessagesMaker. (issue1604804)
Right now, it won't make much difference as GWT gets user classes from the same classpath that GWT classes are loaded from. However, that leads to conflict for shared code where you want to use a different version of something than the version GWT itself needs to run. So, in the future we may provide a way to give separate classpaths. I think of the following guidelines for choosing which classloader to use: - if a class being loaded is bundled with some other class (ie, known to be from the same jar), use that class's classloader to load it. - if you want to load a system class and definitely don't want it to be overridden, use the system classloader - otherwise, use the thread context classloader. Since you have already submitted it and this will generally be used only in tests, this is just FYI and you don't need to update it. http://gwt-code-reviews.appspot.com/1604804/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Add a FakeCssMaker for CSS resources similar to FakeMessagesMaker. (issue1604804)
Right now, it won't make much difference as GWT gets user classes from the same classpath that GWT classes are loaded from. However, that leads to conflict for shared code where you want to use a different version of something than the version GWT itself needs to run. So, in the future we may provide a way to give separate classpaths. I think of the following guidelines for choosing which classloader to use: - if a class being loaded is bundled with some other class (ie, known to be from the same jar), use that class's classloader to load it. - if you want to load a system class and definitely don't want it to be overridden, use the system classloader - otherwise, use the thread context classloader. Since you have already submitted it and this will generally be used only in tests, this is just FYI and you don't need to update it. http://gwt-code-reviews.appspot.com/1604804/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Add NoSuchMethodException to java.lang (issue1529807)
http://gwt-code-reviews.appspot.com/1529807/diff/1/user/super/com/google/gwt/emul/java/lang/NoSuchMethodException.java File user/super/com/google/gwt/emul/java/lang/NoSuchMethodException.java (right): http://gwt-code-reviews.appspot.com/1529807/diff/1/user/super/com/google/gwt/emul/java/lang/NoSuchMethodException.java#newcode21 user/super/com/google/gwt/emul/java/lang/NoSuchMethodException.java:21: * official Java API doc/a for details. The doc needs to very clearly say this will never be thrown by GWT or any GWT libraries and is only provided for compatibility with user code that explicitly throws or catches it. http://gwt-code-reviews.appspot.com/1529807/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Updating npapi plugin to remove gwtId from Jso objects (idenity fix), (issue1469803)
LGTM http://gwt-code-reviews.appspot.com/1469803/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Updating npapi plugin to remove gwtId from Jso objects (idenity fix), (issue1469803)
http://gwt-code-reviews.appspot.com/1469803/diff/1/plugins/npapi/LocalObjectTable.h
File plugins/npapi/LocalObjectTable.h (right):
http://gwt-code-reviews.appspot.com/1469803/diff/1/plugins/npapi/LocalObjectTable.h#newcode100
plugins/npapi/LocalObjectTable.h:100: NPObject* get(int id) {
Given the new method added, please change this to something like
getById.
http://gwt-code-reviews.appspot.com/1469803/diff/1/plugins/npapi/LocalObjectTable.h#newcode109
plugins/npapi/LocalObjectTable.h:109: int get(NPObject* jsObject) {
Please use a more descriptive name, such as getObjectId since there is
now collision with the above.
http://gwt-code-reviews.appspot.com/1469803/diff/1/plugins/npapi/ScriptableInstance.cpp
File plugins/npapi/ScriptableInstance.cpp (right):
http://gwt-code-reviews.appspot.com/1469803/diff/1/plugins/npapi/ScriptableInstance.cpp#newcode434
plugins/npapi/ScriptableInstance.cpp:434: int id =
localObjects.get(obj);
So how does this fix the identity problem? The reason this was added
was to avoid the identity problem, since the same JS object passed to
the plugin twice will get different NPObject wrappers. Has that been
fixed now?
http://gwt-code-reviews.appspot.com/1469803/diff/1/user/test/com/google/gwt/core/client/JsIdentityTest.java
File user/test/com/google/gwt/core/client/JsIdentityTest.java (right):
http://gwt-code-reviews.appspot.com/1469803/diff/1/user/test/com/google/gwt/core/client/JsIdentityTest.java#newcode2
user/test/com/google/gwt/core/client/JsIdentityTest.java:2: * Copyright
2008 Google Inc.
2011
http://gwt-code-reviews.appspot.com/1469803/diff/1/user/test/com/google/gwt/core/client/JsIdentityTest.java#newcode24
user/test/com/google/gwt/core/client/JsIdentityTest.java:24: * @author
cod...@google.com (John McDole)
We don't use @author tags in GWT, and the first line should have a
period. Did you run checkstyle or have Eclipse configured to use it?
http://gwt-code-reviews.appspot.com/1469803/diff/1/user/test/com/google/gwt/core/client/JsIdentityTest.java#newcode34
user/test/com/google/gwt/core/client/JsIdentityTest.java:34: return
com.google.gwt.core.Core;
I'm not sure I follow what all these tests are looking for.
The most basic tests are:
testJavaToJs() { obj=...; assertTrue(jsID(obj, obj)); }
native jsID(a, b) { return a === b; }
testJsToJava() { initJsObj(); a=getJsObj(); b=getJsObj(); assertSame(a,
b); }
native initJsObj() { obj = {}; }
native getJsObj() { return obj; }
Having additional tests that store objects and return them later are
fine, but it is hard for me to tell these cases are covered from the
test and I would prefer a more direct test for them.
http://gwt-code-reviews.appspot.com/1469803/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Updating npapi plugin to remove gwtId from Jso objects (idenity fix), (issue1469803)
http://gwt-code-reviews.appspot.com/1469803/diff/1/plugins/npapi/ScriptableInstance.cpp File plugins/npapi/ScriptableInstance.cpp (right): http://gwt-code-reviews.appspot.com/1469803/diff/1/plugins/npapi/ScriptableInstance.cpp#newcode434 plugins/npapi/ScriptableInstance.cpp:434: int id = localObjects.get(obj); On 2011/07/01 18:50:17, codefu wrote: On 2011/07/01 18:38:17, jat wrote: So how does this fix the identity problem? The reason this was added was to avoid the identity problem, since the same JS object passed to the plugin twice will get different NPObject wrappers. Has that been fixed now? Kelly's patch to chrome is not present in Chrome13 builds. Running the GWT tests shows these passing with Chrome13+NewPlugin and failing in Chrome12+NewPlugin Did you mean is now present? If not, I don't understand the statement. The old plugin should be passing for the Java-JS direction, right? How many people are upgraded to Chrome13? Can we make it so it works on both, by using different plugins based on the Chrome version? If not, can we make it so the new plugin would not get installed on older Chromes by setting a minimum version? We would not want people running older Chromes to get updated to something that works less well. http://gwt-code-reviews.appspot.com/1469803/diff/1/user/test/com/google/gwt/core/client/JsIdentityTest.java File user/test/com/google/gwt/core/client/JsIdentityTest.java (right): http://gwt-code-reviews.appspot.com/1469803/diff/1/user/test/com/google/gwt/core/client/JsIdentityTest.java#newcode34 user/test/com/google/gwt/core/client/JsIdentityTest.java:34: return com.google.gwt.core.Core; On 2011/07/01 18:50:17, codefu wrote: The last test, testJavaArrayArray(), was something I was experiencing working with scheduled tasks leaking. I can certainly remove it. No need to remove a test if it is useful. Please add a test like my first example using JS === to make sure the same Java object passed twice is in fact the same object seen by JS -- you can keep the testjavaObjectStorage test. http://gwt-code-reviews.appspot.com/1469803/diff/1/user/test/com/google/gwt/core/client/JsIdentityTest.java#newcode88 user/test/com/google/gwt/core/client/JsIdentityTest.java:88: assertTrue(obj1 == obj2); Use assertSame here instead. http://gwt-code-reviews.appspot.com/1469803/diff/1/user/test/com/google/gwt/core/client/JsIdentityTest.java#newcode108 user/test/com/google/gwt/core/client/JsIdentityTest.java:108: assertTrue(id2 == get2); Use assertSame. http://gwt-code-reviews.appspot.com/1469803/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Add runtime-locale support for Localizable subtypes. (issue1425816)
Ping http://gwt-code-reviews.appspot.com/1425816/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Add runtime-locale support for Localizable subtypes. (issue1425816)
Reviewers: unnurg, Description: Add runtime-locale support for Localizable subtypes. This is a prerequisitive for later work moving things from being generated at compile-time to being mostly pre-generated during the CLDR import step. What can't be pre-generated is the runtime locales support, so this provides the ability to support runtime locales for pre-generated code. Review by: unn...@google.com Please review this at http://gwt-code-reviews.appspot.com/1425816/ Affected files: M user/src/com/google/gwt/i18n/client/impl/cldr/LocalizedNamesImplBase.java M user/src/com/google/gwt/i18n/rebind/LocaleInfoGenerator.java M user/src/com/google/gwt/i18n/rebind/LocalizableGenerator.java M user/src/com/google/gwt/i18n/tools/I18NSync.java M user/test/com/google/gwt/i18n/I18NSuite.java A user/test/com/google/gwt/i18n/rebind/LocalizableGeneratorTest.java -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Add runtime-locale support for Localizable subtypes. (issue1425816)
http://gwt-code-reviews.appspot.com/1425816/diff/1/user/src/com/google/gwt/i18n/client/impl/cldr/LocalizedNamesImplBase.java
File
user/src/com/google/gwt/i18n/client/impl/cldr/LocalizedNamesImplBase.java
(right):
http://gwt-code-reviews.appspot.com/1425816/diff/1/user/src/com/google/gwt/i18n/client/impl/cldr/LocalizedNamesImplBase.java#newcode60
user/src/com/google/gwt/i18n/client/impl/cldr/LocalizedNamesImplBase.java:60:
return super.loadLikelyRegionCodes();
So what was happening before I added this is that a delegating call
would fail with a visibility error, like this:
public class LocalizedNamesImpl_es_419_runtimeSelection extends
LocalizedNamesImpl {
private LocalizedNamesImpl instance;
protected String[] loadLikelyRegionCodes() {
ensureInstance();
return instance.loadLikelyRegionCodes();
}
}
And it would fail with on the instance.loadLikelyRegionCodes call (in
both javac and Eclipse), even though other calls to protected methods
wouldn't. I'm not sure why, but adding this fixed it.
http://gwt-code-reviews.appspot.com/1425816/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Improve runtime locales support, so runtime locales that are under a (issue1421812)
Reviewers: unnurg, Description: Improve runtime locales support, so runtime locales that are under a more specific compile-time locale do not appear under a more general one. An example would be compile locales of [es, es-419] and runtime locales of [es-es, es-co, es-ar] -- the runtime locales for es should not include es-co and es-ar since they are also under es-419. Review by: unn...@google.com Please review this at http://gwt-code-reviews.appspot.com/1421812/ Affected files: M user/src/com/google/gwt/i18n/rebind/LocaleUtils.java M user/src/com/google/gwt/i18n/server/GwtLocaleImpl.java A user/test/com/google/gwt/i18n/rebind/LocaleUtilsTest.java -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Additional infrastructure for generating source code outside of a (issue1421805)
http://gwt-code-reviews.appspot.com/1421805/diff/1/user/src/com/google/gwt/codegen/server/AbortablePrintWriter.java
File user/src/com/google/gwt/codegen/server/AbortablePrintWriter.java
(right):
http://gwt-code-reviews.appspot.com/1421805/diff/1/user/src/com/google/gwt/codegen/server/AbortablePrintWriter.java#newcode40
user/src/com/google/gwt/codegen/server/AbortablePrintWriter.java:40:
private static OutputStream getOutputStream(PrintWriter pw) {
This is changing, I found this doesn't work on some JVM implementations,
so I have to do it differently -- will upload replacement shortly.
http://gwt-code-reviews.appspot.com/1421805/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Additional infrastructure for generating source code outside of a (issue1421805)
http://gwt-code-reviews.appspot.com/1421805/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Add SafeUri type, similar to SafeHtml but for values in a URL attribute context. (issue1380806)
LGTM with nits.
http://gwt-code-reviews.appspot.com/1380806/diff/29005/user/src/com/google/gwt/resources/client/ImageResource.java
File user/src/com/google/gwt/resources/client/ImageResource.java
(right):
http://gwt-code-reviews.appspot.com/1380806/diff/29005/user/src/com/google/gwt/resources/client/ImageResource.java#newcode127
user/src/com/google/gwt/resources/client/ImageResource.java:127: String
getURL();
Should this be deprecated?
http://gwt-code-reviews.appspot.com/1380806/diff/29005/user/src/com/google/gwt/resources/rg/DataResourceGenerator.java
File user/src/com/google/gwt/resources/rg/DataResourceGenerator.java
(right):
http://gwt-code-reviews.appspot.com/1380806/diff/29005/user/src/com/google/gwt/resources/rg/DataResourceGenerator.java#newcode68
user/src/com/google/gwt/resources/rg/DataResourceGenerator.java:68:
sw.println(new + DataResourcePrototype.class.getName() + ();
Should this be getCanonicalName()? In this case they are the same, but
in general getName() is not going to return a name you can use in the
source.
If you change, change them all in this change.
http://gwt-code-reviews.appspot.com/1380806/diff/29005/user/src/com/google/gwt/resources/rg/ImageResourceGenerator.java
File user/src/com/google/gwt/resources/rg/ImageResourceGenerator.java
(right):
http://gwt-code-reviews.appspot.com/1380806/diff/29005/user/src/com/google/gwt/resources/rg/ImageResourceGenerator.java#newcode481
user/src/com/google/gwt/resources/rg/ImageResourceGenerator.java:481:
new String[]{bundle.getNormalContentsFieldName(),
bundle.getRtlContentsFieldName()};
space between ] and {
http://gwt-code-reviews.appspot.com/1380806/diff/29005/user/src/com/google/gwt/user/client/ui/Image.java
File user/src/com/google/gwt/user/client/ui/Image.java (right):
http://gwt-code-reviews.appspot.com/1380806/diff/29005/user/src/com/google/gwt/user/client/ui/Image.java#newcode301
user/src/com/google/gwt/user/client/ui/Image.java:301: public abstract
void setVisibleRect(Image image, int left, int top, int width, int
height);
On 2011/04/27 17:18:58, xtof wrote:
Maybe undo this line wrap?
Since we changed to 100 chars, my understanding is each file when edited
should be reformatted, though ideally as a separate change to avoid
obfuscating the real change. In this case, I don't care much either
way.
http://gwt-code-reviews.appspot.com/1380806/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Wrap low-priorty log calls with an 'if' test to avoid unnecessary calls (issue1425807)
LGTM http://gwt-code-reviews.appspot.com/1425807/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Additional infrastructure for generating source code outside of a (issue1421805)
Reviewers: unnurg, Description: Additional infrastructure for generating source code outside of a GWT generator. Review by: unn...@google.com Please review this at http://gwt-code-reviews.appspot.com/1421805/ Affected files: A user/src/com/google/gwt/codegen/rebind/GwtCodeGenContext.java A user/src/com/google/gwt/codegen/server/AbortablePrintWriter.java A user/src/com/google/gwt/codegen/server/CodeGenContext.java A user/src/com/google/gwt/codegen/server/CodeGenLogger.java A user/src/com/google/gwt/codegen/server/CodeGenUtils.java A user/src/com/google/gwt/codegen/server/JavaSourceWriter.java A user/src/com/google/gwt/codegen/server/SourceWriter.java A user/src/com/google/gwt/codegen/server/SourceWriterBase.java A user/src/com/google/gwt/codegen/server/SourceWriterBuilder.java A user/src/com/google/gwt/codegen/server/StringSourceWriter.java -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Create a utility class for checking assignability of types for use (issue1420808)
http://gwt-code-reviews.appspot.com/1420808/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Create a utility class for checking assignability of types for use (issue1420808)
Committed at r10058 http://gwt-code-reviews.appspot.com/1420808/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Create a utility class for checking assignability of types for use (issue1420808)
http://gwt-code-reviews.appspot.com/1420808/diff/6004/user/src/com/google/gwt/uibinder/rebind/TypeOracleUtils.java
File user/src/com/google/gwt/uibinder/rebind/TypeOracleUtils.java
(right):
http://gwt-code-reviews.appspot.com/1420808/diff/6004/user/src/com/google/gwt/uibinder/rebind/TypeOracleUtils.java#newcode72
user/src/com/google/gwt/uibinder/rebind/TypeOracleUtils.java:72: if
(paramType instanceof JPrimitiveType argType instanceof
JPrimitiveType) {
On 2011/04/22 20:28:54, tbroyer wrote:
Shouldn't it rather be paramType.isPrimitive() != null?
Since JPrimitiveType is an enum, you can't really have isPrimitive
return anything else, but yes it probably should just to be consistent
with the other is* methods.
http://gwt-code-reviews.appspot.com/1420808/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Create a utility class for checking assignability of types for use (issue1420808)
Reviewers: rjrjr, Description: Create a utility class for checking assignability of types for use in finding compatible constructors/methods. Review by: rj...@google.com Please review this at http://gwt-code-reviews.appspot.com/1420808/ Affected files: M user/src/com/google/gwt/uibinder/elementparsers/DateLabelParser.java A user/src/com/google/gwt/uibinder/rebind/TypeOracleUtils.java A user/test/com/google/gwt/uibinder/rebind/TypeOracleUtilsTest.java -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Create a utility class for checking assignability of types for use (issue1420808)
http://gwt-code-reviews.appspot.com/1420808/diff/1/user/src/com/google/gwt/uibinder/rebind/TypeOracleUtils.java
File user/src/com/google/gwt/uibinder/rebind/TypeOracleUtils.java
(right):
http://gwt-code-reviews.appspot.com/1420808/diff/1/user/src/com/google/gwt/uibinder/rebind/TypeOracleUtils.java#newcode60
user/src/com/google/gwt/uibinder/rebind/TypeOracleUtils.java:60:
JType... argTypes) {
On 2011/04/22 01:13:17, rjrjr wrote:
I don't know that I would ever use this unless it was based on
JClassType.getOverridableMethods().
But rather than hardcoding that choice, perhaps you should make the
first
argument a JMethod[] to let me choose the set myself? Can put @see
pointing to
getOverridableMethods.
Ok, how about a method taking the method list and one taking the type,
which defaults to either getMethods or getOverridableMethods (whichever
you think is more useful). I would think in this application it is most
interesting in this context, since you want to know if you can call the
method with a set of parameters, and whether it is implemented on that
class or a superclass is just an implementation detail.
I'm biting my tongue about the fact that this method is unused. I'm
generally
opposed to speculative coding, but I can imagine cleaning up some
existing code
with this.
It just seemed an obvious extension of the constructor code, but it is
certainly easy enough to leave it out if you prefer.
http://gwt-code-reviews.appspot.com/1420808/diff/1/user/src/com/google/gwt/uibinder/rebind/TypeOracleUtils.java#newcode65
user/src/com/google/gwt/uibinder/rebind/TypeOracleUtils.java:65: if
(method.isStatic() == isStatic methodName.equals(method.getName())
On 2011/04/22 01:13:17, rjrjr wrote:
could you put () around the == statement?
Done.
http://gwt-code-reviews.appspot.com/1420808/diff/1/user/src/com/google/gwt/uibinder/rebind/TypeOracleUtils.java#newcode78
user/src/com/google/gwt/uibinder/rebind/TypeOracleUtils.java:78: *
@param argType
On 2011/04/22 01:13:17, rjrjr wrote:
What's the difference between a paramType and an argType? Seems like
the
distinction matters here, but param and arg look like synonyms to me.
paramType is the type of the formal parameter declaration, while argType
is the type of the argument to be passed in.
Should the args be leftHandType and rightHandType? This applies
throughout the
class of course, not just here.
That would work, although the terminology seems a bit odd in the context
of a method call.
http://gwt-code-reviews.appspot.com/1420808/diff/1/user/src/com/google/gwt/uibinder/rebind/TypeOracleUtils.java#newcode140
user/src/com/google/gwt/uibinder/rebind/TypeOracleUtils.java:140:
JType[] ctorArgTypes = paramTypes;
On 2011/04/22 01:13:17, rjrjr wrote:
Looks like ctorArgTypes is refactoring chaff, should be inlined.
Yes, I created it just so I could inline it, then never did :). Done.
http://gwt-code-reviews.appspot.com/1420808/diff/1/user/src/com/google/gwt/uibinder/rebind/TypeOracleUtils.java#newcode157
user/src/com/google/gwt/uibinder/rebind/TypeOracleUtils.java:157: } else
{
On 2011/04/22 01:13:17, rjrjr wrote:
else if
Done.
http://gwt-code-reviews.appspot.com/1420808/diff/1/user/test/com/google/gwt/uibinder/rebind/TypeOracleUtilsTest.java
File user/test/com/google/gwt/uibinder/rebind/TypeOracleUtilsTest.java
(right):
http://gwt-code-reviews.appspot.com/1420808/diff/1/user/test/com/google/gwt/uibinder/rebind/TypeOracleUtilsTest.java#newcode29
user/test/com/google/gwt/uibinder/rebind/TypeOracleUtilsTest.java:29:
On 2011/04/22 01:13:17, rjrjr wrote:
import static com.google.gwt.uibinder.rebind.TypeOracleUtils.*; might
make this
easier to read.
Done.
http://gwt-code-reviews.appspot.com/1420808/diff/1/user/test/com/google/gwt/uibinder/rebind/TypeOracleUtilsTest.java#newcode145
user/test/com/google/gwt/uibinder/rebind/TypeOracleUtilsTest.java:145:
public void testHasCompatibleMethod() {
On 2011/04/22 01:13:17, rjrjr wrote:
Many lines too long. Did you auto format?
Yes, although I did make some edits since then. The @link tags can't be
split, and Reitveld is wrapping at 80 instead of 100. I will make sure
it is formatted properly before submitting.
http://gwt-code-reviews.appspot.com/1420808/diff/1/user/test/com/google/gwt/uibinder/rebind/TypeOracleUtilsTest.java#newcode146
user/test/com/google/gwt/uibinder/rebind/TypeOracleUtilsTest.java:146:
assertTrue(didn't find static m1(Child),
TypeOracleUtils.hasCompatibleMethod(foo, true, m1,
On 2011/04/22 01:13:17, rjrjr wrote:
s/didn't find/Should have found/
Done.
http://gwt-code-reviews.appspot.com/1420808/diff/1/user/test/com/google/gwt/uibinder/rebind/TypeOracleUtilsTest.java#newcode148
user/test/com/google/gwt/uibinder/rebind/TypeOracleUtilsTest.java:148:
assertFalse(found m1(Child), TypeOracleUtils.hasCompatibleMethod(foo,
false, m1, child));
On 2011/04/22 01:13:17, rjrjr wrote:
s/found/Should not have found/
Done.
