Bug 17429 - Inspector should show event listeners/handlers registered on each node
Summary: Inspector should show event listeners/handlers registered on each node
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated) (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Enhancement
Assignee: Joseph Pecoraro
URL:
Keywords: HasReduction, InRadar, ReviewedForRadar
Depends on: 23318
Blocks:
  Show dependency treegraph
 
Reported: 2008-02-18 17:31 PST by Adam Roben (:aroben)
Modified: 2009-09-28 21:59 PDT (History)
12 users (show)

See Also:


Attachments
Test case (157 bytes, text/html)
2008-05-24 17:50 PDT, David Kilzer (:ddkilzer)
no flags Details
[PATCH] For Discussion (8.19 KB, application/octet-stream)
2009-08-20 22:36 PDT, Joseph Pecoraro
no flags Details
[IMAGE] EventListener Data for a Selected Node (257.36 KB, image/png)
2009-08-20 23:00 PDT, Joseph Pecoraro
no flags Details
[IMAGE] Showing like ObjectProperties (215.48 KB, image/png)
2009-08-25 19:26 PDT, Joseph Pecoraro
no flags Details
[TEST CASE] More elaborate test case that I will be working with. (2.08 KB, text/html)
2009-08-25 20:50 PDT, Joseph Pecoraro
no flags Details
Possible Design (6.53 KB, image/png)
2009-08-27 14:14 PDT, Timothy Hatcher
no flags Details
Possible Design 2 (6.65 KB, image/png)
2009-08-27 20:23 PDT, Timothy Hatcher
no flags Details
Connector Images (1.36 KB, application/zip)
2009-08-27 20:27 PDT, Timothy Hatcher
no flags Details
Connector Images 2 (1.37 KB, application/zip)
2009-08-27 20:45 PDT, Timothy Hatcher
no flags Details
Possible Design 3 (6.63 KB, image/png)
2009-08-27 20:46 PDT, Timothy Hatcher
no flags Details
[PATCH] Event Listeners Version 1 (37.82 KB, patch)
2009-09-24 15:08 PDT, Joseph Pecoraro
no flags Details | Formatted Diff | Diff
[IMAGE] Event Listeners Version 1 (156.52 KB, image/png)
2009-09-24 15:10 PDT, Joseph Pecoraro
no flags Details
[PaTCH] Event Listeners Version 2 (41.38 KB, patch)
2009-09-25 00:06 PDT, Joseph Pecoraro
no flags Details | Formatted Diff | Diff
[PATCH] Event Listeners Version 3 (46.07 KB, patch)
2009-09-25 10:28 PDT, Joseph Pecoraro
no flags Details | Formatted Diff | Diff
[PATCH] Event Listeners Version 4 (47.14 KB, patch)
2009-09-27 14:01 PDT, Joseph Pecoraro
timothy: review+
Details | Formatted Diff | Diff
[PATCH] Removed InspectorController.wrapObject call from the frontend side. (4.56 KB, patch)
2009-09-28 07:22 PDT, Pavel Feldman
eric: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Adam Roben (:aroben) 2008-02-18 17:31:04 PST
The Inspector should show the event listeners/handlers registered on each node. At a minimum we could show the parameters passed to addEventListener, in addition to any onmousedown etc. attributes.
Comment 1 Brady Eidson 2008-02-18 17:48:42 PST
<rdar://problem/5751140>
Comment 2 David Kilzer (:ddkilzer) 2008-05-24 17:50:48 PDT
Created attachment 21328 [details]
Test case

* STEPS TO REPRODUCE
1. Launch Safari/WebKit.
2. Open test case.
3. Inspect button labeled "Inspect Me".
4. Try to find the "onclick" event handler defined on the button's properties.

* RESULTS
The onclick event handler is not shown in the button element's properties.
Comment 3 Tak 2008-09-03 11:33:07 PDT
Id love to see this as well.

It'd be incredibly useful to know that functionX and functionY and functionZ have attached themselves to this node's "click" handler.  

It'd also be useful to be able to inspect up/down to verify/trace the event bubbling. EG: a click handler that manages a UL dom node .. is it attached to the individual LIs or the UL..  eg: Firefox's (not firebug) native Web Inspector for CSS shows the heirarchy of CSS rules that are put into play for any particular node and which ones are overriden, etc.  Something like that for event bubbling would be amazingly uesful.

A consequence of the bubbling might be the ability to mark a particular function as being assigned directly or as a consequence of the bubble.

eg: 

DOM Node.UL
+ click
++ FunctionX (attached directly)
++ FunctionY (attached via bubble from domNode xxxx)
++ FunctionZ (attached via bubble from DomNode xxxx)
Comment 4 Anthony Ricaud 2008-12-10 08:13:58 PST
See http://www.sprymedia.co.uk/article/Visual+Event for an example of implementation. You can try it on http://jquery.com.

I think this is a cool mode and we could also add a Events panel with Styles, Metrics and Properties.
Comment 5 Joseph Pecoraro 2009-08-20 22:36:54 PDT
Created attachment 38356 [details]
[PATCH] For Discussion

Any thoughts on the best way to handle JSC versus v8?  Should I instead make this [Custom], develop a cross platform way to do this, or keep it a JSC guard like EventListener?
Comment 6 Joseph Pecoraro 2009-08-20 23:00:44 PDT
Created attachment 38362 [details]
[IMAGE] EventListener Data for a Selected Node

Reposting.
Comment 7 Timothy Hatcher 2009-08-21 11:27:12 PDT
I like how most of the logica can be shared with V8 and JSC, so I think that approch will work and is pretty clean looking.
Comment 8 Joseph Pecoraro 2009-08-25 19:26:47 PDT
Created attachment 38586 [details]
[IMAGE] Showing like ObjectProperties

I played around with this a little to start a UI and just made the most basic UI that I could, making another ObjectPropertiesSection.  I think I will probably make a nicer UI, but first we should discuss what data we want to make available.  This is what I currently have:

Need not be shown:
- ancestorDepth: would be 0 on the inspected node, and 1..2..3.. for parents to check bubbling
- index: index in Node.h's Vector, presumably affects the order they are fired, used in sorting

Shown to the User:
- isAttribute: eventListener set on the attribute: elem.onclick = function() {...}
- listener: the function itself: elem.addEventListener(..., func, ...);
- type: the event type: elem.addEventListener(type, ..., ...);
- useCapture: fire on the capturing phase: elem.addEventListener(..., ..., true);

Things I think we might want to show:
- function display name like the JavaScript Profiler displays
- the node/element its attached to in the case of "surrounding" nodes

Then its just a matter of making a UI for this.  One that would allow some sort of interaction with the listeners to remove them or display/work with them in the console.
Comment 9 Joseph Pecoraro 2009-08-25 20:23:50 PDT
Here are my ideas for the UI.  They come from my single experience with Automator from years ago!
http://bogojoker.com/snaps/event-listener-ui.png

1. It shows the exact chain of event listeners that are fired.
2. Expand/Collapse capability allows for the UI to not get cluttered with lots of events.
3. Easy to Delete a Listener
4. Possible to Drag a Listener (lots of functionality ideas)

Is this a Good/Bad direction?
Comment 10 Joseph Pecoraro 2009-08-25 20:50:39 PDT
Created attachment 38592 [details]
[TEST CASE] More elaborate test case that I will be working with.

This tests capturing, bubbling, addEventListener registered event listeners, attribute registered event listeners, anonymous functions and named functions.  Its not 100% perfect (SVG has some implications) but its enough to make a thorough test.
Comment 11 Anthony Ricaud 2009-08-26 02:38:05 PDT
IIRC, there is no guarantee on the order of events firing on one element. So it would maybe confuse authors if you expose such a UI.

On the other hand, listing events with capture first, then the target and finally bubbling would be nice.

For "surrounding nodes", web developers often use the term "event delegation" if it can help finding a good wording.

I can't wait to see that landing ! :)

(In reply to comment #9)
> Here are my ideas for the UI.  They come from my single experience with
> Automator from years ago!
> http://bogojoker.com/snaps/event-listener-ui.png
> 
> 1. It shows the exact chain of event listeners that are fired.
> 2. Expand/Collapse capability allows for the UI to not get cluttered with lots
> of events.
> 3. Easy to Delete a Listener
> 4. Possible to Drag a Listener (lots of functionality ideas)
> 
> Is this a Good/Bad direction?
Comment 12 Anthony Ricaud 2009-08-26 02:42:48 PDT
Oh I forgot, we should find a way to list events registered on the window or document.
Comment 13 Sam Weinig 2009-08-26 06:53:25 PDT
(In reply to comment #12)
> Oh I forgot, we should find a way to list events registered on the window or
> document.

If we are going to be adding more information like this to the elements pane, we may want to give some indications of document and window associated with that document.  I was thinking perhaps having the root node have some sort of pseudo-parent or sibling that was the document/window.  Not sure what it would look like.
Comment 14 Joseph Pecoraro 2009-08-26 08:07:05 PDT
(In reply to comment #11)
> IIRC, there is no guarantee on the order of events firing on one element. So it
> would maybe confuse authors if you expose such a UI.

Well, we can follow Node.h's dispatchGenericEvent() to actually produce the exact ordering of all of the event listeners for an event on a given node.  So, whether or not the spec has guaranteed an order we can at least provide developers with the exact ordering that WebKit will produce.

But is the spec really undefined in this regard?  I would be worried about browser compatibility if that were the case.  Here is the most current editors draft on DOM Event Flow:
http://dev.w3.org/2006/webapi/DOM-Level-3-Events/html/DOM3-Events.html#events-Events-flow

I glossed over it and I saw:
"Next, the implementation must determine the current target's candidate event listeners. This shall be the list of all event listeners that have been registered on the current target in their order of registration. Once determined, the candidate event listeners cannot be changed; adding or removing listeners does not affect the current target's candidate event listeners.

Finally, the implementation must process all candidate event listeners in order and trigger each listener if all the following conditions are met..."

Although its still in a draft, I think this is exactly how WebKit handles event flow and so I think we will be up to date if we can show it this way.

> On the other hand, listing events with capture first, then the target and
> finally bubbling would be nice.

Yes, I had originally intended that, and I have prepared tests for that in my attached test case. =)

(In reply to comment #13)
> (In reply to comment #12)
> > Oh I forgot, we should find a way to list events registered on the window or
> > document.
> 
> If we are going to be adding more information like this to the elements pane,
> we may want to give some indications of document and window associated with
> that document.  I was thinking perhaps having the root node have some sort of
> pseudo-parent or sibling that was the document/window.  Not sure what it would
> look like.

Document, window, and any other EventTargets that are not temporary (XMLHttpRequest is temporary).  A quick scan for things that extend EventTarget (see the list below) shows ApplicationCache and Workers would be good candidates as well.

I have had this in the back of my mind, and intentionally named this bug "on each node" for starters.  But I really like your idea on presenting document and window!

dom/MessagePort.h
dom/Node.h
loader/appcache/DOMApplicationCache.h
notifications/Notification.h
page/DOMWindow.h
page/EventSource.h
websockets/WebSocket.h
workers/AbstractWorker.h
workers/WorkerContext.h
xml/XMLHttpRequest.h
xml/XMLHttpRequestUpload.h
Comment 15 Sam Weinig 2009-08-26 09:29:39 PDT
Might it be time for a new pane? Maybe an event specific one?  Maybe a JS/DOM/Object specific one?  (•weinig defers to Tim's guidance)
Comment 16 Joseph Pecoraro 2009-08-26 23:19:51 PDT
Okay, I gave the UI a shot in HTML/CSS.  I'm open to suggestions on tweaks before I try to move it into the inspector:
http://bogojoker.com/inspector/eventui/
Comment 17 Timothy Hatcher 2009-08-27 09:03:42 PDT
I am not sure this UI makes sense. I think it would make sense for event flow, but not unrelated listeners.
Comment 18 Joseph Pecoraro 2009-08-27 13:43:24 PDT
(In reply to comment #17)
> I am not sure this UI makes sense. I think it would make sense for event flow,
> but not unrelated listeners.

Yes, it would be for event flow for a particular event type.  If its in the Element Panel's sidebar I think there would be the new Pane titled "Event Listeners".  Inside that pane would be different sections for each event type only _if_ that type is relevant for that node.  For example two sections titled "Click" and "Mouseover", which would expand and show my suggested UI showing the flow of listeners for that event type.

Take the following example for instance:

  function beta(e) { e.stopPropagation(); }
  elem.addEventListener('click', alpha, false); // Fires 2nd
  elem.addEventListener('click', beta, true);    // Fires 1st
  elem.onclick = gamma; // 3rd

All these listeners are related by the common "click" event type.  Flow would be: beta -> alpha -> gamma, and it would help developers to see that the flow doesn't get past beta... meaning that listener must be stopping the event's propagation.
Comment 19 Timothy Hatcher 2009-08-27 14:14:25 PDT
OK, I see. That might work well then.

But I think the UI needs to be smaller and more compact. We use to have bubbles like this for styles, but it was too large for the Inspector when docked.

I think a square with a bottom arrow would be good. See my attachment.
Comment 20 Timothy Hatcher 2009-08-27 14:14:56 PDT
Created attachment 38685 [details]
Possible Design
Comment 21 Joseph Pecoraro 2009-08-27 20:01:12 PDT
(In reply to comment #20)
> Created an attachment (id=38685) [details]
> Possible Design

I like the design, very simple and direct. I gave it a shot:
http://tr.im/xjwv?grabup

The "connector-point" as I call it may overlap text in the next event listener.  Plus I'm not as good with images, so right now I just have a few of my own that can easily be replaced.  All the gradients are CSS.
Comment 22 Timothy Hatcher 2009-08-27 20:23:24 PDT
Created attachment 38709 [details]
Possible Design 2
Comment 23 Timothy Hatcher 2009-08-27 20:27:53 PDT
Created attachment 38710 [details]
Connector Images
Comment 24 Timothy Hatcher 2009-08-27 20:30:21 PDT
Colors are:

top: rgb(243, 243, 243)
bottom: rgb(207, 207, 207)
border: rgb(128, 128, 128)
arrow is 75% from the left
Comment 25 Timothy Hatcher 2009-08-27 20:45:07 PDT
Created attachment 38711 [details]
Connector Images 2
Comment 26 Timothy Hatcher 2009-08-27 20:46:01 PDT
Created attachment 38712 [details]
Possible Design 3
Comment 27 Anthony Ricaud 2009-09-23 17:07:54 PDT
FYI, Firebug implemented this : http://blog.getfirebug.com/2009/09/18/eventbug-rising/
Comment 28 Joseph Pecoraro 2009-09-23 17:20:34 PDT
(In reply to comment #27)
> FYI, Firebug implemented this :
> http://blog.getfirebug.com/2009/09/18/eventbug-rising/

I guess I had better wrap this up so we're competitive!!
Comment 29 Timothy Hatcher 2009-09-23 20:34:39 PDT
I like our sidebar design better than the new panle design they decided to use.
Comment 30 Joseph Pecoraro 2009-09-24 15:08:53 PDT
Created attachment 40082 [details]
[PATCH] Event Listeners Version 1

Geoffrey Garen just made some impressive (and very clean) changes to EventListeners, EventTargets, etc.  This is compatible with his changes!

Technical Notes and Questions:
- I made the InspectorFrontEnd's ScriptState available so I could construct a particular ScriptObject directly (see InspectorDOMAgent::buildObjectForEventListener). That was because I didn't want to make a function passing a JSObject to the front end and leave v8 by the wayside. Any ideas around this, is there something agnostic I can pass?
- I exported calculatedDisplayName for functions in JSC. I think I will end up dropped this altogether and just taking a JavaScript approach at getting the function name, since the calculated display name seems to provide no benefit, even for anonymous functions. Are there any advantages to calculatedDisplayName?

UI Notes:
- There was discussion today on IRC that the UI would change. The UI in this patch reuses as much CSS as possible so that inner properties tree looks as you would expect.  When we change the inner portion to a better UI those redundant CSS properties can hopefully be dropped.
- Tim Hatcher suggested adding a Gear Menu to switch between just EventListeners for the focused Node, and all EventListeners in the Event Flow. This should come in a future version.
Comment 31 Joseph Pecoraro 2009-09-24 15:10:31 PDT
Created attachment 40083 [details]
[IMAGE] Event Listeners Version 1

This is a screenshot of the Sidebar Pane while on the complex Test Case listed above. It shows the proper flow of events.
Comment 32 Dimitri Glazkov (Google) 2009-09-24 15:13:53 PDT
Adding Chromium Inspector folks to help with the agnostic implementation.
Comment 33 Ojan Vafai 2009-09-24 15:33:00 PDT
This is awesome! Tim, it was not clear to me what the arrows in your design meant until I actually read the bug comments. I can't think of anything better, but I figured I'd mention it in case you come up with better ideas. :)
Comment 34 Pavel Feldman 2009-09-24 22:50:30 PDT
Comment on attachment 40082 [details]
[PATCH] Event Listeners Version 1

Nice work. I've reviewed dom-agent, backend/frontend-related stuff. Did not get into listeners order semantics / frontend rendering too much.
(Note that I remember updating vcproj when adding JS files).
I'll give it a try against Chromium today to see what we could do in order to fill the function name gap!

> +    if (!m_inspectorController)
> +        return;
> +    if (!m_inspectorController->m_domAgent || !m_inspectorController->m_frontend)
> +        return;
> +    m_inspectorController->domAgent()->getEventListenersForNode(callId, nodeId);

       if (InspectorDOMAgent* domAgent = inspectorDOMAgent())
           domAgent->getEventListenersForNode(callId, nodeId);

>  
> +    void getEventListenersForNode(long callId, long nodeId);
> +

Could you move this lower to the DOM-related group? (The one using nodeIds, in idl, h, cpp).


> +void InspectorDOMAgent::getEventListenersForNode(long callId, long nodeId)
> +{
> +    Node* node = nodeForId(nodeId);

node can be 0 due to the asynchronous nature of the frontend -> backend interaction.
remember to call didGetEventListenersForNode in order not to leak memory on the
frontend side if you return from here.

> +    ScriptArray listenersArray = m_frontend->newScriptArray();

frontend has the same lifetime as domAgent, so should be fine without check above.

> +#if USE(JSC)
> +    JSC::JSObject* functionObject = eventListener->jsFunction();
> +    if (functionObject)
> +        value.set("listener", ScriptObject(m_frontend->scriptState(), functionObject));
> +    JSC::JSFunction* function = JSC::asFunction(functionObject);
> +    if (function)
> +        value.set("listenerDisplayName", function->calculatedDisplayName(JSDOMWindow::commonJSGlobalData()));
> +#endif

It would be nice if we could hide this difference between engines by means of Script* classes. Script* are only used in
Inspector so far and serve this very need. But feel free to submit as is - I can extract it later.


>          void setTextNodeValue(long callId, long nodeId, const String& value);
> +        void getEventListenersForNode(long callId, long nodeId);

Good method order here.


>  
> +        ScriptState* scriptState() const { return m_scriptState; }
> +

I was trying to avoid it actually. Where ever you use it, just pass what you want to wrap
into the frontend as is and wrap it there. But I think it is within this USE(JSC) block...


> +
> +WebInspector.EventListeners.getEventListenersForNodeAsync = function(node, callback)
> +{

I think it is Ok for this to live in DOMAgent in order to maintain 1:1 C++/JavaScript mapping.


> +    if (!node || !node.id)
> +        return;

Do you actually expect for this to be called on undefined / non-node?


> +
> +    function mycallback(nodeId, data) {
> +        callback(nodeId, data);
> +    }
> +

You don't need this wrapper, right?


> +                eventListener.node = InjectedScript._nodeForId(eventListener.node);

This is private for a good reason. InjectedScript is going to live in the injected script context, there are no calls to it
from within the frontend JS. You are actually getting the real Node using it, whereas you need a frontend representative.
Use WebInspector.domAgent.nodeForId(nodeId) to get what you want. It should have been pushed given that you used proper push method in C++.

> +
> +        var nodeName = node.localName;

I don't think localName is available on the frontend Node representative. You were getting it since you were talking to the real node.
You should either add it in buildObjectForNode and pass into DOMNode (preferably if it is different from the nodeName) or send it
along with the eventlisteners response and same some memory.
Comment 35 Joseph Pecoraro 2009-09-25 00:06:23 PDT
Created attachment 40100 [details]
[PaTCH] Event Listeners Version 2

Fixed based on Pavel's Review.
Comment 36 Joseph Pecoraro 2009-09-25 00:08:22 PDT
Thanks for the review and pointers Pavel!  I added a new patch, I forgot --binary last time so this one is a bit larger.

> (Note that I remember updating vcproj when adding JS files).

What do I need to do for this?

> I'll give it a try against Chromium today to see what we could do in order to
> fill the function name gap!

I'm more worried about Functions then function names.  In comment 30 I mentioned I may just drop the JSC function name stuff and just use my JavaScript implementation.

> > +    if (!m_inspectorController)
> > +        return;
> > +    if (!m_inspectorController->m_domAgent || !m_inspectorController->m_frontend)
> > +        return;
> > +    m_inspectorController->domAgent()->getEventListenersForNode(callId, nodeId);
> 
>        if (InspectorDOMAgent* domAgent = inspectorDOMAgent())
>            domAgent->getEventListenersForNode(callId, nodeId);

Done.

> > +    void getEventListenersForNode(long callId, long nodeId);
> 
> Could you move this lower to the DOM-related group? (The one using nodeIds, in
> idl, h, cpp).

Done. Including moving the Cookies declarations.

> > +void InspectorDOMAgent::getEventListenersForNode(long callId, long nodeId)
> > +{
> > +    Node* node = nodeForId(nodeId);
> 
> node can be 0 due to the asynchronous nature of the frontend -> backend
> interaction. remember to call didGetEventListenersForNode in order not to leak memory on the
> frontend side if you return from here.

Done. I handle a 0 node at the same time that I handle an empty map of listeners.

> > +    ScriptArray listenersArray = m_frontend->newScriptArray();
> 
> frontend has the same lifetime as domAgent, so should be fine without check
> above.

This is the result array that I always pass to the frontend with didGetEventListenersForNode.

> > +#if USE(JSC)
> > +    JSC::JSObject* functionObject = eventListener->jsFunction();
> > +    if (functionObject)
> > +        value.set("listener", ScriptObject(m_frontend->scriptState(), functionObject));
> > +    JSC::JSFunction* function = JSC::asFunction(functionObject);
> > +    if (function)
> > +        value.set("listenerDisplayName", function->calculatedDisplayName(JSDOMWindow::commonJSGlobalData()));
> > +#endif
> 
> It would be nice if we could hide this difference between engines by means of
> Script* classes. Script* are only used in Inspector so far and serve this very need. But feel free to submit as is - I
> can extract it later.

Yes, that is what I was hoping. I'd be interested to learn how this is done, but I didn't take the time to understand the Script* setup to go about doing this.

> > +        ScriptState* scriptState() const { return m_scriptState; }
> 
> I was trying to avoid it actually. Where ever you use it, just pass what you
> want to wrap into the frontend as is and wrap it there. But I think it is within this
> USE(JSC) block...

Yes. I also mentioned this is in comment 30.  I noticed the separation of concerns but I couldn't think of a way to pass an agnostic representation to the front end.  This could be possible if a Script* object can be made for the above case.


> > +WebInspector.EventListeners.getEventListenersForNodeAsync = function(node, callback)
> > +{
> 
> I think it is Ok for this to live in DOMAgent in order to maintain 1:1
> C++/JavaScript mapping.

I have the same for Cookies.

> > +    if (!node || !node.id)
> > +        return;
> 
> Do you actually expect for this to be called on undefined / non-node?

Simplified to just if (!node).

This case shows up when you keep the inspector open, and the EventListeners Sidebar Pane expanded, you navigate to a new page, and this is triggered without a focused node in the inspector (before anything shows up).  Should I handle this somewhere else instead?

> You don't need this wrapper, right?

Correct, Removed.

> > +                eventListener.node = InjectedScript._nodeForId(eventListener.node);
> 
> This is private for a good reason. InjectedScript is going to live in the
> injected script context, there are no calls to it
> from within the frontend JS. You are actually getting the real Node using it,
> whereas you need a frontend representative.
> Use WebInspector.domAgent.nodeForId(nodeId) to get what you want. It should
> have been pushed given that you used proper push method in C++.

Done. I slipped up here, I had forgotten about WebInspector.domAgent.

> > +        var nodeName = node.localName;
> 
> I don't think localName is available on the frontend Node representative. You
> were getting it since you were talking to the real node.
> You should either add it in buildObjectForNode and pass into DOMNode
> (preferably if it is different from the nodeName) or send it
> along with the eventlisteners response and same some memory.

I added this to buildObjectForNode and the WebInspector.DOMNode constructor.  The MDC [1] says:
- "in compliance with HTML5, the property returns in the case of the internal DOM storage, which is lower case for both HTML elements in HTML DOMs and XHTML elements in XML DOMs."
- "For nodes of any type other than ELEMENT_NODE and ATTRIBUTE_NODE localName is always null."

[1]: https://developer.mozilla.org/en/DOM/node.localName
Comment 37 Pavel Feldman 2009-09-25 06:57:12 PDT
>  #ifndef InspectorDOMAgent_h
>  #define InspectorDOMAgent_h
>  
> +#include "AtomicString.h"
>  #include "EventListener.h"

Could you please add explicit include for "EventTarget.h" here? It might be available in the JSC mode transitively, but is missing in Chromium.

>  #include "ScriptArray.h"
>  #include "ScriptObject.h"
Comment 38 Joseph Pecoraro 2009-09-25 07:56:38 PDT
> Could you please add explicit include for "EventTarget.h" here? It might be
> available in the JSC mode transitively, but is missing in Chromium.

Done. This will be included when I submit PATCH Version 3.
Comment 39 Joseph Pecoraro 2009-09-25 10:28:38 PDT
Created attachment 40116 [details]
[PATCH] Event Listeners Version 3

Changes:
- Removed the JavaScriptCore export of "calculatedDisplayName", use JavaScript solution instead
- Added Gear Menu and Preference for Filtering Event Listeners (defaults to all)
- Nicely display "No Event Listeners" when appropriate
- Uses the "(anonymous function)" UI String instead of my hardcoded string
- Factored out "appropriateSelectorForNode" into utilities.js used here an in StylesSidebarPane
Comment 40 Timothy Hatcher 2009-09-27 13:20:22 PDT
Comment on attachment 40116 [details]
[PATCH] Event Listeners Version 3


> +    thisNode->eventAncestors(ancestors);

No need for "thisNode->".

You also need to add EventListenersSidebarPane.js to WebCore.vcproj.

See http://trac.webkit.org/changeset/48392#file3 for an example.

File a new bug to track improving the UI with ideas that have been mentioned so far.
Comment 41 Joseph Pecoraro 2009-09-27 14:01:42 PDT
Created attachment 40205 [details]
[PATCH] Event Listeners Version 4

This addresses the remaining points, is tested, and is thus r+ as well.
Pavel (pfeldman) has volunteered to commit this! Thanks =)

Changes:
- Added new js file to WebCore.vcproj
- Added (missing) / Removed (old) js files in WebCore.vcproj
- Reordered Gear Options so default "All" appears first
- The event listener, before being pushed to the front end, had a property named "node". This has been changed to "nodeId" for clarity.  Updated the handling on the JS side to reflect this change.
Comment 42 Joseph Pecoraro 2009-09-27 14:18:27 PDT
> > +    thisNode->eventAncestors(ancestors);
> 
> No need for "thisNode->".

Done.

 
> You also need to add EventListenersSidebarPane.js to WebCore.vcproj.
> See http://trac.webkit.org/changeset/48392#file3 for an example.

Done. Including some other fixes.  Thanks for the link to an example.


> File a new bug to track improving the UI with ideas that have been mentioned so
> far.

Done. Bug 29789.

I believe you all got an email, but some of you may not be CC'd.  If you're interested in getting CC'd and providing suggestions / discussion please do so!  Thanks.
Comment 43 Pavel Feldman 2009-09-28 05:41:08 PDT
Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	WebCore/ChangeLog
	M	WebCore/English.lproj/localizedStrings.js
	M	WebCore/WebCore.gypi
	M	WebCore/WebCore.vcproj/WebCore.vcproj
	M	WebCore/dom/Node.cpp
	M	WebCore/dom/Node.h
	M	WebCore/inspector/InspectorBackend.cpp
	M	WebCore/inspector/InspectorBackend.h
	M	WebCore/inspector/InspectorBackend.idl
	M	WebCore/inspector/InspectorDOMAgent.cpp
	M	WebCore/inspector/InspectorDOMAgent.h
	M	WebCore/inspector/InspectorFrontend.cpp
	M	WebCore/inspector/InspectorFrontend.h
	M	WebCore/inspector/front-end/DOMAgent.js
	M	WebCore/inspector/front-end/ElementsPanel.js
	A	WebCore/inspector/front-end/EventListenersSidebarPane.js
	A	WebCore/inspector/front-end/Images/grayConnectorPoint.png
	A	WebCore/inspector/front-end/Images/whiteConnectorPoint.png
	M	WebCore/inspector/front-end/StylesSidebarPane.js
	M	WebCore/inspector/front-end/WebKit.qrc
	M	WebCore/inspector/front-end/inspector.css
	M	WebCore/inspector/front-end/inspector.html
	M	WebCore/inspector/front-end/inspector.js
	M	WebCore/inspector/front-end/utilities.js
Committed r48809
Comment 44 Pavel Feldman 2009-09-28 07:22:10 PDT
Created attachment 40233 [details]
[PATCH] Removed InspectorController.wrapObject call from the frontend side. 

I am sorry, I missed it during the review. We only wrap objects on the inspected page side in order to pass these wrappers as references to the frontend. I am about split inspector controller into two IDLs, one accessible from frontend, another from injected script.

In order for things to work right, you should have either wrapped entire eventListener object (that would result in the need in id for that object) or populate section on the frontend side (as I did in the follow-up fix)
Comment 45 Pavel Feldman 2009-09-28 07:40:33 PDT
Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	WebCore/ChangeLog
	M	WebCore/inspector/front-end/EventListenersSidebarPane.js
Committed r48813
Comment 46 Eric Seidel 2009-09-28 17:13:26 PDT
Comment on attachment 40233 [details]
[PATCH] Removed InspectorController.wrapObject call from the frontend side. 

Please create a new bug for new patches.  This is plenty big already (and besides, it's closed). :)

Unless this was already landed?
Comment 47 Pavel Feldman 2009-09-28 21:59:21 PDT
(In reply to comment #46)
> (From update of attachment 40233 [details])
> Please create a new bug for new patches.  This is plenty big already (and
> besides, it's closed). :)
> 
> Unless this was already landed?

Trivial follow-up fix was landed as r48813.