Web Inspector: update breakpoints according to source frame decorations after live edit. Currently, breakpoint decorations are moved accurately during the editing. But when user saves the result, breakpoints are moved in the model based on a text diff, which is not always accurate. We should move breakpoints in the model based on decorations. Text diff should only be used as a fallback for "revert to revision" feature.
Created attachment 88607 [details] Patch.
View in context: https://bugs.webkit.org/attachment.cgi?id=88607&action=review > Source/WebCore/inspector/front-end/SourceFrame.js:905 > + this._delegate.removeBreakpoint(lineNumber); lineNumber -> Number(lineNumber), check other places too
Comment on attachment 88607 [details] Patch. Please rebaseline and add a test.
Created attachment 88652 [details] Patch.
Comment on attachment 88652 [details] Patch. View in context: https://bugs.webkit.org/attachment.cgi?id=88652&action=review > Source/WebCore/inspector/front-end/SourceFrame.js:909 > + this._delegate.setBreakpoint(Number(lineNumber), breakpoint.condition, breakpoint.enabled); It is not clear whether _updateBreakpointsAfterLiveEdit or this code should take care of breakpoint re-creation.
View in context: https://bugs.webkit.org/attachment.cgi?id=88652&action=review >> Source/WebCore/inspector/front-end/SourceFrame.js:909 >> + this._delegate.setBreakpoint(Number(lineNumber), breakpoint.condition, breakpoint.enabled); > > It is not clear whether _updateBreakpointsAfterLiveEdit or this code should take care of breakpoint re-creation. Yes, I vote for the _updateBreakpointsAfterLiveEdit taking care of this. Note, that the (this._textModel.text !== newSource) condition may take quite a while on big data sources. Before this change, it was executed only if an error occurs.
(In reply to comment #5) > (From update of attachment 88652 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=88652&action=review > > > Source/WebCore/inspector/front-end/SourceFrame.js:909 > > + this._delegate.setBreakpoint(Number(lineNumber), breakpoint.condition, breakpoint.enabled); > > It is not clear whether _updateBreakpointsAfterLiveEdit or this code should take care of breakpoint re-creation. The only place where breakpoints are updated according to decorations is source frame's method _handleSave. The only place where _updateBreakpointsAfterLiveEdit is called is revert callback in _updateResourceContent function (used by "revert to this revision"). These two cases can never be mixed. > Yes, I vote for the _updateBreakpointsAfterLiveEdit taking care of this. Note, that the (this._textModel.text !== newSource) condition may take quite a while on big data sources. Before this change, it was executed only if an error occurs. _updateBreakpointsAfterLiveEdit is a fallback used when we only have old text and new text. I'm not sure if (this._textModel.text !== newSource) may cause any trouble here. If it may, we should just freeze the content until editScriptSource response is received.
Created attachment 88787 [details] Patch. As discussed offline, we should better freeze the content until editScriptSource response is recieved. This is needed to avoid expensive (this._textModel.text !== newSource) check and potential bugs when new editing is started while previous one was not committed yet.
Committed r83294: <http://trac.webkit.org/changeset/83294>