Bug 96198 - [V8] setNamedHiddenWindowReference doesn't need to be a special case
Summary: [V8] setNamedHiddenWindowReference doesn't need to be a special case
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Adam Barth
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-09-09 00:23 PDT by Adam Barth
Modified: 2012-09-10 13:54 PDT (History)
6 users (show)

See Also:


Attachments
Patch (6.95 KB, patch)
2012-09-09 00:27 PDT, Adam Barth
no flags Details | Formatted Diff | Diff
Patch (7.96 KB, patch)
2012-09-09 09:35 PDT, Adam Barth
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Adam Barth 2012-09-09 00:23:35 PDT
[V8] setNamedHiddenWindowReference doesn't need to be a special case
Comment 1 Adam Barth 2012-09-09 00:27:24 PDT
Created attachment 162982 [details]
Patch
Comment 2 Adam Barth 2012-09-09 00:28:05 PDT
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.
Comment 3 Kentaro Hara 2012-09-09 00:31:45 PDT
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?
Comment 4 Kentaro Hara 2012-09-09 00:32:44 PDT
(In reply to comment #2)
> We might want to do that before landing this patch.

Yeah, sounds good.
Comment 5 Adam Barth 2012-09-09 00:41:41 PDT
Yes, this one is trickier than the others.  Let's research it a bit more.
Comment 6 WebKit Review Bot 2012-09-09 01:28:26 PDT
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
Comment 7 Adam Barth 2012-09-09 09:01:35 PDT
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());
}
Comment 8 Adam Barth 2012-09-09 09:35:02 PDT
Created attachment 162999 [details]
Patch
Comment 9 Adam Barth 2012-09-09 09:36:15 PDT
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.
Comment 10 Adam Barth 2012-09-09 10:14:18 PDT
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?
Comment 11 Nate Chapin 2012-09-10 13:42:35 PDT
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.
Comment 12 Adam Barth 2012-09-10 13:44:09 PDT
(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.
Comment 13 WebKit Review Bot 2012-09-10 13:54:21 PDT
Comment on attachment 162999 [details]
Patch

Clearing flags on attachment: 162999

Committed r128102: <http://trac.webkit.org/changeset/128102>
Comment 14 WebKit Review Bot 2012-09-10 13:54:25 PDT
All reviewed patches have been landed.  Closing bug.