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
Andrey Kosyakov
2012-03-20 06:41:01 PDT
Created attachment 132815 [details]
Patch
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 on attachment 132815 [details] Patch Attachment 132815 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/12005268 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. (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. Created attachment 132824 [details]
Patch
Comment on attachment 132824 [details] Patch Attachment 132824 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/11994322 (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. Created attachment 132833 [details]
Patch
Comment on attachment 132833 [details] Patch Attachment 132833 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/12001335 > > > 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.
(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 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. Committed r111529: <http://trac.webkit.org/changeset/111529> |