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

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

[Andrey Adaikin]

This is to track the progress of implementing the editable TextViewer, as described here: https://bugs.webkit.org/show_bug.cgi?id=53588

By this changeset, the editor is supposed to work, i.e. the changes should be saved in the text model, but there are no optimization yet. The optimization part is quite a big amount of work by itself, thus this separation.

[Andrey Adaikin]

Created attachment 82305 [details]
Patch

[Pavel Feldman]

Comment on attachment 82305 [details]
Patch

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 {}

[Andrey Adaikin]

Comment on attachment 82305 [details]
Patch

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

[Andrey Adaikin]

Created attachment 82323 [details]
Patch

[Pavel Podivilov]

Comment on attachment 82323 [details]
Patch

Clearing flags on attachment: 82323

Committed r78483: <http://trac.webkit.org/changeset/78483>

[Pavel Podivilov]

All reviewed patches have been landed.  Closing bug.