Bug 101841 – Web Inspector: migrate text editor to mutation observers

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

[Pavel Feldman]

Otherwise, we miss notifications on the removed lines.

[Pavel Feldman]

Created attachment 173456 [details]
Patch

[Pavel Feldman]

Created attachment 173488 [details]
Patch

[Pavel Feldman]

Created attachment 173501 [details]
Patch

[Vsevolod Vlasov]

Comment on attachment 173501 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=173501&action=review

> Source/WebCore/inspector/front-end/DefaultTextEditor.js:2080
> +        if (false) // For paint debugging.

Maybe add a debug flag at the top of this file?

> Source/WebCore/inspector/front-end/DefaultTextEditor.js:2317
> +        // console.log(this.element.innerText === this._textModel.text() + "\n");

Ditto

[Pavel Feldman]

Created attachment 173626 [details]
Patch

[Vsevolod Vlasov]

Comment on attachment 173626 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=173626&action=review

> Source/WebCore/inspector/front-end/DefaultTextEditor.js:2147
> +            if (mutation.addedNodes.length === 1 && mutation.addedNodes[0].nodeName === "BR")

remove br check?

> Source/WebCore/inspector/front-end/DefaultTextEditor.js:2211
>              // Ensure that the newly inserted line row has no lineNumber.

Shouldn't it be assert?

[Andrey Adaikin]

Comment on attachment 173626 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=173626&action=review

>> Source/WebCore/inspector/front-end/DefaultTextEditor.js:2211
>>              // Ensure that the newly inserted line row has no lineNumber.
> 
> Shouldn't it be assert?

This is to remove the lineNumber property from a line row being detached from the DOM, as it may be (and will be) cached for a future reuse, and we don't ever want to reuse it with a wrong lineNumber attribute.

[Andrey Adaikin]

Comment on attachment 173626 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=173626&action=review

>>> Source/WebCore/inspector/front-end/DefaultTextEditor.js:2211
>>>              // Ensure that the newly inserted line row has no lineNumber.
>> 
>> Shouldn't it be assert?
> 
> This is to remove the lineNumber property from a line row being detached from the DOM, as it may be (and will be) cached for a future reuse, and we don't ever want to reuse it with a wrong lineNumber attribute.

ah we insert a line row here, I missed this... I guess we can get wrong lineNumbers after a drag'n'drop of a peace of code in the editor from one place to another.

[Andrey Adaikin]

Comment on attachment 173626 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=173626&action=review

> Source/WebCore/inspector/front-end/DefaultTextEditor.js:2080
> +        if (false) // For paint debugging.

remove?

> Source/WebCore/inspector/front-end/DefaultTextEditor.js:2186
> +        // Debug check for validating whether text model matches the DOM representation.

meant to leave this comment?

> Source/WebCore/inspector/front-end/DefaultTextEditor.js:2320
>          // This is a "foreign" call outside of this class. Should be before we delete the dirty lines flag.

this comment is no longer valid

[Pavel Feldman]

Committed r134223: <http://trac.webkit.org/changeset/134223>

[WebKit Review Bot]

Re-opened since this is blocked by bug 101939

[Pavel Feldman]

Created attachment 173841 [details]
[Patch] for landing

[Pavel Feldman]

Committed r134379: <http://trac.webkit.org/changeset/134379>

[Csaba Osztrogonác]

(In reply to comment #13)
> Committed r134379: <http://trac.webkit.org/changeset/134379>

It broke zillion tests at least on Qt - http://build.webkit.sed.hu/results/x86-64%20Linux%20Qt%20Release/r134382%20%2845038%29/results.html

[Csaba Osztrogonác]

It seems the root of the problem is that mutation observers is disabled on Qt.
Is there a reason why is it disabled?