[Webkit-unassigned] [Bug 54388] Web Inspector: [Text editor] First implementation of the editable TextViewer without optimization

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=54388


Pavel Feldman <pfeldman at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #82305|review?                     |review-
               Flag|                            |




--- Comment #2 from Pavel Feldman <pfeldman at chromium.org>  2011-02-14 08:31:07 PST ---
(From update of attachment 82305)
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 {}

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.