[Webkit-unassigned] [Bug 61834] Web Inspector: CRASH if Expanding Event Listener on document

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=61834





--- Comment #9 from Joseph Pecoraro <joepeck at webkit.org>  2011-06-01 22:26:16 PST ---
(In reply to comment #8)
> (From update of attachment 95642 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=95642&action=review
> 
> Here is a number of hints on the present testing framework.
> 
> > LayoutTests/http/tests/inspector/elements-test.js:111
> > +    InspectorTest.expandSelectedElementEventListeners(function() {
> 
> We generally use named functions with meaningless names in the callbacks :)

lol! I know I'm kind of breaking the tradition of this totally ugly style, but
since this is test code I was trying to stretch what is acceptable. In these cases,
like setTimeout(), its much clearer to me whats happening. But, let me know if
you do want me to change the style and I will. I should be consistent...


> > LayoutTests/http/tests/inspector/elements-test.js:123
> > +    InspectorTest.runAfterPendingDispatches(function() {
> 
> I'd replace this with the sniffer (InspectorTest.addSniffer) that would fire when event listeners data hits front-end.

That sounds neat, I guess it would be nice, but I don't think
it would be that much different. Maybe it would be clearer.


> > LayoutTests/http/tests/inspector/elements-test.js:135
> > +    InspectorTest.runAfterPendingDispatches(function() {
> 
> I don't think you need to run pending dispatches here.
> 
> > LayoutTests/http/tests/inspector/elements-test.js:150
> > +    InspectorTest.runAfterPendingDispatches(callback);
> 
> And here.

In my tests, it seemed like every expand() in the EventListeners
section required a runAfterPendingDispatches. Expand on the
sidebar pane, eventbar section, and eventually the property lists.
I can double check that though.

> > LayoutTests/inspector/elements/event-listener-sidebar-expected.txt:9
> > +===== Dump Event Listeners =====
> 
> Not sure we need these log entries.

Sounds good. I can limit these.


> > LayoutTests/inspector/elements/event-listener-sidebar-expected.txt:15
> > +(anonymous function)documentisAttribute: falselistenerBody: "function (event) { console.log("click - document - capturing"); }"node: HTMLDocumenttype: "click"useCapture: true
> 
> This formatting could get a bit more love (line breaks). Otherwise "nodeisAttribute" and "truelistenerBody" look confusing.

Yah, I took the easy way out and used textContent. This has the advantage
that if anything changes in the Event Listener's property list it would show
up here and not need test tweaks. However, this test is already tied
pretty heavily to the internal representation. I can spend some time
to pretty this up.


> > LayoutTests/inspector/elements/event-listener-sidebar.html:11
> > +    button.addEventListener("hover", function(event) { console.log("hover - button - bubbling"); }, false);
> 
> Naming the functions would provide even better test coverage.

Will do. I named one, I should name some of the others, and in
different ways.

> 
> > LayoutTests/inspector/elements/event-listener-sidebar.html:29
> > +            InspectorTest.evaluateInPage("setupEventListeners()", callback);
> 
> There is no need for the front-end to be initialized by the time you run setupEventListeners, so you could simply do that all in the onload(), followed with the runTests(); call.

Oh, that sounds good. I was following what I saw in other tests, but
I didn't realize they were doing it because it needed to be done after
the frontend loaded. Nice.

Thanks for the look. I'll try to post an updated patch tomorrow.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.