520572 - watch() on an outer object should (optionally?) set watch on inner object

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Boris Zbarsky [:bzbarsky]]

See the "var, window, and object.watch()" thread in mozilla.dev.tech.js-engine.

Basically, Firebug would like to watch unqualified global variable assignments, and has no way to do so right now.

Comment 1

[John J. Barton]

Honza, until we get this we need to disable direct properties of 'window' level in the Break on Property Change. Otherwise user changes like
  x = 5;
will fail to hit and look like a bug.

Comment 2

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Done (Firebug SVN, R4493)
Honza

Comment 3

[Rob Campbell [:rc] (:robcee)]

um, that svn link points to mozilla's svn repo.

see, http://code.google.com/p/fbug/source/detail?r=4493

Comment 4

[Rob Campbell [:rc] (:robcee)]

any progress on this? Still requested for blocking.

Comment 5

[Robert Sayre]

Could someone add STR here?

Comment 6

[Robert Sayre]

Also, does it happen on the 192 branch? This bug is listed as a Trunk defect.

Comment 7

[Boris Zbarsky [:bzbarsky]]

str:

javascript:window.watch("x", function(propname, oldval, newval) { alert('x set to ' + newval); }); var x = 5; x = 7; window.x = 8; void(0)

Should give three alerts; only gives 1.

And this should be a problem on all branches that have splitwindow (1.8.0 and later for sure).

Comment 8

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Fix

This needs a test (it should be possible to test this if any of our harnesses can pass -z to the shell before running it. Here's a shell session that I used to test:

$ dbg-obj/js -z
js> this.watch('x', function() { print("HERE") } )
js> this.x = 4
HERE
4
js> x = 6
HERE
6

Comment 9

[Brendan Eich [:brendan]]

Comment on attachment 410283 [details] [diff] [review]
Fix

>+    /*
>+     * If, by unwrapping and innerizing, we changed the object check again to

Nit: comma before "check again".

/be

Comment 10

[Blake Kaplan (:mrbkap) (inactive)]

I apparently checked this in a little bit ago... I'm working on the test right now. http://hg.mozilla.org/tracemonkey/rev/b732f3a494a2

Comment 11

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Test

This tests this patch. I chose a basically-random directory to put the test in. bc: do you have any thoughts about where it should go?

In order to test this, I added a new option to evalcx(): evalcx("split") returns a split object to the caller, when they can then use to test split-objecty stuff on. I've verified that a build pre-attachment 410283 [details] [diff] [review] fails the test and post-attachment 410283 [details] [diff] [review] passes it.

Comment 12

[Bob Clary [:bc] (inactive)]

Comment on attachment 412825 [details] [diff] [review]
Test

The location seems fine. You need to test if evalcx exists and default to pass if it does not exist since this will be executed in the browser as well. Just set expect = actual = 'skipped' with a message that the test is skipped if evalcx doesn't exist. 

r+ with that.

Comment 13

[Brendan Eich [:brendan]]

Comment on attachment 412825 [details] [diff] [review]
Test

> static const JSErrorFormatString *
> my_GetErrorMessage(void *userRef, const char *locale, const uintN errorNumber);
> static JSObject *
>-split_setup(JSContext *cx);
>+split_setup(JSContext *cx, JSBool evalcx);

Pre-nit: add a blank line between declarations.

>+    if (srclen == 4) {
>+        if (src[0] == 'l' && src[1] == 'a' && src[2] == 'z' && src[3] == 'y') {
>+            lazy = JS_TRUE;
>+            srclen = 0;
>+        }
>+    } else if (srclen == 5 &&
>+               src[0] == 's' && src[1] == 'p' && src[2] == 'l' &&
>+               src[3] == 'i' && src[4] == 't') {
>+        split = lazy = JS_TRUE;
>         srclen = 0;
>     }

Prettier if you keep parallel structure (nested if in both outer then clauses).

r=me with these picked, thanks.

/be

Comment 14

[Blake Kaplan (:mrbkap) (inactive)]

http://hg.mozilla.org/tracemonkey/rev/9090bb918a4d

Comment 15

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/b732f3a494a2

Comment 16

[Robert Sayre]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/ae02e57e750f

Comment 17

[Samuel Sidler (old account; do not CC)]

These bugs landed after b4 was cut. Moving flag out.