[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





--- Comment #3 from Andrey Adaikin <aandrey at google.com>  2011-02-14 09:07:02 PST ---
(From update of attachment 82305)
View in context: https://bugs.webkit.org/attachment.cgi?id=82305&action=review

>> Source/WebCore/inspector/front-end/TextViewer.js:578
>> +        this.element.addEventListener("keydown", this._handleKeyDown.bind(this), false);
> 
> What about drag'n'drop events?

Emmm... what about them? :) this particular code is just to disable overriding the "keydown" event when the editor is enabled (obviously, it is not needed in this case). Drag'n'drop events work perfectly in both read-only and editable cases.

>> 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?

For example:

10 // Line of code
11
12 // Another line of code
13
14
15 // Yet another line of code

If the caret is on the line 11 (empty line), and you hit backspace, it would remove the line and put the caret at the end of line 10, but no DOM event will fire, except for the DOMSubtreeModified on this.element target. However, if you do the same on line 14, some of these 3 events are fired. I think, there may be a bug in the WebKit with firing the DOMNodeRemoved event - IMO it is very rarely fired.

I also checked this case against the same 3 events that are used in the Elements panel (AFAIU they listen for custom WebInspector.* events from the backend) - and, unlike this "native" case, it worked as expected, and the DOMNodeRemoved is fired way more frequently.

>> 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?

done

>> Source/WebCore/inspector/front-end/TextViewer.js:761
>> +        this.beginDomUpdates();
> 
> Nit: you could do try { } finally { endDomUpdates; } to enable multiple return points.

That was my first thought, but I have not even tried it, so that I should not create a chance to lower the performance :) I checked the performance now, and it seems OK. Done.

BTW, should I use try-finally in all such cases, not only with 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.

1) This code is from a previous CL and has not been changed (only spaces for alignment - maybe that's why the review tool didn't notice it).

2) I don't see why we should introduce another coalescing guardians just for this, because if there should be a loop after calling the _syncDecorationsForLine() - it will be guarded with the same this._domUpdateCoalescingLevel value (if the DOM is modified), and will not reach here again.

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

done

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

done

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

done

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