Bug 163542 - Rename setNeedsStyleRecalc to invalidateStyle
Summary: Rename setNeedsStyleRecalc to invalidateStyle
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-10-17 06:40 PDT by Antti Koivisto
Modified: 2016-10-18 05:32 PDT (History)
0 users

See Also:


Attachments
patch (99.04 KB, patch)
2016-10-17 07:01 PDT, Antti Koivisto
no flags Details | Formatted Diff | Diff
patch (99.26 KB, patch)
2016-10-17 07:26 PDT, Antti Koivisto
no flags Details | Formatted Diff | Diff
patch (99.26 KB, patch)
2016-10-17 08:12 PDT, Antti Koivisto
darin: review+
Details | Formatted Diff | Diff
patch (102.04 KB, patch)
2016-10-18 03:44 PDT, Antti Koivisto
no flags Details | Formatted Diff | Diff
patch (102.15 KB, patch)
2016-10-18 04:46 PDT, Antti Koivisto
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Antti Koivisto 2016-10-17 06:40:58 PDT
Also rename the associated enums.
Comment 1 Antti Koivisto 2016-10-17 07:01:33 PDT
Created attachment 291815 [details]
patch
Comment 2 Antti Koivisto 2016-10-17 07:26:36 PDT
Created attachment 291817 [details]
patch
Comment 3 Antti Koivisto 2016-10-17 08:12:14 PDT
Created attachment 291818 [details]
patch
Comment 4 Darin Adler 2016-10-17 11:35:41 PDT
Comment on attachment 291818 [details]
patch

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

> Source/WebCore/dom/Element.h:549
> +    void invalidateStyle();
> +    WEBCORE_EXPORT void invalidateStyleAndLayers();
> +    void invalidateStyleForSubtree();
> +    void invalidateRenderers();

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?

> Source/WebCore/dom/Node.cpp:803
> +    bool markAncestors = styleInvalidationScope() == Style::InvalidationScope::None || scope == Style::InvalidationScope::Renderers;

I don’t understand why this check is correct. I think this needs a  “why” comment.

> Source/WebCore/dom/Node.h:322
> +    void clearNeedsStyleRecalc() { clearStyleInvalidation(); }

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?

> Source/WebCore/dom/ShadowRoot.cpp:121
> +    // If this was ever used dynamically child styles would need to be invalidated here.

Needs a comma after the word "dynamically". But also, what exactly does "used dynamically" mean? Can we say that in a more straightforward way?

> Source/WebCore/dom/Text.cpp:224
> +    if (styleInvalidationScope() == Style::InvalidationScope::Renderers)

Even though this is the highest one, I think that >= makes more logical sense to me here.

> Source/WebCore/style/StyleInvalidationScope.h:36
> +enum class InvalidationScope {
> +    None,
> +    Element,
> +    Subtree,
> +    Renderers
> +};

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.

> Source/WebCore/style/StyleInvalidationScope.h:41
> +enum class InvalidationMode {
> +    Normal,
> +    RecompositeLayers
> +};

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.

> Source/WebCore/style/StyleTreeResolver.cpp:203
> +    bool shouldReconstructRenderTree = element.styleInvalidationScope() == InvalidationScope::Renderers || parent().change == Detach;

Same thought about >= here.

> Source/WebCore/style/StyleTreeResolver.cpp:236
> +    if (update.change != Detach && (parent().change == Force || element.styleInvalidationScope() == InvalidationScope::Subtree))

Why is "==" right here? What about the Renderers case?

> Source/WebCore/style/StyleTreeResolver.cpp:365
> +            if (text.styleInvalidationScope() == InvalidationScope::Renderers && parent.change != Detach)

Same thought about >= here.
Comment 5 Antti Koivisto 2016-10-17 12:07:19 PDT
> I don’t understand why this check is correct. I think this needs a  “why”
> comment.

I don't really know either (for the latter part).
 
> > Source/WebCore/dom/Node.h:322
> > +    void clearNeedsStyleRecalc() { clearStyleInvalidation(); }
> 
> 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?

Eliminating all NeedsStyleRecalc terminology is bit too much for one patch. This is just where the line is drawn for this one.

> Needs a comma after the word "dynamically". But also, what exactly does
> "used dynamically" mean? Can we say that in a more straightforward way?

I mean "changed after initialization".

> 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.

I'm not entirely happy with the names. Suggestions are welcome. Value 'InvalidationScope::Element' indicates that the style of the element where it is set is invalid. Value 'Subtree' indicates that the element and also all its descendants are invalid. Value 'Renderers' indicates that the renderers for the subtree are invalid and need to be rebuild (also recomputing subtree styles).

> > Source/WebCore/style/StyleTreeResolver.cpp:236
> > +    if (update.change != Detach && (parent().change == Force || element.styleInvalidationScope() == InvalidationScope::Subtree))
> 
> Why is "==" right here? What about the Renderers case?

In Renderers case update.change == Detach. But it could as well be >=.
Comment 6 Antti Koivisto 2016-10-18 03:44:25 PDT
Created attachment 291936 [details]
patch
Comment 7 Antti Koivisto 2016-10-18 04:46:58 PDT
Created attachment 291938 [details]
patch
Comment 8 Antti Koivisto 2016-10-18 05:30:26 PDT
I went with 

namespace Style {

enum class Validity {
    Valid,
    ElementInvalid,
    SubtreeInvalid,
    SubtreeAndRenderersInvalid
};
Comment 9 Antti Koivisto 2016-10-18 05:32:33 PDT
https://trac.webkit.org/r207458