Closed Bug 520572 Opened 11 years ago Closed 10 years ago

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

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Tracking Status
status1.9.2 --- beta5-fixed

People

(Reporter: bzbarsky, Assigned: mrbkap)

References

Details

(Whiteboard: [firebug-p2] fixed-in-tracemonkey)

Attachments

(2 files)

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.
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.
Whiteboard: [firebug-p2]
Flags: blocking1.9.2?
Done (Firebug SVN, R4493)
Honza
um, that svn link points to mozilla's svn repo.

see, http://code.google.com/p/fbug/source/detail?r=4493
any progress on this? Still requested for blocking.
OS: Mac OS X → All
Hardware: x86 → All
Could someone add STR here?
Also, does it happen on the 192 branch? This bug is listed as a Trunk defect.
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).
Attached patch FixSplinter Review
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
Assignee: general → mrbkap
Status: NEW → ASSIGNED
Attachment #410283 - Flags: review?(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
Attachment #410283 - Flags: review?(brendan) → review+
Flags: blocking1.9.2? → blocking1.9.2+
I apparently checked this in a little bit ago... I'm working on the test right now. http://hg.mozilla.org/tracemonkey/rev/b732f3a494a2
Whiteboard: [firebug-p2] → [firebug-p2] fixed-in-tracemonkey
Attached patch TestSplinter Review
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.
Attachment #412825 - Flags: review?(brendan)
Attachment #412825 - Flags: review?(bclary)
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.
Attachment #412825 - Flags: review?(bclary) → review+
Attachment #412825 - Flags: review?(brendan) → review+
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
Depends on: 529679
http://hg.mozilla.org/mozilla-central/rev/b732f3a494a2
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
These bugs landed after b4 was cut. Moving flag out.
You need to log in before you can comment on or make changes to this bug.