Bug 105797 – Web Inspector: refactor DefaultTextEditor's private methods

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

[Andrey Lushnikov]

A pre-work is needed for splitting classes from DefaultTextEditor.js into four files: DefaultTextEditor.js (DefaultTextEditor class), TextEditorMainPanel.js (TextEditorMainPanel and TextEditorMainChunk classes), TextEditorGutterPanel.js (TextEditorGutterPanel and TextEditorGutterChunk classes) and TextEditorChunkedPanel.js (TextEditorChunkedPanel class).

Refactor DefaultTextEditor so that different classes do not use each-others private methods and fields if they will reside in different files.

[Andrey Lushnikov]

Created attachment 180793 [details]
Patch

[Pavel Feldman]

Comment on attachment 180793 [details]
Patch

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

> Source/WebCore/inspector/front-end/DefaultTextEditor.js:414
> +        this._mainPanel.handleTextInput(e);

Just add this listener within the main panel.

> Source/WebCore/inspector/front-end/DefaultTextEditor.js:450
>              this._delegate.populateTextAreaContextMenu(contextMenu, target && target.lineNumber);

You should instead pass event.target here (this._mainPanel.handleContextMenu(event);)

> Source/WebCore/inspector/front-end/DefaultTextEditor.js:458
> +        var firstVisibleLineNumber = this._mainPanel.findFirstVisibleLineNumber(visibleFrom);

lineNumberAtOffset(visibleFrom)

> Source/WebCore/inspector/front-end/DefaultTextEditor.js:491
> +        this._mainPanel.setLastSelection(textRange);

I'd rather introduce mainPanel.setSelection that would do this all. _restoreSelection does not need to be public then.

> Source/WebCore/inspector/front-end/DefaultTextEditor.js:590
> +        this.freeCachedElements();

You already have willHide, free cached elements from there.

> Source/WebCore/inspector/front-end/DefaultTextEditor.js:1030
> +    this.element.addEventListener("scroll", this.scroll.bind(this), false);

Just create this.element in TextEditorChunkedPanel, add listener there.

> Source/WebCore/inspector/front-end/DefaultTextEditor.js:1307
> +        return this._textEditor.totalHeight(this._expandedLineRows[0], this._expandedLineRows[this._expandedLineRows.length - 1]);

rename textEditor to chunkedPanel

[Andrey Lushnikov]

Comment on attachment 180793 [details]
Patch

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

>> Source/WebCore/inspector/front-end/DefaultTextEditor.js:414
>> +        this._mainPanel.handleTextInput(e);
> 
> Just add this listener within the main panel.

fixed

>> Source/WebCore/inspector/front-end/DefaultTextEditor.js:450
>>              this._delegate.populateTextAreaContextMenu(contextMenu, target && target.lineNumber);
> 
> You should instead pass event.target here (this._mainPanel.handleContextMenu(event);)

fixed

>> Source/WebCore/inspector/front-end/DefaultTextEditor.js:458
>> +        var firstVisibleLineNumber = this._mainPanel.findFirstVisibleLineNumber(visibleFrom);
> 
> lineNumberAtOffset(visibleFrom)

fixed

>> Source/WebCore/inspector/front-end/DefaultTextEditor.js:491
>> +        this._mainPanel.setLastSelection(textRange);
> 
> I'd rather introduce mainPanel.setSelection that would do this all. _restoreSelection does not need to be public then.

fixed

>> Source/WebCore/inspector/front-end/DefaultTextEditor.js:590
>> +        this.freeCachedElements();
> 
> You already have willHide, free cached elements from there.

fixed

>> Source/WebCore/inspector/front-end/DefaultTextEditor.js:1030
>> +    this.element.addEventListener("scroll", this.scroll.bind(this), false);
> 
> Just create this.element in TextEditorChunkedPanel, add listener there.

done

>> Source/WebCore/inspector/front-end/DefaultTextEditor.js:1307
>> +        return this._textEditor.totalHeight(this._expandedLineRows[0], this._expandedLineRows[this._expandedLineRows.length - 1]);
> 
> rename textEditor to chunkedPanel

renamed

[Andrey Lushnikov]

Created attachment 180802 [details]
Patch

[Pavel Feldman]

Comment on attachment 180802 [details]
Patch

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

> Source/WebCore/inspector/front-end/DefaultTextEditor.js:437
> +            this._mainPanel.contextMenu(event.target, contextMenu);

populateContextMenu

> Source/WebCore/inspector/front-end/DefaultTextEditor.js:462
> +        return this._mainPanel.getSelection();

selection()

> Source/WebCore/inspector/front-end/DefaultTextEditor.js:-484
> -        return this._mainPanel._lastSelection;

lastSelection

> Source/WebCore/inspector/front-end/DefaultTextEditor.js:556
> +        this._boundSelectionChangeListener = this._mainPanel.handleSelectionChange.bind(this._mainPanel);

You should register this listener within mainPanel's wasShown instead.

> Source/WebCore/inspector/front-end/DefaultTextEditor.js:806
> +        throw new Error("createNewChunk() is not implemented");

... should be implemented by descendants

[WebKit Review Bot]

Comment on attachment 180802 [details]
Patch

Clearing flags on attachment: 180802

Committed r138527: <http://trac.webkit.org/changeset/138527>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.