* SUMMARY __defineGetter__/__defineSetter__ should throw exceptions. * TEST debug("__defineGetter__ and __defineSetter__ should throw exceptions when acting on sealed objects"); var o14 = {a:14}; Object.seal(o14); shouldThrow("o14.__defineGetter__('a', function(){})"); shouldThrow("o14.__defineGetter__('b', function(){})"); shouldThrow("o14.__defineSetter__('a', function(){})"); shouldThrow("o14.__defineSetter__('b', function(){})"); debug("__defineGetter__ and __defineSetter__ should throw exceptions when acting on frozen objects"); var o15 = {a:15}; Object.freeze(o15); shouldThrow("o15.__defineGetter__('a', function(){})"); shouldThrow("o15.__defineGetter__('b', function(){})"); shouldThrow("o15.__defineSetter__('a', function(){})"); shouldThrow("o15.__defineSetter__('b', function(){})"); * NOTES - WebKit fails silently - Firefox throws exceptions - Chrome fails silently I think throwing an exception makes more sense. It at least lets the developer know something went wrong. I don't think there is any specification for these functions.
Created attachment 249180 [details] [PATCH] Proposed Fix This is a potential breaking change, but I think it is for the better. 1. Non-configurable properties are rare. 2. Another browser (Firefox) already has this behavior. 3. This has the potential to help catch an issue where an error would happen silently.
Comment on attachment 249180 [details] [PATCH] Proposed Fix r=me Would be nice to put that true into a local variable named "shouldThrow", so that it's clear at the call site what it means.
http://trac.webkit.org/changeset/181868
This caused Bug 144373. I'm going to roll-out and re-approach this later after making IDL properties configurable, which in and of itself is a large enough change to make and see if it has fallout.
See Bug 144373 for how this broke live.com. Lets try re-enabling this after the following have been fixed: <https://webkit.org/b/122213> WebIDL operations should be configurable <https://webkit.org/b/134364> DOM attributes on prototypes should be configurable
Note: Also broke slack.com and Slack.app (<rdar://problem/20807563>).
Blocker now fixed. I will retest this with Slack.app / live.com and see if we can re-land it.
live.com issue no longer happens, it boiled down to: HTMLElementPrototype.__defineGetter__("innerText", function() { ... }); slack.com issue still exists, it boils down to: window.__defineGetter__("onhashchange", function() { ... }); Seems these "onfoo" properties are still not configurable but probably should be. I'll file a new bug and track that.
Created attachment 282516 [details] Patch
Comment on attachment 282516 [details] Patch r=me. Has the slack.com compatibility issue outlined in https://bugs.webkit.org/show_bug.cgi?id=142934#c8 been resolved?
Comment on attachment 282516 [details] Patch (In reply to comment #10) > Comment on attachment 282516 [details] > Patch > > r=me. > > Has the slack.com compatibility issue outlined in > https://bugs.webkit.org/show_bug.cgi?id=142934#c8 been resolved? I believe so. One of the tests cover this particular case.
Comment on attachment 282516 [details] Patch Clearing flags on attachment: 282516 Committed r202755: <http://trac.webkit.org/changeset/202755>
All reviewed patches have been landed. Closing bug.