[V8] setNamedHiddenWindowReference doesn't need to be a special case
Created attachment 162982 [details] Patch
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 on attachment 162982 [details] Patch Sounds reasonable to me, but I'm not 100% sure. Would you cc a guy who wrote this logic?
(In reply to comment #2) > We might want to do that before landing this patch. Yeah, sounds good.
Yes, this one is trickier than the others. Let's research it a bit more.
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
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()); }
Created attachment 162999 [details] Patch
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.
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 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.
(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 on attachment 162999 [details] Patch Clearing flags on attachment: 162999 Committed r128102: <http://trac.webkit.org/changeset/128102>
All reviewed patches have been landed. Closing bug.