[gwt-contrib] Re: Fix this->window confusion in DevMode (issue1620805)

2012-01-04 Thread stephen . haberman
> body.substring(0, i + 1) + fun + body.substring(i + 1); Yuck. Yeah. :-) Have you guys already talked about just throwing the exception on the Java side? I think that is what John referenced on comment 12 in the issue: "we cannot guarantee NPEs in production mode (typically you will get

[gwt-contrib] Re: Fix this->window confusion in DevMode (issue1620805)

2012-01-04 Thread jat
But the point is it is almost certainly an error to call a method on a null value, and catching it in DevMode would be better than replicating the behavior of prod mode in this case. http://gwt-code-reviews.appspot.com/1620805/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: Fix this->window confusion in DevMode (issue1620805)

2012-01-04 Thread stephen . haberman
Agreed it's almost certainly an error, I just assumed drifting DevMode to be stricter than prod would end up as another bug report down the road ("hey, this failed in devmode, but not in my production app"). Perfectly willing to defer to you guys though; my original spike for this did use a Java

[gwt-contrib] Re: Fix this->window confusion in DevMode (issue1620805)

2012-01-04 Thread stephen . haberman
Okay, updated the patch. Within the rewritten JSNI methods, before making the jump to javascript, we check for null instances (assuming the method isn't static). http://gwt-code-reviews.appspot.com/1620805/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: Fix this->window confusion in DevMode (issue1620805)

2012-01-04 Thread stephen . haberman
http://gwt-code-reviews.appspot.com/1620805/diff/8001/dev/core/src/com/google/gwt/dev/shell/JavaScriptHost.java File dev/core/src/com/google/gwt/dev/shell/JavaScriptHost.java (right): http://gwt-code-reviews.appspot.com/1620805/diff/8001/dev/core/src/com/google/gwt/dev/shell/JavaScriptHost.java#

[gwt-contrib] Re: Fix this->window confusion in DevMode (issue1620805)

2012-01-05 Thread stephen . haberman
http://gwt-code-reviews.appspot.com/1620805/diff/8001/dev/core/src/com/google/gwt/dev/shell/rewrite/RewriteJsniMethods.java File dev/core/src/com/google/gwt/dev/shell/rewrite/RewriteJsniMethods.java (right): http://gwt-code-reviews.appspot.com/1620805/diff/8001/dev/core/src/com/google/gwt/dev/sh

[gwt-contrib] Re: Fix this->window confusion in DevMode (issue1620805)

2012-01-05 Thread stephen . haberman
http://gwt-code-reviews.appspot.com/1620805/diff/12001/dev/core/src/com/google/gwt/dev/shell/JavaScriptHost.java File dev/core/src/com/google/gwt/dev/shell/JavaScriptHost.java (right): http://gwt-code-reviews.appspot.com/1620805/diff/12001/dev/core/src/com/google/gwt/dev/shell/JavaScriptHost.jav

[gwt-contrib] Re: Fix this->window confusion in DevMode (issue1620805)

2012-04-09 Thread rdayal
On 2012/02/28 05:16:42, stephenh wrote: > "instance" in the sense of "instance method" Done. submitted. http://gwt-code-reviews.appspot.com/1620805/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: Fix this->window confusion in DevMode (issue1620805)

2012-05-08 Thread rdayal
On 2012/04/09 18:33:00, rdayal wrote: On 2012/02/28 05:16:42, stephenh wrote: > > "instance" in the sense of "instance method" > > Done. submitted. We had to roll this back because it broke a bunch of test code in the Google environment. I think that this patch is in the right, but the test

[gwt-contrib] Re: Fix this->window confusion in DevMode (issue1620805)

2012-05-08 Thread stephen . haberman
We had to roll this back because it broke a bunch of test code Cool, I saw that, was wondering what had happened. I think that this patch is in the right, but the test code was not carefully written Good, I was hoping you thought the patch was still right. Stephen, is that ok with you?

[gwt-contrib] Re: Fix this->window confusion in DevMode (issue1620805)

2012-05-08 Thread John Tamplin
On Tue, May 8, 2012 at 2:38 PM, wrote: > > We had to roll this back because it broke a bunch of test code >> > > Cool, I saw that, was wondering what had happened. > > > I think that this patch is in the right, but the test code was not >> carefully written >> > > Good, I was hoping you thought

[gwt-contrib] Re: Fix this->window confusion in DevMode (issue1620805)

2012-05-08 Thread stephen . haberman
it seems likely that external code we can't test also does so True, although my assumption is that since this is a DevMode-only behavior (null becomes window), we're actually doing everyone a favor by fixing the behavior to match web mode, and in doing so highlighting previously-missed/potenti

[gwt-contrib] Re: Fix this->window confusion in DevMode (issue1620805)

2012-05-08 Thread John Tamplin
On Tue, May 8, 2012 at 3:23 PM, wrote: > True, although my assumption is that since this is a DevMode-only > behavior (null becomes window), we're actually doing everyone a favor by > fixing the behavior to match web mode, and in doing so highlighting > previously-missed/potential bugs in their a

[gwt-contrib] Re: Fix this->window confusion in DevMode (issue1620805)

2012-05-08 Thread Stephen Haberman
> I agree, but asking anyone to have to make changes in working code Two things, one is that I don't think any code that relies on this behavior could really be called "working". It's dev mode mistakingly acting differently than web mode. > as a consequence of upgrading increases the cost, even

[gwt-contrib] Re: Fix this->window confusion in DevMode (issue1620805)

2012-05-21 Thread Stephen Haberman
> I'd rather spend that time working on SuperDevMode and moving people > over to that, where this whole issue of differing implementations > between web mode and dev mode goes away. +1 to that. > I just don't think this issue is one where we should start to break > backwards compatibility. That