Closed
Bug 688017
Opened 13 years ago
Closed 6 years ago
Remove js_GetterOnlyPropertyStub
Categories
(Core :: XPConnect, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: bholley, Unassigned)
Details
(Keywords: addon-compat, dev-doc-needed)
Attachments
(3 files)
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
mrbkap
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
Waldo
:
review+
|
Details | Diff | Splinter Review |
As has been pointed out in bug 686979, this violates WebIDL.
Reporter | ||
Comment 1•13 years ago
|
||
First, we remove them from dom/workers. Flagging bent for review.
Attachment #561329 -
Flags: review?(bent.mozilla)
Reporter | ||
Comment 2•13 years ago
|
||
Then, from xpconnect. Flagging mrbkap.
Attachment #561330 -
Flags: review?(mrbkap)
Reporter | ||
Comment 3•13 years ago
|
||
Finally, remove them from the js engine. Flagging Waldo.
Does this need SR?
Review: jwalden+bmo
Reporter | ||
Updated•13 years ago
|
Attachment #561331 -
Flags: review?(jwalden+bmo)
Updated•13 years ago
|
Attachment #561330 -
Flags: review?(mrbkap) → review+
Reporter | ||
Comment 4•13 years ago
|
||
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)
Comment 5•13 years ago
|
||
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.
Comment 7•13 years ago
|
||
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?
Updated•13 years ago
|
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.
Comment 9•13 years ago
|
||
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.
Comment 10•13 years ago
|
||
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?
Comment 11•13 years ago
|
||
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.
Keywords: addon-compat,
dev-doc-needed
Reporter | ||
Comment 13•13 years ago
|
||
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.
Reporter | ||
Comment 15•13 years ago
|
||
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)
Reporter | ||
Updated•13 years ago
|
Attachment #561329 -
Flags: review?(bent.mozilla)
Reporter | ||
Comment 16•13 years ago
|
||
I've stopped driving this for now per the comment in bug 686979.
Assignee: bobbyholley+bmo → nobody
Comment 17•6 years ago
|
||
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
You need to log in
before you can comment on or make changes to this bug.
Description
•