Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make the DOMException constructor accept a name #22

Merged
merged 1 commit into from Apr 17, 2015
Merged

Make the DOMException constructor accept a name #22

merged 1 commit into from Apr 17, 2015

Conversation

domenic
Copy link
Member

@domenic domenic commented Oct 24, 2014

It will automatically set the code from the name (if it is one of the legacy names that have codes).

It now just calls super(message) to initialize the message and all of the internal error-related state.

The "creating exceptions" section was refactored to work in terms of the new, fully-functional constructor.

Closes https://www.w3.org/Bugs/Public/show_bug.cgi?id=27062.

@domenic
Copy link
Member Author

domenic commented Oct 24, 2014

/cc @allenwb in case he has time to help review: is this the correct way to define an Error-subclass constructor? I copied the "super" trick from TypedArray but realize that constructors with call behavior might be a bit different.

@domenic
Copy link
Member Author

domenic commented Oct 24, 2014

@heycam
Copy link
Collaborator

heycam commented Nov 14, 2014

Thanks for the patch Domenic. I have two questions:

  1. At the moment, the spec doesn't explicitly say which global to things like Object.prototype from, when describing the behaviour of the initial objects, and mostly it's implicit that it comes from the associated ECMAScript global object of the thing that's being defined. Here, you're explicitly grabbing the ECMAScript Realm to find the Error constructor. Explicit is good, but is this different from "the current global environment"? (@bzbarsky was talking me today about the difference, and I think it's the same, but I'm not sure.)
  2. Why use DefinePropertyOrThrow?

One style nit:

I try to avoid using "let" to update the value of an already-declared variable, since it looks a bit weird/wrong to me. I realise the ES spec does this though. I tend to use "let" at variable declaration time and "set" to update a variable's value.

More generally, I'm getting concerned about both the inconsistency about the algorithm description style in the spec versus what the ECMAScript spec uses, and also the growing inconsistency within Web IDL itself. For example your patch you use "PropertyDescriptor{...}" whereas the rest of the spec is using "Property Descriptor {...}". I suspect this example is just a case of the ES spec changing style at some point and me failing to update. Another example is the use of completion types rather than handling exceptions more implicitly. I'd like the spec to be consistent with itself; I would be happy to take PRs that Web IDL to use whatever the desired terminology and spec description style is. (Not meant to be a roadblock to getting this in, just something to keep in mind.)

Thanks!

@allenwb
Copy link

allenwb commented Nov 14, 2014

@domenic this looks generally ok.
The "current realm" is always the realm of the active function, so it is sufficient to simply say %Error% rather than what you are doing in steps 2-3.

super calls from constructors now follow the [[Prototype]] chain of the constructor so rather than accessing %Error% it would be more consistent with ES6 to do:

Let super be the result of calling the [[GetPrototypeOf]] internal method of F.

@Haycam If this algorithm is following ES6 conventions, the "current realm" is always the same as the realm of the active function (in this case DOMException) and %foo% always means the foo value associated with the current realm.

DefinePropertyOrThrow is an ES6 abstraction operation that is a helpper wrapper for calling the [[DefineOwnProperty]] internal method. The main thing it does is convert a false value returned from [[DefineOwnProperty]] into a throw of TypeError.

In the ES6 spec. we generally try to only use 'set' when settingthe value of internal slots and other long-lived mutable state. We use let on "local variables", even when reassigning them.

I tend to agree that it is somewhat dangerous to too closely like the notation of the two specification. At the very least you probably need to refer to a specific ES version so your notation can be interpreted relative to that rather than as a moving target.

Personally, I think it would be even better to specify things like this in terms of ES source code. Then you don't have to try to track a moving target of ES spec. conventions.

@heycam
Copy link
Collaborator

heycam commented Nov 14, 2014

Regarding DefinePropertyOrThrow, my question was more about why we want to throw an exception here if assigning to name and code fails.

@allenwb
Copy link

allenwb commented Nov 14, 2014

@heycam what else would you do if you can't create those properties? Throwing is the normal ES behavior for such failures.

@domenic
Copy link
Member Author

domenic commented Nov 24, 2014

Updated with @allenwb's suggestion, which also happens to resolve @heycam's question 1. And for 2, indeed, not sure what you would do except throw.

For the general style issue, I agree that it's problematic :(. Not sure when we can take the time to resolve that...

@bzbarsky
Copy link
Collaborator

bzbarsky commented Jan 2, 2015

I am planning to land support for this in Gecko pretty soon (not exactly Domenic's spec, but the existence of the constructor and its taking two arguments, etc). @heycam, are you still planning to merge this one?

@heycam
Copy link
Collaborator

heycam commented Jan 6, 2015

Domenic's patch looks good. @bzbarsky when you say "not exactly", what differences? Should we make sure we align the spec and the code?

@bzbarsky
Copy link
Collaborator

bzbarsky commented Jan 6, 2015

In Gecko DOMException is an interface so far. So there are all sorts of differences in terms of properties being accessors on the prototype, not own value props.

The IDL I'm using at the moment for the constructor is:

Constructor(optional DOMString message = "", optional DOMString name)

and mapping the name, if passed, to the right code, etc. So apart from where the props live and whether they're modifiable, I think I'm matching the proposed spec.

@heycam
Copy link
Collaborator

heycam commented Jan 6, 2015

And presumably Gecko's DOMExceptions will become a built-in thing and not an interface in the future. Cool, I'll get Domenic's patch merged.

@bzbarsky
Copy link
Collaborator

bzbarsky commented Jan 7, 2015

I expect that in Gecko we'll add some WebIDL extensions to allow DOMException to continue being an interface but have behavior identical to the spec's... (e.g. IDL support for own value properties).

@domenic
Copy link
Member Author

domenic commented Mar 2, 2015

ping

It will automatically set the code from the name (if it is one of the legacy names that have codes).

It now just calls `super(message)` to initialize the message and all of the internal error-related state.

The "creating exceptions" section was refactored to work in terms of the new, fully-functional constructor.

Closes https://www.w3.org/Bugs/Public/show_bug.cgi?id=27062.
@domenic
Copy link
Member Author

domenic commented Apr 13, 2015

@heycam @bzbarsky ping. Rebased on top of the current stuff and made some minor updates for the NewTarget stuff in ES.

@heycam
Copy link
Collaborator

heycam commented Apr 17, 2015

Thanks Domenic, and sorry again for the delay. This looks good.

I'm not sure if there is great consistency in the ES spec about whether exception-ignoring [[DefineOwnProperty]] calls or DefinePropertyOrThrow should be used in general, but the latter (which you've changed to, for setting message and code) does look more common. I wonder if there are other places in Web IDL where we should switch to DefinePropertyOrThrow?

Now that you have introduced the «1, 2, 3» syntax for abstract list values, there are probably a bunch of places in the spec that we should update to use it.

heycam added a commit that referenced this pull request Apr 17, 2015
Make the DOMException constructor accept a name
@heycam heycam merged commit 370fc8c into whatwg:gh-pages Apr 17, 2015
@bzbarsky
Copy link
Collaborator

In theory, DefinePropertyOrThrow is identical to [[DefineOwnProperty]] if the object's [[DefineOwnProperty]] is the one defined by http://people.mozilla.org/~jorendorff/es6-draft.html#sec-ordinary-object-internal-methods-and-internal-slots-defineownproperty-p-desc and the object was just created (so doesn't have a property with that name).

But yes, I would think that what engines use internally is closer to DefinePropertyOrThrow, in that if it fails (e.g. due to OOM, which the spec doesn't really consider) an exception will be thrown.

@heycam
Copy link
Collaborator

heycam commented Apr 17, 2015

Yeah, that's fair. Though the author could replace window.Object with a constructor that defines some non-configurable properties on the instance after it's created?

@bzbarsky
Copy link
Collaborator

That doesn't affect objects created via object literal {} (which is what e.g. dictionary IDL-to-ES conversion is supposed to use) or via other invocations of ObjectCreate with an empty internalSlotsList.

@heycam
Copy link
Collaborator

heycam commented Apr 17, 2015

Ah indeed, great.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants