Bug 54661 - Web Inspector: [Text editor] Optimize editing updates in main panel
Summary: Web Inspector: [Text editor] Optimize editing updates in main panel
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated) (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 53588
  Show dependency treegraph
 
Reported: 2011-02-17 07:47 PST by Andrey Adaikin
Modified: 2011-02-18 05:39 PST (History)
11 users (show)

See Also:


Attachments
Patch (23.26 KB, patch)
2011-02-17 07:55 PST, Andrey Adaikin
no flags Details | Formatted Diff | Diff
Patch (23.31 KB, patch)
2011-02-18 02:17 PST, Andrey Adaikin
no flags Details | Formatted Diff | Diff
Patch (23.66 KB, patch)
2011-02-18 04:57 PST, Andrey Adaikin
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andrey Adaikin 2011-02-17 07:47:54 PST
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.
Comment 1 Andrey Adaikin 2011-02-17 07:55:08 PST
Created attachment 82805 [details]
Patch
Comment 2 Andrey Adaikin 2011-02-18 02:17:21 PST
Created attachment 82937 [details]
Patch
Comment 3 Pavel Feldman 2011-02-18 02:46:53 PST
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?
Comment 4 Andrey Adaikin 2011-02-18 04:42:51 PST
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
Comment 5 Andrey Adaikin 2011-02-18 04:57:24 PST
Created attachment 82944 [details]
Patch
Comment 6 Pavel Feldman 2011-02-18 05:33:24 PST
Comment on attachment 82944 [details]
Patch

Lets do the tests and the refactoring we discussed offline.
Comment 7 Pavel Podivilov 2011-02-18 05:39:19 PST
Comment on attachment 82944 [details]
Patch

Clearing flags on attachment: 82944

Committed r78998: <http://trac.webkit.org/changeset/78998>
Comment 8 Pavel Podivilov 2011-02-18 05:39:29 PST
All reviewed patches have been landed.  Closing bug.