Bug 29816 – Web Inspector: Encapsulate JS listeners specifics into ScriptObject.

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector (Deprecated)

[Pavel Feldman]

We should not use ifdef(JSC) in dom agent logic...

[Pavel Feldman]

Created attachment 40247 [details]
patch

[Timothy Hatcher]

Comment on attachment 40247 [details]
patch


> -#if USE(JSC)
> -    JSC::JSObject* functionObject = eventListener->jsFunction();
> -    if (functionObject)
> -        value.set("listener", ScriptObject(m_frontend->scriptState(), functionObject));
> -#endif
> +    value.set("listener", ScriptObject::getHandlerFunctionBody(m_frontend->scriptState(), eventListener.get()));

The original code got the function object, the new code gets the function body as a string. How do we pass over other functions to the Inspector now (like in the Console or properties pane)? We should be consistent, either way.

While it is nice to avoid #ifdefs where possible, I don't think a static function on ScriptObject that is specific to event listeners is better.

I would argue for just putting the V8 code into InspectorDOMAgent.cpp or have a way to get a ScriptObject from a EventListener*.

[Pavel Feldman]

(In reply to comment #2)
> The original code got the function object, the new code gets the function body
> as a string. How do we pass over other functions to the Inspector now (like in
> the Console or properties pane)? We should be consistent, either way.
> 

Detailed answer for the sake of documentation.

When we need to pass a function or an object to the frontend, we wrap it with ObjectProxy containing the original object's id. We use this id to resolve the original object later, for example when frontend requests object properties.

There are different types of ids and we use them in order to resolve inspected page objects differently. When some natural ids are available, we use them (like node id from DOMAgent mapping or call frame number from stack). Otherwise we call InspectorController.wrapObject and cache original object by the generated id key.

We use generated ids for console evaluation and for watch expressions so that evaluation results are not collected immediately and could be explored later. This wrapObject cache is cleared upon console cleanup. I have a pending action item to implement explicit 'release' for object proxies so that watch expressions could release evaluation results upon refresh. Currently they leak.

Back to our case. In order to pass event handler consistently, we need to come up with yet another type of the id. So that when frontend queries for event listener properties we can resolve the listener. But there is no natural key for this case: combined key (node id) x (event type) does not work. This leaves an option of using artificial id by means of InspectorController.wrapObject and then release it when node is deselected. This option is not bad, it is just that release is not implemented yet. I will actually do it right now.

So the reason I was sending the body only is that I was basically avoiding complexity. As I understood, the plan was not to use ObjectPropertiesSection for event listeners' rendering (Joe?) and I thought that in the new UI, body would be sufficient. Who wants to implement another renderer of the object proxies...

> While it is nice to avoid #ifdefs where possible, I don't think a static
> function on ScriptObject that is specific to event listeners is better.
> 
> I would argue for just putting the V8 code into InspectorDOMAgent.cpp or have a
> way to get a ScriptObject from a EventListener*.

But if we'd like to explore function properties as a tree (consistently with object properties) and reuse ObjectPropertiesSection, I agree that wrapping this function and passing it into frontend as a ScriptObject (as we do in other places) makes perfect sense. Let me implement InspectorController.releaseObject() and come up with the better implementation for this one!

[Timothy Hatcher]

Lets either land a change with #ifdefs or make real ScriptObjects like you mention. But the ScriptObject::getHandlerFunctionBody function does not sit well with me.

[Pavel Feldman]

Created attachment 40257 [details]
patch

As agreed on IRC, adding getEventListenerHandlerBody into ScriptEventListener.h.

[Pavel Feldman]

Created attachment 40258 [details]
(same with null change in ScriptObject.h reverted)

[Pavel Feldman]

Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	WebCore/ChangeLog
	M	WebCore/bindings/js/ScriptEventListener.cpp
	M	WebCore/bindings/js/ScriptEventListener.h
	M	WebCore/bindings/v8/ScriptEventListener.cpp
	M	WebCore/bindings/v8/ScriptEventListener.h
	M	WebCore/inspector/InspectorDOMAgent.cpp
Committed r48866

[Joseph Pecoraro]

(In reply to comment #3)
> Detailed answer for the sake of documentation.

I'm just catching up on this weeks emails. Thanks for this comment!  It helps in understanding the big picture. 

(In reply to comment #5)
> As agreed on IRC, adding getEventListenerHandlerBody into
> ScriptEventListener.h.

I was interested in the front-end getting the raw function so that JavaScript in the front-end could make use of that function even if it it was inaccessible via the DOM.  A use cases would have been applying that same listener to another element.  However, since interaction between the front-end and inspected page is serialized I guess it wouldn't be that useful to provide the raw function to the inspector code.  Just providing the function's body was probably a good decision.

Also, if there were any UI suggestions in your IRC discussion please throw a comment on bug 29789.

Cheers.