Bug 96198

Summary: [V8] setNamedHiddenWindowReference doesn't need to be a special case
Product: WebKit Reporter: Adam Barth <abarth>
Component: New BugsAssignee: Adam Barth <abarth>
Status: RESOLVED FIXED    
Severity: Normal CC: dglazkov, eric, haraken, japhet, ojan, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch none

Adam Barth
Reported 2012-09-09 00:23:35 PDT
[V8] setNamedHiddenWindowReference doesn't need to be a special case
Attachments
Patch (6.95 KB, patch)
2012-09-09 00:27 PDT, Adam Barth
no flags
Patch (7.96 KB, patch)
2012-09-09 09:35 PDT, Adam Barth
no flags
Adam Barth
Comment 1 2012-09-09 00:27:24 PDT
Adam Barth
Comment 2 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.
Kentaro Hara
Comment 3 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?
Kentaro Hara
Comment 4 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.
Adam Barth
Comment 5 2012-09-09 00:41:41 PDT
Yes, this one is trickier than the others. Let's research it a bit more.
WebKit Review Bot
Comment 6 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
Adam Barth
Comment 7 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()); }
Adam Barth
Comment 8 2012-09-09 09:35:02 PDT
Adam Barth
Comment 9 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.
Adam Barth
Comment 10 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?
Nate Chapin
Comment 11 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.
Adam Barth
Comment 12 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.
WebKit Review Bot
Comment 13 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>
WebKit Review Bot
Comment 14 2012-09-10 13:54:25 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.