Bug 58029 – Web Inspector: update breakpoints according to source frame decorations after live edit.

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

[Pavel Podivilov]

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.

[Pavel Podivilov]

Created attachment 88607 [details]
Patch.

[Andrey Adaikin]

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

[Pavel Feldman]

Comment on attachment 88607 [details]
Patch.

Please rebaseline and add a test.

[Pavel Podivilov]

Created attachment 88652 [details]
Patch.

[Pavel Feldman]

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.

[Andrey Adaikin]

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.

[Pavel Podivilov]

(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.

[Pavel Podivilov]

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.

[Pavel Podivilov]

Committed r83294: <http://trac.webkit.org/changeset/83294>