<html>
<head>
<base href="https://bugs.webkit.org/" />
</head>
<body><span class="vcard"><a class="email" href="mailto:darin@apple.com" title="Darin Adler <darin@apple.com>"> <span class="fn">Darin Adler</span></a>
</span> changed
<a class="bz_bug_link
bz_status_NEW "
title="NEW - Rename setNeedsStyleRecalc to invalidateStyle"
href="https://bugs.webkit.org/show_bug.cgi?id=163542">bug 163542</a>
<br>
<table border="1" cellspacing="0" cellpadding="8">
<tr>
<th>What</th>
<th>Removed</th>
<th>Added</th>
</tr>
<tr>
<td style="text-align:right;">Attachment #291818 Flags</td>
<td>review?
</td>
<td>review+
</td>
</tr></table>
<p>
<div>
<b><a class="bz_bug_link
bz_status_NEW "
title="NEW - Rename setNeedsStyleRecalc to invalidateStyle"
href="https://bugs.webkit.org/show_bug.cgi?id=163542#c4">Comment # 4</a>
on <a class="bz_bug_link
bz_status_NEW "
title="NEW - Rename setNeedsStyleRecalc to invalidateStyle"
href="https://bugs.webkit.org/show_bug.cgi?id=163542">bug 163542</a>
from <span class="vcard"><a class="email" href="mailto:darin@apple.com" title="Darin Adler <darin@apple.com>"> <span class="fn">Darin Adler</span></a>
</span></b>
<pre>Comment on <span class=""><a href="attachment.cgi?id=291818&action=diff" name="attach_291818" title="patch">attachment 291818</a> <a href="attachment.cgi?id=291818&action=edit" title="patch">[details]</a></span>
patch
View in context: <a href="https://bugs.webkit.org/attachment.cgi?id=291818&action=review">https://bugs.webkit.org/attachment.cgi?id=291818&action=review</a>
<span class="quote">> Source/WebCore/dom/Element.h:549
> + void invalidateStyle();
> + WEBCORE_EXPORT void invalidateStyleAndLayers();
> + void invalidateStyleForSubtree();
> + void invalidateRenderers();</span >
There’s no question that these names are clearer than the old way.
However, I am not clear on when I should call each one of these. Is there something simple we could write in a comment to make it clear how to choose from among these four? In particular, when would I not have to invalidate layers? When I invalidate a subtree is there some reason I don’t need to worry about layers?
<span class="quote">> Source/WebCore/dom/Node.cpp:803
> + bool markAncestors = styleInvalidationScope() == Style::InvalidationScope::None || scope == Style::InvalidationScope::Renderers;</span >
I don’t understand why this check is correct. I think this needs a “why” comment.
<span class="quote">> Source/WebCore/dom/Node.h:322
> + void clearNeedsStyleRecalc() { clearStyleInvalidation(); }</span >
This function seems to have a peculiar name now that it’s not paired with a setNeedsStyleRecalc function. Do we really need to keep it?
<span class="quote">> Source/WebCore/dom/ShadowRoot.cpp:121
> + // If this was ever used dynamically child styles would need to be invalidated here.</span >
Needs a comma after the word "dynamically". But also, what exactly does "used dynamically" mean? Can we say that in a more straightforward way?
<span class="quote">> Source/WebCore/dom/Text.cpp:224
> + if (styleInvalidationScope() == Style::InvalidationScope::Renderers)</span >
Even though this is the highest one, I think that >= makes more logical sense to me here.
<span class="quote">> Source/WebCore/style/StyleInvalidationScope.h:36
> +enum class InvalidationScope {
> + None,
> + Element,
> + Subtree,
> + Renderers
> +};</span >
The word "scope" makes sense when telling something to invalidate. But not as much sense when it’s indicating how much is invalid. It’s also unclear to me that when renderers are "invalidated" that also means the entire subtree is invalidated. Maybe all this is more obvious to someone working in this area, but I am not so sure.
<span class="quote">> Source/WebCore/style/StyleInvalidationScope.h:41
> +enum class InvalidationMode {
> + Normal,
> + RecompositeLayers
> +};</span >
Maybe this looks better at the call site when setting this mode. But at sites where we use this, it seems that a boolean shouldRecompositeLayers() or something like that would be easier to read.
<span class="quote">> Source/WebCore/style/StyleTreeResolver.cpp:203
> + bool shouldReconstructRenderTree = element.styleInvalidationScope() == InvalidationScope::Renderers || parent().change == Detach;</span >
Same thought about >= here.
<span class="quote">> Source/WebCore/style/StyleTreeResolver.cpp:236
> + if (update.change != Detach && (parent().change == Force || element.styleInvalidationScope() == InvalidationScope::Subtree))</span >
Why is "==" right here? What about the Renderers case?
<span class="quote">> Source/WebCore/style/StyleTreeResolver.cpp:365
> + if (text.styleInvalidationScope() == InvalidationScope::Renderers && parent.change != Detach)</span >
Same thought about >= here.</pre>
</div>
</p>
<hr>
<span>You are receiving this mail because:</span>
<ul>
<li>You are the assignee for the bug.</li>
</ul>
</body>
</html>