http://gwt-code-reviews.appspot.com/725801/diff/12001/13004
File dev/core/src/com/google/gwt/dev/shell/GWTBridgeImpl.java (right):
http://gwt-code-reviews.appspot.com/725801/diff/12001/13004#newcode26
dev/core/src/com/google/gwt/dev/shell/GWTBridgeImpl.java:26: protected
static ThreadLocal<String> uniqueID =
On 2010/08/02 19:30:47, tobyr wrote:
I don't think these changes to GWT* are necessary. See comment in
DevModeLoggingFixes.
Added a TODO
http://gwt-code-reviews.appspot.com/725801/diff/12001/13006
File
dev/core/src/com/google/gwt/dev/shell/rewrite/UseMirroredClasses.java
(right):
http://gwt-code-reviews.appspot.com/725801/diff/12001/13006#newcode39
dev/core/src/com/google/gwt/dev/shell/rewrite/UseMirroredClasses.java:39:
private static class MyMethodVisitor extends MethodAdapter {
On 2010/08/02 19:30:47, tobyr wrote:
Better name for this? How about MethodInterceptor?
Done.
http://gwt-code-reviews.appspot.com/725801/diff/12001/13006#newcode43
dev/core/src/com/google/gwt/dev/shell/rewrite/UseMirroredClasses.java:43:
{
On 2010/08/02 18:49:59, fredsa wrote:
Make this is a static initialization block
static {
...
}
Done.
http://gwt-code-reviews.appspot.com/725801/diff/12001/13006#newcode45
dev/core/src/com/google/gwt/dev/shell/rewrite/UseMirroredClasses.java:45:
mirroredMethods = new HashMap<String, String>();
On 2010/08/02 19:30:47, tobyr wrote:
This is fairly high maintenance, and I can think of a lot of ways to
improve it
(mostly using annotations). The improvements are also pretty
incremental, so
they don't have to be big bang.
It's not necessary that you do that now, but maybe you should add a
TODO to that
effect so we don't allow this code to grow unwieldy?
Added a TODO and will come back to this later this week or early next
week.
http://gwt-code-reviews.appspot.com/725801/diff/12001/13006#newcode48
dev/core/src/com/google/gwt/dev/shell/rewrite/UseMirroredClasses.java:48:
"com/google/gwt/logging/impl/DevModeLoggingFixes.getName");
On 2010/08/02 18:49:59, fredsa wrote:
For compile time safety & IDE refactoring, you could get the
class+method name
from, say: DevModeLoggingFixes.class.getName().replace(".", "/") +
".getName"
Unfortunately, I don't have access to the DevModeLoggingFixes class,
which is in gwt-user. There are a few open issues due to this fact, and
there's a TODO for me to fix this later this week or early next week.
http://gwt-code-reviews.appspot.com/725801/diff/12001/13006#newcode67
dev/core/src/com/google/gwt/dev/shell/rewrite/UseMirroredClasses.java:67:
String mirrorClassMethod = mirroredMethods.get(owner + "." + name);
On 2010/08/02 19:30:47, tobyr wrote:
If you're not going to change this code to handle the above, then you
might
consider optimizing it into a two-level lookup to avoid generating
garbage. It
will run for every single instruction for every single class in the
client
program, right? That could be millions of times.
Done.
http://gwt-code-reviews.appspot.com/725801/diff/12001/13006#newcode67
dev/core/src/com/google/gwt/dev/shell/rewrite/UseMirroredClasses.java:67:
String mirrorClassMethod = mirroredMethods.get(owner + "." + name);
On 2010/08/02 19:30:47, tobyr wrote:
This won't catch dispatches via subtypes on instance or static
methods. Here's
some example code:
public class TestIntercept {
static class A {
public void foo() {
}
public static void bar() {
}
}
static class B extends A {
}
public static void main(String[] args) {
new A().foo();
new B().foo();
A.bar();
B.bar();
}
}
public static void main(java.lang.String[]);
Code:
0: new #2; //class TestIntercept$A
3: dup
4: invokespecial #3; //Method TestIntercept$A."<init>":()V
7: invokevirtual #4; //Method TestIntercept$A.foo:()V
10: new #5; //class TestIntercept$B
13: dup
14: invokespecial #6; //Method TestIntercept$B."<init>":()V
17: invokevirtual #7; //Method TestIntercept$B.foo:()V
20: invokestatic #8; //Method TestIntercept$A.bar:()V
23: invokestatic #9; //Method TestIntercept$B.bar:()V
26: return
}
If you had an intercept rule for A.foo, or A.bar, then your intercept
rule would
not have caught the calls to them via the subclass B. Writing that
code is
tricky, because you need to look at the class hierarchy of the owner
of the
invoke you're examining.
I don't know how important this is to you for your specific case, (How
many
people subclass Logger or LogManager?), but you should at least
document that
this code doesn't work under those general circumstances.
Agreed on the wave to ignore this issue for now. Added a comment about
it not working to the javadoc.
http://gwt-code-reviews.appspot.com/725801/diff/12001/13006#newcode68
dev/core/src/com/google/gwt/dev/shell/rewrite/UseMirroredClasses.java:68:
if (mirrorClassMethod != null) {
On 2010/08/02 19:30:47, tobyr wrote:
As a code style, I personally prefer early returns. It prevents having
lots of
levels of indentation, like you have here. It's also
easier(personally) to see
all the code paths.
Done.
http://gwt-code-reviews.appspot.com/725801/diff/12001/13006#newcode73
dev/core/src/com/google/gwt/dev/shell/rewrite/UseMirroredClasses.java:73:
if (temp.length > 1) {
On 2010/08/02 19:30:47, tobyr wrote:
Shouldn't this be an assert?
assert(false); not failing for me when I tried using that here - perhaps
because it's in gwt-user?
http://gwt-code-reviews.appspot.com/725801/diff/12001/13006#newcode83
dev/core/src/com/google/gwt/dev/shell/rewrite/UseMirroredClasses.java:83:
// using the method descriptor string
On 2010/08/02 19:30:47, tobyr wrote:
You should only change the argument types if you're changing from a
virtual
dispatch to a static dispatch. You'll still need to change the owner
though.
Done.
http://gwt-code-reviews.appspot.com/725801/diff/12001/13012
File user/src/com/google/gwt/logging/client/LogConfiguration.java
(right):
http://gwt-code-reviews.appspot.com/725801/diff/12001/13012#newcode63
user/src/com/google/gwt/logging/client/LogConfiguration.java:63:
root.setUseParentHandlers(false);
On 2010/08/02 18:49:59, fredsa wrote:
A quick comments here as to why we need to set this to false would be
good
Done.
http://gwt-code-reviews.appspot.com/725801/diff/12001/13013
File user/src/com/google/gwt/logging/impl/DevModeLoggingFixes.java
(right):
http://gwt-code-reviews.appspot.com/725801/diff/12001/13013#newcode25
user/src/com/google/gwt/logging/impl/DevModeLoggingFixes.java:25: * This
class is used by the byte code rewriting which happens in DevMode only.
On 2010/08/02 18:49:59, fredsa wrote:
Add link to the rewriting class for reference, or an @see javadoc if
you prefer
tiny nit, smaller than a nit below: "prefix which" -> "prefix, which"
Done.
http://gwt-code-reviews.appspot.com/725801/diff/12001/13013#newcode36
user/src/com/google/gwt/logging/impl/DevModeLoggingFixes.java:36: *
Replaces all logger.getName() calls
On 2010/08/02 18:49:59, fredsa wrote:
Also mention in the javadoc what the rewrite does.
Done.
http://gwt-code-reviews.appspot.com/725801/diff/12001/13013#newcode39
user/src/com/google/gwt/logging/impl/DevModeLoggingFixes.java:39: return
logger.getName().replace(GWT.getUniqueThreadId() + ".", "");
On 2010/08/02 18:49:59, fredsa wrote:
delegate to removeLoggerPrefix()
Done.
http://gwt-code-reviews.appspot.com/725801/diff/12001/13013#newcode69
user/src/com/google/gwt/logging/impl/DevModeLoggingFixes.java:69: return
GWT.getUniqueThreadId() + ".";
On 2010/08/02 19:30:47, tobyr wrote:
For simplicity's sake, why not just use the actual thread id?
I haven't been able to move this class to gwt-dev, and while it's in
gwt-user, it needs the GWT class to access the Thread class (since we
don't emulate java.util.Thread in GWT). There's a TODO for me to move
this class to gwt-dev and to remove these functions from the GWT class.
http://gwt-code-reviews.appspot.com/725801/diff/12001/13013#newcode72
user/src/com/google/gwt/logging/impl/DevModeLoggingFixes.java:72:
private static String removeLoggerPrefix(String name) {
On 2010/08/02 19:30:47, tobyr wrote:
Dead code?
Actually - I just forgot to delegate to it in the getName function
above, which is now fixed.
http://gwt-code-reviews.appspot.com/725801/diff/12001/13013#newcode73
user/src/com/google/gwt/logging/impl/DevModeLoggingFixes.java:73: return
name.replace(getLoggerPrefix(), "");
On 2010/08/02 18:49:59, fredsa wrote:
We could potentially replace additional occurrences of the prefix in
the user's
logger name, although admittedly unlikely.
You could use a regex with a '^' anchor, or a simple substring() based
on length
of the prefix.
Done.
http://gwt-code-reviews.appspot.com/725801/diff/12001/13014
File user/src/com/google/gwt/logging/impl/LoggerImplRegular.java
(right):
http://gwt-code-reviews.appspot.com/725801/diff/12001/13014#newcode32
user/src/com/google/gwt/logging/impl/LoggerImplRegular.java:32: public
static Logger staticGetLoggerHelper(String name) {
On 2010/08/02 19:30:47, tobyr wrote:
Is this being called from somewhere else? I don't understand why it
got
out-lined and made public.
Good catch - this was left over from the previous version I had and is
no longer needed. Reverted.
http://gwt-code-reviews.appspot.com/725801/show
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
