[Webkit-unassigned] [Bug 17134] Inspector should have an integrated JavaScript debugger

[bugzilla-daemon at webkit.org]

http://bugs.webkit.org/show_bug.cgi?id=17134





------- Comment #12 from aroben at apple.com  2008-04-02 10:57 PDT -------
(In reply to comment #11)
> (From update of attachment 19836 [edit])
> +    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;

Changed to match your example. Thanks for the tutorial!

> 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.

It is legal, in that nothing bad will come of it -- both operations are no-ops.

> 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*.

I can switch this class to using "exec" instead of "state" in a subsequent
patch.

Thanks for the review!


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