Closed Bug 1139351 Opened 6 years ago Closed 5 years ago

Address some remaining review comments in bug 1113369 comment 62

Categories

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

x86_64
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: jorendorff, Unassigned)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

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?
Jason, are you planning to do this, or should I?
Flags: needinfo?(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.
Flags: needinfo?(jorendorff)
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)?
Flags: needinfo?(jorendorff)
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
Depends on: 1163423
Depends on: 1135993
No longer depends on: 1163423
(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.
Flags: needinfo?(jorendorff)
I think bug 1135993 fixed all that was left to fix here.
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.