Address review feedback

[leobalter]

Ref #304

Address review feedback from @ljharb.

---

why do you need to wrap the completion record value in a new record, as opposed to just passing on the completion record (when its type is normal or return)?

This seems like a bug, I'm removing the Completion wrapping.

Additionally, since the only possible types of completion record from the invocation is "abrupt" or "normal or return", it kind of seems like you could branch on whether it's abrupt, and not have to explicitly mention normal/return.

I'd say for the editorial style. If check if the type is abrupt, I'd be compelled to add an assertion step saying completion type must be normal or return to clarity it won't be "continue or break". The positive if for normal or return makes the spec steps more clear. I'd prefer to defer this decision to an eventual PR to ECMA-262 if possible.

Can eval, Function, and https://tc39.es/proposal-realms/#sec-performrealmeval be made to maximally share steps/AOs? It seems like it'd be helpful if they could all be expressed in terms of the same operation(s).

I agree the 3 of them seem good enough to make an abstraction, but this also seems more appropriate to a PR to ECMA-262? I can add an editorial note to point out the intention.

Should the name of the [[Realm]] slot be the same on functions and Realm instances? Perhaps it should, if it's holding the same types of values, but perhaps it'd be better to differentiate the two?

Thanks for catching this! We don't need this [[Realm]] internal in Wrapped Function Exotic Objects so I'm removing it for now. With some further review, the name would actually create weird hazard in Realm.prototype.evaluate.

"Perform" is typically the verb associated with calling an AO and ignoring any non-abrupt return value. Can https://tc39.es/proposal-realms/#sec-performrealmimportvalue start with something besides "Perform"?

Trying to not bikeshed too much over the name, I'm renaming the abstraction to RealmImportValue, an alternative would be ImportRealmValue but I don't think there is no perfect option here.

I'm not sure what all the "Extensible web:" phrases are in the notes - we don't have this anywhere in the spec, and it doesn't link anywhere. What's the purpose of that note prefix?

These are just editorial notes to help connecting to the ongoing integration with HTML, I don't think they should all be part of the final PR to ECMA-262.

In https://tc39.es/proposal-realms/#sec-hostimportmodulebindingdynamically: the "failure path" sentence reads oddly to me, and I'm not sure what it means exactly. Also, for the "success path" case, should the completion record's type be constrained to be "normal"?

Note taken. I'm gonna try to address this abstraction in a separate PR as I also have feedback from @syg about it.

[leobalter]

I added [[Call]] to the table to acknowledge the different behavior

[ljharb]

Can those "extensible web" notes be marked as editors notes then? that way they can't possibly show up in the rendered main spec, and they'll get special styling to note them as such

[leobalter]

@ljharb: Can those "extensible web" notes be marked as editors notes then? that way they can't possibly show up in the rendered main spec, and they'll get special styling to note them as such

done! 53da06d

[leobalter]

value disappears, I need to send _result_ here

[leobalter]

f11bbef

[caridy]

LGTM