Probes: fix breakpoint mode inconsistency when creating from popup menu.

Fixes #419. This fixes two things: The Breakpoint.mode setter didn't check the validity of the supplied mode. Now, if you try to set auto-continue when no actions are attached, it will instead set the mode to enabled. There was a race between setting the probe breakpoint action and setting the breakpoint's mode to auto-continue. A short timer was added as a temporary workaround until Breakpoints have an API for batching mutations.
burg · Dec 9, 2013 · 2f28b50 · 2f28b50
1 parent d262590
commit 2f28b50
Show file tree

Hide file tree

Showing 3 changed files with 14 additions and 5 deletions.
diff --git a/Source/WebInspectorUI/UserInterface/Breakpoint.js b/Source/WebInspectorUI/UserInterface/Breakpoint.js
@@ -146,7 +146,10 @@ WebInspector.Breakpoint.prototype = {
         if (this._mode === mode)
             return;
 
-        this._mode = mode || WebInspector.Breakpoint.Mode.Enabled;
+        if (!mode || (mode === WebInspector.Breakpoint.Mode.AutoContinue && !this.actions.length))
+            mode = WebInspector.Breakpoint.Mode.Enabled;
+
+        this._mode = mode;
         this.dispatchEventToListeners(WebInspector.Breakpoint.Event.ModeDidChange);
     },
 

diff --git a/Source/WebInspectorUI/UserInterface/DebuggerManager.js b/Source/WebInspectorUI/UserInterface/DebuggerManager.js
@@ -630,6 +630,9 @@ WebInspector.DebuggerManager.prototype = {
             return;
 
         // If the breakpoint exists in the backend, remove it.
+        // FIXME: several outstanding modifications may race as the first
+        // one triggers removal of the breakpoint.id, and subsequent
+        // modifications take the else branch and set the 'new' breakpoint first.
         if (breakpoint.id)
             this._removeBreakpoint(breakpoint, setNewBreakpoint.bind(this));
         else

diff --git a/Source/WebInspectorUI/UserInterface/TextEditor.js b/Source/WebInspectorUI/UserInterface/TextEditor.js
@@ -1005,12 +1005,15 @@ WebInspector.TextEditor.prototype = {
                     }
                 }
 
-                // If we created the breakpoint just for the probe, default to auto-continue.
-                if (didCreateBreakpoint)
-                    foundBreakpoint.mode = WebInspector.Breakpoint.Mode.AutoContinue;
-
                 var newAction = foundBreakpoint.createAction(WebInspector.BreakpointAction.Type.Probe);
                 newAction.data = expression;
+
+                // If we created the breakpoint just for the probe, default to auto-continue (after action is added).
+                // FIXME: this is a horrible hack to mitigate protocol races on multiple breakpoint mutations. This could
+                // be fixed by adding lightweight mutation batching to Breakpoint, like CodeMirror does for DOM updates.
+                if (didCreateBreakpoint)
+                    setTimeout(function() { foundBreakpoint.mode = WebInspector.Breakpoint.Mode.AutoContinue; }, 100);
+
                 popover.dismiss();
             }