[DevTools] Event Listeners Sidebar shows window listeners

[DevTools] Event Listeners Sidebar shows window listeners

BUG=469159
R=pfeldman@chromium.org

[kozy, 2015-03-30 13:39:25]

kozyatinskiy@chromium.org changed reviewers:
+ lushnikov@chromium.org, sergeyv@chromium.org

[kozy, 2015-03-30 13:40:38]

ptal.

[pfeldman, 2015-03-30 14:36:20]

https://codereview.chromium.org/1042853004/diff/1/Source/devtools/protocol.json
File Source/devtools/protocol.json (left):

https://codereview.chromium.org/1042853004/diff/1/Source/devtools/protocol.js...
Source/devtools/protocol.json:2135: { "name": "nodeId", "$ref": "NodeId",
"description": "Target <code>DOMNode</code> id." },
Why would it be in DOM domain? For example, it might make sense for worjers
where there is no DOM. Wdyt?

[kozy, 2015-03-31 20:09:16]

https://codereview.chromium.org/1042853004/diff/1/Source/devtools/protocol.json
File Source/devtools/protocol.json (left):

https://codereview.chromium.org/1042853004/diff/1/Source/devtools/protocol.js...
Source/devtools/protocol.json:2135: { "name": "nodeId", "$ref": "NodeId",
"description": "Target <code>DOMNode</code> id." },
On 2015/03/30 14:36:20, pfeldman wrote:
> Why would it be in DOM domain? For example, it might make sense for worjers
> where there is no DOM. Wdyt?

I suppose that supporting it for workers should be separated issue and I don't
want to divide this method into two parts because it seems like there is no real
case when we want to receive only window or only node event listeners.
I can add enum for EventListener that describes its type: Node or window if in
our codebase we use empty nodeId for document.
Wdyt?

[pfeldman, 2015-04-01 08:46:45]

I don't see how supporting essential events such as "message", "error", etc. is
a separate issue. These are exact same events, they should work in the devtools
on SW. It seems like we should split Event Listeners sidebar into two sections.
Top one will be event listeners of today, bottom one would contain a target
selector and the global object listeners. And representing it right would give
you the right hints on which agent the code belongs. In this case, it seems to
be runtime, not DOM.

[kozy, 2015-04-09 11:04:34]

Pavel, please take a look.

[pfeldman, 2015-04-10 08:45:12]

https://codereview.chromium.org/1042853004/diff/40001/Source/core/events/Even...
File Source/core/events/EventListenerMap.h (right):

https://codereview.chromium.org/1042853004/diff/40001/Source/core/events/Even...
Source/core/events/EventListenerMap.h:93: struct EventListenerInfo {
Extract into a separate class, place under core/inspector.

https://codereview.chromium.org/1042853004/diff/40001/Source/core/inspector/I...
File Source/core/inspector/InjectedScript.cpp (right):

https://codereview.chromium.org/1042853004/diff/40001/Source/core/inspector/I...
Source/core/inspector/InjectedScript.cpp:265: ScriptFunctionCall
function(injectedScriptObject(), "findObjectById");
Just use InjectedScript::findObjectById, there is no need to go into host,
inline it here.

https://codereview.chromium.org/1042853004/diff/40001/Source/core/inspector/I...
File Source/core/inspector/InspectorRuntimeAgent.cpp (right):

https://codereview.chromium.org/1042853004/diff/40001/Source/core/inspector/I...
Source/core/inspector/InspectorRuntimeAgent.cpp:157: if (!target)
Lets return error here.

https://codereview.chromium.org/1042853004/diff/40001/Source/core/inspector/I...
Source/core/inspector/InspectorRuntimeAgent.cpp:193: if (objectGroupId) {
This part can be reused w/ DOM.

https://codereview.chromium.org/1042853004/diff/40001/Source/devtools/front_e...
File Source/devtools/front_end/elements/EventListenersSidebarPane.js (right):

https://codereview.chromium.org/1042853004/diff/40001/Source/devtools/front_e...
Source/devtools/front_end/elements/EventListenersSidebarPane.js:286:
WebInspector.ObjectEventListenersSidebarPane = function()
Lets use file-per-class, these are not small classes.

https://codereview.chromium.org/1042853004/diff/40001/Source/devtools/front_e...
Source/devtools/front_end/elements/EventListenersSidebarPane.js:346:
_addListener: function (eventListener, treeItemMap)
Annotate.

https://codereview.chromium.org/1042853004/diff/40001/Source/devtools/front_e...
Source/devtools/front_end/elements/EventListenersSidebarPane.js:413:
properties.push(this._runtimeModel.createRemotePropertyFromPrimitiveValue("attachment",
payload.isAttribute ? "attribute" : "script"));
What is this?

https://codereview.chromium.org/1042853004/diff/40001/Source/devtools/protoco...
File Source/devtools/protocol.json (right):

https://codereview.chromium.org/1042853004/diff/40001/Source/devtools/protoco...
Source/devtools/protocol.json:817: { "name": "useCapture", "type": "boolean",
"description": "<code>EventListener</code>'s useCapture." },
I don't think you need this for non-DOM listeners.

https://codereview.chromium.org/1042853004/diff/40001/Source/devtools/protoco...
Source/devtools/protocol.json:818: { "name": "isAttribute", "type": "boolean",
"description": "<code>EventListener</code>'s isAttribute." },
ditto

https://codereview.chromium.org/1042853004/diff/40001/Source/devtools/protoco...
Source/devtools/protocol.json:819: { "name": "location", "$ref":
"Debugger.Location", "description": "Handler code location." },
Lets drop this and use it from the handler.

https://codereview.chromium.org/1042853004/diff/40001/Source/devtools/protoco...
Source/devtools/protocol.json:912: "name": "getEventListeners",
hidden: true

[kozy, 2015-04-13 14:26:54]

ptal.

https://codereview.chromium.org/1042853004/diff/40001/Source/core/events/Even...
File Source/core/events/EventListenerMap.h (right):

https://codereview.chromium.org/1042853004/diff/40001/Source/core/events/Even...
Source/core/events/EventListenerMap.h:93: struct EventListenerInfo {
On 2015/04/10 08:45:11, pfeldman wrote:
> Extract into a separate class, place under core/inspector.

Done.

https://codereview.chromium.org/1042853004/diff/40001/Source/core/inspector/I...
File Source/core/inspector/InjectedScript.cpp (right):

https://codereview.chromium.org/1042853004/diff/40001/Source/core/inspector/I...
Source/core/inspector/InjectedScript.cpp:265: ScriptFunctionCall
function(injectedScriptObject(), "findObjectById");
On 2015/04/10 08:45:11, pfeldman wrote:
> Just use InjectedScript::findObjectById, there is no need to go into host,
> inline it here.

Done.

https://codereview.chromium.org/1042853004/diff/40001/Source/core/inspector/I...
File Source/core/inspector/InspectorRuntimeAgent.cpp (right):

https://codereview.chromium.org/1042853004/diff/40001/Source/core/inspector/I...
Source/core/inspector/InspectorRuntimeAgent.cpp:157: if (!target)
On 2015/04/10 08:45:11, pfeldman wrote:
> Lets return error here.

Done.

https://codereview.chromium.org/1042853004/diff/40001/Source/core/inspector/I...
Source/core/inspector/InspectorRuntimeAgent.cpp:193: if (objectGroupId) {
On 2015/04/10 08:45:11, pfeldman wrote:
> This part can be reused w/ DOM.

Done.

https://codereview.chromium.org/1042853004/diff/40001/Source/devtools/front_e...
File Source/devtools/front_end/elements/EventListenersSidebarPane.js (right):

https://codereview.chromium.org/1042853004/diff/40001/Source/devtools/front_e...
Source/devtools/front_end/elements/EventListenersSidebarPane.js:286:
WebInspector.ObjectEventListenersSidebarPane = function()
On 2015/04/10 08:45:11, pfeldman wrote:
> Lets use file-per-class, these are not small classes.

Done.

https://codereview.chromium.org/1042853004/diff/40001/Source/devtools/front_e...
Source/devtools/front_end/elements/EventListenersSidebarPane.js:346:
_addListener: function (eventListener, treeItemMap)
On 2015/04/10 08:45:11, pfeldman wrote:
> Annotate.

Done.

https://codereview.chromium.org/1042853004/diff/40001/Source/devtools/front_e...
Source/devtools/front_end/elements/EventListenersSidebarPane.js:413:
properties.push(this._runtimeModel.createRemotePropertyFromPrimitiveValue("attachment",
payload.isAttribute ? "attribute" : "script"));
On 2015/04/10 08:45:11, pfeldman wrote:
> What is this?

Removed.

https://codereview.chromium.org/1042853004/diff/40001/Source/devtools/protoco...
File Source/devtools/protocol.json (right):

https://codereview.chromium.org/1042853004/diff/40001/Source/devtools/protoco...
Source/devtools/protocol.json:817: { "name": "useCapture", "type": "boolean",
"description": "<code>EventListener</code>'s useCapture." },
On 2015/04/10 08:45:12, pfeldman wrote:
> I don't think you need this for non-DOM listeners.

I suppose it can be useful for non-DOM listeners. If useCapture is true then
listener will be called before DOM listeners otherwise after.

https://codereview.chromium.org/1042853004/diff/40001/Source/devtools/protoco...
Source/devtools/protocol.json:818: { "name": "isAttribute", "type": "boolean",
"description": "<code>EventListener</code>'s isAttribute." },
On 2015/04/10 08:45:11, pfeldman wrote:
> ditto

Removed.

https://codereview.chromium.org/1042853004/diff/40001/Source/devtools/protoco...
Source/devtools/protocol.json:819: { "name": "location", "$ref":
"Debugger.Location", "description": "Handler code location." },
On 2015/04/10 08:45:12, pfeldman wrote:
> Lets drop this and use it from the handler.

If we have some event with a lot of listener functions we will produce call for
each function on listener element expanding. I think we need to pass location
here.

https://codereview.chromium.org/1042853004/diff/40001/Source/devtools/protoco...
Source/devtools/protocol.json:912: "name": "getEventListeners",
On 2015/04/10 08:45:12, pfeldman wrote:
> hidden: true

Done.

[kozy, 2015-04-14 08:10:10]

Screenshots:
https://drive.google.com/a/chromium.org/file/d/0B7RLEApsOKBLU19xSmZSeHlXRVU/v...
https://drive.google.com/a/chromium.org/file/d/0B7RLEApsOKBLTWYtYVp0emctalk/v...

[kozy, 2015-04-17 12:24:13]

Please take a look!

Window event listeners are added to event listeners sidebar in elements panel.
Window event listener for node in document:
https://drive.google.com/a/chromium.org/file/d/0B7RLEApsOKBLdnJBVDhhV1d2OEE/v...
No window event listeners if "Selected node only" is checked:
https://drive.google.com/a/chromium.org/file/d/0B7RLEApsOKBLcTNjeWlSamJrbUU/v...
Window event listeners for node in frame:
https://drive.google.com/a/chromium.org/file/d/0B7RLEApsOKBLMzNTd1ktc0tjekU/v...

[kozy, 2015-04-17 12:58:48]

You can not look it now.
I am going to reuse EventListenersSidebarPane.js in sources panel. I should
extract it to components and change inheritance EventListenersSidebarPane from
ElementsSidebarPane.

[kozy, 2015-04-17 13:27:34]

Please take a look!

[kozy, 2015-04-17 13:43:31]

Patchset #8 (id:140001) has been deleted

[kozy, 2015-04-17 14:12:23]

Patchset #10 (id:200001) has been deleted

[pfeldman, 2015-04-17 16:22:06]

Please split this into a series of patches:
1) extract EventListenerInfo
2) agents code
3) refactor sidebar pane
4) glues everything together

[sergeyv, 2015-04-23 09:38:30]

sergeyv@chromium.org changed reviewers:
- sergeyv@chromium.org