Bug 216257

Created attachment 408345 [details]
For EWS

Comment on attachment 408345 [details]
For EWS

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

> Source/WebCore/dom/Element.cpp:3422
> +    auto* style = hasNodeFlag(NodeFlag::IsComputedStyleInvalidFlag) ? nullptr : existingComputedStyle();
>      if (!style)
>          style = const_cast<Element&>(*this).resolveComputedStyle(ResolveComputedStyleMode::RenderedOnly);

Shouldn't we clear the flag in resolveComputedStyle?

Comment on attachment 408345 [details]
For EWS

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

>> Source/WebCore/dom/Element.cpp:3422
>>          style = const_cast<Element&>(*this).resolveComputedStyle(ResolveComputedStyleMode::RenderedOnly);
> 
> Shouldn't we clear the flag in resolveComputedStyle?

Whoops — that’s right, we should clear the flag when we cache the computed style in that function. Will update the patch to do so!

After chatting with Antti, we both think this new flag should exist in ElementRareData alongside the computedStyle it is about, too.

(In reply to Wenson Hsieh from comment #9)
> After chatting with Antti, we both think this new flag should exist in
> ElementRareData alongside the computedStyle it is about, too.

…but as it turns out, doing so would require us to increase the size of ElementRareData, so just having the flag here is more optimal :/

Created attachment 408462 [details]
Patch for landing

Committed r266887: <https://trac.webkit.org/changeset/266887>

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

Comment on attachment 408462 [details]
Patch for landing

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

> Source/WebCore/dom/Element.cpp:1976
> +    if (hasRareData() && elementRareData()->computedStyle())
> +        setNodeFlag(NodeFlag::IsComputedStyleInvalidFlag);

Do we really need to check computedStyle for null? Isn’t it harmless to set this flag if there is no computed style?

Comment on attachment 408462 [details]
Patch for landing

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

>> Source/WebCore/dom/Element.cpp:1976
>> +        setNodeFlag(NodeFlag::IsComputedStyleInvalidFlag);
> 
> Do we really need to check computedStyle for null? Isn’t it harmless to set this flag if there is no computed style?

Good question! The thought here was that if we hadn’t already set a computed style at this point, then we shouldn’t need to mark as invalid. However, after adding code to clear the flag when setting computed style below, I agree this check is not very useful. The only other codepath that calls setComputedStyle is in `Element::storeDisplayContentsStyle`, so we should be able to remove this if we make sure to clear the flag out there as well.

I’ll do this in a followup.

Reopening to attach new patch.

Created attachment 408486 [details]
Address comment

Committed r266899: <https://trac.webkit.org/changeset/266899>

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