[webkit-reviews] review denied: [Bug 54388] Web Inspector: [Text editor] First implementation of the editable TextViewer without optimization : [Attachment 82305] Patch

[bugzilla-daemon at webkit.org]

Pavel Feldman <pfeldman at chromium.org> has denied Andrey Adaikin
<aandrey at google.com>'s request for review:
Bug 54388: Web Inspector: [Text editor] First implementation of the editable
TextViewer without optimization
https://bugs.webkit.org/show_bug.cgi?id=54388

Attachment 82305: Patch
https://bugs.webkit.org/attachment.cgi?id=82305&action=review

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

Few nits and it is good to land.

> Source/WebCore/inspector/front-end/TextViewer.js:578
> +	   this.element.addEventListener("keydown",
this._handleKeyDown.bind(this), false);

What about drag'n'drop events?

> Source/WebCore/inspector/front-end/TextViewer.js:584
> +    // For some reasons, in a few corner cases the events above are not able
to catch the editings.

When does this happen?

> Source/WebCore/inspector/front-end/TextViewer.js:704
> +	   for (var i = 0; i < this._textModel.linesCount; ++i) {

No {} for single line body. Why does this happen here? Should it be
encapsulated in the highlighter?

> Source/WebCore/inspector/front-end/TextViewer.js:761
> +	   this.beginDomUpdates();

Nit: you could do try { } finally { endDomUpdates; } to enable multiple return
points.

> Source/WebCore/inspector/front-end/TextViewer.js:990
> +		   setTimeout(function() {

Workarounds like this should be encapsulated in separate methods and provided
with FIXME. You might also want to introduce coalescing for events like this.

> Source/WebCore/inspector/front-end/TextViewer.js:1098
> +	       ++startLine

;

> Source/WebCore/inspector/front-end/TextViewer.js:1106
> +	       --endLine

;

> Source/WebCore/inspector/front-end/TextViewer.js:1143
> +	   for (var i = 0; i < textContents.length; ++i) {

No {}