Bug 222406 – Render tree updates for Text node content mutations should happen during rendering update

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Antti Koivisto]

Render tree should be mutated lazily as a part of rendering update like in all other cases.

[Antti Koivisto]

Created attachment 421504 [details]
wip

[Antti Koivisto]

Created attachment 421523 [details]
patch

[Antti Koivisto]

Created attachment 421525 [details]
patch

[Antti Koivisto]

Created attachment 421546 [details]
patch

[Simon Fraser (smfr)]

Comment on attachment 421546 [details]
patch

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

> Source/WebCore/ChangeLog:35
> +        This is because subsequent steps in multipart commands expect render tree to be up to date.

Shouldn't this be handled by the next step triggering a render tree update? It's going to be hard for author of new code to know if they have to call updateStyleIfNeeded() or not.

> Source/WebCore/ChangeLog:47
> +        Pending text update mat ref the node so this refcount assert is not correct.

mat ref?

[Antti Koivisto]

> Shouldn't this be handled by the next step triggering a render tree update?

In theory yes, but it is very difficult to discover all the paths that would need it as there is no guarantee test coverage is complete. In practice missing cases would be discover via regressions.

> It's going to be hard for author of new code to know if they have to call
> updateStyleIfNeeded() or not.

These just maintain the existing behaviour (sync render tree update on Text content mutations). 

Anyone working on this code needs to be careful about updates in any case, this patch in no way changes that.

[Antti Koivisto]

(In reply to Antti Koivisto from comment #6)
> These just maintain the existing behaviour (sync render tree update on Text
> content mutations). 

...in editing code specifically.

[Antti Koivisto]

Created attachment 421631 [details]
patch

[Antti Koivisto]

Created attachment 421636 [details]
patch

[Antti Koivisto]

Created attachment 421643 [details]
patch

[Antti Koivisto]

Created attachment 421645 [details]
alternative approach

FWIW, here is a patch I made for this bug earlier. The idea is to let the first paint triggered by a resource load to always happen, even if the size has been computed to zero. It is bit hacky but so is rest of the code.

[Antti Koivisto]

Created attachment 421647 [details]
patch

[Antti Koivisto]

Created attachment 421664 [details]
patch

[Antti Koivisto]

Created attachment 421667 [details]
patch

[Antti Koivisto]

Created attachment 421679 [details]
patch

[Simon Fraser (smfr)]

Comment on attachment 421679 [details]
patch

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

> Source/WebCore/dom/CharacterData.cpp:105
> -    if (is<Text>(*this) && parentNode())
> +    if (is<Text>(*this))

This doesn't seem to be mentioned in the changelog.

> Source/WebCore/editing/CompositeEditCommand.cpp:1062
> +    if (!nodes.isEmpty())
> +        document().updateLayoutIgnorePendingStylesheets();

This is still pretty mysterious.

[Antti Koivisto]

> This doesn't seem to be mentioned in the changelog.

The check just moves to the function.

[Antti Koivisto]

Created attachment 421738 [details]
patch

[EWS]

Committed r273621: <https://commits.webkit.org/r273621>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 421738 [details].

[Radar WebKit Bug Importer]

<rdar://problem/74822830>

[Darin Adler]

Comment on attachment 421738 [details]
patch

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

> Source/WebCore/style/StyleUpdate.cpp:104
> +    auto result = m_texts.ensure(&text, [&] {
> +        return WTFMove(textUpdate);
> +    });

I think this is identical to:

    auto result = m_texts.add(&text, WTFMove(textUpdate));

Maybe because it looks like WTFMove causes a "move" that the ensure form seems safer, but pretty sure it’s no different.

[Antti Koivisto]

> Maybe because it looks like WTFMove causes a "move" that the ensure form
> seems safer, but pretty sure it’s no different.

Yeah, it just looked less dangerous. I think you are right that it is equivalent.

[Antti Koivisto]

Reverted in r275657 due to PLT regression.

[Antti Koivisto]

Created attachment 426446 [details]
patch for relanding

[EWS]

Committed r276287 (236769@main): <https://commits.webkit.org/236769@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 426446 [details].