Bug 81658 - Web Inspector: expose queryEventListeners() to console command line API
Summary: Web Inspector: expose queryEventListeners() to console command line API
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated) (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Andrey Kosyakov
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-03-20 06:41 PDT by Andrey Kosyakov
Modified: 2012-03-21 06:22 PDT (History)
14 users (show)

See Also:


Attachments
Patch (23.48 KB, patch)
2012-03-20 06:44 PDT, Andrey Kosyakov
no flags Details | Formatted Diff | Diff
Patch (25.75 KB, patch)
2012-03-20 07:38 PDT, Andrey Kosyakov
no flags Details | Formatted Diff | Diff
Patch (24.35 KB, patch)
2012-03-20 08:39 PDT, Andrey Kosyakov
yurys: review+
buildbot: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andrey Kosyakov 2012-03-20 06:41:01 PDT
This adds queryEventListeners() call to command line API in the inspector console. Only the JS listeners in current world are returned.
We primarily want this to be accessible to Web Inspector extensions, although this might be handy for having a closer look at the listeners to the interactive users as well.
Comment 1 Andrey Kosyakov 2012-03-20 06:44:16 PDT
Created attachment 132815 [details]
Patch
Comment 2 Yury Semikhatsky 2012-03-20 07:12:19 PDT
Comment on attachment 132815 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=132815&action=review

> Source/WebCore/bindings/js/JSInjectedScriptHostCustom.cpp:231
> +        eventListeners->putDirect(exec->globalData(), Identifier(exec, "node"), toJS(exec, globalObject, listenersArray[i].node));

Do we really want to return event listeners for ancestors?

> Source/WebCore/inspector/InjectedScriptHost.h:95
> +    void queryEventListenersImpl(Node*, Vector<EventListenerInfo>& listenersArray);

Why is this Impl suffix?
Comment 3 Build Bot 2012-03-20 07:13:39 PDT
Comment on attachment 132815 [details]
Patch

Attachment 132815 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12005268
Comment 4 Yury Semikhatsky 2012-03-20 07:17:19 PDT
Comment on attachment 132815 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=132815&action=review

> LayoutTests/inspector/console/command-line-api-queryEventListeners.html:20
> +<div id="inner">

Please add a few attribute handlers as they will also be listed as the listeners.
Comment 5 Andrey Kosyakov 2012-03-20 07:36:36 PDT
(In reply to comment #2)
> (From update of attachment 132815 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=132815&action=review
> 
> > Source/WebCore/bindings/js/JSInjectedScriptHostCustom.cpp:231
> > +        eventListeners->putDirect(exec->globalData(), Identifier(exec, "node"), toJS(exec, globalObject, listenersArray[i].node));
> 
> Do we really want to return event listeners for ancestors?

This is for consistency with what we do in the DOM agent/elements panel -- presumably, caller would be interested in the ancestors, given these may have interceptors or handlers for bubbling event. This could go under a boolean flag, perhaps.

> > Source/WebCore/inspector/InjectedScriptHost.h:95
> > +    void queryEventListenersImpl(Node*, Vector<EventListenerInfo>& listenersArray);
> 
> Why is this Impl suffix?

Just for consistency with inspectImpl(). I'm not particularly attached to it, though, so will gladly drop.

> Please add a few attribute handlers as they will also be listed as the listeners.

Fixed.
Comment 6 Andrey Kosyakov 2012-03-20 07:38:51 PDT
Created attachment 132824 [details]
Patch
Comment 7 Build Bot 2012-03-20 08:04:54 PDT
Comment on attachment 132824 [details]
Patch

Attachment 132824 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/11994322
Comment 8 Andrey Kosyakov 2012-03-20 08:37:47 PDT
(In reply to comment #2)
> (From update of attachment 132815 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=132815&action=review
> 
> > Source/WebCore/bindings/js/JSInjectedScriptHostCustom.cpp:231
> > +        eventListeners->putDirect(exec->globalData(), Identifier(exec, "node"), toJS(exec, globalObject, listenersArray[i].node));
> 
> Do we really want to return event listeners for ancestors?

On a second thought, I'd let the caller do the work of traversing the ancestry -- this will make the interface simpler.
Comment 9 Andrey Kosyakov 2012-03-20 08:39:12 PDT
Created attachment 132833 [details]
Patch
Comment 10 Build Bot 2012-03-20 09:08:52 PDT
Comment on attachment 132833 [details]
Patch

Attachment 132833 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12001335
Comment 11 Yury Semikhatsky 2012-03-20 09:32:41 PDT
> > > Source/WebCore/inspector/InjectedScriptHost.h:95
> > > +    void queryEventListenersImpl(Node*, Vector<EventListenerInfo>& listenersArray);
> > 
> > Why is this Impl suffix?
> 
> Just for consistency with inspectImpl(). I'm not particularly attached to it, though, so will gladly drop.
> 
Can we remove it from inspectImpl instead. Other methods seem not to use such suffix.
Comment 12 Pavel Feldman 2012-03-20 09:59:33 PDT
(In reply to comment #11)
> > > > Source/WebCore/inspector/InjectedScriptHost.h:95
> > > > +    void queryEventListenersImpl(Node*, Vector<EventListenerInfo>& listenersArray);
> > > 
> > > Why is this Impl suffix?
> > 
> > Just for consistency with inspectImpl(). I'm not particularly attached to it, though, so will gladly drop.
> > 
> Can we remove it from inspectImpl instead. Other methods seem not to use such suffix.

If we drop Impl suffix, there will be no way to tell whether InjectedScriptHost::inspect is custom or not. We used this pattern for all the base implementations of custom members. I think it is useful.
Comment 13 Pavel Feldman 2012-03-20 17:15:19 PDT
Comment on attachment 132833 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=132833&action=review

> Source/WebCore/inspector/InjectedScriptSource.js:580
> +    "monitorEvents", "unmonitorEvents", "inspect", "copy", "clear", "queryEventListeners"

I'd call it getEventListeners, there is no query per se.

> Source/WebCore/inspector/InjectedScriptSource.js:690
> +        return InjectedScriptHost.queryEventListeners(node);

I'd add a request parameter determining whether caller is interested in self node's listeners or hierarchy ones as well.
Comment 14 Andrey Kosyakov 2012-03-21 06:22:34 PDT
Committed r111529: <http://trac.webkit.org/changeset/111529>