Closed Bug 688017 Opened 13 years ago Closed 6 years ago

Remove js_GetterOnlyPropertyStub

Categories

(Core :: XPConnect, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: bholley, Unassigned)

Details

(Keywords: addon-compat, dev-doc-needed)

Attachments

(3 files)

As has been pointed out in bug 686979, this violates WebIDL.
First, we remove them from dom/workers. Flagging bent for review.
Attachment #561329 - Flags: review?(bent.mozilla)
Then, from xpconnect. Flagging mrbkap.
Attachment #561330 - Flags: review?(mrbkap)
Finally, remove them from the js engine. Flagging Waldo. Does this need SR? Review: jwalden+bmo
Attachment #561331 - Flags: review?(jwalden+bmo)
Attachment #561330 - Flags: review?(mrbkap) → review+
Comment on attachment 561329 [details] [diff] [review] part 1 - Remove use of js_GetterOnlyPropertyStub in dom/workers. v1 Oh hm, looks like this broke some worker tests: https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=1593f1b3f20e Cancelling request for bent's review for now until I investigate this.
Attachment #561329 - Flags: review?(bent.mozilla)
Try run for 1593f1b3f20e is complete. Detailed breakdown of the results available here: https://tbpl.mozilla.org/?tree=Try&rev=1593f1b3f20e Results (out of 171 total builds): exception: 2 success: 140 warnings: 27 failure: 2 Builds available at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/bobbyholley@gmail.com-1593f1b3f20e
Jonas and I think that WebIDL is wrong here, and we're going to file a bug. Let's not jump at removing this just yet.
How so is WebIDL wrong? We want the DOM to look like it's made up of regular old JS objects (which might sometimes perhaps be proxies). Having a getter, and not having a setter, is the logical way to do that -- |readonly attribute foo| will look like |{ get foo() { ... } }|. What am I missing?
Attachment #561331 - Flags: review?(jwalden+bmo) → review+
This is a *big* change that hasn't been debated anywhere. As far as I know all readonly DOM properties in all browsers has thrown since the dawn of time. Changing that requires more discussion than has happened in this bug. I'd say this should be explicitly brought up on both dev-platform as well as public-script-coord to make sure that people in mozilla and in other browsers are ok with this change.
ES5 had that debate, as I recall, when it decided to make assignments to getter-only properties and such throw only in strict mode. We didn't have fallout (any at all, as I recall). I'll be quite surprised if there's any fire here.
You're aware that the DOM that WebIDL specs for this is that implemented in the IE10 platform previews, and it's the DOM that IE9 ships, right?
And as far as IE9 is concerned, assigning to readonly properties doesn't throw at all, in any circumstances. (IE9 doesn't support strict mode.)
As far as I know non-strict JS has never thrown for assignments to read-only properties, so I bet the discussion was pretty short: "We don't throw when assigning to readonly properties, starting to do so would break the world, let's do it just in strict mode". So the premise is quite different compared to DOM APIs. IE not throwing is a much more interesting data point though. I still think this should be at least brought up in dev-platform though.
Waldo, sicking - What's the way forward on this and bug 686979?
So I looked around on the other browsers. Safari, Chrome and IE9 all do *not* throw for trying to set a read-only property. So I think the way forward is to follow that lead and make Gecko also not throw. Unless in strict mode of course.
Comment on attachment 561329 [details] [diff] [review] part 1 - Remove use of js_GetterOnlyPropertyStub in dom/workers. v1 (In reply to Jonas Sicking (:sicking) from comment #14) > So I looked around on the other browsers. Safari, Chrome and IE9 all do > *not* throw for trying to set a read-only property. So I think the way > forward is to follow that lead and make Gecko also not throw. Unless in > strict mode of course. Ok! In that case, reflagging bent for review.
Attachment #561329 - Flags: review?(bent.mozilla)
Attachment #561329 - Flags: review?(bent.mozilla)
I've stopped driving this for now per the comment in bug 686979.
Assignee: bobbyholley+bmo → nobody
Per policy at https://wiki.mozilla.org/Bug_Triage/Projects/Bug_Handling/Bug_Husbandry#Inactive_Bugs. If this bug is not an enhancement request or a bug not present in a supported release of Firefox, then it may be reopened.
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → INACTIVE
js_GetterOnlyPropertyStub looks gone to me.
Resolution: INACTIVE → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: