date:20110329


Comment by yegor.jb...@gmail.com:

Well detailed and well scoped. I think this spec is great. Except I think  
protobuf compiler command is protoc, not protocc (one 'c' at the end).


For more information:
http://code.google.com/p/google-web-toolkit/wiki/ProtocolBuffers

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: Comment on ProtocolBuffers in google-web-toolkit


Comment by yegor.jb...@gmail.com:

Oh, I forgot. This will probably come with AutoBean, but it might be good  
to mention how code splitting would work. The framework should guarantee  
that a single protobuf module is code-splittable, i.e. not compiled in  
its entirety to all code fragments.


For more information:
http://code.google.com/p/google-web-toolkit/wiki/ProtocolBuffers

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

Re: [gwt-contrib] Re: LongLibBase improperly stomps on global 'a' variable. This patch reuses globalTemp (_) instead. (issue1389803)

2011-03-29 Thread Scott Blum

Sounds good.  Now that I think about it, it may not make much performance
difference if time tends to be dominated by the actual math ops, but I
suppose every bit helps?

On Tue, Mar 29, 2011 at 10:35 AM, Daniel Rice (דניאל רייס)
r...@google.comwrote:

 I did the reimplementation.  Being less familiar with the intricacies of
 JavaScript, it seems likely that I missed an opportunity to preserve the
 nativeness of the array.  I'd be happy to take a look at this after the 2.3
 release.

 On Mon, Mar 28, 2011 at 5:52 PM, Scott Blum sco...@google.com wrote:

 Something smells fishy here.  I'm quite certain that this used to be
 implemented strictly as a two-double array in web mode, a true array.  At
 some point it was modified to use 3 elements instead of 2, but I don't think
 it should have lost its nativity.


 On Mon, Mar 28, 2011 at 5:36 PM, cromwell...@google.com wrote:



 http://gwt-code-reviews.appspot.com/1389803/diff/1/dev/core/super/com/google/gwt/lang/LongLibBase.java
 File dev/core/super/com/google/gwt/lang/LongLibBase.java (right):


 http://gwt-code-reviews.appspot.com/1389803/diff/1/dev/core/super/com/google/gwt/lang/LongLibBase.java#newcode325
 dev/core/super/com/google/gwt/lang/LongLibBase.java:325: _.l = l, _.m =
 m, _.h = h, _);
 On 2011/03/28 21:31:25, scottb wrote:

 Stupid question, but why can't we just return {l:l, m:m, h:h}?


 Good question. It looks like LongEmul is not a JSO, because they want to
 reuse it in both JVM and ProdMode, so it's a Java type. This code may
 have existed before @GwtScriptOnly or SingleJsoImpl. If I were during
 this today, I'd make LongEmul an interface, use JSO for ProdMode, and
 JRE impl for everything else.

 I guess we'll revisit it later.


 http://gwt-code-reviews.appspot.com/1389803/


  --
 http://groups.google.com/group/Google-Web-Toolkit-Contributors




-- 
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: Issue 5129: Accomodate RunAsync in ActivityManager (issue1383802)

2011-03-29 Thread Ray Ryan

It's still unclear to me that your AbstractAsyncActivity actually works. It
seems like it will just produce a single split point, as Thomas suggested of
my first patch here. Have you seen it make multiple fragments?

On Tue, Mar 29, 2011 at 2:36 AM, Antoine DESSAIGNE 
antoine.dessai...@gmail.com wrote:

 Hi,

 Should I modify my patch and repost it or do you go with the
 http://gwt-code-reviews.appspot.com/1386806/ option ?

 Again I truely think that it's not necessary to add anything other than the
 simple AbstractAsyncActivity class (we may call it otherwise though).

 Antoine.

 2011/3/25 rj...@google.com

 This struck me as being more complicated than warranted. I took a crack
 at it myself in http://gwt-code-reviews.appspot.com/1386806/, but I may
 be making some bad assumptions.



 http://gwt-code-reviews.appspot.com/1383802/diff/1/user/src/com/google/gwt/activity/shared/AbstractAsyncActivity.java
 File user/src/com/google/gwt/activity/shared/AbstractAsyncActivity.java
 (right):


 http://gwt-code-reviews.appspot.com/1383802/diff/1/user/src/com/google/gwt/activity/shared/AbstractAsyncActivity.java#newcode26
 user/src/com/google/gwt/activity/shared/AbstractAsyncActivity.java:26: *
 onCancel.
 These docs are a bit confusing. All Activities are async, but what
 you're really talking about here is code splitting via GWT.runAsync()


 http://gwt-code-reviews.appspot.com/1383802/diff/1/user/src/com/google/gwt/activity/shared/AsyncActivityProvider.java
 File user/src/com/google/gwt/activity/shared/AsyncActivityProvider.java
 (right):


 http://gwt-code-reviews.appspot.com/1383802/diff/1/user/src/com/google/gwt/activity/shared/AsyncActivityProvider.java#newcode85
 user/src/com/google/gwt/activity/shared/AsyncActivityProvider.java:85: *
 Transforms the given synchronous activity into an asynchronous one.
 Again, synchronous activity isn't a good term.

 http://gwt-code-reviews.appspot.com/1383802/




-- 
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Adds support for the CSS_ATTRIBUTE_START parse context to HtmlTemplateParser. (issue1392801)


Reviewers: jlabanca, pdr,

Description:
Adds support for the CSS_ATTRIBUTE_START parse context to
HtmlTemplateParser.


Please review this at http://gwt-code-reviews.appspot.com/1392801/

Affected files:
  M user/src/com/google/gwt/safehtml/rebind/HtmlTemplateParser.java
  M user/src/com/google/gwt/safehtml/rebind/ParsedHtmlTemplate.java
  M user/test/com/google/gwt/safehtml/rebind/HtmlTemplateParserTest.java


Index: user/src/com/google/gwt/safehtml/rebind/HtmlTemplateParser.java
===
--- user/src/com/google/gwt/safehtml/rebind/HtmlTemplateParser.java	 
(revision 9880)
+++ user/src/com/google/gwt/safehtml/rebind/HtmlTemplateParser.java	 
(working copy)

@@ -74,10 +74,14 @@
  * ddThis context corresponds to a parameter that appears at the very  
start of
  * a URL-valued HTML attribute's value; in the above example this applies  
to

  * parameter #1.
+ * dt{@link HtmlContext.Type#CSS_ATTRIBUTE_START}
+ * ddThis context corresponds to a parameter that appears at the very
+ * beginning of a {@code style} attribute's value; in the above example  
this

+ * applies to parameter #0.
  * dt{@link HtmlContext.Type#CSS_ATTRIBUTE}
  * ddThis context corresponds to a parameter that appears in the context  
of a

- * {@code style} attribute; in the above example this applies to
- * parameter #0.
+ * {@code style} attribute, except at the very beginning of the attribute's
+ * value.
  * dt{@link HtmlContext.Type#ATTRIBUTE_VALUE}
  * ddThis context corresponds to a parameter that appears within an  
attribute

  * and is not in one of the more specific in-attribute contexts above. In
@@ -250,7 +254,11 @@
   if (streamHtmlParser.isUrlStart()) {
 return new HtmlContext(HtmlContext.Type.URL_START, tag, attribute);
   } else if (streamHtmlParser.inCss()) {
-return new HtmlContext(HtmlContext.Type.CSS_ATTRIBUTE, tag,  
attribute);

+if (streamHtmlParser.getValueIndex() == 0) {
+  return new HtmlContext(HtmlContext.Type.CSS_ATTRIBUTE_START,  
tag, attribute);

+} else {
+  return new HtmlContext(HtmlContext.Type.CSS_ATTRIBUTE, tag,  
attribute);

+}
   } else {
 return new HtmlContext(
 HtmlContext.Type.ATTRIBUTE_VALUE, tag, attribute);
Index: user/src/com/google/gwt/safehtml/rebind/ParsedHtmlTemplate.java
===
--- user/src/com/google/gwt/safehtml/rebind/ParsedHtmlTemplate.java	 
(revision 9880)
+++ user/src/com/google/gwt/safehtml/rebind/ParsedHtmlTemplate.java	 
(working copy)

@@ -64,7 +64,11 @@
   /**
* CSS (style) attribute context.
*/
-  CSS_ATTRIBUTE
+  CSS_ATTRIBUTE,
+  /**
+   * At the very start of a CSS (style) attribute context.
+   */
+  CSS_ATTRIBUTE_START
 }

 private final Type type;
Index: user/test/com/google/gwt/safehtml/rebind/HtmlTemplateParserTest.java
===
--- user/test/com/google/gwt/safehtml/rebind/HtmlTemplateParserTest.java	 
(revision 9880)
+++ user/test/com/google/gwt/safehtml/rebind/HtmlTemplateParserTest.java	 
(working copy)

@@ -134,9 +134,14 @@
 // Test correct detection of CSS context.
 assertParseTemplateResult(
 [L(div class=\), P((ATTRIBUTE_VALUE,div,class),0), L(\  
style=\), 

++ P((CSS_ATTRIBUTE_START,div,style),2), L(\Hello ), 
++ P((TEXT,null,null),1)],
+div class=\{0}\ style=\{2}\Hello {1});
+assertParseTemplateResult(
+[L(div class=\), P((ATTRIBUTE_VALUE,div,class),0), L(\  
style=\color:green; ), 

 + P((CSS_ATTRIBUTE,div,style),2), L(\Hello ), 
 + P((TEXT,null,null),1)],
-div class=\{0}\ style=\{2}\Hello {1});
+div class=\{0}\ style=\color:green; {2}\Hello {1});
 assertParseTemplateResult(
 [L(div), P((TEXT,null,null),0), L(stylefoo ), 
 + P((CSS,null,null),1), L(/style)],


--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] [google-web-toolkit] r9907 committed - Cherry picking r9880 into releases/2.3m1


Revision: 9907
Author:   rchan...@google.com
Date: Fri Mar 25 07:04:13 2011
Log:  Cherry picking r9880 into releases/2.3m1

http://code.google.com/p/google-web-toolkit/source/detail?r=9907

Modified:
  
/releases/2.3/user/src/com/google/gwt/requestfactory/server/LocatorServiceLayer.java
  
/releases/2.3/user/src/com/google/gwt/requestfactory/server/ResolverServiceLayer.java
  
/releases/2.3/user/src/com/google/gwt/requestfactory/server/ServiceLayer.java
  
/releases/2.3/user/src/com/google/gwt/requestfactory/server/ServiceLayerDecorator.java
  
/releases/2.3/user/test/com/google/gwt/requestfactory/RequestFactoryJreSuite.java
  
/releases/2.3/user/test/com/google/gwt/requestfactory/RequestFactorySuite.java
  
/releases/2.3/user/test/com/google/gwt/requestfactory/shared/ServiceInheritanceTest.java


===
---  
/releases/2.3/user/src/com/google/gwt/requestfactory/server/LocatorServiceLayer.java	 
Tue Mar 22 11:45:18 2011
+++  
/releases/2.3/user/src/com/google/gwt/requestfactory/server/LocatorServiceLayer.java	 
Fri Mar 25 07:04:13 2011

@@ -56,7 +56,7 @@
 // Enclosing class may be a parent class, so invoke on service class
 Class? declaringClass = contextMethod.getDeclaringClass();
 Class? serviceClass =
-getTop().resolveServiceClass((Class? extends RequestContext)  
declaringClass);
+ 
getTop().resolveServiceClass(declaringClass.asSubclass(RequestContext.class));

 return locator.getInstance(serviceClass);
   }

===
---  
/releases/2.3/user/src/com/google/gwt/requestfactory/server/ResolverServiceLayer.java	 
Tue Mar 22 11:45:18 2011
+++  
/releases/2.3/user/src/com/google/gwt/requestfactory/server/ResolverServiceLayer.java	 
Fri Mar 25 07:04:13 2011

@@ -119,7 +119,7 @@
   public Method resolveDomainMethod(Method requestContextMethod) {
 Class? declaringClass = requestContextMethod.getDeclaringClass();
 Class? searchIn =
-getTop().resolveServiceClass((Class? extends RequestContext)  
declaringClass);
+ 
getTop().resolveServiceClass(declaringClass.asSubclass(RequestContext.class));

 Class?[] parameterTypes = requestContextMethod.getParameterTypes();
 Class?[] domainArgs = new Class?[parameterTypes.length];
 for (int i = 0, j = domainArgs.length; i  j; i++) {
===
---  
/releases/2.3/user/test/com/google/gwt/requestfactory/RequestFactorySuite.java	 
Fri Jan 14 04:27:42 2011
+++  
/releases/2.3/user/test/com/google/gwt/requestfactory/RequestFactorySuite.java	 
Fri Mar 25 07:04:13 2011

@@ -26,6 +26,7 @@
 import com.google.gwt.requestfactory.shared.BoxesAndPrimitivesTest;
 import com.google.gwt.requestfactory.shared.ComplexKeysTest;
 import com.google.gwt.requestfactory.shared.LocatorTest;
+import com.google.gwt.requestfactory.shared.ServiceInheritanceTest;

 import junit.framework.Test;

@@ -46,6 +47,7 @@
 suite.addTestSuite(RequestFactoryExceptionPropagationTest.class);
 suite.addTestSuite(RequestFactoryPolymorphicTest.class);
 suite.addTestSuite(RequestFactoryUnicodeEscapingTest.class);
+suite.addTestSuite(ServiceInheritanceTest.class);
 return suite;
   }
 }
===
---  
/releases/2.3/user/test/com/google/gwt/requestfactory/shared/ServiceInheritanceTest.java	 
Tue Mar 22 11:45:18 2011
+++  
/releases/2.3/user/test/com/google/gwt/requestfactory/shared/ServiceInheritanceTest.java	 
Fri Mar 25 07:04:13 2011

@@ -26,21 +26,17 @@
 public class ServiceInheritanceTest extends GWTTestCase {

   /**
-   * Generic locator returns an instance of the class named in
-   * the @{@link Service} annotation
+   * ServiceLocator that returns the base class or subclass implementation
+   * specified in the @{@link Service} annotation.
*/
-  public static class AnyServiceLocator implements ServiceLocator {
-
-@Override
+  public static class SumServiceLocator implements ServiceLocator {
 public Object getInstance(Class? clazz) {
-  assertTrue(BaseImpl.class.isAssignableFrom(clazz));
-  try {
-return clazz.newInstance();
-  } catch (InstantiationException e) {
-throw new RuntimeException(e);
-  } catch (IllegalAccessException e) {
-throw new RuntimeException(e);
-  }
+  if (BaseImpl.class.equals(clazz)) {
+return new BaseImpl();
+  } else if (SubclassImpl.class.equals(clazz)) {
+return new SubclassImpl();
+  }
+  return null;
 }
   }

@@ -48,42 +44,63 @@
* The factory under test.
*/
   protected interface Factory extends RequestFactory {
-SumService sumContext();
-SumServiceBase sumBaseContext();
+SumServiceBase baseContext();
+SumServiceSub subContext();
   }

   /**
-   * Specifies a service which extends a base class
+   * Specifies the base class implementation
*/
-  @Service(value = SubclassImpl.class, locator = AnyServiceLocator.class)
-  interface SumService extends

[gwt-contrib] Fixes issue 6189. WebAppCreator was not scanning war/WEB-INF/lib to fill up .classpath (issue1393801)


Reviewers: jlabanca,

Description:
Fixes issue 6189. WebAppCreator was not scanning war/WEB-INF/lib to fill
up .classpath


Please review this at http://gwt-code-reviews.appspot.com/1393801/

Affected files:
  M user/src/com/google/gwt/user/tools/WebAppCreator.java
  M user/test/com/google/gwt/user/tools/WebAppCreatorTest.java


Index: user/src/com/google/gwt/user/tools/WebAppCreator.java
===
--- user/src/com/google/gwt/user/tools/WebAppCreator.java   (revision 9906)
+++ user/src/com/google/gwt/user/tools/WebAppCreator.java   (working copy)
@@ -542,7 +542,7 @@
 replacements.put(@copyServletDeps, copyServletDeps);

 // Collect the list of server libs to include on the eclipse classpath.
-File libDirectory = new File(outDir + warFolder + WEB-INF/lib);
+File libDirectory = new File(outDir + / + warFolder  
+ /WEB-INF/lib);

 StringBuilder serverLibs = new StringBuilder();
 if (libDirectory.exists()) {
   for (File file : libDirectory.listFiles()) {
Index: user/test/com/google/gwt/user/tools/WebAppCreatorTest.java
===
--- user/test/com/google/gwt/user/tools/WebAppCreatorTest.java	(revision  
9906)
+++ user/test/com/google/gwt/user/tools/WebAppCreatorTest.java	(working  
copy)

@@ -15,6 +15,7 @@
  */
 package com.google.gwt.user.tools;

+import com.google.gwt.dev.util.Util;
 import com.google.gwt.user.tools.WebAppCreator.ArgProcessor;

 import junit.framework.TestCase;
@@ -265,6 +266,32 @@
   }

   /**
+   * Generate a .classpath containing a .jar in war/WEB-INF/lib
+   */
+  public void testCreatorInlyEclipseWithJars() throws IOException,  
WebAppCreatorException {

+runCreator(-out, projectFolder, -XnoEclipse, -junit, mockJar,
+MY_PROJECT);
+
+String libDir = war + File.separatorChar
++ WEB-INF + File.separatorChar
++ lib;
+assertTrue(new File(projectFolder + File.separatorChar +  
libDir).mkdirs());

+
+String libJarName = libDir + File.separatorChar + foo.jar;
+File libFile = new File(projectFolder + File.separatorChar +  
libJarName);

+assertTrue(libFile.createNewFile());
+
+runCreator(-out, projectFolder, -XonlyEclipse, -junit, mockJar,
+MY_PROJECT);
+
+assertFileExists(.classpath);
+File classpathFile = new File(projectFolder + File.separatorChar  
+ .classpath);
+String classpathContents =  
Util.readURLAsString(classpathFile.toURI().toURL());

+assertTrue(.classpath does not contain  + libJarName,
+classpathContents.contains(libJarName));
+  }
+
+  /**
* Test the main method.
*/
   public void testMain() {


--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Public (issue1394801)


Reviewers: jlabanca,

Description:
Public
Fixes new project template to use 'clean' style. Issue 6208.


Please review this at http://gwt-code-reviews.appspot.com/1394801/

Affected files:
  M  
user/src/com/google/gwt/user/tools/templates/sample/_srcFolder_/_moduleFolder_/_moduleShortName_.gwt.xmlsrc



Index:  
user/src/com/google/gwt/user/tools/templates/sample/_srcFolder_/_moduleFolder_/_moduleShortName_.gwt.xmlsrc

===
---  
user/src/com/google/gwt/user/tools/templates/sample/_srcFolder_/_moduleFolder_/_moduleShortName_.gwt.xmlsrc	 
(revision 9906)
+++  
user/src/com/google/gwt/user/tools/templates/sample/_srcFolder_/_moduleFolder_/_moduleShortName_.gwt.xmlsrc	 
(working copy)

@@ -6,7 +6,7 @@
   !-- Inherit the default GWT style sheet.  You can change   --
   !-- the theme of your GWT application by uncommenting  --
   !-- any one of the following lines.--
-  inherits name='com.google.gwt.user.theme.standard.Standard'/
+  inherits name='com.google.gwt.user.theme.clean.Clean'/
   !-- inherits name='com.google.gwt.user.theme.chrome.Chrome'/ --
   !-- inherits name='com.google.gwt.user.theme.dark.Dark'/ --



--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: Fixes issue 6189. WebAppCreator was not scanning war/WEB-INF/lib to fill up .classpath (issue1393801)


http://gwt-code-reviews.appspot.com/1393801/

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: Public (issue1394801)


http://gwt-code-reviews.appspot.com/1394801/

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: Supress errors when building the Type Oracle. (issue1385810)


http://gwt-code-reviews.appspot.com/1385810/

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: Public (issue1394801)



http://gwt-code-reviews.appspot.com/1394801/diff/1/user/src/com/google/gwt/user/tools/templates/sample/_srcFolder_/_moduleFolder_/_moduleShortName_.gwt.xmlsrc
File
user/src/com/google/gwt/user/tools/templates/sample/_srcFolder_/_moduleFolder_/_moduleShortName_.gwt.xmlsrc
(right):

http://gwt-code-reviews.appspot.com/1394801/diff/1/user/src/com/google/gwt/user/tools/templates/sample/_srcFolder_/_moduleFolder_/_moduleShortName_.gwt.xmlsrc#newcode9
user/src/com/google/gwt/user/tools/templates/sample/_srcFolder_/_moduleFolder_/_moduleShortName_.gwt.xmlsrc:9:
inherits name='com.google.gwt.user.theme.clean.Clean'/
On 2011/03/29 18:27:03, jlabanca wrote:

The standard version should be commented out below the Clean version.


Done.

http://gwt-code-reviews.appspot.com/1394801/

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: Supress errors when building the Type Oracle. (issue1385810)

Updated patch addresses issue of logging errors in DevMode, updated
JavaToJavaScriptCompiler to re-use the reportErrors() method and
incorporated most feedback.

http://gwt-code-reviews.appspot.com/1385810/diff/1/dev/core/src/com/google/gwt/dev/javac/CompilationStateBuilder.java
File dev/core/src/com/google/gwt/dev/javac/CompilationStateBuilder.java
(right):

http://gwt-code-reviews.appspot.com/1385810/diff/1/dev/core/src/com/google/gwt/dev/javac/CompilationStateBuilder.java#newcode257
dev/core/src/com/google/gwt/dev/javac/CompilationStateBuilder.java:257:
for (CompilationUnit unit : resultUnits) {
On 2011/03/28 22:25:41, jbrosenberg wrote:

DEBUG or TRACE? The warning message suggests setting logLevel to

DEBUG, but the

actual logging here is for TRACE?

That's a little tricky. Errors are by default logged at
TreeLogger.ERROR and warnings are logged at TreeLogger.ERROR. Passing
the 'errorLogLevel' as TreeLogger.TRACE prints warnings as
TreeLogger.DEBUG, so to see them all, you need to set the level to
DEBUG.

I changed the third parameter and documented reportErrors() more
clearly.

http://gwt-code-reviews.appspot.com/1385810/diff/1/dev/core/src/com/google/gwt/dev/javac/CompilationStateBuilder.java#newcode367
dev/core/src/com/google/gwt/dev/javac/CompilationStateBuilder.java:367:
}
On 2011/03/28 22:46:33, scottb wrote:

Seems kinda.. ugh... how often do we need to report a problem in a

context where

we don't have access to the real compilation unit map?

The only caller to ProblemReporter who is passing a null map is
ConditionWhenTypeAssignable... hardly seems worth the extra effort of

exposing

the entire cached unit map.

I understand you don't want to expose the internals of the cache, but
this is pretty comparable to what we already do in
CompilationState.getCompilationUnitMap().

What do you think we should do in this case? So just swallow the
errors? I don't think we'll win any thanks for that. I may have
identified a second place where we need to invoke
logMissingTypeWithHints() with no access to CompilationState in
TypeMap.java (there are numerous reports of ICE's from this point in the
code that aren't really broken compilers, just missing source code.)

http://gwt-code-reviews.appspot.com/1385810/diff/1/dev/core/src/com/google/gwt/dev/javac/ProblemReporter.java
File dev/core/src/com/google/gwt/dev/javac/ProblemReporter.java (right):

http://gwt-code-reviews.appspot.com/1385810/diff/1/dev/core/src/com/google/gwt/dev/javac/ProblemReporter.java#newcode40
dev/core/src/com/google/gwt/dev/javac/ProblemReporter.java:40: public
class ProblemReporter {
On 2011/03/28 22:46:33, scottb wrote:

Wonder if we should pick a different name, JDT has a ProblemReporter

also, just

to confuse things.

Renamed to CompilationProblemReporter.

http://gwt-code-reviews.appspot.com/1385810/diff/1/dev/core/src/com/google/gwt/dev/javac/ProblemReporter.java#newcode47
dev/core/src/com/google/gwt/dev/javac/ProblemReporter.java:47:
ListCompilationUnit toVisit = new ArrayListCompilationUnit();
On 2011/03/28 22:46:33, scottb wrote:

This should be a LinkedList/Queue if you need to remove from the

front.

Thanks - Done.

http://gwt-code-reviews.appspot.com/1385810/diff/1/dev/core/src/com/google/gwt/dev/javac/ProblemReporter.java#newcode53
dev/core/src/com/google/gwt/dev/javac/ProblemReporter.java:53:
On 2011/03/28 22:25:41, jbrosenberg wrote:

javadoc?

Done.

http://gwt-code-reviews.appspot.com/1385810/diff/1/dev/core/src/com/google/gwt/dev/javac/ProblemReporter.java#newcode124
dev/core/src/com/google/gwt/dev/javac/ProblemReporter.java:124: * {@link
com.google.gwt.dev.shell.CompilingClassLoader}
On 2011/03/28 22:46:33, scottb wrote:

period

Done.

http://gwt-code-reviews.appspot.com/1385810/diff/1/dev/core/src/com/google/gwt/dev/javac/ProblemReporter.java#newcode138
dev/core/src/com/google/gwt/dev/javac/ProblemReporter.java:138: URL
sourceURL = Util.findSourceInClassPath(cl, missingType);
On 2011/03/28 22:46:33, scottb wrote:

Only called from this class, move here?

I think that method might be of general use. Everything else I moved is
related specifically to error reporting.

http://gwt-code-reviews.appspot.com/1385810/diff/1/dev/core/src/com/google/gwt/dev/javac/ProblemReporter.java#newcode157
dev/core/src/com/google/gwt/dev/javac/ProblemReporter.java:157: // For
Object in particular, there's a special warning.
On 2011/03/28 22:25:41, jbrosenberg wrote:

Is this comment here for Object, or for java.lang in general? Or is

it for the

else if clause above? Above we have a message for anything that's

not

java.lang.Object, and below we have something for anything that's

java.lang.*.

Can it be a bit more clear, or maybe more comments to explain?

My change was to add the one line at the top of the function.

I agree, the comments don't seem to reflect the code. I'll update them.

[gwt-contrib] Re: Adds support for the CSS_ATTRIBUTE_START parse context to HtmlTemplateParser. (issue1392801)


I think the entire attribute state could be useful, and I think thats a
good reason to use state booleans instead of more enums.  The number of
enums could grow exponentially (CSS_ATTRIBUTE_START_ENTIRE?).

Since this is implementation code and we can change it as needed, I'll
LGTM if you want to submit this version.  We can refactor later if
needed.


On 2011/03/29 18:49:04, xtof wrote:

FWIW, I think I will also want to add another state that indicates

that the

template variable comprises the entire attribute, for URLs (i.e. img
src={0}, but not img src={0}/{1}).  So there will be at least

one more

state coming.



On Tue, Mar 29, 2011 at 11:37, Christoph Kern mailto:x...@google.com

wrote:


 I could do that. One concern I have though with this approach is

that the

 state exposed in HtmlContext would get more complicated.  Right now,

code

 using the parser can simply do a switch statement over the contexts

(see

 SafeHtmlTemplatesImplMethodCreator#emitParameterExpression); if we

make the

 state multi-dimensional, client code would likely get more

complicated.


 What do you think?


 On Tue, Mar 29, 2011 at 11:26, mailto:jlaba...@google.com wrote:

 Knowing that you are at the start of a context type seems generally
 useful for all context types, including TEXT/ATTRIBUTE/URL (we

already

 have URL_START).

 Instead of adding CSS_ATTRIBUTE_START, would it be better to add
 HtmlContext#isStart(), or event HtmlContext#getValueIndex()?


 http://gwt-code-reviews.appspot.com/1392801/





http://gwt-code-reviews.appspot.com/1392801/

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: Public (issue1394801)


LGTM

http://gwt-code-reviews.appspot.com/1394801/

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: ie9 bug fixes. (issue1383809)


LGTM

As long as you put a more description comment in the IE9 tabIndex impl


http://gwt-code-reviews.appspot.com/1383809/diff/4001/user/src/com/google/gwt/dom/client/DOMImplIE9.java
File user/src/com/google/gwt/dom/client/DOMImplIE9.java (right):

http://gwt-code-reviews.appspot.com/1383809/diff/4001/user/src/com/google/gwt/dom/client/DOMImplIE9.java#newcode67
user/src/com/google/gwt/dom/client/DOMImplIE9.java:67: return
elem.tabIndex  65535 ? elem.tabIndex : -(elem.tabIndex % 65535) - 1;
The comment doesn't really explain what this is doing.  If IE9 returns
negative values for elem.tabIndex, wouldn't elem.tabIndex  65535 always
be true?

http://gwt-code-reviews.appspot.com/1383809/

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: Adds support for the CSS_ATTRIBUTE_START parse context to HtmlTemplateParser. (issue1392801)


http://gwt-code-reviews.appspot.com/1392801/

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: Adds support for the CSS_ATTRIBUTE_START parse context to HtmlTemplateParser. (issue1392801)


LGTM

I don't actually see the TODO though.  You can add it before submitting.

On 2011/03/29 19:28:17, xtof wrote:

On Tue, Mar 29, 2011 at 12:14, mailto:jlaba...@google.com wrote:



 I think the entire attribute state could be useful, and I think

thats a

 good reason to use state booleans instead of more enums.  The number

of

 enums could grow exponentially (CSS_ATTRIBUTE_START_ENTIRE?).

Right.




 Since this is implementation code and we can change it as needed,

I'll

 LGTM if you want to submit this version.  We can refactor later if
 needed.

 Sounds good.  I added TODO(xtof): Consider refactoring such that

state

related to the position of the template variable in an attribute is

exposed

separately (as HtmlContext#isAttributeStart(), etc). In doing so,

consider

trade off between combinatorial explosion of possible states vs

complexity

of client code.



Sound reasonable?





 On 2011/03/29 18:49:04, xtof wrote:

 FWIW, I think I will also want to add another state that indicates

 that the

 template variable comprises the entire attribute, for URLs (i.e.

img

 src={0}, but not img src={0}/{1}).  So there will be at least

 one more

 state coming.


  On Tue, Mar 29, 2011 at 11:37, Christoph Kern

mailto:x...@google.com


 wrote:

   I could do that. One concern I have though with this approach is

 that the

  state exposed in HtmlContext would get more complicated.  Right

now,


 code

  using the parser can simply do a switch statement over the

contexts


 (see

  SafeHtmlTemplatesImplMethodCreator#emitParameterExpression); if

we


 make the

  state multi-dimensional, client code would likely get more

 complicated.

 
  What do you think?

 
 
  On Tue, Mar 29, 2011 at 11:26, mailto:jlaba...@google.com

wrote:

 
  Knowing that you are at the start of a context type seems

generally

  useful for all context types, including TEXT/ATTRIBUTE/URL (we

 already

  have URL_START).
 
  Instead of adding CSS_ATTRIBUTE_START, would it be better to add
  HtmlContext#isStart(), or event HtmlContext#getValueIndex()?
 
 
  http://gwt-code-reviews.appspot.com/1392801/
 
 
 


 http://gwt-code-reviews.appspot.com/1392801/





http://gwt-code-reviews.appspot.com/1392801/

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: Adds support for the CSS_ATTRIBUTE_START parse context to HtmlTemplateParser. (issue1392801)

2011-03-29 Thread Christoph Kern

On Tue, Mar 29, 2011 at 12:14, jlaba...@google.com wrote:

 I think the entire attribute state could be useful, and I think thats a
 good reason to use state booleans instead of more enums.  The number of
 enums could grow exponentially (CSS_ATTRIBUTE_START_ENTIRE?).

Right.


 Since this is implementation code and we can change it as needed, I'll
 LGTM if you want to submit this version.  We can refactor later if
 needed.

 Sounds good.  I added TODO(xtof): Consider refactoring such that state
related to the position of the template variable in an attribute is exposed
separately (as HtmlContext#isAttributeStart(), etc). In doing so, consider
trade off between combinatorial explosion of possible states vs complexity
of client code.

Sound reasonable?



 On 2011/03/29 18:49:04, xtof wrote:

 FWIW, I think I will also want to add another state that indicates

 that the

 template variable comprises the entire attribute, for URLs (i.e. img
 src={0}, but not img src={0}/{1}).  So there will be at least

 one more

 state coming.


  On Tue, Mar 29, 2011 at 11:37, Christoph Kern mailto:x...@google.com

 wrote:

   I could do that. One concern I have though with this approach is

 that the

  state exposed in HtmlContext would get more complicated.  Right now,

 code

  using the parser can simply do a switch statement over the contexts

 (see

  SafeHtmlTemplatesImplMethodCreator#emitParameterExpression); if we

 make the

  state multi-dimensional, client code would likely get more

 complicated.

 
  What do you think?

 
 
  On Tue, Mar 29, 2011 at 11:26, mailto:jlaba...@google.com wrote:
 
  Knowing that you are at the start of a context type seems generally
  useful for all context types, including TEXT/ATTRIBUTE/URL (we

 already

  have URL_START).
 
  Instead of adding CSS_ATTRIBUTE_START, would it be better to add
  HtmlContext#isStart(), or event HtmlContext#getValueIndex()?
 
 
  http://gwt-code-reviews.appspot.com/1392801/
 
 
 


 http://gwt-code-reviews.appspot.com/1392801/


-- 
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] [google-web-toolkit] r9908 committed - Fixes issue 6189. WebAppCreator was not scanning war/WEB-INF/lib to fi...


Revision: 9908
Author:   rchan...@google.com
Date: Tue Mar 29 09:58:14 2011
Log:  Fixes issue 6189. WebAppCreator was not scanning war/WEB-INF/lib  
to fill up .classpath


Review at http://gwt-code-reviews.appspot.com/1393801

Review by: jlaba...@google.com
http://code.google.com/p/google-web-toolkit/source/detail?r=9908

Modified:
 /trunk/user/src/com/google/gwt/user/tools/WebAppCreator.java
 /trunk/user/test/com/google/gwt/user/tools/WebAppCreatorTest.java

===
--- /trunk/user/src/com/google/gwt/user/tools/WebAppCreator.java	Mon Mar  7  
08:21:08 2011
+++ /trunk/user/src/com/google/gwt/user/tools/WebAppCreator.java	Tue Mar 29  
09:58:14 2011

@@ -542,7 +542,7 @@
 replacements.put(@copyServletDeps, copyServletDeps);

 // Collect the list of server libs to include on the eclipse classpath.
-File libDirectory = new File(outDir + warFolder + WEB-INF/lib);
+File libDirectory = new File(outDir + / + warFolder  
+ /WEB-INF/lib);

 StringBuilder serverLibs = new StringBuilder();
 if (libDirectory.exists()) {
   for (File file : libDirectory.listFiles()) {
===
--- /trunk/user/test/com/google/gwt/user/tools/WebAppCreatorTest.java	Mon  
Mar  7 08:21:08 2011
+++ /trunk/user/test/com/google/gwt/user/tools/WebAppCreatorTest.java	Tue  
Mar 29 09:58:14 2011

@@ -15,6 +15,7 @@
  */
 package com.google.gwt.user.tools;

+import com.google.gwt.dev.util.Util;
 import com.google.gwt.user.tools.WebAppCreator.ArgProcessor;

 import junit.framework.TestCase;
@@ -263,6 +264,32 @@
 assertFileDoesNotExist(test/com/foo/HelloJUnit.gwt.xml);
 assertFileDoesNotExist(test/com/foo/client/HelloTest.java);
   }
+
+  /**
+   * Generate a .classpath containing a .jar in war/WEB-INF/lib
+   */
+  public void testCreatorOnlyEclipseWithJars() throws IOException,  
WebAppCreatorException {

+runCreator(-out, projectFolder, -XnoEclipse, -junit, mockJar,
+MY_PROJECT);
+
+String libDir = war + File.separatorChar
++ WEB-INF + File.separatorChar
++ lib;
+assertTrue(new File(projectFolder + File.separatorChar +  
libDir).mkdirs());

+
+String libJarName = libDir + File.separatorChar + foo.jar;
+File libFile = new File(projectFolder + File.separatorChar +  
libJarName);

+assertTrue(libFile.createNewFile());
+
+runCreator(-out, projectFolder, -XonlyEclipse, -junit, mockJar,
+MY_PROJECT);
+
+assertFileExists(.classpath);
+File classpathFile = new File(projectFolder + File.separatorChar  
+ .classpath);
+String classpathContents =  
Util.readURLAsString(classpathFile.toURI().toURL());

+assertTrue(.classpath does not contain  + libJarName,
+classpathContents.contains(libJarName));
+  }

   /**
* Test the main method.

--
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)


Thanks for your changes!

There's one more thing:  In the past couple of days we've had some
discussions around the in URL valued attribute context.

The main issue discussed is that unlike SafeHtml, SafeUri is not
generally composable:  Two SafeHtmls concatenated together make another
SafeHtml, but for SafeUri this doesn't really work.
For example,
String attackerControlled = evil();
SafeUri uri0 = UriUtils.fromTrustedString(javascript:void(0););
SafeUri uri1 = UriUtils.fromString(attackerControlled);

sanitizeUri will leave attackerControlled alone (it looks like a
relative URL), and both uri0 and uri1 are individually harmless.
However, concatenated together, the resulting URI will execute the
attacker controlled script when dereferenced.

With that in mind, it seems safest to only allow SafeUri to be
interpolated in a uri-valued attribute if the template variable makes up
the whole attribute.  I.e.,

@Template(img src='{0}'/)
SafeHtml image(SafeUri uri);

would be allowed, but
@Template(img src='{0}/{1}'/)
SafeHtml image(SafeUri baseUri, String path);
would not.

If that sounds reasonable to you too, I'll make a change to expose a
corresponding parse state in the parser (ENTIRE_URL_ATTRIBUTE).

Thanks!
--xtof






http://gwt-code-reviews.appspot.com/1380806/diff/5003/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/5003/user/src/com/google/gwt/user/client/ui/Image.java#newcode145
user/src/com/google/gwt/user/client/ui/Image.java:145:
impl.createStructure(UriUtils.fromTrustedString(url), left, top, width,
height));
I'm a bit concerned about the use of UriUtils.fromTrustedString here --
this code can't really guarantee that url is trusted, it may come from
the Image(String url, ...) c'tor.

Perhaps this could be made a bit cleaner as follows:

- use SafeUri internally in ClippedState
- introduce UriUtils#fromUntrustedString, with the same implementation
as fromTrustedString, but documented to be used as an unsafe cast from
String to SafeUri, to be used to support legacy APIs.
- Implement the  public Image(String url,...) constructors in terms of
the Image(SafeUri url, ...) ones, using fromUntrustedString.

(note, I haven't completely thought this through and doing so might end
up making the change more messy rather than cleaner; if that's the case
please ignore this suggestion :)

http://gwt-code-reviews.appspot.com/1380806/

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: Supress errors when building the Type Oracle. (issue1385810)

2011-03-29 Thread scottb

CompilationProblemReporter isn't showing up for me in patch set 2.

http://gwt-code-reviews.appspot.com/1385810/diff/1/dev/core/src/com/google/gwt/dev/javac/CompilationStateBuilder.java
File dev/core/src/com/google/gwt/dev/javac/CompilationStateBuilder.java
(right):

I understand you don't want to expose the internals of the cache, but

this is

pretty comparable to what we already do in
CompilationState.getCompilationUnitMap().

I wouldn't say it's comparable. This requires you to blow through an
additional layer of abstraction, making unrelated pieces of the system
coupled, for no better reason than that there happens to be a
statically-accessible instance of CompilationStateBuilder. But you
can't really ensure that the active compilation state has anything at
all to do with the cache you're hitting, except in practice. I could
imagine how this could lead to wrong results in a future scenario where
pieces of the GWT SDK are running in an IDE.

What do you think we should do in this case? So just swallow the

errors? I

don't think we'll win any thanks for that.

Well, I pushed back because I wasn't sure how important the detailed
dependency trace actually was for ConditionWhenTypeAssignable. If it
really is very important, I think the better option would be to modify
DeferredBindingQuery to make the compilationState accessible (instead of
just the TypeOracle). CompilationState can be gotten from
StandardGeneratorContext from Rule.isApplicable().

Either that, or thread the error reporting logic through TypeOracle
itself such that querying TypeOracle for a type which has been removed
by reason of errors will itself generate the appropriate error logging
-- but I think that's more than you probably want to bite off right now.

I may have identified a second
place where we need to invoke logMissingTypeWithHints() with no access

CompilationState in TypeMap.java (there are numerous reports of ICE's

from this

point in the code that aren't really broken compilers, just missing

source

code.)

I think we might want to dig into that problem independently of this CL.
The J2JS compiler runs its own JDT compile and does its own error
reporting, supposedly independent of the CompState/TypeOracle build. I
have a hard time imagining how anything you're doing in this patch would
cause ICEs to come out of TypeMap by reason of missing source code. If
that's already a problem, I think we need to figure out why.

http://gwt-code-reviews.appspot.com/1385810/diff/1/dev/core/src/com/google/gwt/dev/javac/ProblemReporter.java
File dev/core/src/com/google/gwt/dev/javac/ProblemReporter.java (right):

http://gwt-code-reviews.appspot.com/1385810/diff/1/dev/core/src/com/google/gwt/dev/javac/ProblemReporter.java#newcode179
dev/core/src/com/google/gwt/dev/javac/ProblemReporter.java:179: if
(Util.isCompilationUnitOnDisk(location)) {
Good question. My desire to move it is partially based on a desire to
rework this method entirely. This code is super old, it predates
CompilationStateBuilder by a lot, and I've been wanting to remove
unit.getSource() for ages (the blocker there is WebModeCompiler uses
it).

What I'd really like to do is make GeneratedCompilationUnit default
access, do an instanceof GeneratedCompilationUnit, and from there do
something specific to GCU/GeneratedUnit. I wouldn't wish for you to do
all that in this patch, but going ahead and moving over
Util.isCompilationUnitOnDisk sets the stage for killing it.

http://gwt-code-reviews.appspot.com/1385810/diff/1/dev/core/src/com/google/gwt/dev/javac/ProblemReporter.java#newcode248
dev/core/src/com/google/gwt/dev/javac/ProblemReporter.java:248:
TreeLogger.Type warnLogLevel;
boolean sounds good, my suggestion was meant to be tongue-in-cheek. :)

http://gwt-code-reviews.appspot.com/1385810/diff/6001/dev/core/src/com/google/gwt/dev/GWTShell.java
File dev/core/src/com/google/gwt/dev/GWTShell.java (right):

http://gwt-code-reviews.appspot.com/1385810/diff/6001/dev/core/src/com/google/gwt/dev/GWTShell.java#newcode202
dev/core/src/com/google/gwt/dev/GWTShell.java:202:
persistentCacheDir.mkdirs();
LGTM, but submit by itself? Or is the race condition related to this
patch?

http://gwt-code-reviews.appspot.com/1385810/diff/6001/dev/core/src/com/google/gwt/dev/jjs/JavaToJavaScriptCompiler.java
File dev/core/src/com/google/gwt/dev/jjs/JavaToJavaScriptCompiler.java
(right):

[gwt-contrib] Re: Adds support for the CSS_ATTRIBUTE_START parse context to HtmlTemplateParser. (issue1392801)


On 2011/03/29 19:30:16, jlabanca wrote:

LGTM



I don't actually see the TODO though.  You can add it before

submitting.

Oops, forgot to export git before uploading.  Done.

Thanks!
--xtof

http://gwt-code-reviews.appspot.com/1392801/

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: Adds support for the CSS_ATTRIBUTE_START parse context to HtmlTemplateParser. (issue1392801)


http://gwt-code-reviews.appspot.com/1392801/

--
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)

2011-03-29 Thread t . broyer


I totally agree with your analysis and ENTIRE_URL_ATTRIBUTE state (I had
similar concerns, but couldn't find a use case where it would be
problematic: I didn't think about the javascript: URLs; and there
actually are similar issues with data: URLs).

Should I wait for the ENTIRE_URL_ATTRIBUTE state (I'll probably only
have time this week-end, so maybe you'll have it before I come back to
this code, but just in case), or go with the other changes and let you
do that last change (should only be a matter of adding a warning/error
in SafeHtmlTemplatesImplMethodCreator#emitParameterExpression and remove
the check and sanitization in #emitAttributeContextParameterExpression)


http://gwt-code-reviews.appspot.com/1380806/diff/5003/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/5003/user/src/com/google/gwt/user/client/ui/Image.java#newcode145
user/src/com/google/gwt/user/client/ui/Image.java:145:
impl.createStructure(UriUtils.fromTrustedString(url), left, top, width,
height));
On 2011/03/29 20:11:11, xtof wrote:

I'm a bit concerned about the use of UriUtils.fromTrustedString here


To tell the truth, me too!


-- this code can't really guarantee that url is trusted,
it may come from the Image(String url, ...) c'tor.


Yes, it's mostly required for backwards compatibility with the 'unsafe'
API.


Perhaps this could be made a bit cleaner as follows:



- use SafeUri internally in ClippedState


I had an iteration in my local repo with such a change, then thought
that the fromTrustedString could maybe be inlined by the compiler, while
I suppose it wouldn't be possible with the SafeUri.


- introduce UriUtils#fromUntrustedString, with the same implementation

as

fromTrustedString, but documented to be used as an unsafe cast from

String to

SafeUri, to be used to support legacy APIs.


Nice idea!


- Implement the  public Image(String url,...) constructors in terms of

the

Image(SafeUri url, ...) ones, using fromUntrustedString.


I'm really not sure about that last one, as the getUrl would still have
to be a String for backwards compat', and the DOM-level accesses are all
String-based too.


(note, I haven't completely thought this through and doing so might

end up

making the change more messy rather than cleaner; if that's the case

please

ignore this suggestion :)


I'll give it a try ;-)

http://gwt-code-reviews.appspot.com/1380806/

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: Issue 5700 (issue1388804)

2011-03-29 Thread Ray Ryan

On Fri, Mar 25, 2011 at 7:26 AM, akito.noz...@gmail.com wrote:

 I have a simple question on one of the test. As I was making this
 correction I noticed that one of my test case comment is wrong.

 The last remove test is actually incorrect (my comment). My question is
 what is the expected output of the last test. If you have 2 nested event
 bus, and if you call removeHandlers on the inner eventbus. Do you expect
 this remove to propagate to the outer event bus and remove it from its
 registration?


I really hadn't thought about nesting the things before your patch came in.
You'll note that there is no support for nesting among activities in
general.

With the current setup, the semantics seem okay. Am I reading it right?

   - I nest innerBus in outerBus
   - I register innerHandler and outerHandler on them, respectively
   - If I call innerBus.removeHandlers(), innerHandler will no longer
   receive events
   - If I instead call outerBus.removeHandlers(), neither handler will
   receive events any longer

The only problem is that the HandlerRegistrations vended by InnerResettable
cannot be gc'd until removeHandlers is called on both InnerResettable and
OuterResettable.

If that's the case, I think this implementation is fine, if we javadoc
carefully. I think your test shouldn't be about the specific intermediate
count on outerBus, but rather focus that both busses get to zero when their
removes are called.

Am I answering the right question?

rjrjr


 If the answer is no it shouldn't propagate to the upper eventbus. The
 current setup will work nicely. The only weird thing is that the upper
 Resettable event bus thinks the event is still attached.

 If the answer is yes and the eventbus should be removed from the top. I
 came up with few ways to deal with the situation with pros and cons.
 1. Recognize that you are wrapping a resettable event bus and don't
 record the registrationHandler at this level (simple pass through). On
 removeHandlers call removeHandhlers on the wrapped bus.
 * Pros: No new memory leak introduced.
 * Cons: If other wrapper is used this method will not work correctly.
 For example ResettableEventBus -CountingEventBus - ResettableEventBus
 - SimpleEventBus.

 2. Create a event that fires when things are removed from simple event
 bus (RemoveEvent of some sort).
 * Pros: Will work regardless of how complicated the nesting gets.
 * Cons: How do we know when to unregister our ResettableEventBus from
 this event? Simple solution is to unregister when nothing is left in the
 registration and add it back when something gets attached. Also more
 complex. Should user be allowed to attach to this event?

 Number 2 is definitely the best solution but rises many questions...

 Comment/Suggestion?


 http://gwt-code-reviews.appspot.com/1388804/



-- 
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)

2011-03-29 Thread Christoph Kern

On Tue, Mar 29, 2011 at 13:30, t.bro...@gmail.com wrote:

 I totally agree with your analysis and ENTIRE_URL_ATTRIBUTE state (I had
 similar concerns, but couldn't find a use case where it would be
 problematic: I didn't think about the javascript: URLs; and there
 actually are similar issues with data: URLs).

Yes, indeed.



 Should I wait for the ENTIRE_URL_ATTRIBUTE state (I'll probably only
 have time this week-end, so maybe you'll have it before I come back to
 this code, but just in case), or go with the other changes and let you
 do that last change (should only be a matter of adding a warning/error
 in SafeHtmlTemplatesImplMethodCreator#emitParameterExpression and remove
 the check and sanitization in #emitAttributeContextParameterExpression)

 I'll try to send out this change today.  If possible, it would be
preferable to constrain SafeUri template parameters from the beginning,
otherwise it'll introduce a breaking change later on.




 http://gwt-code-reviews.appspot.com/1380806/diff/5003/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/5003/user/src/com/google/gwt/user/client/ui/Image.java#newcode145
 user/src/com/google/gwt/user/client/ui/Image.java:145:
 impl.createStructure(UriUtils.fromTrustedString(url), left, top, width,
 height));
 On 2011/03/29 20:11:11, xtof wrote:

 I'm a bit concerned about the use of UriUtils.fromTrustedString here


 To tell the truth, me too!

  -- this code can't really guarantee that url is trusted,

 it may come from the Image(String url, ...) c'tor.


 Yes, it's mostly required for backwards compatibility with the 'unsafe'
 API.


  Perhaps this could be made a bit cleaner as follows:


  - use SafeUri internally in ClippedState


 I had an iteration in my local repo with such a change, then thought
 that the fromTrustedString could maybe be inlined by the compiler, while
 I suppose it wouldn't be possible with the SafeUri.

 True.  I'm not sure how much of a concern this is, I'd let rjrjr or
jlabanca comment on that.


  - introduce UriUtils#fromUntrustedString, with the same implementation

 as

 fromTrustedString, but documented to be used as an unsafe cast from

 String to

 SafeUri, to be used to support legacy APIs.


 Nice idea!


  - Implement the  public Image(String url,...) constructors in terms of

 the

 Image(SafeUri url, ...) ones, using fromUntrustedString.


 I'm really not sure about that last one, as the getUrl would still have
 to be a String for backwards compat', and the DOM-level accesses are all
 String-based too.

 I see.  Since ClippedState is strictly internal, it probably doesn't make a
big difference.  I think for the time being it would also be fine to leave
as it is currently, but replace fromTrustedString with fromUntrustedString
and add some comments about what's going on.  Just to avoid the impression
that the code guarantees something that it really can't guarantee.


  (note, I haven't completely thought this through and doing so might

 end up

 making the change more messy rather than cleaner; if that's the case

 please

 ignore this suggestion :)


 I'll give it a try ;-)


 http://gwt-code-reviews.appspot.com/1380806/


-- 
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: Issue 1054 (issue1380807)

2011-03-29 Thread zhuyi


http://gwt-code-reviews.appspot.com/1380807/

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: Issue 1054 (issue1380807)

2011-03-29 Thread zhuyi



http://gwt-code-reviews.appspot.com/1380807/diff/1001/user/src/com/google/gwt/user/rebind/rpc/FieldSerializerCreator.java
File user/src/com/google/gwt/user/rebind/rpc/FieldSerializerCreator.java
(right):

http://gwt-code-reviews.appspot.com/1380807/diff/1001/user/src/com/google/gwt/user/rebind/rpc/FieldSerializerCreator.java#newcode469
user/src/com/google/gwt/user/rebind/rpc/FieldSerializerCreator.java:469:
if (true.equals(Shared.shouldSerializeFinalFields())) {
On 2011/03/24 12:46:01, bobv wrote:

Use a enum instead of string comparisons.


Done.

http://gwt-code-reviews.appspot.com/1380807/diff/1001/user/src/com/google/gwt/user/server/rpc/impl/SerializabilityUtil.java
File
user/src/com/google/gwt/user/server/rpc/impl/SerializabilityUtil.java
(right):

http://gwt-code-reviews.appspot.com/1380807/diff/1001/user/src/com/google/gwt/user/server/rpc/impl/SerializabilityUtil.java#newcode276
user/src/com/google/gwt/user/server/rpc/impl/SerializabilityUtil.java:276:
 !field.getDeclaringClass().toString().contains(java.lang.Enum);
On 2011/03/24 12:46:01, bobv wrote:

The use of toString() for any purpose other than debugging is an

anti-pattern.


!field.getDeclaringClass().equals(Enum.class)


Done.

http://gwt-code-reviews.appspot.com/1380807/diff/1001/user/test/com/google/gwt/user/client/rpc/FinalFieldsTest.java
File user/test/com/google/gwt/user/client/rpc/FinalFieldsTest.java
(right):

http://gwt-code-reviews.appspot.com/1380807/diff/1001/user/test/com/google/gwt/user/client/rpc/FinalFieldsTest.java#newcode2
user/test/com/google/gwt/user/client/rpc/FinalFieldsTest.java:2: *
Copyright 2010 Google Inc.
On 2011/03/24 12:46:01, bobv wrote:

Update copyright dates in new files.


Done.

http://gwt-code-reviews.appspot.com/1380807/diff/1001/user/test/com/google/gwt/user/client/rpc/FinalFieldsTest.java#newcode35
user/test/com/google/gwt/user/client/rpc/FinalFieldsTest.java:35:
service.transfer_object(node, new AsyncCallback() {
On 2011/03/24 12:46:01, bobv wrote:

Don't use a raw type for these callbacks.


Done.

http://gwt-code-reviews.appspot.com/1380807/diff/1001/user/test/com/google/gwt/user/client/rpc/FinalFieldsTestFalseNoWarn.java
File
user/test/com/google/gwt/user/client/rpc/FinalFieldsTestFalseNoWarn.java
(right):

http://gwt-code-reviews.appspot.com/1380807/diff/1001/user/test/com/google/gwt/user/client/rpc/FinalFieldsTestFalseNoWarn.java#newcode39
user/test/com/google/gwt/user/client/rpc/FinalFieldsTestFalseNoWarn.java:39:
public void onSuccess(Object result) {
On 2011/03/24 12:46:01, bobv wrote:

Verify the values of the unserialized final fields in the object.


Done.

http://gwt-code-reviews.appspot.com/1380807/diff/1001/user/test/com/google/gwt/user/client/rpc/FinalFieldsTestService.java
File
user/test/com/google/gwt/user/client/rpc/FinalFieldsTestService.java
(right):

http://gwt-code-reviews.appspot.com/1380807/diff/1001/user/test/com/google/gwt/user/client/rpc/FinalFieldsTestService.java#newcode20
user/test/com/google/gwt/user/client/rpc/FinalFieldsTestService.java:20:
* TODO: document me.
On 2011/03/24 12:46:01, bobv wrote:

Do so.


Done.

http://gwt-code-reviews.appspot.com/1380807/diff/1001/user/test/com/google/gwt/user/client/rpc/FinalFieldsTestService.java#newcode46
user/test/com/google/gwt/user/client/rpc/FinalFieldsTestService.java:46:
FinalFieldsNode transfer_object(FinalFieldsNode node);
On 2011/03/24 12:46:01, bobv wrote:

Use camel-cased method names.


Done.

http://gwt-code-reviews.appspot.com/1380807/

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Recursively creates directories for the persistent unit cache. (issue1395801)


Reviewers: tobyr, scottb,

Description:
Recursively creates directories for the persistent unit cache.


Please review this at http://gwt-code-reviews.appspot.com/1395801/

Affected files:
  M dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java


Index: dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java
===
--- dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java	 
(revision 9908)
+++ dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java	(working  
copy)

@@ -295,7 +295,7 @@
 logger.log(TreeLogger.TRACE, Persistent unit cache dir set to: 
 + this.cacheDirectory.getAbsolutePath());

-if (!cacheDirectory.isDirectory()  !cacheDirectory.mkdir()) {
+if (!cacheDirectory.isDirectory()  !cacheDirectory.mkdirs()) {
   logger.log(TreeLogger.ERROR, Unable to initialize cache. Couldn't  
create directory 

   + cacheDirectory.getAbsolutePath() + .);
   throw new UnableToCompleteException();


--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: Fixes issue 6189. WebAppCreator was not scanning war/WEB-INF/lib to fill up .classpath (issue1393801)


Submitted as r9908
On 2011/03/29 18:25:08, rchandia wrote:

http://gwt-code-reviews.appspot.com/1393801/diff/1/user/test/com/google/gwt/user/tools/WebAppCreatorTest.java

File user/test/com/google/gwt/user/tools/WebAppCreatorTest.java

(right):


http://gwt-code-reviews.appspot.com/1393801/diff/1/user/test/com/google/gwt/user/tools/WebAppCreatorTest.java#newcode271

user/test/com/google/gwt/user/tools/WebAppCreatorTest.java:271: public

void

testCreatorInlyEclipseWithJars() throws IOException,

WebAppCreatorException {

On 2011/03/29 18:19:43, jlabanca wrote:
 testCreatorInlyEclipseWithJars = testCreatorOnlyEclipseWithJars



Done.




http://gwt-code-reviews.appspot.com/1393801/

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: Public (issue1394801)


Submitted as r9908
On 2011/03/29 19:15:10, jlabanca wrote:

LGTM




http://gwt-code-reviews.appspot.com/1394801/

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: Recursively creates directories for the persistent unit cache. (issue1395801)

2011-03-29 Thread scottb


LGTM

http://gwt-code-reviews.appspot.com/1395801/

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] [google-web-toolkit] r9909 committed - Public...


Revision: 9909
Author:   rchan...@google.com
Date: Tue Mar 29 11:24:05 2011
Log:  Public
Fixes new project template to use 'clean' style. Issue 6208.

Review at http://gwt-code-reviews.appspot.com/1394801

Review by: jlaba...@google.com
http://code.google.com/p/google-web-toolkit/source/detail?r=9909

Modified:
  
/trunk/user/src/com/google/gwt/user/tools/templates/sample/_srcFolder_/_moduleFolder_/_moduleShortName_.gwt.xmlsrc


===
---  
/trunk/user/src/com/google/gwt/user/tools/templates/sample/_srcFolder_/_moduleFolder_/_moduleShortName_.gwt.xmlsrc	 
Mon Mar  7 08:21:08 2011
+++  
/trunk/user/src/com/google/gwt/user/tools/templates/sample/_srcFolder_/_moduleFolder_/_moduleShortName_.gwt.xmlsrc	 
Tue Mar 29 11:24:05 2011

@@ -6,7 +6,8 @@
   !-- Inherit the default GWT style sheet.  You can change   --
   !-- the theme of your GWT application by uncommenting  --
   !-- any one of the following lines.--
-  inherits name='com.google.gwt.user.theme.standard.Standard'/
+  inherits name='com.google.gwt.user.theme.clean.Clean'/
+  !-- inherits name='com.google.gwt.user.theme.standard.Standard'/ --
   !-- inherits name='com.google.gwt.user.theme.chrome.Chrome'/ --
   !-- inherits name='com.google.gwt.user.theme.dark.Dark'/ --

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Add tests of nesting semantics for ResettableEventBus (issue1395802)

2011-03-29 Thread rjrjr


Reviewers: jlabanca,

Description:
Add tests of nesting semantics for ResettableEventBus


Please review this at http://gwt-code-reviews.appspot.com/1395802/

Affected files:
  M user/test/com/google/gwt/event/shared/ResettableEventBusTest.java


Index: user/test/com/google/gwt/event/shared/ResettableEventBusTest.java
===
--- user/test/com/google/gwt/event/shared/ResettableEventBusTest.java	 
(revision 9908)
+++ user/test/com/google/gwt/event/shared/ResettableEventBusTest.java	 
(working copy)

@@ -58,4 +58,62 @@
 });
 assertNotFired(mouse1, mouse2, mouse3);
   }
+
+  public void testNestedResetInnerFirst() {
+CountingEventBus wrapped = new CountingEventBus();
+ResettableEventBus wideScope = new ResettableEventBus(wrapped);
+ResettableEventBus narrowScope = new ResettableEventBus(wideScope);
+
+TypeMouseDownHandler type = MouseDownEvent.getType();
+
+wideScope.addHandler(type, mouse1);
+narrowScope.addHandler(type, mouse2);
+
+wrapped.fireEvent(new MouseDownEvent() {
+});
+assertFired(mouse1, mouse2);
+
+reset();
+
+/*
+ * When I remove handlers from the narrow resettable, it should have  
no effect

+ * on handlers registered with the wider instance.
+ */
+
+narrowScope.removeHandlers();
+
+wrapped.fireEvent(new MouseDownEvent() {
+});
+assertFired(mouse1);
+assertNotFired(mouse2);
+  }
+
+  public void testNestedResetOuterFirst() {
+CountingEventBus wrapped = new CountingEventBus();
+ResettableEventBus wideScope = new ResettableEventBus(wrapped);
+ResettableEventBus narrowScope = new ResettableEventBus(wideScope);
+
+TypeMouseDownHandler type = MouseDownEvent.getType();
+
+wideScope.addHandler(type, mouse1);
+narrowScope.addHandler(type, mouse2);
+
+wrapped.fireEvent(new MouseDownEvent() {
+});
+assertFired(mouse1, mouse2);
+
+reset();
+
+/*
+ * When I remove handlers from the first resettable, handlers  
registered

+ * by the narrower scoped one that wraps it should also be severed.
+ */
+
+wideScope.removeHandlers();
+
+wrapped.fireEvent(new MouseDownEvent() {
+});
+assertNotFired(mouse1);
+assertNotFired(mouse2);
+  }
 }


--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] [google-web-toolkit] r9910 committed - Recursively creates directories for the persistent unit cache....


Revision: 9910
Author:   zun...@google.com
Date: Tue Mar 29 11:29:33 2011
Log:  Recursively creates directories for the persistent unit cache.

Review at http://gwt-code-reviews.appspot.com/1395801

http://code.google.com/p/google-web-toolkit/source/detail?r=9910

Modified:
 /trunk/dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java

===
--- /trunk/dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java	 
Mon Mar 28 11:28:56 2011
+++ /trunk/dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java	 
Tue Mar 29 11:29:33 2011

@@ -295,7 +295,7 @@
 logger.log(TreeLogger.TRACE, Persistent unit cache dir set to: 
 + this.cacheDirectory.getAbsolutePath());

-if (!cacheDirectory.isDirectory()  !cacheDirectory.mkdir()) {
+if (!cacheDirectory.isDirectory()  !cacheDirectory.mkdirs()) {
   logger.log(TreeLogger.ERROR, Unable to initialize cache. Couldn't  
create directory 

   + cacheDirectory.getAbsolutePath() + .);
   throw new UnableToCompleteException();

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Roll-back due to test breakage. (issue1396801)


Reviewers: pdr,

Description:
Roll-back due to test breakage.


Please review this at http://gwt-code-reviews.appspot.com/1396801/

Affected files:
  M user/src/com/google/gwt/user/tools/WebAppCreator.java
  M user/test/com/google/gwt/user/tools/WebAppCreatorTest.java


Index: user/src/com/google/gwt/user/tools/WebAppCreator.java
===
--- user/src/com/google/gwt/user/tools/WebAppCreator.java   (revision 9910)
+++ user/src/com/google/gwt/user/tools/WebAppCreator.java   (working copy)
@@ -542,7 +542,7 @@
 replacements.put(@copyServletDeps, copyServletDeps);

 // Collect the list of server libs to include on the eclipse classpath.
-File libDirectory = new File(outDir + / + warFolder  
+ /WEB-INF/lib);

+File libDirectory = new File(outDir + warFolder + WEB-INF/lib);
 StringBuilder serverLibs = new StringBuilder();
 if (libDirectory.exists()) {
   for (File file : libDirectory.listFiles()) {
Index: user/test/com/google/gwt/user/tools/WebAppCreatorTest.java
===
--- user/test/com/google/gwt/user/tools/WebAppCreatorTest.java	(revision  
9910)
+++ user/test/com/google/gwt/user/tools/WebAppCreatorTest.java	(working  
copy)

@@ -15,7 +15,6 @@
  */
 package com.google.gwt.user.tools;

-import com.google.gwt.dev.util.Util;
 import com.google.gwt.user.tools.WebAppCreator.ArgProcessor;

 import junit.framework.TestCase;
@@ -266,32 +265,6 @@
   }

   /**
-   * Generate a .classpath containing a .jar in war/WEB-INF/lib
-   */
-  public void testCreatorOnlyEclipseWithJars() throws IOException,  
WebAppCreatorException {

-runCreator(-out, projectFolder, -XnoEclipse, -junit, mockJar,
-MY_PROJECT);
-
-String libDir = war + File.separatorChar
-+ WEB-INF + File.separatorChar
-+ lib;
-assertTrue(new File(projectFolder + File.separatorChar +  
libDir).mkdirs());

-
-String libJarName = libDir + File.separatorChar + foo.jar;
-File libFile = new File(projectFolder + File.separatorChar +  
libJarName);

-assertTrue(libFile.createNewFile());
-
-runCreator(-out, projectFolder, -XonlyEclipse, -junit, mockJar,
-MY_PROJECT);
-
-assertFileExists(.classpath);
-File classpathFile = new File(projectFolder + File.separatorChar  
+ .classpath);
-String classpathContents =  
Util.readURLAsString(classpathFile.toURI().toURL());

-assertTrue(.classpath does not contain  + libJarName,
-classpathContents.contains(libJarName));
-  }
-
-  /**
* Test the main method.
*/
   public void testMain() {


--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: Roll-back due to test breakage. (issue1396801)

2011-03-29 Thread pdr


On 2011/03/29 21:54:47, rchandia wrote:

LGTM

http://gwt-code-reviews.appspot.com/1396801/

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: Add tests of nesting semantics for ResettableEventBus (issue1395802)


LGTM

http://gwt-code-reviews.appspot.com/1395802/

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] [google-web-toolkit] r9911 committed - Roll-back due to test breakage....


Revision: 9911
Author:   rchan...@google.com
Date: Tue Mar 29 12:09:40 2011
Log:  Roll-back due to test breakage.

Review at http://gwt-code-reviews.appspot.com/1396801

Review by: p...@google.com
http://code.google.com/p/google-web-toolkit/source/detail?r=9911

Modified:
 /trunk/user/src/com/google/gwt/user/tools/WebAppCreator.java
 /trunk/user/test/com/google/gwt/user/tools/WebAppCreatorTest.java

===
--- /trunk/user/src/com/google/gwt/user/tools/WebAppCreator.java	Tue Mar 29  
09:58:14 2011
+++ /trunk/user/src/com/google/gwt/user/tools/WebAppCreator.java	Tue Mar 29  
12:09:40 2011

@@ -542,7 +542,7 @@
 replacements.put(@copyServletDeps, copyServletDeps);

 // Collect the list of server libs to include on the eclipse classpath.
-File libDirectory = new File(outDir + / + warFolder  
+ /WEB-INF/lib);

+File libDirectory = new File(outDir + warFolder + WEB-INF/lib);
 StringBuilder serverLibs = new StringBuilder();
 if (libDirectory.exists()) {
   for (File file : libDirectory.listFiles()) {
===
--- /trunk/user/test/com/google/gwt/user/tools/WebAppCreatorTest.java	Tue  
Mar 29 09:58:14 2011
+++ /trunk/user/test/com/google/gwt/user/tools/WebAppCreatorTest.java	Tue  
Mar 29 12:09:40 2011

@@ -15,7 +15,6 @@
  */
 package com.google.gwt.user.tools;

-import com.google.gwt.dev.util.Util;
 import com.google.gwt.user.tools.WebAppCreator.ArgProcessor;

 import junit.framework.TestCase;
@@ -264,32 +263,6 @@
 assertFileDoesNotExist(test/com/foo/HelloJUnit.gwt.xml);
 assertFileDoesNotExist(test/com/foo/client/HelloTest.java);
   }
-
-  /**
-   * Generate a .classpath containing a .jar in war/WEB-INF/lib
-   */
-  public void testCreatorOnlyEclipseWithJars() throws IOException,  
WebAppCreatorException {

-runCreator(-out, projectFolder, -XnoEclipse, -junit, mockJar,
-MY_PROJECT);
-
-String libDir = war + File.separatorChar
-+ WEB-INF + File.separatorChar
-+ lib;
-assertTrue(new File(projectFolder + File.separatorChar +  
libDir).mkdirs());

-
-String libJarName = libDir + File.separatorChar + foo.jar;
-File libFile = new File(projectFolder + File.separatorChar +  
libJarName);

-assertTrue(libFile.createNewFile());
-
-runCreator(-out, projectFolder, -XonlyEclipse, -junit, mockJar,
-MY_PROJECT);
-
-assertFileExists(.classpath);
-File classpathFile = new File(projectFolder + File.separatorChar  
+ .classpath);
-String classpathContents =  
Util.readURLAsString(classpathFile.toURI().toURL());

-assertTrue(.classpath does not contain  + libJarName,
-classpathContents.contains(libJarName));
-  }

   /**
* Test the main method.

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: Add tests of nesting semantics for ResettableEventBus (issue1395802)

2011-03-29 Thread Ray Ryan

I wasn't trying to catch the bug, I was trying to illustrate the bits that
work already.

On Tue, Mar 29, 2011 at 3:18 PM, pjul...@gmail.com wrote:



 http://gwt-code-reviews.appspot.com/1395802/diff/1/user/test/com/google/gwt/event/shared/ResettableEventBusTest.java
 File user/test/com/google/gwt/event/shared/ResettableEventBusTest.java
 (right):


 http://gwt-code-reviews.appspot.com/1395802/diff/1/user/test/com/google/gwt/event/shared/ResettableEventBusTest.java#newcode83
 user/test/com/google/gwt/event/shared/ResettableEventBusTest.java:83:
 narrowScope.removeHandlers();
 With respect, I think these tests miss the bug completely.
 ResettableEventBus has registrations which is a set.

 If you're calling removeHandlers, you're not triggering the bug because
 this blindly clears the registrations set.  To trigger the bug, you
 need to save the HandlerRegistration object that was returned to you.
 If you call removeHandler() on the HandlerRegistration object, you'll
 find that nesting doesn't work properly.


 http://gwt-code-reviews.appspot.com/1395802/


-- 
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: Supress errors when building the Type Oracle. (issue1385810)


http://gwt-code-reviews.appspot.com/1385810/

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: Supress errors when building the Type Oracle. (issue1385810)


Updated patch.  sorry about the missing file in last patch.


http://gwt-code-reviews.appspot.com/1385810/diff/1/dev/core/src/com/google/gwt/dev/javac/ProblemReporter.java
File dev/core/src/com/google/gwt/dev/javac/ProblemReporter.java (right):

http://gwt-code-reviews.appspot.com/1385810/diff/1/dev/core/src/com/google/gwt/dev/javac/ProblemReporter.java#newcode179
dev/core/src/com/google/gwt/dev/javac/ProblemReporter.java:179: if
(Util.isCompilationUnitOnDisk(location)) {
On 2011/03/29 20:14:13, scottb wrote:

Good question.  My desire to move it is partially based on a desire to

rework

this method entirely.  This code is super old, it predates
CompilationStateBuilder by a lot, and I've been wanting to remove
unit.getSource() for ages (the blocker there is WebModeCompiler uses

it).


What I'd really like to do is make GeneratedCompilationUnit default

access, do

an instanceof GeneratedCompilationUnit, and from there do something

specific to

GCU/GeneratedUnit.  I wouldn't wish for you to do all that in this

patch, but

going ahead and moving over Util.isCompilationUnitOnDisk sets the

stage for

killing it.


Done.

http://gwt-code-reviews.appspot.com/1385810/diff/6001/dev/core/src/com/google/gwt/dev/GWTShell.java
File dev/core/src/com/google/gwt/dev/GWTShell.java (right):

http://gwt-code-reviews.appspot.com/1385810/diff/6001/dev/core/src/com/google/gwt/dev/GWTShell.java#newcode202
dev/core/src/com/google/gwt/dev/GWTShell.java:202:
persistentCacheDir.mkdirs();
On 2011/03/29 20:14:14, scottb wrote:

LGTM, but submit by itself?  Or is the race condition related to this

patch?

submitted in a separate review (and moved to PersistentUnitCache)

http://gwt-code-reviews.appspot.com/1385810/diff/6001/dev/core/src/com/google/gwt/dev/jjs/impl/TypeMap.java
File dev/core/src/com/google/gwt/dev/jjs/impl/TypeMap.java (right):

http://gwt-code-reviews.appspot.com/1385810/diff/6001/dev/core/src/com/google/gwt/dev/jjs/impl/TypeMap.java#newcode141
dev/core/src/com/google/gwt/dev/jjs/impl/TypeMap.java:141: //
CompilationProblemReporter.logMissingTypeErrorWithHints() here?
On 2011/03/29 20:14:14, scottb wrote:

As mentioned elsewhere, something is dreadfully wrong if missing

source code is

causing ICEs here.  I think we should tackle that independently and

not mix in

this CL.


Reverted file.

http://gwt-code-reviews.appspot.com/1385810/

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: Supress errors when building the Type Oracle. (issue1385810)


http://gwt-code-reviews.appspot.com/1385810/

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] [google-web-toolkit] r9912 committed - Adds support for the CSS_ATTRIBUTE_START parse context to HtmlTemplate...


Revision: 9912
Author:   x...@google.com
Date: Tue Mar 29 12:27:56 2011
Log:  Adds support for the CSS_ATTRIBUTE_START parse context to  
HtmlTemplateParser.


Review at http://gwt-code-reviews.appspot.com/1392801

http://code.google.com/p/google-web-toolkit/source/detail?r=9912

Modified:
 /trunk/user/src/com/google/gwt/safehtml/rebind/HtmlTemplateParser.java
 /trunk/user/src/com/google/gwt/safehtml/rebind/ParsedHtmlTemplate.java
 /trunk/user/test/com/google/gwt/safehtml/rebind/HtmlTemplateParserTest.java

===
--- /trunk/user/src/com/google/gwt/safehtml/rebind/HtmlTemplateParser.java	 
Thu Dec  9 08:34:53 2010
+++ /trunk/user/src/com/google/gwt/safehtml/rebind/HtmlTemplateParser.java	 
Tue Mar 29 12:27:56 2011

@@ -74,10 +74,14 @@
  * ddThis context corresponds to a parameter that appears at the very  
start of
  * a URL-valued HTML attribute's value; in the above example this applies  
to

  * parameter #1.
+ * dt{@link HtmlContext.Type#CSS_ATTRIBUTE_START}
+ * ddThis context corresponds to a parameter that appears at the very
+ * beginning of a {@code style} attribute's value; in the above example  
this

+ * applies to parameter #0.
  * dt{@link HtmlContext.Type#CSS_ATTRIBUTE}
  * ddThis context corresponds to a parameter that appears in the context  
of a

- * {@code style} attribute; in the above example this applies to
- * parameter #0.
+ * {@code style} attribute, except at the very beginning of the attribute's
+ * value.
  * dt{@link HtmlContext.Type#ATTRIBUTE_VALUE}
  * ddThis context corresponds to a parameter that appears within an  
attribute

  * and is not in one of the more specific in-attribute contexts above. In
@@ -210,7 +214,11 @@
*/
   private HtmlContext getHtmlContextFromParseState()
   throws UnableToCompleteException {
-
+// TODO(xtof): Consider refactoring such that state related to the  
position

+// of the template variable in an attribute is exposed separately (as
+// HtmlContext#isAttributeStart(), etc). In doing so, consider trade  
off

+// between combinatorial explosion of possible states vs. complexity of
+// client code.
 if (streamHtmlParser.getState().equals(HtmlParser.STATE_ERROR)) {
   logger.log(TreeLogger.ERROR,
   Parsing template resulted in parse error: 
@@ -250,7 +258,11 @@
   if (streamHtmlParser.isUrlStart()) {
 return new HtmlContext(HtmlContext.Type.URL_START, tag, attribute);
   } else if (streamHtmlParser.inCss()) {
-return new HtmlContext(HtmlContext.Type.CSS_ATTRIBUTE, tag,  
attribute);

+if (streamHtmlParser.getValueIndex() == 0) {
+  return new HtmlContext(HtmlContext.Type.CSS_ATTRIBUTE_START,  
tag, attribute);

+} else {
+  return new HtmlContext(HtmlContext.Type.CSS_ATTRIBUTE, tag,  
attribute);

+}
   } else {
 return new HtmlContext(
 HtmlContext.Type.ATTRIBUTE_VALUE, tag, attribute);
===
--- /trunk/user/src/com/google/gwt/safehtml/rebind/ParsedHtmlTemplate.java	 
Thu Dec  9 08:34:53 2010
+++ /trunk/user/src/com/google/gwt/safehtml/rebind/ParsedHtmlTemplate.java	 
Tue Mar 29 12:27:56 2011

@@ -64,7 +64,11 @@
   /**
* CSS (style) attribute context.
*/
-  CSS_ATTRIBUTE
+  CSS_ATTRIBUTE,
+  /**
+   * At the very start of a CSS (style) attribute context.
+   */
+  CSS_ATTRIBUTE_START
 }

 private final Type type;
===
---  
/trunk/user/test/com/google/gwt/safehtml/rebind/HtmlTemplateParserTest.java	 
Thu Dec  9 08:34:53 2010
+++  
/trunk/user/test/com/google/gwt/safehtml/rebind/HtmlTemplateParserTest.java	 
Tue Mar 29 12:27:56 2011

@@ -134,10 +134,15 @@
 // Test correct detection of CSS context.
 assertParseTemplateResult(
 [L(div class=\), P((ATTRIBUTE_VALUE,div,class),0), L(\  
style=\), 

-+ P((CSS_ATTRIBUTE,div,style),2), L(\Hello ), 
++ P((CSS_ATTRIBUTE_START,div,style),2), L(\Hello ), 
 + P((TEXT,null,null),1)],
 div class=\{0}\ style=\{2}\Hello {1});
 assertParseTemplateResult(
+[L(div class=\), P((ATTRIBUTE_VALUE,div,class),0), L(\  
style=\color:green; ), 

++ P((CSS_ATTRIBUTE,div,style),2), L(\Hello ), 
++ P((TEXT,null,null),1)],
+div class=\{0}\ style=\color:green; {2}\Hello {1});
+assertParseTemplateResult(
 [L(div), P((TEXT,null,null),0), L(stylefoo ), 
 + P((CSS,null,null),1), L(/style)],
 div{0}stylefoo {1}/style);

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] [google-web-toolkit] r9913 committed - Add tests of nesting semantics for ResettableEventBus...


Revision: 9913
Author:   rj...@google.com
Date: Tue Mar 29 12:31:22 2011
Log:  Add tests of nesting semantics for ResettableEventBus

Review at http://gwt-code-reviews.appspot.com/1395802

Review by: jlaba...@google.com
http://code.google.com/p/google-web-toolkit/source/detail?r=9913

Modified:
 /trunk/user/test/com/google/gwt/event/shared/ResettableEventBusTest.java

===
---  
/trunk/user/test/com/google/gwt/event/shared/ResettableEventBusTest.java	 
Fri Sep 10 17:56:09 2010
+++  
/trunk/user/test/com/google/gwt/event/shared/ResettableEventBusTest.java	 
Tue Mar 29 12:31:22 2011

@@ -58,4 +58,62 @@
 });
 assertNotFired(mouse1, mouse2, mouse3);
   }
-}
+
+  public void testNestedResetInnerFirst() {
+CountingEventBus wrapped = new CountingEventBus();
+ResettableEventBus wideScope = new ResettableEventBus(wrapped);
+ResettableEventBus narrowScope = new ResettableEventBus(wideScope);
+
+TypeMouseDownHandler type = MouseDownEvent.getType();
+
+wideScope.addHandler(type, mouse1);
+narrowScope.addHandler(type, mouse2);
+
+wrapped.fireEvent(new MouseDownEvent() {
+});
+assertFired(mouse1, mouse2);
+
+reset();
+
+/*
+ * When I remove handlers from the narrow resettable, it should have  
no effect

+ * on handlers registered with the wider instance.
+ */
+
+narrowScope.removeHandlers();
+
+wrapped.fireEvent(new MouseDownEvent() {
+});
+assertFired(mouse1);
+assertNotFired(mouse2);
+  }
+
+  public void testNestedResetOuterFirst() {
+CountingEventBus wrapped = new CountingEventBus();
+ResettableEventBus wideScope = new ResettableEventBus(wrapped);
+ResettableEventBus narrowScope = new ResettableEventBus(wideScope);
+
+TypeMouseDownHandler type = MouseDownEvent.getType();
+
+wideScope.addHandler(type, mouse1);
+narrowScope.addHandler(type, mouse2);
+
+wrapped.fireEvent(new MouseDownEvent() {
+});
+assertFired(mouse1, mouse2);
+
+reset();
+
+/*
+ * When I remove handlers from the first resettable, handlers  
registered

+ * by the narrower scoped one that wraps it should also be severed.
+ */
+
+wideScope.removeHandlers();
+
+wrapped.fireEvent(new MouseDownEvent() {
+});
+assertNotFired(mouse1);
+assertNotFired(mouse2);
+  }
+}

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: Add tests of nesting semantics for ResettableEventBus (issue1395802)

2011-03-29 Thread rjrjr


submitted at r9913

http://gwt-code-reviews.appspot.com/1395802/

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: Supress errors when building the Type Oracle. (issue1385810)

2011-03-29 Thread scottb


LGTM, just nits.


http://gwt-code-reviews.appspot.com/1385810/diff/8012/dev/core/src/com/google/gwt/dev/cfg/DeferredBindingQuery.java
File dev/core/src/com/google/gwt/dev/cfg/DeferredBindingQuery.java
(right):

http://gwt-code-reviews.appspot.com/1385810/diff/8012/dev/core/src/com/google/gwt/dev/cfg/DeferredBindingQuery.java#newcode49
dev/core/src/com/google/gwt/dev/cfg/DeferredBindingQuery.java:49:
TypeOracle typeOracle, String testType, CompilationState
compilationState) {
Nit, could remove the typeOracle param and field, since it's just
compState.getTypeOracle().

http://gwt-code-reviews.appspot.com/1385810/diff/8012/dev/core/src/com/google/gwt/dev/javac/CompilationProblemReporter.java
File
dev/core/src/com/google/gwt/dev/javac/CompilationProblemReporter.java
(right):

http://gwt-code-reviews.appspot.com/1385810/diff/8012/dev/core/src/com/google/gwt/dev/javac/CompilationProblemReporter.java#newcode74
dev/core/src/com/google/gwt/dev/javac/CompilationProblemReporter.java:74:
while (toVisit.peek() != null) {
!toVisit.isEmpty()

http://gwt-code-reviews.appspot.com/1385810/diff/8012/dev/core/src/com/google/gwt/dev/javac/CompilationProblemReporter.java#newcode107
dev/core/src/com/google/gwt/dev/javac/CompilationProblemReporter.java:107:
final MapString, CompilationUnit unitMap) {
Since this method and logMissingTypeErrorWithHints() are both public,
they should probably either both take a CompilationState or both take
the unit map. for consistentcy.

http://gwt-code-reviews.appspot.com/1385810/diff/8012/dev/core/src/com/google/gwt/dev/javac/CompilationProblemReporter.java#newcode163
dev/core/src/com/google/gwt/dev/javac/CompilationProblemReporter.java:163:
String typeName) {
private?

http://gwt-code-reviews.appspot.com/1385810/

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Add missing CSS_ATTRIBUTE_START case in SafeHtmlTemplates code generator. (issue1395803)


Reviewers: jlabanca, rjrjr, pdr,

Description:
Add missing CSS_ATTRIBUTE_START case in SafeHtmlTemplates code
generator.


Please review this at http://gwt-code-reviews.appspot.com/1395803/

Affected files:
  M  
user/src/com/google/gwt/safehtml/rebind/SafeHtmlTemplatesImplMethodCreator.java

  M user/test/com/google/gwt/safehtml/client/SafeHtmlTemplatesTest.java


Index:  
user/src/com/google/gwt/safehtml/rebind/SafeHtmlTemplatesImplMethodCreator.java

===
---  
user/src/com/google/gwt/safehtml/rebind/SafeHtmlTemplatesImplMethodCreator.java	 
(revision 9912)
+++  
user/src/com/google/gwt/safehtml/rebind/SafeHtmlTemplatesImplMethodCreator.java	 
(working copy)

@@ -245,6 +245,7 @@
 break;

   case CSS_ATTRIBUTE:
+  case CSS_ATTRIBUTE_START:
 // TODO(xtof): Improve support for CSS.
 logger.log(TreeLogger.WARN, Template with variable in CSS  
context: 
 + The template code generator cannot guarantee HTML-safety  
of 

Index: user/test/com/google/gwt/safehtml/client/SafeHtmlTemplatesTest.java
===
--- user/test/com/google/gwt/safehtml/client/SafeHtmlTemplatesTest.java	 
(revision 9912)
+++ user/test/com/google/gwt/safehtml/client/SafeHtmlTemplatesTest.java	 
(working copy)

@@ -62,6 +62,9 @@

 @Template(spanimg src=\{0}/{1}\//span)
 SafeHtml templateWithTwoPartUriAttribute(String baseUrl, String  
urlPart);

+
+@Template(span style='{0}; color: green;'/span)
+SafeHtml templateWithStyleAttribute(String style);
   }

   public void testSimpleTemplate() {
@@ -107,4 +110,10 @@
 templates.templateWithTwoPartUriAttribute(
 BAD_URL, xy).asString());
   }
+
+  public void testTemplateWithStyleAttribute() {
+Assert.assertEquals(
+span style='background: purple; color: green;'/span,
+templates.templateWithStyleAttribute(background:  
purple).asString());

+  }
 }


--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] [google-web-toolkit] r9914 committed - Add missing CSS_ATTRIBUTE_START case in SafeHtmlTemplates code generat...


Revision: 9914
Author:   x...@google.com
Date: Tue Mar 29 14:59:28 2011
Log:  Add missing CSS_ATTRIBUTE_START case in SafeHtmlTemplates code  
generator.


Review at: http://gwt-code-reviews.appspot.com/1395803

http://code.google.com/p/google-web-toolkit/source/detail?r=9914

Modified:
  
/trunk/user/src/com/google/gwt/safehtml/rebind/SafeHtmlTemplatesImplMethodCreator.java

 /trunk/user/test/com/google/gwt/safehtml/client/SafeHtmlTemplatesTest.java

===
---  
/trunk/user/src/com/google/gwt/safehtml/rebind/SafeHtmlTemplatesImplMethodCreator.java	 
Thu Mar  3 13:21:55 2011
+++  
/trunk/user/src/com/google/gwt/safehtml/rebind/SafeHtmlTemplatesImplMethodCreator.java	 
Tue Mar 29 14:59:28 2011

@@ -245,6 +245,7 @@
 break;

   case CSS_ATTRIBUTE:
+  case CSS_ATTRIBUTE_START:
 // TODO(xtof): Improve support for CSS.
 logger.log(TreeLogger.WARN, Template with variable in CSS  
context: 
 + The template code generator cannot guarantee HTML-safety  
of 

===
---  
/trunk/user/test/com/google/gwt/safehtml/client/SafeHtmlTemplatesTest.java	 
Thu Dec  9 08:34:53 2010
+++  
/trunk/user/test/com/google/gwt/safehtml/client/SafeHtmlTemplatesTest.java	 
Tue Mar 29 14:59:28 2011

@@ -62,6 +62,9 @@

 @Template(spanimg src=\{0}/{1}\//span)
 SafeHtml templateWithTwoPartUriAttribute(String baseUrl, String  
urlPart);

+
+@Template(span style='{0}; color: green;'/span)
+SafeHtml templateWithStyleAttribute(String style);
   }

   public void testSimpleTemplate() {
@@ -107,4 +110,10 @@
 templates.templateWithTwoPartUriAttribute(
 BAD_URL, xy).asString());
   }
-}
+
+  public void testTemplateWithStyleAttribute() {
+Assert.assertEquals(
+span style='background: purple; color: green;'/span,
+templates.templateWithStyleAttribute(background:  
purple).asString());

+  }
+}

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: Add missing CSS_ATTRIBUTE_START case in SafeHtmlTemplates code generator. (issue1395803)