Bug 96198 – [V8] setNamedHiddenWindowReference doesn't need to be a special case

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Adam Barth]

[V8] setNamedHiddenWindowReference doesn't need to be a special case

[Adam Barth]

Created attachment 162982 [details]
Patch

[Adam Barth]

I haven't looked into the history here to see why this code is the way it is.  We might want to do that before landing this patch.

[Kentaro Hara]

Comment on attachment 162982 [details]
Patch

Sounds reasonable to me, but I'm not 100% sure. Would you cc a guy who wrote this logic?

[Kentaro Hara]

(In reply to comment #2)
> We might want to do that before landing this patch.

Yeah, sounds good.

[Adam Barth]

Yes, this one is trickier than the others.  Let's research it a bit more.

[WebKit Review Bot]

Comment on attachment 162982 [details]
Patch

Attachment 162982 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13801329

New failing tests:
fast/dom/Window/customized-property-survives-gc.html

[Adam Barth]

Interesting.  The generated code for window.location is different than for other properites:

static v8::Handle<v8::Value> cryptoAttrGetter(v8::Local<v8::String> name, const v8::AccessorInfo& info)
{
    INC_STATS("DOM.DOMWindow.crypto._get");
    DOMWindow* imp = V8DOMWindow::toNative(info.Holder());
    RefPtr<Crypto> result = imp->crypto();
    v8::Handle<v8::Value> wrapper = result.get() ? getDOMObjectMap(info.GetIsolate()).get(result.get()) : v8Undefined();
    if (wrapper.IsEmpty()) {
        wrapper = toV8(result.get(), info.Holder(), info.GetIsolate());
        if (!wrapper.IsEmpty())
            V8DOMWrapper::setNamedHiddenReference(info.Holder(), "crypto", wrapper);
    }
    return wrapper;
}

static v8::Handle<v8::Value> locationAttrGetter(v8::Local<v8::String> name, const v8::AccessorInfo& info)
{
    INC_STATS("DOM.DOMWindow.location._get");
    v8::Handle<v8::Object> holder = info.Holder();
    DOMWindow* imp = V8DOMWindow::toNative(holder);
    return toV8(imp->location(), info.Holder(), info.GetIsolate());
}

[Adam Barth]

Created attachment 162999 [details]
Patch

[Adam Barth]

I'm not happy about the special case for location in CodeGeneratorV8.pm.  The issue is that window.location is neither readonly nor replaceable.  In principle, it should be readonly, except that there's a legacy setter for window.location that navigates the frame.

[Adam Barth]

Looks like we originally used to do this just for DOMWindow, but Nate generalized it to work with other objects as well in <https://bugs.webkit.org/show_bug.cgi?id=36777>.

Nate: Is there any reason why we can't use the general-purpose logic for DOMWindow as well?

[Nate Chapin]

Comment on attachment 162999 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=162999&action=review

If there was a reason the difference between DOMWindow handling and everything else, I've long since forgotten and haven't succeeded in deriving it again. I'm guessing I cargo-culted it.

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:988
>      # the newly created wrapper into an internal field of the holder object.
> -    if (!IsNodeSubType($dataNode) && $attrName ne "self" && (IsWrapperType($returnType) && ($attribute->type =~ /^readonly/ || $attribute->signature->extendedAttributes->{"Replaceable"})
> +    if (!IsNodeSubType($dataNode) && $attrName ne "self" && (IsWrapperType($returnType) && ($attribute->type =~ /^readonly/ || $attribute->signature->extendedAttributes->{"Replaceable"} || $attrName eq "location")
>          && $returnType ne "EventTarget" && $returnType ne "SerializedScriptValue" && $returnType ne "DOMWindow" 

This if statement is the most embarrassing code I've written in my career thus far. I'm sad to add to it, but it's still better than the current state of the code.

[Adam Barth]

(In reply to comment #11)
> (From update of attachment 162999 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=162999&action=review
> 
> If there was a reason the difference between DOMWindow handling and everything else, I've long since forgotten and haven't succeeded in deriving it again. I'm guessing I cargo-culted it.
> 
> > Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:988
> >      # the newly created wrapper into an internal field of the holder object.
> > -    if (!IsNodeSubType($dataNode) && $attrName ne "self" && (IsWrapperType($returnType) && ($attribute->type =~ /^readonly/ || $attribute->signature->extendedAttributes->{"Replaceable"})
> > +    if (!IsNodeSubType($dataNode) && $attrName ne "self" && (IsWrapperType($returnType) && ($attribute->type =~ /^readonly/ || $attribute->signature->extendedAttributes->{"Replaceable"} || $attrName eq "location")
> >          && $returnType ne "EventTarget" && $returnType ne "SerializedScriptValue" && $returnType ne "DOMWindow" 
> 
> This if statement is the most embarrassing code I've written in my career thus far. I'm sad to add to it, but it's still better than the current state of the code.

I'll take a look to see if there's some way of making it better.

[WebKit Review Bot]

Comment on attachment 162999 [details]
Patch

Clearing flags on attachment: 162999

Committed r128102: <http://trac.webkit.org/changeset/128102>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.