Bug 96421 – Split per-resolve logic out from StyleResolver.

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Dimitri Glazkov (Google)]

Split per-resolve logic out from StyleResolver.

[Dimitri Glazkov (Google)]

Created attachment 163428 [details]
WIP: Exploring the problem.

[Dimitri Glazkov (Google)]

Just for learning purposes, I gave a quick shot to splitting out per-resolve logic, and it's actually not that bad. Found lots of interesting boogers. Will study them individually.

[Takashi Sakamoto]

Created attachment 184683 [details]
WIP

[Takashi Sakamoto]

Created attachment 184684 [details]
WIP

[Takashi Sakamoto]

Created attachment 184695 [details]
WIP

[Eric Seidel (no email)]

Comment on attachment 184695 [details]
WIP

state isn't a very good variable name.  But I'm not sure I have better suggestions.  even resolveState would be better (and more grep-able), but I understand that's longer.  Not sure.

[Eric Seidel (no email)]

Comment on attachment 184695 [details]
WIP

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

> Source/WebCore/css/CSSToStyleMap.h:43
> +    static void mapFillAttachment(StyleResolveState&, CSSPropertyID, FillLayer*, CSSValue*);
> +    static void mapFillClip(StyleResolveState&, CSSPropertyID, FillLayer*, CSSValue*);

Why are these all static?  We should just have the CSSToStyleMap wrap the state intsead of the Resolver, no?  CSSToStyleMap wuld then just be a statck object, and never kept around.

[Early Warning System Bot]

Comment on attachment 184695 [details]
WIP

Attachment 184695 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/16113305

[Takashi Sakamoto]

Created attachment 184702 [details]
WIP

[Takashi Sakamoto]

Created attachment 184714 [details]
WIP

[Takashi Sakamoto]

Comment on attachment 184695 [details]
WIP

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

Thank you for reviewing.

I renamed StyleResolveState to ResolveState.

>> Source/WebCore/css/CSSToStyleMap.h:43
>> +    static void mapFillClip(StyleResolveState&, CSSPropertyID, FillLayer*, CSSValue*);
> 
> Why are these all static?  We should just have the CSSToStyleMap wrap the state intsead of the Resolver, no?  CSSToStyleMap wuld then just be a statck object, and never kept around.

I see.
I made CSSToStyleMap a stack object.

[WebKit Review Bot]

Comment on attachment 184714 [details]
WIP

Attachment 184714 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16119294

New failing tests:
media/track/track-css-matching.html
media/track/track-css-matching-timestamps.html

[WebKit Review Bot]

Comment on attachment 184714 [details]
WIP

Attachment 184714 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16113400

New failing tests:
media/track/track-css-matching.html
media/track/track-css-matching-timestamps.html

[Build Bot]

Comment on attachment 184714 [details]
WIP

Attachment 184714 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/16124330

[Build Bot]

Comment on attachment 184714 [details]
WIP

Attachment 184714 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://queues.webkit.org/results/16117366

[Build Bot]

Comment on attachment 184714 [details]
WIP

Attachment 184714 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/16111592

[Takashi Sakamoto]

Created attachment 184927 [details]
WIP

[WebKit Review Bot]

Comment on attachment 184927 [details]
WIP

Attachment 184927 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16160398

New failing tests:
media/track/track-css-matching.html
media/track/track-css-matching-timestamps.html

[WebKit Review Bot]

Comment on attachment 184927 [details]
WIP

Attachment 184927 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16150566

New failing tests:
media/track/track-css-matching.html
media/track/track-css-matching-timestamps.html

[Build Bot]

Comment on attachment 184927 [details]
WIP

Attachment 184927 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/16152480

New failing tests:
inspector/styles/region-style-crash.html
media/track/track-css-matching.html
media/track/track-css-matching-timestamps.html

[Takashi Sakamoto]

Looking at inspector/styles/region-style-crash.html regression, I think, inspector would not support CSS regions correctly.
InspectorCSSAgent::getMatchedStylesForNode doesn't check RenderRegion. However, since StyleResolver doesn't initialize m_regionForStyling member variable, pseudoStyleRulesForElement sometimes returns CSSRules for CSS Regions.

Talking about the followings, https://bugs.webkit.org/show_bug.cgi?id=108024 was filed. I think, HTMLTagNames.in was not updated when some WebVTTElement's patch was landed:
- media/track/track-css-matching.html
- media/track/track-css-matching-timestamps.html

[Build Bot]

Comment on attachment 184927 [details]
WIP

Attachment 184927 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://queues.webkit.org/results/16150683

New failing tests:
inspector/styles/region-style-crash.html
media/track/track-css-matching.html
media/track/track-css-matching-timestamps.html

[Dimitri Glazkov (Google)]

Comment on attachment 184927 [details]
WIP

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

This patch is super-ambitious. I hope you have a plan on how to chunk it up into smaller pieces that help us better understand how its effects :)

> Source/WebCore/css/CSSToStyleMap.h:76
> +    ResolveState& m_state;

state is mysterious. Variable should probably match the name of the class.

> Source/WebCore/css/ResolveState.h:1
> +/*

Just an idea: it might be easier (save reviewers' sanity!) to first refactor this in-place, then move the new class out into its own file.

> Source/WebCore/css/ResolveState.h:4
> + *

Aren't we supposed to add teh Google line here?

> Source/WebCore/css/ResolveState.h:22
> +#ifndef ResolveState_h

Better name needed. It's a state, yes. But it's also something that looks like both intermediate state and the actual result of style resolution. It's like a builder, but not quite... Hmmm..

> Source/WebCore/css/SelectorChecker.cpp:-72
> -bool SelectorChecker::matches(const CSSSelector* selector, Element* element, bool isFastCheckableSelector) const

Not sure what's happening in this file. Sounds like some other unrelated refactoring. BTW, I am splitting fast path off into its own class in bug 106860.

[Antti Koivisto]

I'm not sure It needs a file. The state is inherent part of the StyleResolver. I would probably go with StyleResolver::State. If it is is a separate file then StyleResolverState would be a good name.

You should follow strict constness. Functions that shouldn't logically change the state should always get const StyleResolver::State&

[Takashi Sakamoto]

Created attachment 185211 [details]
WIP

[Takashi Sakamoto]

Comment on attachment 184927 [details]
WIP

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

Thank you for reviewing.

I did the followings:
- renamed ResolveState to StyleResolverState. Since the state is used outside of StyleResolver, i.e. StyleBuilder, I would like to use StyleResolverState instead of StyleResolver::State.
- focused on StyleResolverState. So I removed a patch for making SelectorChecker an on-stack object.
- tried to added "const" to StyleResolverState& parameter.

In this patch, I gathered all states (including intermediate ones) to StyleResolverState. If I could add StyleResolverState, I would like to split the gathered states into followings:
- states which are active during matchRules.
- states which are active during applyMatchedProperties.
- states which are always active, e.g. m_element, m_rootElementStyle, and so on. The state is used in StyleBuilder and StyleResolver (sibling cache).

>> Source/WebCore/css/ResolveState.h:22
>> +#ifndef ResolveState_h
> 
> Better name needed. It's a state, yes. But it's also something that looks like both intermediate state and the actual result of style resolution. It's like a builder, but not quite... Hmmm..

Yeah. I'm thinking of the following:
(1) firstly, move all variables, including intermediate state, to state. (this patch)
(2) next, I would like to move some of variables to other classes, i.e. Collector and Applier. For example, Collector is for collecting matching rules. It would have m_matchedRules, one of intermediate states. Applier is for applying matched properties and working with StyleBuilder and CSSToStyleMap. It would have m_lineHeightValue, m_fontDirty, and so on.

I think, finally, StyleResolverState would have only important states, m_element, m_rootElementStyle, m_parentStyle, ...

[Takashi Sakamoto]

(In reply to comment #26)

> I did the followings:
> - renamed ResolveState to StyleResolverState. Since the state is used outside of StyleResolver, i.e. StyleBuilder, I would like to use StyleResolverState instead of StyleResolver::State.
> - focused on StyleResolverState. So I removed a patch for making SelectorChecker an on-stack object.
> - tried to added "const" to StyleResolverState& parameter.

I forgot to say the following:
- I removed ResolveState.h and moved the code into StyleResolver.h.

> I removed a patch for making SelectorChecker an on-stack object.

I would like to file another bug for making SelectorChecker an on-stack object. This enables  StyleResolverState to have SelectorChecker::mode.

[Hajime Morrita]

Comment on attachment 185211 [details]
WIP

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

Naming game aside, you might want to reduce the size of this patch by splitting 
- One for extracting new class
- Another for passing the new class around

> Source/WebCore/css/BasicShapeFunctions.cpp:-103
> -static Length convertToLength(const StyleResolver* styleResolver, CSSPrimitiveValue* value)

Seems natural to be a method of the new state class. Could be done in separate change though.

> Source/WebCore/css/StyleResolver.h:144
> +class StyleResolverState {

The class name basically means nothing. It is better to clarify this per-Element nature.

> Source/WebCore/css/StyleResolver.h:289
> +    void initForStyleResolve(StyleResolverState&, Element*, RenderStyle* parentStyle = 0, PseudoId = NOPSEUDO);

Passing StyleResolverState to StyleResolver is too... redundant. Had better to be a inner class or to have a typedef.

[WebKit Review Bot]

Comment on attachment 185211 [details]
WIP

Attachment 185211 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16022221

New failing tests:
media/track/track-css-matching.html
media/track/track-css-matching-timestamps.html

[Dimitri Glazkov (Google)]

(In reply to comment #26)
> (From update of attachment 184927 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=184927&action=review
> 
> Thank you for reviewing.
> 
> I did the followings:
> - renamed ResolveState to StyleResolverState. Since the state is used outside of StyleResolver, i.e. StyleBuilder, I would like to use StyleResolverState instead of StyleResolver::State.
> - focused on StyleResolverState. So I removed a patch for making SelectorChecker an on-stack object.
> - tried to added "const" to StyleResolverState& parameter.
> 
> In this patch, I gathered all states (including intermediate ones) to StyleResolverState. If I could add StyleResolverState, I would like to split the gathered states into followings:
> - states which are active during matchRules.
> - states which are active during applyMatchedProperties.
> - states which are always active, e.g. m_element, m_rootElementStyle, and so on. The state is used in StyleBuilder and StyleResolver (sibling cache).
> 
> >> Source/WebCore/css/ResolveState.h:22
> >> +#ifndef ResolveState_h
> > 
> > Better name needed. It's a state, yes. But it's also something that looks like both intermediate state and the actual result of style resolution. It's like a builder, but not quite... Hmmm..
> 
> Yeah. I'm thinking of the following:
> (1) firstly, move all variables, including intermediate state, to state. (this patch)
> (2) next, I would like to move some of variables to other classes, i.e. Collector and Applier. For example, Collector is for collecting matching rules. It would have m_matchedRules, one of intermediate states. Applier is for applying matched properties and working with StyleBuilder and CSSToStyleMap. It would have m_lineHeightValue, m_fontDirty, and so on.

This might be the wrong approach. This patch is already 300Kb. You will need a super-brave reviewer to land it as-is. It's certainly not me :)

[Build Bot]

Comment on attachment 185211 [details]
WIP

Attachment 185211 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/16151260

New failing tests:
inspector/styles/region-style-crash.html
media/track/track-css-matching.html
media/track/track-css-matching-timestamps.html

[Build Bot]

Comment on attachment 185211 [details]
WIP

Attachment 185211 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://queues.webkit.org/results/16217298

New failing tests:
inspector/styles/region-style-crash.html
media/track/track-css-matching.html
fast/frames/sandboxed-iframe-scripting.html
media/track/track-css-matching-timestamps.html

[Takashi Sakamoto]

Created attachment 185416 [details]
WIP

[Build Bot]

Comment on attachment 185416 [details]
WIP

Attachment 185416 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://queues.webkit.org/results/16217393

[Build Bot]

Comment on attachment 185416 [details]
WIP

Attachment 185416 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/16199552

[Takashi Sakamoto]

Created attachment 185427 [details]
WIP: added m_state

[Takashi Sakamoto]

Thank you, Morita-san and Dimitri.

I would like to change my plan:
(1) implement class StyleResolver::State and add m_state to StyleResolver. (this patch)

(2) make CSSSelector an on-stack object and move m_selectorChecker.mode() to m_state.

(3) refactor CanvasRenderingContext2D.

(4) make m_state an on-stack object and provides state value for methods as a parameter.

(5) split intermediate states into other classes. 

Now this patch is almost the same as "just adding m_state to access some member variables in StyleResolver", e.g. m_style -> m_state.m_style and so on.

[Build Bot]

Comment on attachment 185427 [details]
WIP: added m_state

Attachment 185427 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://queues.webkit.org/results/16196561

New failing tests:
fast/frames/sandboxed-iframe-scripting.html

[Eric Seidel (no email)]

Death by a-thousand flaky tests.

[Eric Seidel (no email)]

I think this patch is a huge step in the right direction.  The only interesting bits to review seem to be making sure that the right members got moved onto State.  Obviously there are many follow-up patches to write to add accessors to State and clean-up all the callsites, but this is definitely in the right direction!

[Antti Koivisto]

Comment on attachment 185427 [details]
WIP: added m_state

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

Looking good!

> Source/WebCore/css/StyleResolver.h:446
> +    class State {
> +        WTF_MAKE_NONCOPYABLE(State);
> +    public:
> +        State()
> +        : m_element(0)
> +        , m_styledElement(0)
> +        , m_parentNode(0)
> +        , m_parentStyle(0)
> +        , m_rootElementStyle(0)
> +        , m_regionForStyling(0)
> +        , m_sameOriginOnly(false)
> +        , m_pseudoStyle(NOPSEUDO)
> +        , m_elementLinkState(NotInsideLink)

You can make this struct to signal that it is supposed to have public members is not supposed to have functions. Please drop the m_ prefix for the public members, that's what we usually do and the code will look nicer.

[Antti Koivisto]

"and is not supposed to have functions"

[Antti Koivisto]

m_state.m_parentStyle -> m_state.parentStyle etc.

[Eric Seidel (no email)]

(In reply to comment #41)
> (From update of attachment 185427 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=185427&action=review
> 
> You can make this struct to signal that it is supposed to have public members is not supposed to have functions. Please drop the m_ prefix for the public members, that's what we usually do and the code will look nicer.

I agree with Antti that these should either have no m_ prefix or have accessors.  But I also feel that could be done in a second patch to keep this one small.  That said, you'll probably end up changing the same lines again, so up to you.

(Its probably easy to search for 'state.m_' and replace it with 'state.' to get what you want from this diff)

[Antti Koivisto]

> I agree with Antti that these should either have no m_ prefix or have accessors.  But I also feel that could be done in a second patch to keep this one small.  That said, you'll probably end up changing the same lines again, so up to you.

Is shouldn't change the patch size at all, no? Hits all the same lines.

> (Its probably easy to search for 'state.m_' and replace it with 'state.' to get what you want from this diff)

...and is a trivial search replace.

[Eric Seidel (no email)]

Yup. :)  I believe we're in agreement. :)

[Takashi Sakamoto]

Created attachment 185696 [details]
Patch

[Takashi Sakamoto]

Comment on attachment 185427 [details]
WIP: added m_state

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

Thank you for reviewing.

>> Source/WebCore/css/StyleResolver.h:446
>> +        , m_elementLinkState(NotInsideLink)
> 
> You can make this struct to signal that it is supposed to have public members is not supposed to have functions. Please drop the m_ prefix for the public members, that's what we usually do and the code will look nicer.

Done.

[Hajime Morrita]

Comment on attachment 185696 [details]
Patch

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

It looks the patch becomes out of date. Please rebase it and let's cq?

> Source/WebCore/css/StyleResolver.h:460
> +        // FIXME: to make it easier to review, these member variables are

Had better file a bug for this.

[Takashi Sakamoto]

Created attachment 185930 [details]
Patch

[Takashi Sakamoto]

Comment on attachment 185696 [details]
Patch

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

>> Source/WebCore/css/StyleResolver.h:460
>> +        // FIXME: to make it easier to review, these member variables are
> 
> Had better file a bug for this.

I see. Done. I filed bug 108563.

[Takashi Sakamoto]

Created attachment 185933 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 185933 [details]
Patch

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

I do not think that "State"'s eventual destiny is as a struct, but rather as a class which holds all the useful data about an in-progress element resolve, and some related logic.  But I also think that this is a great step in the right direction and we don't have to get everythign right on the first pass. :)  I'm happy to r+ this, but I Think you should give antti 12 hours to make sure he doesn't want another round.

> Source/WebCore/css/SVGCSSStyleSelector.cpp:568
> +            IntPoint location(item->x->computeLength<int>(state.style.get(), state.rootElementStyle),

This .get() is very unfortunate. :(  But again, we could add an accessor in another pass.

> Source/WebCore/css/StyleResolver.cpp:980
> +    State& state = m_state;
> +    state.pseudoStyle = pseudoID;

I think this function eventually moves onto State.

> Source/WebCore/css/StyleResolver.cpp:1620
> -    return m_style.release();
> +    return state.style.release();

This will end up being takeStyle(), presumably.

> Source/WebCore/css/StyleResolver.h:481
> +        // FIXME(bug 108563): to make it easier to review, these member
> +        // variables are public. However we should add methods to access
> +        // these variables.

Agreed. :)  As well as move some of the clear(), and init() logic into this class.

> Source/WebCore/css/StyleResolver.h:497
> +        bool distributedToInsertionPoint;
> +
> +        bool elementAffectedByClassRules;

Also in a second pass we should shrink the size of this class by using bitfields.  It doesn't cost us anything and has some marginal benefit.

> Source/WebCore/css/StyleResolver.h:503
> +        // A buffer used to hold the set of matched rules for an element,
> +        // and a temporary buffer used for merge sorting.
> +        Vector<const RuleData*, 32> matchedRules;

Seems like a rather large internal capacity for this, I wonder how it was tuned.

[Takashi Sakamoto]

Created attachment 186284 [details]
Patch

[Takashi Sakamoto]

Comment on attachment 185933 [details]
Patch

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

Thank you for reviewing.

>> Source/WebCore/css/SVGCSSStyleSelector.cpp:568
>> +            IntPoint location(item->x->computeLength<int>(state.style.get(), state.rootElementStyle),
> 
> This .get() is very unfortunate. :(  But again, we could add an accessor in another pass.

Yeah. I would like to refactor this in another patch.

>> Source/WebCore/css/StyleResolver.cpp:980
>> +    state.pseudoStyle = pseudoID;
> 
> I think this function eventually moves onto State.

Yeah, I agree.

>> Source/WebCore/css/StyleResolver.h:497
>> +        bool elementAffectedByClassRules;
> 
> Also in a second pass we should shrink the size of this class by using bitfields.  It doesn't cost us anything and has some marginal benefit.

I changed these member variables to use bitfields in this patch. Is this ok? Or I have to split this into another patch?

>> Source/WebCore/css/StyleResolver.h:503
>> +        Vector<const RuleData*, 32> matchedRules;
> 
> Seems like a rather large internal capacity for this, I wonder how it was tuned.

I'm thinking of the followings...
(1) use OwnPtr<Vector<const RuleData*, 32> > matchedRules and add "ensureMatchedRules".
(2) check how many "const RuleData*" are stored in matchedRules (average, minimum, maximum, ...)

[Takashi Sakamoto]

Antti, would you check the patch?

[Build Bot]

Comment on attachment 186284 [details]
Patch

Attachment 186284 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/16356899

[Eric Seidel (no email)]

Comment on attachment 186284 [details]
Patch

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

> Source/WebCore/css/StyleResolver.h:490
> +        bool sameOriginOnly : 1;

Bit-fields must be grouped all together in order to work.  Having them interspersed like this won't do anything. :)  This will still take up the full space of "bool" because the next member isn't also a bool bitfield.

Generaly we place all the bitfields at the end of a class, just for this reason.

I think we should just do the bitfield thing in a separate pass.

Note that for bitfields to pack in the MSVC compiler all members must have the same type.  For example, elementLinkState probably only needs like 3 bits, but if you wanted it to pack with the bools, we'd have to make all the bools and that enum be "unsigned" in order to pack correctly on MSVC.

Bitfields are sadly kinda complicated, and althoguh we could land this as is, it won't really do what you expect it to. :)

[Takashi Sakamoto]

Created attachment 186303 [details]
Patch

[Takashi Sakamoto]

Comment on attachment 186303 [details]
Patch

This patch is the same as last Thursday's patch.

I reverted bitfields code and just rebased current master.

[Takashi Sakamoto]

Thank you, Eric.

I uploaded a new patch, which is the same as Thursday's one (I just rebased).

I will wait until JST tomorrow morning and will submit if I have no issues.

(In reply to comment #58)
> (From update of attachment 186284 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=186284&action=review
> 
> > Source/WebCore/css/StyleResolver.h:490
> > +        bool sameOriginOnly : 1;
> 
> Bit-fields must be grouped all together in order to work.  Having them interspersed like this won't do anything. :)  This will still take up the full space of "bool" because the next member isn't also a bool bitfield.
> 
> Generaly we place all the bitfields at the end of a class, just for this reason.
> 
> I think we should just do the bitfield thing in a separate pass.
> 
> Note that for bitfields to pack in the MSVC compiler all members must have the same type.  For example, elementLinkState probably only needs like 3 bits, but if you wanted it to pack with the bools, we'd have to make all the bools and that enum be "unsigned" in order to pack correctly on MSVC.
> 
> Bitfields are sadly kinda complicated, and althoguh we could land this as is, it won't really do what you expect it to. :)

[Eric Seidel (no email)]

Comment on attachment 186303 [details]
Patch

LGTM.

[Eric Seidel (no email)]

I think we should land this and stat working on the next patch.  Thanks.

[WebKit Review Bot]

Comment on attachment 186303 [details]
Patch

Clearing flags on attachment: 186303

Committed r141742: <http://trac.webkit.org/changeset/141742>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Antti Koivisto]

Comment on attachment 186303 [details]
Patch

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

> Source/WebCore/css/StyleResolver.h:476
> +        State()
> +        : element(0)
> +        , styledElement(0)
> +        , parentNode(0)
> +        , parentStyle(0)
> +        , rootElementStyle(0)
> +        , regionForStyling(0)
> +        , sameOriginOnly(false)
> +        , pseudoStyle(NOPSEUDO)
> +        , elementLinkState(NotInsideLink)
> +        , distributedToInsertionPoint(false)
> +        , elementAffectedByClassRules(false)
> +        , applyPropertyToRegularStyle(true)
> +        , applyPropertyToVisitedLinkStyle(false)
> +#if ENABLE(CSS_SHADERS)
> +        , hasPendingShaders(false)
> +#endif
> +        , lineHeightValue(0)
> +        , fontDirty(false)
> +        , hasUAAppearance(false)
> +        , backgroundData(BackgroundFillLayer) { }

Please move the constructor to .cpp. There is no need to have all this in the header.

[Antti Koivisto]

(In reply to comment #58)
> Bitfields are sadly kinda complicated, and althoguh we could land this as is, it won't really do what you expect it to. :)

Bitfields should not be used anyway if the object is not important for memory consumption. This one is not.

[Antti Koivisto]

(In reply to comment #53)
> I do not think that "State"'s eventual destiny is as a struct, but rather as a class which holds all the useful data about an in-progress element resolve, and some related logic.  But I also think that this is a great step in the right direction and we don't have to get everythign right on the first pass. :)  I'm happy to r+ this, but I Think you should give antti 12 hours to make sure he doesn't want another round.

We should be careful about the "related logic" part as you can easily end up with a confused type where people hang their favorite functions. A pure state object has a pretty clearly defined purpose.

[Antti Koivisto]

StyleResolver::Query?
StyleResolver::Request?
StyleResolver::ElementContext?

[Antti Koivisto]

StyleQuery?

[Eric Seidel (no email)]

I'm OK with ElementContext or REquest or Query.  If this is a Request or Query I think it's construction is going to feel a bit different though.  It feels more like an in-process state object, so ElementContext or ResolveState would be higher on my list than reqest/query.

I'm excited to get a chance to help clean even more of this up once I'm done with current parser obligations. :)

[Hajime Morrita]

(In reply to comment #68)
> We should be careful about the "related logic" part as you can easily end up with a confused type where people hang their favorite functions. A pure state object has a pretty clearly defined purpose.

Well, one lesson learned from NodeRenderingContext is that this actually happens.
The pure state approach might be too strict, but limiting dependency would be vital here.
We never want this class to depend StyleResolver.