Bug 54661 – Web Inspector: [Text editor] Optimize editing updates in main panel

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

[Andrey Adaikin]

Optimize editing updates in the main panel. Instead of rebuilding all text chunks, now we update only those that has been changed. The same is to be done for the gutter panel in a separate CL.

[Andrey Adaikin]

Created attachment 82805 [details]
Patch

[Andrey Adaikin]

Created attachment 82937 [details]
Patch

[Pavel Feldman]

Comment on attachment 82937 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=82937&action=review

It looks sane, but I am a bit concerned with the code bloat. Can we introduce new structures / abstractions to keep the code in a readable state?

> Source/WebCore/inspector/front-end/TextViewer.js:44
> +    this._mainPanel = new WebInspector.TextEditorMainPanel(this._textModel, url, syncScrollListener, syncDecorationsForLineListener, enterTextChangeMode, exitTextChangeMode);

6 parameters in the constructor makes it look like a bad design.

> Source/WebCore/inspector/front-end/TextViewer.js:167
> +        // FIXME: UPDATE GUTTER SMARTLY!

No need to shout :)

> Source/WebCore/inspector/front-end/TextViewer.js:203
> +        var mainChunk = this._mainPanel.chunkForLine(lineNumber);

It sounds like main/gutterChunk separation starts hitting us (code bloat)

> Source/WebCore/inspector/front-end/TextViewer.js:773
> +    _markSkippedPaintLines: function(startLine, endLine)

What is "skipped line" and why do we mark it?

> Source/WebCore/inspector/front-end/TextViewer.js:1199
> +        // Update the chunks in range: firstChunkNumber <= index <= lastChunkNumber

Can this method receive only ranges?

[Andrey Adaikin]

Comment on attachment 82937 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=82937&action=review

>> Source/WebCore/inspector/front-end/TextViewer.js:44
>> +    this._mainPanel = new WebInspector.TextEditorMainPanel(this._textModel, url, syncScrollListener, syncDecorationsForLineListener, enterTextChangeMode, exitTextChangeMode);
> 
> 6 parameters in the constructor makes it look like a bad design.

yes! :) another option that I can think of is to just pass the "this" reference, as we already do for Chunk's constructor. can suggest anything else?

>> Source/WebCore/inspector/front-end/TextViewer.js:167
>> +        // FIXME: UPDATE GUTTER SMARTLY!
> 
> No need to shout :)

this is just for me. I can remove it.

>> Source/WebCore/inspector/front-end/TextViewer.js:203
>> +        var mainChunk = this._mainPanel.chunkForLine(lineNumber);
> 
> It sounds like main/gutterChunk separation starts hitting us (code bloat)

should I move the classes on it's own files (and maybe package=directory)? next CL? :)

>> Source/WebCore/inspector/front-end/TextViewer.js:773
>> +    _markSkippedPaintLines: function(startLine, endLine)
> 
> What is "skipped line" and why do we mark it?

we can not paint lines during an editing phase (this._dirtyLines !== undefined), neither we can just skip them, as it was before, because we will loose the callbacks from the highlighter for expanded lines.

>> Source/WebCore/inspector/front-end/TextViewer.js:1199
>> +        // Update the chunks in range: firstChunkNumber <= index <= lastChunkNumber
> 
> Can this method receive only ranges?

yes, the other 3 arguments can be calculated again

[Andrey Adaikin]

Created attachment 82944 [details]
Patch

[Pavel Feldman]

Comment on attachment 82944 [details]
Patch

Lets do the tests and the refactoring we discussed offline.

[Pavel Podivilov]

Comment on attachment 82944 [details]
Patch

Clearing flags on attachment: 82944

Committed r78998: <http://trac.webkit.org/changeset/78998>

[Pavel Podivilov]

All reviewed patches have been landed.  Closing bug.