[webkit-reviews] review denied: [Bug 70181] Web Inspector: Unindent edited text by pressing Shift + Tab : [Attachment 113751] Indent/unindent multiple lines

[bugzilla-daemon at webkit.org]

Pavel Feldman <pfeldman at chromium.org> has denied Nikita Vasilyev
<me at elv1s.ru>'s request for review:
Bug 70181: Web Inspector: Unindent edited text by pressing Shift + Tab
https://bugs.webkit.org/show_bug.cgi?id=70181

Attachment 113751: Indent/unindent multiple lines
https://bugs.webkit.org/attachment.cgi?id=113751&action=review

------- Additional Comments from Pavel Feldman <pfeldman at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=113751&action=review


> Source/WebCore/inspector/front-end/TextViewer.js:1065
> +	       if (range.isEmpty()) {

Lets apply indent when there is multiline selection only (Eclipse model). I.e.
if (range.linesCount())
    newRange = this.indentLines(range);
else
    ...

> Source/WebCore/inspector/front-end/TextViewer.js:1079
> +    indentLines: function(range)

Should be called _indentLines (since is private to this file).

> Source/WebCore/inspector/front-end/TextViewer.js:1083
> +	   if (this._lastEditedRange)

It looks like you should set lastEditedRange to the newRange in the end of this
method.

> Source/WebCore/inspector/front-end/TextViewer.js:1096
> +    unindentLines: function(range)

_unindentLines

> Source/WebCore/inspector/front-end/TextViewer.js:1099
> +	       this._textModel.markUndoableState();

Same comment on the undoable state, should set lastEditedRange in the end.

> Source/WebCore/inspector/front-end/TextViewer.js:1105
> +	   for (var lineNumber = range.startLine; lineNumber <= range.endLine;
lineNumber++) {

Again, I would use Eclipse model here: go through lines, check whether line
starts with indent. If all lines do, trim them. Otherwise do nothing.