[webkit-reviews] review denied: [Bug 30995] TimelineAgent should not add events to the timeline if there are no event listeners. : [Attachment 42278] A proposed solution for this.

[bugzilla-daemon at webkit.org]

Timothy Hatcher <timothy at hatcher.name> has denied Kelly Norton
<knorton at google.com>'s request for review:
Bug 30995: TimelineAgent should not add events to the timeline if there are no
event listeners.
https://bugs.webkit.org/show_bug.cgi?id=30995

Attachment 42278: A proposed solution for this.
https://bugs.webkit.org/attachment.cgi?id=42278&action=review

------- Additional Comments from Timothy Hatcher <timothy at hatcher.name>
> +// static

We normally don't add comments like this.


> +inline InspectorTimelineAgent*
InspectorTimelineAgent::retrieveIfEventHasListeners(InspectorTimelineAgent*
timelineAgent,
> +    const AtomicString& eventType, Node* node, const Vector< RefPtr<
ContainerNode > >& ancestors)

Will this be used by other callers later? If not it might be best just to put
this in Node.cpp so there are less headers included by
InspectorTimelineAgent.h.
I can see a variation of this being used by XHRs (we don't want to show
readystate or load records if there are no listeners.)


> +    for (size_t i = ancestors.size(); i; --i) {
> +	   ContainerNode* ancestor = ancestors[i - 1].get();
> +	   if (ancestor->hasEventListeners(eventType))
> +	       return timelineAgent;
> +    }

This is better written as:

    for (ssize_t i = (ancestors.size() - 1); i >= 0; --i) {
	ContainerNode* ancestor = ancestors[i].get();
	if (ancestor->hasEventListeners(eventType))
	    return timelineAgent;
    }