This is a fake property for getting a RuntimeObjectImp wrapper for a plug-in element in some sort of pseudo-virtual call dispatch. There is no need for such a complicated trick, as far as I can tell.
Created attachment 49240 [details] proposed patch
Comment on attachment 49240 [details] proposed patch > +namespace JSC { > +namespace Bindings { > +class Instance; > +} > +} That indentation is probably "right", but it looks so wrong! > + HTMLElement* el = static_cast<JSHTMLElement*>(object)->impl(); How about using the entire word "element" here instead of "el"? > + Instance* instance = 0; > - RefPtr<Instance> instance = imp->getInternalInstance(); Any danger in the lifetime of instance here? r=me
Committed <http://trac.webkit.org/changeset/55104>. > How about using the entire word "element" here instead of "el"? Ok. > Any danger in the lifetime of instance here? I was sure there isn't, but while writing the answer, I became unsure. Let me just revert this part...
Changed back to RefPtr in r55105. A plain pointer may or may not be OK, but that's not what I'm fixing here.
The chromium is all geborked! Strange that http://build.webkit.org/console seems to incriminate this change, but that the EWS bots didn't. /Users/cltbld/Desktop/BuildSlaveData/WebKit-BuildSlave/chromium-mac-release/build/WebCore/WebCore.gyp/../bindings/v8/custom/V8HTMLPlugInElementCustom.cpp:77: error: no ‘v8::Handle<v8::Value> WebCore::V8HTMLAppletElement::namedPropertyGetter(v8::Local<v8::String>, const v8::AccessorInfo&)’ member function declared in class ‘WebCore::V8HTMLAppletElement’
We relied on HasOverridingNameGetter as hint for custom getters here. Removing it made things go kablooey. I think we'll need to special-case this in code generator.
(In reply to comment #6) > We relied on HasOverridingNameGetter as hint for custom getters here. Removing > it made things go kablooey. > > I think we'll need to special-case this in code generator. Chromium build fix was http://trac.webkit.org/changeset/55111, looks like we're ok now.
I think that the right fix would be to remove namedPropertyGetter instead.
(In reply to comment #8) > I think that the right fix would be to remove namedPropertyGetter instead. That would remove ability to query underlying NP object by named properties. Do we want that?
In JSC, this named getter was only used for getting a "RuntimeObject" corresponding to the plugin (applet/embed/object) element via an __apple_runtime_object property. A RuntimeObject is a JavaScript object that wraps e.g. an NPObject or an Objective C object that are used to communicate with plug-ins. JS code from web sites was never supposed to access the __apple_runtime_object property, and as far as I can tell, it never did. This was purely a (super confusing) internal hack. It it somehow different for v8?
In JSC you have getOwnPropertySlotDelegate, which can be used to both intercept named property lookup (like in HTMLApplet|Object|EmbedElement) or other things like security checks in History. In V8, there's a distinct API to hook up named property getters and a distinct API for security checks.
Why are the V8 bindings different here? It should try and conform to WebKit's JS engine's conventions if it wishes to be in the WebKit tree.
(In reply to comment #12) > Why are the V8 bindings different here? It should try and conform to WebKit's > JS engine's conventions if it wishes to be in the WebKit tree. Try we must, and try we do. Some concepts translate easily from JSC to V8 API, some don't. We don't want to see breakages like this, either. Hence the major refactoring of the bindings we've been doing in the last 3 months.