Summary: | Web Inspector: Uncaught Exception: Breakpoint at specified location already exists. | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Devin Rousso <hi> | ||||||
Component: | Web Inspector | Assignee: | Devin Rousso <hi> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | commit-queue, hi, inspector-bugzilla-changes, joepeck, webkit-bug-importer | ||||||
Priority: | P2 | Keywords: | InRadar | ||||||
Version: | WebKit Nightly Build | ||||||||
Hardware: | All | ||||||||
OS: | All | ||||||||
URL: | https://docs.google.com/spreadsheets/d/1ctYXqr9pEmQLC4X6UkKTKWGSWXquhlIbCLrQXCqUpP0/edit#gid=0 | ||||||||
Attachments: |
|
Description
Devin Rousso
2019-04-17 13:52:22 PDT
Created attachment 374644 [details]
Patch
Comment on attachment 374644 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=374644&action=review > Source/WebInspectorUI/ChangeLog:11 > + When "adjusting" a `WI.Breakpoint` (e.g. removing and then re-adding with a different > + configuration), make sure to only re-add the `WI.Breakpoint` to the `WI.Target` it was just > + removed from, rather than iterate over all `WI.targets`. I'm not sure I understand. We set each breakpoint in all targets, so if we remove and re-add a breakpoint it should be removed and re-added to all targets. > Source/WebInspectorUI/UserInterface/Controllers/DebuggerManager.js:931 > + _setBreakpoint(breakpoint, target) I still prefer `specificTarget` to avoid the shadow below (let target of targets). Comment on attachment 374644 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=374644&action=review >> Source/WebInspectorUI/ChangeLog:11 >> + removed from, rather than iterate over all `WI.targets`. > > I'm not sure I understand. > > We set each breakpoint in all targets, so if we remove and re-add a breakpoint it should be removed and re-added to all targets. Unless you mean we remove from all target and individually add back to each target? Comment on attachment 374644 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=374644&action=review >>> Source/WebInspectorUI/ChangeLog:11 >>> + removed from, rather than iterate over all `WI.targets`. >> >> I'm not sure I understand. >> >> We set each breakpoint in all targets, so if we remove and re-add a breakpoint it should be removed and re-added to all targets. > > Unless you mean we remove from all target and individually add back to each target? Previously, the pseudo-code looked like this: for each target (tA): <WI.DebuggerManager.prototype._removeBreakpoint> remove breakpoint from tA -> callback { <WI.DebuggerManager.prototype._removeBreakpoint> for each target (tB): <WI.DebuggerManager.prototype._addBreakpoint> add breakpoint to tB <WI.DebuggerManager.prototype._addBreakpoint> } This is a problem because each iteration of the inner `for` is re-adding the breakpoint to ALL targets (one-to-many), not just the one it was just removed from. The new pseudo-code looks like this: for each target (tA): <WI.DebuggerManager.prototype._removeBreakpoint> remove breakpoint from tA -> callback { <WI.DebuggerManager.prototype._removeBreakpoint> add breakpoint to tA <WI.DebuggerManager.prototype._addBreakpoint> } Now, we only re-add the breakpoint to the target it was just removed from (one-to-one). >> Source/WebInspectorUI/UserInterface/Controllers/DebuggerManager.js:931 >> + _setBreakpoint(breakpoint, target) > > I still prefer `specificTarget` to avoid the shadow below (let target of targets). Oh oops :P I'll change it back Created attachment 374655 [details]
Patch
Comment on attachment 374655 [details]
Patch
r-me
> Now, we only re-add the breakpoint to the target it was just removed from
> (one-to-one).
Okay good that is what I was expecting!
Comment on attachment 374655 [details] Patch Clearing flags on attachment: 374655 Committed r247715: <https://trac.webkit.org/changeset/247715> All reviewed patches have been landed. Closing bug. |