1139351 - Address some remaining review comments in bug 1113369 comment 62

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Jason Orendorff [:jorendorff]]

Edited version of bug 1113369 comment 62 below:

>+++ b/dom/bindings/Codegen.py
>@@ -10200,140 +10197,157 @@ class CGDOMJSProxyHandler_defineProperty
>+                  return js::IsInNonStrictPropertySet(cx)
>+                         ? opresult.succeed()
>+                         : ThrowErrorMessage(cx, MSG_NO_INDEXED_SETTER, "${name}");

This should turn into a "return opresult.fail(something)" instead of the manual js::IsInNonStrictPropertySet check.

>+                      return js::IsInNonStrictPropertySet(cx)
>+                             ? opresult.succeed()
>+                             : ThrowErrorMessage(cx, MSG_NO_NAMED_SETTER, "${name}");

Same here.

> class CGDOMJSProxyHandler_delete(ClassMethod):

Again, the uses of failCantDelete here in the "indexedBody" and "namedBody" bits will lead to misleading error messages.  Followup to fix that.

> DOMProxyHandler::defineProperty(JSContext* cx, JS::Handle<JSObject*> proxy, JS::Handle<jsid> id,

The failGetterOnly bit here is a bit weird, but it's preexisting-weird.  Please file a followup on figuring out what it's trying to do and documenting it?

Comment 1

[Boris Zbarsky [:bzbarsky]]

Jason, are you planning to do this, or should I?

Comment 2

[Jason Orendorff [:jorendorff]]

I was planning to do it, but I do have about 40 patches to land first. Your comments that are not echoed above are already addressed in my local tree.

Comment 3

[Boris Zbarsky [:bzbarsky]]

So I took a stab at this.  The problem with the NO_NAMED_SETTER/NO_INDEXED_SETTER cases is that right now they report the name of the interface involved, but in the new world I see no provisions for it.  Could we have some way of doing that (e.g. via the clasp->name)?

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: What I have so far

The messages here are wrong for the case of no indexed/named setter, since {0} ends up being the prop name, not the object type

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: Small testcase to exercise some of these codepaths

Comment 6

[Jason Orendorff [:jorendorff]]

(In reply to Boris Zbarsky [:bz] from comment #3)
> So I took a stab at this.  The problem with the
> NO_NAMED_SETTER/NO_INDEXED_SETTER cases is that right now they report the
> name of the interface involved, but in the new world I see no provisions for
> it.  Could we have some way of doing that (e.g. via the clasp->name)?

This has since been resolved exactly as proposed. I don't know if there's anything left to do in this bug or not.

Comment 7

[Boris Zbarsky [:bzbarsky]]

I think bug 1135993 fixed all that was left to fix here.