all done
http://gwt-code-reviews.appspot.com/437801/diff/1/3
File /dev/core/src/com/google/gwt/dev/shell/BrowserChannelServer.java
(right):
http://gwt-code-reviews.appspot.com/437801/diff/1/3#newcode43
/dev/core/src/com/google/gwt/dev/shell/BrowserChannelServer.java:43: *
should be instantiating this class.
On 2010/05/03 20:32:27, scottb wrote:
Instead of this last sentence, just make the constructor private or
default
access.
Done.
http://gwt-code-reviews.appspot.com/437801/diff/1/4
File /dev/core/src/com/google/gwt/dev/shell/DevModeLogManager.java
(right):
http://gwt-code-reviews.appspot.com/437801/diff/1/4#newcode16
/dev/core/src/com/google/gwt/dev/shell/DevModeLogManager.java:16:
On 2010/05/03 20:32:27, scottb wrote:
No blank line here.
Done.
http://gwt-code-reviews.appspot.com/437801/diff/1/4#newcode27
/dev/core/src/com/google/gwt/dev/shell/DevModeLogManager.java:27: public
class DevModeLogManager extends LogManager {
On 2010/05/03 20:32:27, scottb wrote:
My main comment on this class is... a user can have multiple hosted
mode
sessions open concurrently, and each is supposed to live in its own
isolated
world. But this implementation appears to lump all of the individual
clients
into the same bucket.
I think you need a per-client LogManager. A pretty easy way to do
this is with
a ThreadLocal<LogManager> for clients.
If you go this route, individual delegation methods could look like
this:
public boolean addLogger(Logger logger) {
return getDelegate().addLogger(logger);
}
where getDelegate() returns either the serverLogManager, or a
client-specific
one.
Done.
http://gwt-code-reviews.appspot.com/437801/diff/1/4#newcode34
/dev/core/src/com/google/gwt/dev/shell/DevModeLogManager.java:34:
protected static LogManager delegate;
On 2010/05/03 20:32:27, scottb wrote:
Suggest naming this "serverLogManager" or something like that.
Done.
http://gwt-code-reviews.appspot.com/437801/diff/1/4#newcode39
/dev/core/src/com/google/gwt/dev/shell/DevModeLogManager.java:39: //
delegate calls to there.
On 2010/05/03 20:32:27, scottb wrote:
The code you want will probably look something like this:
Class<? extends LogManager> clazz =
Class.forName(logManagerClass).asSubclass(LogManager.class);
Constructor<? extends LogManager> ctor =
clazz.getDeclaredConstructor();
ctor.setAccessible(true);
delegate = ctor.newInstance();
(mostly copied from ModuleSpace.rebindAndCreate())
The nice thing about implementing this is, you won't need
LogManagerWithExposedConstructor anymore, since you can instantiate it
via
reflection.
Done. However, I still needed a LogManger with an exposed constructor
for the clientLogManagers and also to create in case of error. I could
do both of these with reflection if you have a strong preference -
mostly I wanted to avoid the code to handle all of the exceptions that
the reflection method throws (but shouldn't fire since I actually know
for sure how the LogManager class behaves)
http://gwt-code-reviews.appspot.com/437801/diff/1/4#newcode63
/dev/core/src/com/google/gwt/dev/shell/DevModeLogManager.java:63:
On 2010/05/03 20:32:27, scottb wrote:
It seems like you'd want to forward all of the public methods, right?
Otherwise
you could break e.g. servlet code that uses loggging.
Mmm.. once upon a time I had this set up so that server code was going
to super and client code was going to a delegate, which was why this was
ok (the client code LogManager doesn't support any methods other than
addLogger and getLogger... I think though that if I'm going to support
the old logging.manager properties, I can't do that anymore, and I will
need to delegate all the calls... done
http://gwt-code-reviews.appspot.com/437801/diff/1/5
File /dev/core/test/com/google/gwt/dev/DevModeBaseTest.java (right):
http://gwt-code-reviews.appspot.com/437801/diff/1/5#newcode24
/dev/core/test/com/google/gwt/dev/DevModeBaseTest.java:24: String
kManagerProperty = "java.util.logging.manager";
On 2010/05/03 20:32:27, scottb wrote:
We don't use this style in GWT. If you want to make it a constant,
make it a
static final field in the class and use THIS_FORMAT for the name.
Done.
http://gwt-code-reviews.appspot.com/437801/show
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
