Fix second half of issue 1151, the first change (r6765) only fixed FunctionGe...

Fix second half of issue 1151, the first change (r6765) only fixed FunctionGetPrototype, not FunctionSetPrototype.


Committed: http://code.google.com/p/v8/source/detail?r=6893

[Mads Ager (chromium), 2011-02-21 14:10:51]

LGTM

http://codereview.chromium.org/6548008/diff/6/src/accessors.cc
File src/accessors.cc (right):

http://codereview.chromium.org/6548008/diff/6/src/accessors.cc#newcode470
src/accessors.cc:470: if (!function->should_have_prototype()) return
Heap::undefined_value();
Does the return value here have any significance? In particular, should we
return |function| here so the only thing that is different is that we don't get
a prototype property?

[Mads Ager (chromium), 2011-02-21 14:17:07]

Hmm, what does should_have_prototype mean here? Does it mean that the function
should not initially have a prototype? Maybe the assert is wrong here and we
should allow setting the prototype (and getting it once set)?

[Rico, 2011-02-22 09:47:03]

As discussed offline we did this inconsistently and did not match JSC. I
uploaded a new version that mimics the behavior of Safari and still will not
allow to use objects that has should_have_prototype false as a constructor.

[Mads Ager (chromium), 2011-02-22 10:17:07]

I think this can be simplified, but this looks like the right fix.

http://codereview.chromium.org/6548008/diff/4001/src/accessors.cc
File src/accessors.cc (right):

http://codereview.chromium.org/6548008/diff/4001/src/accessors.cc#newcode450
src/accessors.cc:450: if (!function->should_have_prototype()) {
Can we simplify this? How about moving this before the
!function->has_prototype() check and doing something like:

while (!function->should_have_prototype()) {
  found_it = false;
  function = FindInPrototypeChain<JSFunction>(object->GetPrototype(),
&found_it);
  ASSERT(found_it);  // There has to be one because we hit the getter
}

That should be all that we need. Then the original code will do the right thing
as far as I can tell?

http://codereview.chromium.org/6548008/diff/4001/src/accessors.cc#newcode453
src/accessors.cc:453: Handle<String> proto_name =
This code does not use handles. You can use Heap::prototype_symbol() below
instead.

http://codereview.chromium.org/6548008/diff/4001/src/accessors.cc#newcode486
src/accessors.cc:486: return object->SetLocalPropertyIgnoreAttributes(
Please add a comment stating that object does not have a prototype property
since we hit this accessor.

http://codereview.chromium.org/6548008/diff/4001/src/accessors.cc#newcode487
src/accessors.cc:487: *Factory::NewStringFromAscii(CStrVector("prototype")),
Heap::prototype_symbol()

[Rico, 2011-02-22 12:22:44]

Comments addressed

http://codereview.chromium.org/6548008/diff/4001/src/accessors.cc
File src/accessors.cc (right):

http://codereview.chromium.org/6548008/diff/4001/src/accessors.cc#newcode450
src/accessors.cc:450: if (!function->should_have_prototype()) {
On 2011/02/22 10:17:07, Mads Ager wrote:
> Can we simplify this? How about moving this before the
> !function->has_prototype() check and doing something like:
> 
> while (!function->should_have_prototype()) {
>   found_it = false;
>   function = FindInPrototypeChain<JSFunction>(object->GetPrototype(),
> &found_it);
>   ASSERT(found_it);  // There has to be one because we hit the getter
> }
> 
> That should be all that we need. Then the original code will do the right
thing
> as far as I can tell?

Done.

http://codereview.chromium.org/6548008/diff/4001/src/accessors.cc#newcode453
src/accessors.cc:453: Handle<String> proto_name =
On 2011/02/22 10:17:07, Mads Ager wrote:
> This code does not use handles. You can use Heap::prototype_symbol() below
> instead. 

Done.

http://codereview.chromium.org/6548008/diff/4001/src/accessors.cc#newcode486
src/accessors.cc:486: return object->SetLocalPropertyIgnoreAttributes(
On 2011/02/22 10:17:07, Mads Ager wrote:
> Please add a comment stating that object does not have a prototype property
> since we hit this accessor.

Done.

http://codereview.chromium.org/6548008/diff/4001/src/accessors.cc#newcode487
src/accessors.cc:487: *Factory::NewStringFromAscii(CStrVector("prototype")),
On 2011/02/22 10:17:07, Mads Ager wrote:
> Heap::prototype_symbol()

Done.

[Mads Ager (chromium), 2011-02-22 12:24:31]

LGTM

http://codereview.chromium.org/6548008/diff/6001/src/accessors.cc
File src/accessors.cc (right):

http://codereview.chromium.org/6548008/diff/6001/src/accessors.cc#newcode453
src/accessors.cc:453: ASSERT(found_it);  // There has to be one because we hit
the getter
Period at end of comment. Maybe move the comment to the line above the assert?