[webkit-reviews] review granted: [Bug 17134] Inspector should have an integrated JavaScript debugger : [Attachment 19836] Make JavaScriptDebugListeners able to listen to particular Pages

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Adam Roben (aroben)
<aroben at apple.com>'s request for review:
Bug 17134: Inspector should have an integrated JavaScript debugger
http://bugs.webkit.org/show_bug.cgi?id=17134

Attachment 19836: Make JavaScriptDebugListeners able to listen to particular
Pages
http://bugs.webkit.org/attachment.cgi?id=19836&action=edit

------- Additional Comments from Darin Adler <darin at apple.com>
+    PageListenersMap::iterator it = m_pageListenersMap.find(page);
+    if (it == m_pageListenersMap.end()) {
+	 pair<PageListenersMap::iterator, bool> result =
m_pageListenersMap.add(page, new ListenerSet);
+	 ASSERT(result.second);
+	 it = result.first;
+    }

The above is not a great idiom, because it unnecessarily does two hash table
lookups when adding a new entry. A better way is:

    pair<PageListenersMap::iterator, bool> result =
m_pageListenersMap.add(page, 0);
    if (result.second)
	result.first->second = new ListenerSet;
    ListenerSet* listeners = result.first->second;

Is it legal to add a listener when it's already in, or remove a listener if
it's not in? If not, then I suggest adding some assertions.

The use of "state" instead of "exec" for ExecState* arguments is quite
unconventional. I suggest you either use "exec" or push to get "state" used
elsewhere. Maybe we can switch using do-webcore-rename, because I bet "exec" is
not used for anything except ExecState*.

r=me