Bug 17134 – Inspector should have an integrated JavaScript debugger

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector (Deprecated)

[Adam Roben (:aroben)]

The Web Inspector should have an integrated JavaScript debugger. This lack is one of the chief complaints I have heard from people who are used to working with Firebug. The Inspector won't really be an all-in-one tool for web development until we have this.

[Adam Roben (:aroben)]

<rdar://problem/5719552>

[Timothy Hatcher]

Created attachment 19754 [details]
Proof of concept patch

I ma attaching this proof of concept patch that gets basic debugging working in the Inspector.

[Adam Roben (:aroben)]

*** Bug 17135 has been marked as a duplicate of this bug. ***

[Adam Roben (:aroben)]

Created attachment 19836 [details]
Make JavaScriptDebugListeners able to listen to particular Pages

This patch augments JavaScriptDebugServer so that JavaScriptDebugListeners may register to receive callbacks only for specific Pages. Using this we will be able to make the InspectorController register for callbacks for its inspected Page.

[Adam Roben (:aroben)]

Created attachment 19837 [details]
[WIP] Add an EventLoop abstraction (Windows-only right now)

This patch isn't for review yet. It adds an EventLoop abstraction that we can use in the Inspector to spin a nested event loop while JS is paused.

[Adam Roben (:aroben)]

Comment on attachment 19837 [details]
[WIP] Add an EventLoop abstraction (Windows-only right now)

Note that for the moment this is a Windows-only patch. We'll need to flesh it out for other platforms before landing (at least with stub implementations of EventLoop::cycle).

[Adam Roben (:aroben)]

Created attachment 19838 [details]
[WIP] Add Widget::isPluginView

This patch is not ready to be reviewed.

This patch adds a virtual method to Widget, bool isPluginView() const, that will tell you if a Widget is a PluginView. This is useful for finding all the PluginViews in a Frame.

[Adam Roben (:aroben)]

Created attachment 19839 [details]
[WIP] Make the Inspector able to pause/step JS code

This patch is not ready for review.

This patch depends on the following patches:

From Bug 17133:
 * Attachment 19590 [details]
 * Attachment 19729 [details]
 * Attachment 19788 [details]

From this bug:
 * Attachment 19836 [details]
 * Attachment 19837 [details]
 * Attachment 19838 [details]

Changes from attachment 19754 [details]:
 * InspectorController is now a JavaScriptDebugListener.
 * No InspectorClient methods are added. Instead, we handle the event loop entirely within WebCore.
 * Unused arguments that were being passed down to JS in the debugging methods have been removed.

[Adam Roben (:aroben)]

(In reply to comment #8)
> Changes from attachment 19754 [details] [edit]:
>  * InspectorController is now a JavaScriptDebugListener.
>  * No InspectorClient methods are added. Instead, we handle the event loop
> entirely within WebCore.
>  * Unused arguments that were being passed down to JS in the debugging methods
> have been removed.

Oh, and:
 * Adds a PageGroupJavaScriptPauser class that pauses JavaScript for an entire PageGroup.

The behavioral difference between attachment 19754 [details] and this patch (attachment 19839 [details]) is that this patch allows you to interact with the WebView being debugged, e.g. by scrolling it, selecting text, clicking links, etc.

Whether or not this is a good thing is TBD since you could conceivably screw up your debugging by, e.g., scrolling the window just before evaluating window.scrollTop, but it does match Firebug's behavior.

[Adam Roben (:aroben)]

(In reply to comment #8)
> Created an attachment (id=19839) [edit]
> [WIP] Make the Inspector able to pause/step JS code
> 
> This patch is not ready for review.
> 
> This patch depends on the following patches:
> 
> From Bug 17133:
>  * Attachment 19590 [details] [edit]
>  * Attachment 19729 [details] [edit]
>  * Attachment 19788 [details] [edit]

Updated versions of these have all landed now.

[Darin Adler]

Comment on attachment 19836 [details]
Make JavaScriptDebugListeners able to listen to particular Pages

+    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

[Adam Roben (:aroben)]

(In reply to comment #11)
> (From update of attachment 19836 [details] [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!

[Adam Roben (:aroben)]

Comment on attachment 19836 [details]
Make JavaScriptDebugListeners able to listen to particular Pages

Committed as r31570

[Adam Roben (:aroben)]

(In reply to comment #12)
> (In reply to comment #11)
> > 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.

Committed the rename in r31571.

[Adam Roben (:aroben)]

Created attachment 20628 [details]
Add wrappers around showWindow/closeWindow

[Adam Roben (:aroben)]

Comment on attachment 20628 [details]
Add wrappers around showWindow/closeWindow

Committed in r32210

[Adam Roben (:aroben)]

Created attachment 20668 [details]
Make InspectorController a JavaScriptDebugListener

[Timothy Hatcher]

Comment on attachment 20668 [details]
Make InspectorController a JavaScriptDebugListener

r=me, but should reloadInspectedPageAndStartDebugging be startDebuggingAndRelaodInspectedPage to match the order of operations?

[Adam Roben (:aroben)]

(In reply to comment #18)
> (From update of attachment 20668 [details] [edit])
> r=me, but should reloadInspectedPageAndStartDebugging be
> startDebuggingAndRelaodInspectedPage to match the order of operations?

Yes, I'll rename it.

[Adam Roben (:aroben)]

Comment on attachment 20668 [details]
Make InspectorController a JavaScriptDebugListener

Committed in r32217