SO::constructProperty is never overridden, anywhere

RESOLVED FIXED

Status

defect
RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: stejohns, Assigned: stejohns)

Tracking

Details

Attachments

(2 attachments)

Not even in Flash/AIR. So let's eliminate it and combine the single instance into the single callsite.
Posted patch PatchSplinter Review
Assignee: nobody → stejohns
Attachment #516399 - Flags: review?(edwsmith)
FWIW: setAtomPropertyIsEnumerable is also never overridden, so could be made nonvirtual.
Comment on attachment 516399 [details] [diff] [review]
Patch

In the old code, we only replaced the receiver in the virtual call, which was only used for kObjectType atoms.  In the new code, it happens for primitives too.  Which one is right?  If we dont have a test case to detect the difference, then we need one.  If we have one, is the change not detectable?
Attachment #516399 - Flags: review?(edwsmith) → review-
good catch. I'll try to find out.
Yow... apparently we don't have a testcase that tickles *either* branch of the default case.
(In reply to comment #5)
> Yow... apparently we don't have a testcase that tickles *either* branch of the
> default case.

I stand corrected: we have a handful that touch the non-primitive case, but none for the primitive case. I'll remedy that.
So this code can be tickled by something like

   var a = 1;
   var b = new a.foo();

which will of course always throw

   TypeError: Error #1007: Instantiation attempted on a non-constructor 

But if we finesse it by doing, say,

   Number.prototype.foo = Number;
   var a = 1;
   var b = new a.foo();

we'll get a useful test.
Posted patch TestcaseSplinter Review
Testcase that tickles this path for all primitive types, both in failure and success modes.

Testcase passes both with and without the actual patch in question applied, so you may want to revisit your R-.
Attachment #516399 - Attachment is obsolete: true
Attachment #519728 - Flags: review?(edwsmith)
Attachment #516399 - Attachment is obsolete: false
Comment on attachment 516399 [details] [diff] [review]
Patch

Can you add a comment explaining why the "replace receiver" is there?
Attachment #516399 - Flags: review- → review+
Attachment #519728 - Flags: review?(edwsmith) → review+
TR 6101:82b4008b9b32
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.