Bug 81658

Summary: Web Inspector: expose queryEventListeners() to console command line API
Product: WebKit Reporter: Andrey Kosyakov <caseq>
Component: Web Inspector (Deprecated)Assignee: Andrey Kosyakov <caseq>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, apavlov, bweinstein, haraken, japhet, joepeck, keishi, loislo, pfeldman, pmuellr, rik, timothy, webkit.review.bot, yurys
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
Patch
none
Patch
none
Patch yurys: review+, buildbot: commit-queue-

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>