Bug 35260 – Eliminate __apple_runtime_object

• Component: Plug-ins
• Status: RESOLVED FIXED

[Alexey Proskuryakov]

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.

[Alexey Proskuryakov]

Created attachment 49240 [details]
proposed patch

[Darin Adler]

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

[Alexey Proskuryakov]

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...

[Alexey Proskuryakov]

Changed back to RefPtr in r55105. A plain pointer may or may not be OK, but that's not what I'm fixing here.

[Eric Seidel (no email)]

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’

[Dimitri Glazkov (Google)]

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.

[Nate Chapin]

(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.

[Alexey Proskuryakov]

I think that the right fix would be to remove namedPropertyGetter instead.

[Dimitri Glazkov (Google)]

(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?

[Alexey Proskuryakov]

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?

[Dimitri Glazkov (Google)]

 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.

[Sam Weinig]

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.

[Dimitri Glazkov (Google)]

(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.