Changeset 171784 in webkit

Timestamp:

Jul 29, 2014 8:48:54 PM (10 years ago)

Author:

Brian Burg

Message:

Web Inspector: breakpoints are always speculatively resolved when restored from local storage
​https://bugs.webkit.org/show_bug.cgi?id=135396

Reviewed by Timothy Hatcher.

A longstanding quirk/optimization in the frontend is that we immediately set a breakpoint
as resolved if the breakpoint was successfully set in the backend. This ensures that clicking in
the gutter immediately produces a resolved breakpoint with only one round-trip.

However, not all breakpoints should be speculatively resolved, because the corresponding resource
may not be loaded yet. This situation causes problems for code that assumes a resolved breakpoint
also has a valid sourceCodeLocation.sourceCode.

• UserInterface/Controllers/DebuggerManager.js:

(WebInspector.DebuggerManager.restoreBreakpointsSoon): Don't leak the variable to global scope.
(WebInspector.DebuggerManager):
(WebInspector.DebuggerManager.prototype.speculativelyResolveBreakpoint):
(WebInspector.DebuggerManager.prototype.addBreakpoint): Speculatively resolve here if requested
using the success callback passed to _setBreakpoint.

(WebInspector.DebuggerManager.prototype.didSetBreakpoint): Emit simulated
Debugger.breakpointResolved events since they are only sent by the backend when a script is parsed.

(WebInspector.DebuggerManager.prototype._setBreakpoint):

• UserInterface/Views/SourceCodeTextEditor.js:

(WebInspector.SourceCodeTextEditor.prototype.textEditorBreakpointAdded): Request speculative resolve.

Location:

trunk/Source/WebInspectorUI

Files:

• ChangeLog (modified) (1 diff)
• UserInterface/Controllers/DebuggerManager.js (modified) (5 diffs)
• UserInterface/Views/SourceCodeTextEditor.js (modified) (1 diff)

trunk/Source/WebInspectorUI/ChangeLog

@@ +1 @@
+2014-07-29  Brian J. Burg  <burg@cs.washington.edu>
+
+        Web Inspector: breakpoints are always speculatively resolved when restored from local storage
+        https://bugs.webkit.org/show_bug.cgi?id=135396
+
+        Reviewed by Timothy Hatcher.
+
+        A longstanding quirk/optimization in the frontend is that we immediately set a breakpoint
+        as resolved if the breakpoint was successfully set in the backend. This ensures that clicking in
+        the gutter immediately produces a resolved breakpoint with only one round-trip.
+
+        However, not all breakpoints should be speculatively resolved, because the corresponding resource
+        may not be loaded yet. This situation causes problems for code that assumes a resolved breakpoint
+        also has a valid sourceCodeLocation.sourceCode.
+
+        * UserInterface/Controllers/DebuggerManager.js:
+        (WebInspector.DebuggerManager.restoreBreakpointsSoon): Don't leak the variable to global scope.
+        (WebInspector.DebuggerManager):
+        (WebInspector.DebuggerManager.prototype.speculativelyResolveBreakpoint):
+        (WebInspector.DebuggerManager.prototype.addBreakpoint): Speculatively resolve here if requested
+        using the success callback passed to _setBreakpoint.
+
+        (WebInspector.DebuggerManager.prototype.didSetBreakpoint): Emit simulated
+        Debugger.breakpointResolved events since they are only sent by the backend when a script is parsed.
+
+        (WebInspector.DebuggerManager.prototype._setBreakpoint):
+        * UserInterface/Views/SourceCodeTextEditor.js:
+        (WebInspector.SourceCodeTextEditor.prototype.textEditorBreakpointAdded): Request speculative resolve.
+
 2014-07-29  Joseph Pecoraro  <pecoraro@apple.com>
 

trunk/Source/WebInspectorUI/UserInterface/Controllers/DebuggerManager.js

@@ -68 +68 @@
 
     function restoreBreakpointsSoon() {
-        for (cookie of this._breakpointsSetting.value)
+        for (var cookie of this._breakpointsSetting.value)
             this.addBreakpoint(new WebInspector.Breakpoint(cookie));
     }
@@ -228 +228 @@
     },
 
-    addBreakpoint: function(breakpoint, skipEventDispatch)
+    addBreakpoint: function(breakpoint, skipEventDispatch, shouldSpeculativelyResolve)
     {
         console.assert(breakpoint instanceof WebInspector.Breakpoint, "Bad argument to DebuggerManger.addBreakpoint: ", breakpoint);
@@ -250 +250 @@
         this._breakpoints.push(breakpoint);
 
+        function speculativelyResolveBreakpoint(breakpoint) {
+            breakpoint.resolved = true;
+        }
+
         if (!breakpoint.disabled)
-            this._setBreakpoint(breakpoint);
+            this._setBreakpoint(breakpoint, shouldSpeculativelyResolve ? speculativelyResolveBreakpoint.bind(null, breakpoint) : null);
 
         if (!skipEventDispatch)
@@ -525 +529 @@
         this.breakpointsEnabled = true;
 
-        function didSetBreakpoint(error, breakpointIdentifier)
+        function didSetBreakpoint(error, breakpointIdentifier, locations)
         {
             if (error)
@@ -533 +537 @@
 
             breakpoint.identifier = breakpointIdentifier;
-            breakpoint.resolved = true;
+
+            // Debugger.setBreakpoint returns a single location.
+            if (!(locations instanceof Array))
+                locations = [locations];
+
+            for (var location of locations)
+                this.breakpointResolved(breakpointIdentifier, location);
 
             if (typeof callback === "function")

trunk/Source/WebInspectorUI/UserInterface/Views/SourceCodeTextEditor.js

@@ -840 +840 @@
 
         this._ignoreBreakpointAddedBreakpoint = breakpoint;
-        WebInspector.debuggerManager.addBreakpoint(breakpoint);
+
+        var shouldSkipEventDispatch = false;
+        var shouldSpeculativelyResolveBreakpoint = true;
+        WebInspector.debuggerManager.addBreakpoint(breakpoint, shouldSkipEventDispatch, shouldSpeculativelyResolveBreakpoint);
         delete this._ignoreBreakpointAddedBreakpoint;