[gwt-contrib] Re: Adds unit tests for extending JavaScriptObject. Tests a loosening of the (issue1369805)

[scottb]

LGTM


http://gwt-code-reviews.appspot.com/1369805/diff/1013/dev/core/test/com/google/gwt/dev/javac/JSORestrictionsTest.java
File dev/core/test/com/google/gwt/dev/javac/JSORestrictionsTest.java
(right):

http://gwt-code-reviews.appspot.com/1369805/diff/1013/dev/core/test/com/google/gwt/dev/javac/JSORestrictionsTest.java#newcode34
dev/core/test/com/google/gwt/dev/javac/JSORestrictionsTest.java:34: /*
Nah, I'm not even sure we need it anymore.  -XstartOnFirstThread was a
dev mode thing, maybe even a SWT thing.  I'm not sure it's even needed
anymore, much less for running a JUnit test that doesn't do any
graphics.  It might even be better to nuke it altogether instead of
merely moving it, but either way it doesn't belong here.

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

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

[gwt-contrib] Re: Adds unit tests for extending JavaScriptObject. Tests a loosening of the (issue1369805)

[zundel]

http://gwt-code-reviews.appspot.com/1369805/diff/1013/dev/core/test/com/google/gwt/dev/javac/JSORestrictionsTest.java
File dev/core/test/com/google/gwt/dev/javac/JSORestrictionsTest.java
(right):

http://gwt-code-reviews.appspot.com/1369805/diff/1013/dev/core/test/com/google/gwt/dev/javac/JSORestrictionsTest.java#newcode34
dev/core/test/com/google/gwt/dev/javac/JSORestrictionsTest.java:34: /*
On 2011/03/04 16:20:54, scottb wrote:

LG, but it might be better to move this to JavaCompilationSuite since

most of

the tests in that suite have the same issue.



Added it there, but wouldn't it be best to also leave it in
JSORestrictionsTest in case you run the test case standalone?

http://gwt-code-reviews.appspot.com/1369805/diff/1013/dev/core/test/com/google/gwt/dev/javac/JSORestrictionsTest.java#newcode96
dev/core/test/com/google/gwt/dev/javac/JSORestrictionsTest.java:96:
System.err.print(goodCode.toString());
On 2011/03/04 16:20:54, scottb wrote:

Debug statement


oops

http://gwt-code-reviews.appspot.com/1369805/diff/1013/dev/core/test/com/google/gwt/dev/javac/JSORestrictionsTest.java#newcode193
dev/core/test/com/google/gwt/dev/javac/JSORestrictionsTest.java:193:
.append("  public static class CommonBase extends JavaScriptObject
implements CommonInterface {\n");
On 2011/03/04 16:20:54, scottb wrote:

Style nit: probably better to break the string literals sooner than

have to

break the outer formatting.


Done.

http://gwt-code-reviews.appspot.com/1369805/diff/1013/dev/core/test/com/google/gwt/dev/javac/JSORestrictionsTest.java#newcode207
dev/core/test/com/google/gwt/dev/javac/JSORestrictionsTest.java:207:
System.err.print(goodCode.toString());
On 2011/03/04 16:20:54, scottb wrote:

Debug statement


Done.

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

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

[gwt-contrib] Re: Adds unit tests for extending JavaScriptObject. Tests a loosening of the (issue1369805)

[zundel]

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

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

[gwt-contrib] Re: Adds unit tests for extending JavaScriptObject. Tests a loosening of the (issue1369805)

[scottb]

LGTM w/ nits.  I think this is actually much better than the first
formulation, and has the added benefit of testing JsoRestrictionsChecker
directly.


http://gwt-code-reviews.appspot.com/1369805/diff/1013/dev/core/test/com/google/gwt/dev/javac/JSORestrictionsTest.java
File dev/core/test/com/google/gwt/dev/javac/JSORestrictionsTest.java
(right):

http://gwt-code-reviews.appspot.com/1369805/diff/1013/dev/core/test/com/google/gwt/dev/javac/JSORestrictionsTest.java#newcode34
dev/core/test/com/google/gwt/dev/javac/JSORestrictionsTest.java:34: /*
LG, but it might be better to move this to JavaCompilationSuite since
most of the tests in that suite have the same issue.

http://gwt-code-reviews.appspot.com/1369805/diff/1013/dev/core/test/com/google/gwt/dev/javac/JSORestrictionsTest.java#newcode96
dev/core/test/com/google/gwt/dev/javac/JSORestrictionsTest.java:96:
System.err.print(goodCode.toString());
Debug statement

http://gwt-code-reviews.appspot.com/1369805/diff/1013/dev/core/test/com/google/gwt/dev/javac/JSORestrictionsTest.java#newcode193
dev/core/test/com/google/gwt/dev/javac/JSORestrictionsTest.java:193:
.append("  public static class CommonBase extends JavaScriptObject
implements CommonInterface {\n");
Style nit: probably better to break the string literals sooner than have
to break the outer formatting.

http://gwt-code-reviews.appspot.com/1369805/diff/1013/dev/core/test/com/google/gwt/dev/javac/JSORestrictionsTest.java#newcode207
dev/core/test/com/google/gwt/dev/javac/JSORestrictionsTest.java:207:
System.err.print(goodCode.toString());
Debug statement

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

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

[gwt-contrib] Re: Adds unit tests for extending JavaScriptObject. Tests a loosening of the (issue1369805)

[zundel]

Scott suggested moving these tests to JSORestrictionsTest, which is not
just the right place, but by only testing from source, it side-steps the
JavaScriptObject bytecode issue that was causing such a huge pain.


http://gwt-code-reviews.appspot.com/1369805/diff/1013/dev/core/src/com/google/gwt/dev/javac/BytecodeSignatureMaker.java
File dev/core/src/com/google/gwt/dev/javac/BytecodeSignatureMaker.java
(right):

http://gwt-code-reviews.appspot.com/1369805/diff/1013/dev/core/src/com/google/gwt/dev/javac/BytecodeSignatureMaker.java#newcode193
dev/core/src/com/google/gwt/dev/javac/BytecodeSignatureMaker.java:193:
CompileDependencyVisitor v =
visitCompileDependenciesInBytecode(byteCode);
ooh, I couldn't resist this little refactor when I happened to glance at
this code.

http://gwt-code-reviews.appspot.com/1369805/diff/1013/dev/core/test/com/google/gwt/dev/javac/JSORestrictionsTest.java
File dev/core/test/com/google/gwt/dev/javac/JSORestrictionsTest.java
(right):

http://gwt-code-reviews.appspot.com/1369805/diff/1013/dev/core/test/com/google/gwt/dev/javac/JSORestrictionsTest.java#newcode34
dev/core/test/com/google/gwt/dev/javac/JSORestrictionsTest.java:34: /*
updated as per scottb's OOB suggestion

http://gwt-code-reviews.appspot.com/1369805/diff/1013/dev/core/test/com/google/gwt/dev/javac/JSORestrictionsTest.java#newcode249
dev/core/test/com/google/gwt/dev/javac/JSORestrictionsTest.java:249:
removed locally

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

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

[gwt-contrib] Re: Adds unit tests for extending JavaScriptObject. Tests a loosening of the (issue1369805)

[zundel]

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

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

[gwt-contrib] Re: Adds unit tests for extending JavaScriptObject. Tests a loosening of the (issue1369805)

[scottb]

On Thu, Mar 3, 2011 at 3:27 PM,  wrote:

This works out great for TypeOracleMediatorFromByteCodeTest, but when

we

compile from source in TypeOracleMediatorFromSourceTest, the place we
need to get in to modify the Type Oracle instance is nested inside of
CompilationState which is also nested inside of

CompilationStateBuilder.

 That means disruption to these other two classes.  Seems like the

test

would be even more intrusive if I did that - what do you think?


I have a solution for you!

In TypeOracleMediatorFromSourceTest.buildTypeOracle(), you have a chance
to mess with the type oracle before you run the checks.  So here is
where you'd inject your call to
typeOracle.setupHackyAlternateJsoClassForTesting(...).

Have the implementation of setupHackyAlternateJsoClassForTesting() do a
jsoSingleImpls.clear(), computeSingleJsoImplData(allTypes.values()).

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

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

[gwt-contrib] Re: Adds unit tests for extending JavaScriptObject. Tests a loosening of the (issue1369805)

[scottb]

On Thu, Mar 3, 2011 at 3:27 PM,  wrote:

This works out great for TypeOracleMediatorFromByteCodeTest, but when

we

compile from source in TypeOracleMediatorFromSourceTest, the place we
need to get in to modify the Type Oracle instance is nested inside of
CompilationState which is also nested inside of

CompilationStateBuilder.

 That means disruption to these other two classes.  Seems like the

test

would be even more intrusive if I did that - what do you think?


I have a solution for you!

In TypeOracleMediatorFromSourceTest.buildTypeOracle(), you have a chance
to mess with the type oracle before you run the checks.  So here is
where you'd inject your call to
typeOracle.setupHackyAlternateJsoClassForTesting(...).

Have the implementation of setupHackyAlternateJsoClassForTesting() do


http://gwt-code-reviews.appspot.com/1369805/diff/3006/dev/core/test/com/google/gwt/dev/javac/TypeOracleMediatorTestBase.java
File
dev/core/test/com/google/gwt/dev/javac/TypeOracleMediatorTestBase.java
(right):

http://gwt-code-reviews.appspot.com/1369805/diff/3006/dev/core/test/com/google/gwt/dev/javac/TypeOracleMediatorTestBase.java#newcode219
dev/core/test/com/google/gwt/dev/javac/TypeOracleMediatorTestBase.java:219:
// caching strategy that involves the TypeOracle.
Use multiline comment.

http://gwt-code-reviews.appspot.com/1369805/diff/3006/dev/core/test/com/google/gwt/dev/javac/mediatortest/JavaScriptObjectCommonInterfaceImpl.java
File
dev/core/test/com/google/gwt/dev/javac/mediatortest/JavaScriptObjectCommonInterfaceImpl.java
(right):

http://gwt-code-reviews.appspot.com/1369805/diff/3006/dev/core/test/com/google/gwt/dev/javac/mediatortest/JavaScriptObjectCommonInterfaceImpl.java#newcode1
dev/core/test/com/google/gwt/dev/javac/mediatortest/JavaScriptObjectCommonInterfaceImpl.java:1:
package com.google.gwt.dev.javac.mediatortest;
Oh yeah, don't forget copy headers.

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

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

[gwt-contrib] Re: Adds unit tests for extending JavaScriptObject. Tests a loosening of the (issue1369805)

[zundel]

Sigh: None of these alternative options are panning out for me

On 2011/03/03 15:05:21, zundel wrote:

After sleeping on the static rogue setter, I don't like it much either

and

came up with some alternatives:



1) Would it be more palatable to turn setJavaScriptObjectClass() it

into an

instance method?  That way, we wouldn't have to reset the static

variable

each time in tearDown and the override would last for only one

instance of a

TypeOracle.  That is the simplest thing I can think of.


This works out great for TypeOracleMediatorFromByteCodeTest, but when we
compile from source in TypeOracleMediatorFromSourceTest, the place we
need to get in to modify the Type Oracle instance is nested inside of
CompilationState which is also nested inside of CompilationStateBuilder.
 That means disruption to these other two classes.  Seems like the test
would be even more intrusive if I did that - what do you think?


2) We could make the setter just add to a static list of alternative

class

names to be treated like JavaScriptObject.  That would incur a slight
run-time penalty in computeSingleJsoImplData() to check against a list

of

strings instead of just a single one.  But then we wouldn't have to

worry

about resetting the name.


This one would still work, but has the unpalatable runtime penalty.



3) Do you think it would be feasable to taking
com.google.gwt...mediatortest.JavaScriptObject.class object's bytecode

and

re-write the package using ASM?


This would work great for JavaScriptObject itself, but not references in
the several derived classes we create and compile into binary in the
mediatortest packages.  We'd have to rewrite the JavaScriptObject
references in those classes too.


4) You suggested squirreling away a copy of
com/google/gwt/core/client/JavaScriptObject.class somewhere on the

classpath

for the gwt-dev project and using it as the source for the bytecode.

This

would require some minor refactoring of CheckedJavaResource and

subclasses

in TypeOracleMediatorTest.  To me, this is kind of obscure black magic
(comparted to how the rest of the tests are implemented.)


Same problem as #3, we could handle JavaScriptObject that way, but we'd
also have to handle all the derived classes the same way so that they
derived from com.google.gwt.core.client.JavaScriptObject.

No wonder there were no tests for this logic before :(



On Wed, Mar 2, 2011 at 10:02 PM, Scott Blum 

wrote:


> On Wed, Mar 2, 2011 at 7:21 PM, Eric Ayers

 wrote:

>
>> @Scott: I was not planning to revert the diff cheese. This uses the
>> recently updated official gwt-format.xml autoformatting for

Eclipse.  I've

>> already volunteered to go through and run the autoformatter to

bring

>> existing source up to date.
>
>
> Right, I'm just making the point that it would be great if you could

land

> that first, so that when you submit this change, it won't mix the

formatting

> & semantic changes into one CL.
>
> Per our face to face, it'd also be good if you could leave a TODO in
> TypeOracle to remove the rogue setter once
> TypeOracleMediatorFromByteCodeTest is gone.
>





--
Eric Z. Ayers
Google Web Toolkit, Atlanta, GA USA




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

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

[gwt-contrib] Re: Adds unit tests for extending JavaScriptObject. Tests a loosening of the (issue1369805)

[zundel]

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

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

[gwt-contrib] Re: Adds unit tests for extending JavaScriptObject. Tests a loosening of the (issue1369805)

[Scott Blum]

On Thu, Mar 3, 2011 at 1:47 PM,  wrote:

>
>
> http://gwt-code-reviews.appspot.com/1369805/diff/3001/dev/core/test/com/google/gwt/dev/javac/TypeOracleMediatorTestBase.java
> File
> dev/core/test/com/google/gwt/dev/javac/TypeOracleMediatorTestBase.java
> (right):
>
>
> http://gwt-code-reviews.appspot.com/1369805/diff/3001/dev/core/test/com/google/gwt/dev/javac/TypeOracleMediatorTestBase.java#newcode218
> dev/core/test/com/google/gwt/dev/javac/TypeOracleMediatorTestBase.java:218:
> null, getByteCode(aClass), System.currentTimeMillis());
> On 2011/03/03 17:36:26, zundel wrote:
>
>> On 2011/03/03 17:14:07, scottb wrote:
>> > Unrelated, but shouldn't this be getLastModified() instead of
>>
> current time?
>
>  I dunno, do you want it to use caching?
>>
>
> Its intended to represent the timestamp of the source file or class file
> resource as you mention, but in order to compute dependency between
> types at the type oracle level.  The idea is to use it for determining
> when to re-run generators or to use the cache of the previous generator
> run.
>
> I figured for unit testing we would want to invalidate any previous
> cache.


Ahh, gotcha.  Mind throwing a comment there while you're in there?

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

[gwt-contrib] Re: Adds unit tests for extending JavaScriptObject. Tests a loosening of the (issue1369805)

[zundel]

http://gwt-code-reviews.appspot.com/1369805/diff/3001/dev/core/test/com/google/gwt/dev/javac/TypeOracleMediatorTestBase.java
File
dev/core/test/com/google/gwt/dev/javac/TypeOracleMediatorTestBase.java
(right):

http://gwt-code-reviews.appspot.com/1369805/diff/3001/dev/core/test/com/google/gwt/dev/javac/TypeOracleMediatorTestBase.java#newcode218
dev/core/test/com/google/gwt/dev/javac/TypeOracleMediatorTestBase.java:218:
null, getByteCode(aClass), System.currentTimeMillis());
On 2011/03/03 17:36:26, zundel wrote:

On 2011/03/03 17:14:07, scottb wrote:
> Unrelated, but shouldn't this be getLastModified() instead of

current time?


I dunno, do you want it to use caching?


Its intended to represent the timestamp of the source file or class file
resource as you mention, but in order to compute dependency between
types at the type oracle level.  The idea is to use it for determining
when to re-run generators or to use the cache of the previous generator
run.

I figured for unit testing we would want to invalidate any previous
cache.

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

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

[gwt-contrib] Re: Adds unit tests for extending JavaScriptObject. Tests a loosening of the (issue1369805)

[Scott Blum]

On Thu, Mar 3, 2011 at 12:36 PM,  wrote:

>
>
> http://gwt-code-reviews.appspot.com/1369805/diff/3001/dev/core/test/com/google/gwt/dev/javac/TypeOracleMediatorTestBase.java
> File
> dev/core/test/com/google/gwt/dev/javac/TypeOracleMediatorTestBase.java
> (right):
>
>
> http://gwt-code-reviews.appspot.com/1369805/diff/3001/dev/core/test/com/google/gwt/dev/javac/TypeOracleMediatorTestBase.java#newcode218
> dev/core/test/com/google/gwt/dev/javac/TypeOracleMediatorTestBase.java:218:
> null, getByteCode(aClass), System.currentTimeMillis());
> On 2011/03/03 17:14:07, scottb wrote:
>
>> Unrelated, but shouldn't this be getLastModified() instead of current
>>
> time?
>
> I dunno, do you want it to use caching?


U... your question makes me realize I have no actual clue how that test
works or what the significance of that timestamp is!

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

[gwt-contrib] Re: Adds unit tests for extending JavaScriptObject. Tests a loosening of the (issue1369805)

[zundel]

http://gwt-code-reviews.appspot.com/1369805/diff/3001/dev/core/test/com/google/gwt/dev/javac/TypeOracleMediatorTestBase.java
File
dev/core/test/com/google/gwt/dev/javac/TypeOracleMediatorTestBase.java
(right):

http://gwt-code-reviews.appspot.com/1369805/diff/3001/dev/core/test/com/google/gwt/dev/javac/TypeOracleMediatorTestBase.java#newcode218
dev/core/test/com/google/gwt/dev/javac/TypeOracleMediatorTestBase.java:218:
null, getByteCode(aClass), System.currentTimeMillis());
On 2011/03/03 17:14:07, scottb wrote:

Unrelated, but shouldn't this be getLastModified() instead of current

time?

I dunno, do you want it to use caching?

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

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

[gwt-contrib] Re: Adds unit tests for extending JavaScriptObject. Tests a loosening of the (issue1369805)

[scottb]

http://gwt-code-reviews.appspot.com/1369805/diff/3001/dev/core/test/com/google/gwt/dev/javac/TypeOracleMediatorTestBase.java
File
dev/core/test/com/google/gwt/dev/javac/TypeOracleMediatorTestBase.java
(right):

http://gwt-code-reviews.appspot.com/1369805/diff/3001/dev/core/test/com/google/gwt/dev/javac/TypeOracleMediatorTestBase.java#newcode218
dev/core/test/com/google/gwt/dev/javac/TypeOracleMediatorTestBase.java:218:
null, getByteCode(aClass), System.currentTimeMillis());
Unrelated, but shouldn't this be getLastModified() instead of current
time?

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

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

[gwt-contrib] Re: Adds unit tests for extending JavaScriptObject. Tests a loosening of the (issue1369805)

[Scott Blum]

On Thu, Mar 3, 2011 at 10:04 AM, Eric Ayers  wrote:

> After sleeping on the static rogue setter, I don't like it much either and
> came up with some alternatives:
>
> 1) Would it be more palatable to turn setJavaScriptObjectClass() it into an
> instance method?  That way, we wouldn't have to reset the static variable
> each time in tearDown and the override would last for only one instance of a
> TypeOracle.  That is the simplest thing I can think of.
>

I think that would definitely be an improvement on the static method.


> 2) We could make the setter just add to a static list of alternative class
> names to be treated like JavaScriptObject.  That would incur a slight
> run-time penalty in computeSingleJsoImplData() to check against a list of
> strings instead of just a single one.  But then we wouldn't have to worry
> about resetting the name.
>

Nah, instance method is better than this.


> 3) Do you think it would be feasable to taking
> com.google.gwt...mediatortest.JavaScriptObject.class object's bytecode and
> re-write the package using ASM?
>

Actually, that might not be too hard at all.


> 4) You suggested squirreling away a copy of
> com/google/gwt/core/client/JavaScriptObject.class somewhere on the classpath
> for the gwt-dev project and using it as the source for the bytecode.  This
> would require some minor refactoring of CheckedJavaResource and subclasses
> in TypeOracleMediatorTest.  To me, this is kind of obscure black magic
> (comparted to how the rest of the tests are implemented.)
>

Hmm, MutableJavaResource is a bit of a beast these days.  Kind of cumbersome
that reified Class objects are the primary api there.  If Class objects were
a convenience, and the underlying api was in terms of stuff you could get
from a Class object, then you could have a Class convenience constructor for
most of the cases, and special-case JavaScriptObject to work from bytes.
 You could get the bytes easily enough by instantiating a
CompilationStateTestBase and grabbings its 'state', which will already
contain a Unit for JSO, the one from JavaResourceBase.JAVASCRIPTOBJECT.

I guess it all depends on how serious you are about removing
TypeOracleMediatorFromByteCodeTest.  If you are going to get rid of that
anyway, MutableJavaResource gets a lot simpler, and it becomes pretty easy
to just utilize JavaResourceBase.JAVASCRIPTOBJECT.

Scott

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

[gwt-contrib] Re: Adds unit tests for extending JavaScriptObject. Tests a loosening of the (issue1369805)

[Eric Ayers]

After sleeping on the static rogue setter, I don't like it much either and
came up with some alternatives:

1) Would it be more palatable to turn setJavaScriptObjectClass() it into an
instance method?  That way, we wouldn't have to reset the static variable
each time in tearDown and the override would last for only one instance of a
TypeOracle.  That is the simplest thing I can think of.

2) We could make the setter just add to a static list of alternative class
names to be treated like JavaScriptObject.  That would incur a slight
run-time penalty in computeSingleJsoImplData() to check against a list of
strings instead of just a single one.  But then we wouldn't have to worry
about resetting the name.

3) Do you think it would be feasable to taking
com.google.gwt...mediatortest.JavaScriptObject.class object's bytecode and
re-write the package using ASM?

4) You suggested squirreling away a copy of
com/google/gwt/core/client/JavaScriptObject.class somewhere on the classpath
for the gwt-dev project and using it as the source for the bytecode.  This
would require some minor refactoring of CheckedJavaResource and subclasses
in TypeOracleMediatorTest.  To me, this is kind of obscure black magic
(comparted to how the rest of the tests are implemented.)

On Wed, Mar 2, 2011 at 10:02 PM, Scott Blum  wrote:

> On Wed, Mar 2, 2011 at 7:21 PM, Eric Ayers  wrote:
>
>> @Scott: I was not planning to revert the diff cheese. This uses the
>> recently updated official gwt-format.xml autoformatting for Eclipse.  I've
>> already volunteered to go through and run the autoformatter to bring
>> existing source up to date.
>
>
> Right, I'm just making the point that it would be great if you could land
> that first, so that when you submit this change, it won't mix the formatting
> & semantic changes into one CL.
>
> Per our face to face, it'd also be good if you could leave a TODO in
> TypeOracle to remove the rogue setter once
> TypeOracleMediatorFromByteCodeTest is gone.
>



-- 
Eric Z. Ayers
Google Web Toolkit, Atlanta, GA USA

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

[gwt-contrib] Re: Adds unit tests for extending JavaScriptObject. Tests a loosening of the (issue1369805)

[Scott Blum]

On Wed, Mar 2, 2011 at 7:21 PM, Eric Ayers  wrote:

> @Scott: I was not planning to revert the diff cheese. This uses the
> recently updated official gwt-format.xml autoformatting for Eclipse.  I've
> already volunteered to go through and run the autoformatter to bring
> existing source up to date.


Right, I'm just making the point that it would be great if you could land
that first, so that when you submit this change, it won't mix the formatting
& semantic changes into one CL.

Per our face to face, it'd also be good if you could leave a TODO in
TypeOracle to remove the rogue setter once
TypeOracleMediatorFromByteCodeTest is gone.

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

[gwt-contrib] Re: Adds unit tests for extending JavaScriptObject. Tests a loosening of the (issue1369805)

[Eric Ayers]

@Scott: I was not planning to revert the diff cheese. This uses the recently
updated official gwt-format.xml autoformatting for Eclipse.  I've already
volunteered to go through and run the autoformatter to bring existing source
up to date.

On Wed, Mar 2, 2011 at 7:12 PM,  wrote:

> You'll want to revert your diff-cheese anyhow before committing, but
> yes, that help.
>
> LG except for TypeOracle hack.  Just make your tests use the correct
> FQTN for JavaScriptObject.
>
>
>
>
> http://gwt-code-reviews.appspot.com/1369805/diff/3001/dev/core/src/com/google/gwt/dev/javac/typemodel/TypeOracle.java
> File dev/core/src/com/google/gwt/dev/javac/typemodel/TypeOracle.java
> (right):
>
>
> http://gwt-code-reviews.appspot.com/1369805/diff/3001/dev/core/src/com/google/gwt/dev/javac/typemodel/TypeOracle.java#newcode156
> dev/core/src/com/google/gwt/dev/javac/typemodel/TypeOracle.java:156: *
> @param className Overrides the fullly qualified class name for
> JavaScriptObject.
> fully
>
>
>
> http://gwt-code-reviews.appspot.com/1369805/diff/3001/dev/core/test/com/google/gwt/dev/javac/TypeOracleMediatorTestBase.java
> File
> dev/core/test/com/google/gwt/dev/javac/TypeOracleMediatorTestBase.java
> (right):
>
>
> http://gwt-code-reviews.appspot.com/1369805/diff/3001/dev/core/test/com/google/gwt/dev/javac/TypeOracleMediatorTestBase.java#newcode599
> dev/core/test/com/google/gwt/dev/javac/TypeOracleMediatorTestBase.java:599:
> protected static final CheckedJavaResource CU_JavaScriptObject = new
> CheckedJavaResource(
> With a little kung-fu, you could make the fully-qualified type name here
> be the same as the REAL JavaScriptObject FQTN, and thus avoid the
> egregious for-testing hackery in TypeOracle.
>
>
> http://gwt-code-reviews.appspot.com/1369805/
>



-- 
Eric Z. Ayers
Google Web Toolkit, Atlanta, GA USA

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

[gwt-contrib] Re: Adds unit tests for extending JavaScriptObject. Tests a loosening of the (issue1369805)

[zundel]

updated patch


http://gwt-code-reviews.appspot.com/1369805/diff/3001/dev/core/src/com/google/gwt/dev/javac/typemodel/TypeOracle.java
File dev/core/src/com/google/gwt/dev/javac/typemodel/TypeOracle.java
(right):

http://gwt-code-reviews.appspot.com/1369805/diff/3001/dev/core/src/com/google/gwt/dev/javac/typemodel/TypeOracle.java#newcode156
dev/core/src/com/google/gwt/dev/javac/typemodel/TypeOracle.java:156: *
@param className Overrides the fullly qualified class name for
JavaScriptObject.
On 2011/03/03 00:12:08, scottb wrote:

fully


Done.

http://gwt-code-reviews.appspot.com/1369805/diff/3001/dev/core/src/com/google/gwt/dev/javac/typemodel/TypeOracle.java#newcode159
dev/core/src/com/google/gwt/dev/javac/typemodel/TypeOracle.java:159:
jsoClassName = className;
On 2011/03/02 23:01:59, jaimeyap wrote:

This worries me a little bit :).



Can we make this protected or package protected?


 I created a TypeOracleDelegate class and changed the method to package
protected in typemodel.TypeOracle.  I think its a bit awkward, but it
hides it from rogue Generator developers.

http://gwt-code-reviews.appspot.com/1369805/diff/3001/dev/core/src/com/google/gwt/dev/javac/typemodel/TypeOracle.java#newcode941
dev/core/src/com/google/gwt/dev/javac/typemodel/TypeOracle.java:941:
On 2011/03/02 23:01:59, jaimeyap wrote:

whitespace


Done.

http://gwt-code-reviews.appspot.com/1369805/diff/3001/dev/core/test/com/google/gwt/dev/javac/TypeOracleMediatorTestBase.java
File
dev/core/test/com/google/gwt/dev/javac/TypeOracleMediatorTestBase.java
(right):

http://gwt-code-reviews.appspot.com/1369805/diff/3001/dev/core/test/com/google/gwt/dev/javac/TypeOracleMediatorTestBase.java#newcode599
dev/core/test/com/google/gwt/dev/javac/TypeOracleMediatorTestBase.java:599:
protected static final CheckedJavaResource CU_JavaScriptObject = new
CheckedJavaResource(
On 2011/03/03 00:12:08, scottb wrote:

With a little kung-fu, you could make the fully-qualified type name

here be the

same as the REAL JavaScriptObject FQTN, and thus avoid the egregious

for-testing

hackery in TypeOracle.


I tried, but my kung-fu wasn't good enough.

The Byte code test needs byte code.  We use reflection to get it from a
class instance.  But JavaScriptObject.java isn't a part of the gwt-dev
project, so JavaScriptObject.class it isn't resolved when I tried using
it.   I suppose we could hack up and build.xml files and eclipse
projects and try to get JavaScriptObject referenced by gwt-dev, but that
seems like a lot of work.  Did you having something else in mind?

http://gwt-code-reviews.appspot.com/1369805/diff/3001/dev/core/test/com/google/gwt/dev/javac/TypeOracleMediatorTestBase.java#newcode648
dev/core/test/com/google/gwt/dev/javac/TypeOracleMediatorTestBase.java:648:
};
On 2011/03/02 23:01:59, jaimeyap wrote:

insert newline


Done.

http://gwt-code-reviews.appspot.com/1369805/diff/3001/dev/core/test/com/google/gwt/dev/javac/TypeOracleMediatorTestBase.java#newcode660
dev/core/test/com/google/gwt/dev/javac/TypeOracleMediatorTestBase.java:660:
};
On 2011/03/02 23:01:59, jaimeyap wrote:

newline


Done.

http://gwt-code-reviews.appspot.com/1369805/diff/3001/dev/core/test/com/google/gwt/dev/javac/TypeOracleMediatorTestBase.java#newcode669
dev/core/test/com/google/gwt/dev/javac/TypeOracleMediatorTestBase.java:669:
};
On 2011/03/02 23:01:59, jaimeyap wrote:

newline


Done.

http://gwt-code-reviews.appspot.com/1369805/diff/3001/dev/core/test/com/google/gwt/dev/javac/TypeOracleMediatorTestBase.java#newcode678
dev/core/test/com/google/gwt/dev/javac/TypeOracleMediatorTestBase.java:678:
};
On 2011/03/02 23:01:59, jaimeyap wrote:

newline


Done.

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

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

[gwt-contrib] Re: Adds unit tests for extending JavaScriptObject. Tests a loosening of the (issue1369805)

[scottb]

You'll want to revert your diff-cheese anyhow before committing, but
yes, that help.

LG except for TypeOracle hack.  Just make your tests use the correct
FQTN for JavaScriptObject.


http://gwt-code-reviews.appspot.com/1369805/diff/3001/dev/core/src/com/google/gwt/dev/javac/typemodel/TypeOracle.java
File dev/core/src/com/google/gwt/dev/javac/typemodel/TypeOracle.java
(right):

http://gwt-code-reviews.appspot.com/1369805/diff/3001/dev/core/src/com/google/gwt/dev/javac/typemodel/TypeOracle.java#newcode156
dev/core/src/com/google/gwt/dev/javac/typemodel/TypeOracle.java:156: *
@param className Overrides the fullly qualified class name for
JavaScriptObject.
fully

http://gwt-code-reviews.appspot.com/1369805/diff/3001/dev/core/test/com/google/gwt/dev/javac/TypeOracleMediatorTestBase.java
File
dev/core/test/com/google/gwt/dev/javac/TypeOracleMediatorTestBase.java
(right):

http://gwt-code-reviews.appspot.com/1369805/diff/3001/dev/core/test/com/google/gwt/dev/javac/TypeOracleMediatorTestBase.java#newcode599
dev/core/test/com/google/gwt/dev/javac/TypeOracleMediatorTestBase.java:599:
protected static final CheckedJavaResource CU_JavaScriptObject = new
CheckedJavaResource(
With a little kung-fu, you could make the fully-qualified type name here
be the same as the REAL JavaScriptObject FQTN, and thus avoid the
egregious for-testing hackery in TypeOracle.

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

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

[gwt-contrib] Re: Adds unit tests for extending JavaScriptObject. Tests a loosening of the (issue1369805)

[Eric Ayers]

In TypeOracleMediatorTestBase, the only thing I changed was to add new unit
tests, and add a tearDown() method. The rest is reformatting cheese.  Does
that help?

On Wed, Mar 2, 2011 at 6:56 PM,  wrote:

> I'm getting lost in the diff cheese.  Can you post a review sans
> auto-format diffs?
>
>
> http://gwt-code-reviews.appspot.com/1369805/
>



-- 
Eric Z. Ayers
Google Web Toolkit, Atlanta, GA USA

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

[gwt-contrib] Re: Adds unit tests for extending JavaScriptObject. Tests a loosening of the (issue1369805)

[scottb]

I'm getting lost in the diff cheese.  Can you post a review sans
auto-format diffs?

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

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

[gwt-contrib] Re: Adds unit tests for extending JavaScriptObject. Tests a loosening of the (issue1369805)

[jaimeyap]

I will let Scott have the final word.

Tests look good. Just a nit on on exposing a public setter for the value
of JSO.


http://gwt-code-reviews.appspot.com/1369805/diff/3001/dev/core/src/com/google/gwt/dev/javac/typemodel/TypeOracle.java
File dev/core/src/com/google/gwt/dev/javac/typemodel/TypeOracle.java
(right):

http://gwt-code-reviews.appspot.com/1369805/diff/3001/dev/core/src/com/google/gwt/dev/javac/typemodel/TypeOracle.java#newcode159
dev/core/src/com/google/gwt/dev/javac/typemodel/TypeOracle.java:159:
jsoClassName = className;
This worries me a little bit :).

Can we make this protected or package protected?

http://gwt-code-reviews.appspot.com/1369805/diff/3001/dev/core/src/com/google/gwt/dev/javac/typemodel/TypeOracle.java#newcode941
dev/core/src/com/google/gwt/dev/javac/typemodel/TypeOracle.java:941:
whitespace

http://gwt-code-reviews.appspot.com/1369805/diff/3001/dev/core/test/com/google/gwt/dev/javac/TypeOracleMediatorTestBase.java
File
dev/core/test/com/google/gwt/dev/javac/TypeOracleMediatorTestBase.java
(right):

http://gwt-code-reviews.appspot.com/1369805/diff/3001/dev/core/test/com/google/gwt/dev/javac/TypeOracleMediatorTestBase.java#newcode648
dev/core/test/com/google/gwt/dev/javac/TypeOracleMediatorTestBase.java:648:
};
insert newline

http://gwt-code-reviews.appspot.com/1369805/diff/3001/dev/core/test/com/google/gwt/dev/javac/TypeOracleMediatorTestBase.java#newcode660
dev/core/test/com/google/gwt/dev/javac/TypeOracleMediatorTestBase.java:660:
};
newline

http://gwt-code-reviews.appspot.com/1369805/diff/3001/dev/core/test/com/google/gwt/dev/javac/TypeOracleMediatorTestBase.java#newcode669
dev/core/test/com/google/gwt/dev/javac/TypeOracleMediatorTestBase.java:669:
};
newline

http://gwt-code-reviews.appspot.com/1369805/diff/3001/dev/core/test/com/google/gwt/dev/javac/TypeOracleMediatorTestBase.java#newcode678
dev/core/test/com/google/gwt/dev/javac/TypeOracleMediatorTestBase.java:678:
};
newline

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

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

[gwt-contrib] Re: Adds unit tests for extending JavaScriptObject. Tests a loosening of the (issue1369805)

[zundel]

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

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