Summary: | Web Inspector: update breakpoints according to source frame decorations after live edit. | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Pavel Podivilov <podivilov> | ||||||||
Component: | Web Inspector (Deprecated) | Assignee: | Pavel Podivilov <podivilov> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | aandrey, apavlov, bweinstein, joepeck, keishi, loislo, pfeldman, pmuellr, rik, timothy, yurys | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | All | ||||||||||
OS: | All | ||||||||||
Attachments: |
|
Description
Pavel Podivilov
2011-04-07 05:00:24 PDT
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> |