[gwt-contrib] Re: Add a FakeCssMaker for CSS resources similar to FakeMessagesMaker. (issue1604804)

[skybrian]

LGTM



http://gwt-code-reviews.appspot.com/1604804/diff/1/user/src/com/google/gwt/junit/FakeCssMaker.java
File user/src/com/google/gwt/junit/FakeCssMaker.java (right):

http://gwt-code-reviews.appspot.com/1604804/diff/1/user/src/com/google/gwt/junit/FakeCssMaker.java#newcode2
user/src/com/google/gwt/junit/FakeCssMaker.java:2: * Copyright 2009
Google Inc.
2011

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

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

[gwt-contrib] Re: Add a FakeCssMaker for CSS resources similar to FakeMessagesMaker. (issue1604804)

[jat]

http://gwt-code-reviews.appspot.com/1604804/diff/1/user/src/com/google/gwt/junit/FakeCssMaker.java
File user/src/com/google/gwt/junit/FakeCssMaker.java (right):

http://gwt-code-reviews.appspot.com/1604804/diff/1/user/src/com/google/gwt/junit/FakeCssMaker.java#newcode44
user/src/com/google/gwt/junit/FakeCssMaker.java:44:
FakeCssMaker.class.getClassLoader(), new Class[] {cssClass},
Is this the right classloader to use?  FakeCssMaker is part of GWT,
while cssClass will typically be a user class.

I suggest using the thread context classloader instead.

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

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

[gwt-contrib] Re: Add a FakeCssMaker for CSS resources similar to FakeMessagesMaker. (issue1604804)

[jat]

Right now, it won't make much difference as GWT gets user classes from
the same classpath that GWT classes are loaded from.  However, that
leads to conflict for shared code where you want to use a different
version of something than the version GWT itself needs to run.  So, in
the future we may provide a way to give separate classpaths.

I think of the following guidelines for choosing which classloader to
use:
 - if a class being loaded is bundled with some other class (ie, known
to be from the same jar), use that class's classloader to load it.
 - if you want to load a system class and definitely don't want it to be
overridden, use the system classloader
 - otherwise, use the thread context classloader.

Since you have already submitted it and this will generally be used only
in tests, this is just FYI and you don't need to update it.

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

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

[gwt-contrib] Re: Add a FakeCssMaker for CSS resources similar to FakeMessagesMaker. (issue1604804)

[jat]

Right now, it won't make much difference as GWT gets user classes from
the same classpath that GWT classes are loaded from.  However, that
leads to conflict for shared code where you want to use a different
version of something than the version GWT itself needs to run.  So, in
the future we may provide a way to give separate classpaths.

I think of the following guidelines for choosing which classloader to
use:
 - if a class being loaded is bundled with some other class (ie, known
to be from the same jar), use that class's classloader to load it.
 - if you want to load a system class and definitely don't want it to be
overridden, use the system classloader
 - otherwise, use the thread context classloader.

Since you have already submitted it and this will generally be used only
in tests, this is just FYI and you don't need to update it.

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

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

[gwt-contrib] Re: Add a FakeCssMaker for CSS resources similar to FakeMessagesMaker. (issue1604804)

[jasonmheim]

On 2011/11/30 20:26:19, jat wrote:

http://gwt-code-reviews.appspot.com/1604804/diff/1/user/src/com/google/gwt/junit/FakeCssMaker.java

File user/src/com/google/gwt/junit/FakeCssMaker.java (right):



http://gwt-code-reviews.appspot.com/1604804/diff/1/user/src/com/google/gwt/junit/FakeCssMaker.java#newcode44

user/src/com/google/gwt/junit/FakeCssMaker.java:44:
FakeCssMaker.class.getClassLoader(), new Class[] {cssClass},
Is this the right classloader to use?  FakeCssMaker is part of GWT,

while

cssClass will typically be a user class.



I suggest using the thread context classloader instead.


Interesting comment. Under what circumstances would they be in different
classloaders? The JUnit testing environment for which this class is
intended shouldn't have multiple classloaders. Then again, if that
somehow occurred then I would think that the right classloader would be
that of 'cssClass', so maybe it's worth revisiting.

For what it's worth, this is the same technique that FakeMessagesMaker
uses without issue. I submitted the change prior to seeing your comment,
but I'm happy to make the change to both this and FakeMessagesMaker if
you think it has value.


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

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

[gwt-contrib] Re: Add a FakeCssMaker for CSS resources similar to FakeMessagesMaker. (issue1604804)

[jasonmheim]

On 2011/11/30 21:05:17, jat wrote:

Right now, it won't make much difference as GWT gets user classes from

the same

classpath that GWT classes are loaded from.  However, that leads to

conflict for

shared code where you want to use a different version of something

than the

version GWT itself needs to run.  So, in the future we may provide a

way to give

separate classpaths.



I think of the following guidelines for choosing which classloader to

use:

  - if a class being loaded is bundled with some other class (ie, known

to be

from the same jar), use that class's classloader to load it.
  - if you want to load a system class and definitely don't want it to

be

overridden, use the system classloader
  - otherwise, use the thread context classloader.



Since you have already submitted it and this will generally be used

only in

tests, this is just FYI and you don't need to update it.


Thanks for the extra info, I agree with your assessment.

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

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