Bug 96421 - Split per-resolve logic out from StyleResolver.
Summary: Split per-resolve logic out from StyleResolver.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Dimitri Glazkov (Google)
URL:
Keywords:
Depends on:
Blocks: 89879
  Show dependency treegraph
 
Reported: 2012-09-11 13:57 PDT by Dimitri Glazkov (Google)
Modified: 2013-02-04 15:50 PST (History)
19 users (show)

See Also:


Attachments
WIP: Exploring the problem. (123.62 KB, patch)
2012-09-11 13:58 PDT, Dimitri Glazkov (Google)
no flags Details | Formatted Diff | Diff
WIP (349.72 KB, patch)
2013-01-25 00:02 PST, Takashi Sakamoto
no flags Details | Formatted Diff | Diff
WIP (349.70 KB, patch)
2013-01-25 00:08 PST, Takashi Sakamoto
no flags Details | Formatted Diff | Diff
WIP (348.87 KB, patch)
2013-01-25 01:17 PST, Takashi Sakamoto
no flags Details | Formatted Diff | Diff
WIP (348.97 KB, patch)
2013-01-25 02:00 PST, Takashi Sakamoto
no flags Details | Formatted Diff | Diff
WIP (327.94 KB, patch)
2013-01-25 03:10 PST, Takashi Sakamoto
no flags Details | Formatted Diff | Diff
WIP (329.09 KB, patch)
2013-01-27 18:25 PST, Takashi Sakamoto
no flags Details | Formatted Diff | Diff
WIP (300.78 KB, patch)
2013-01-29 03:03 PST, Takashi Sakamoto
no flags Details | Formatted Diff | Diff
WIP (136.57 KB, patch)
2013-01-30 00:45 PST, Takashi Sakamoto
no flags Details | Formatted Diff | Diff
WIP: added m_state (136.59 KB, patch)
2013-01-30 01:15 PST, Takashi Sakamoto
no flags Details | Formatted Diff | Diff
Patch (135.49 KB, patch)
2013-01-31 00:06 PST, Takashi Sakamoto
no flags Details | Formatted Diff | Diff
Patch (135.36 KB, patch)
2013-01-31 21:03 PST, Takashi Sakamoto
no flags Details | Formatted Diff | Diff
Patch (135.35 KB, patch)
2013-01-31 21:14 PST, Takashi Sakamoto
no flags Details | Formatted Diff | Diff
Patch (135.81 KB, patch)
2013-02-03 18:21 PST, Takashi Sakamoto
no flags Details | Formatted Diff | Diff
Patch (135.77 KB, patch)
2013-02-03 22:08 PST, Takashi Sakamoto
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dimitri Glazkov (Google) 2012-09-11 13:57:44 PDT
Split per-resolve logic out from StyleResolver.
Comment 1 Dimitri Glazkov (Google) 2012-09-11 13:58:25 PDT
Created attachment 163428 [details]
WIP: Exploring the problem.
Comment 2 Dimitri Glazkov (Google) 2012-09-11 14:00:27 PDT
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.
Comment 3 Takashi Sakamoto 2013-01-25 00:02:30 PST
Created attachment 184683 [details]
WIP
Comment 4 Takashi Sakamoto 2013-01-25 00:08:55 PST
Created attachment 184684 [details]
WIP
Comment 5 Takashi Sakamoto 2013-01-25 01:17:02 PST
Created attachment 184695 [details]
WIP
Comment 6 Eric Seidel (no email) 2013-01-25 01:21:10 PST
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.
Comment 7 Eric Seidel (no email) 2013-01-25 01:23:09 PST
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.
Comment 8 Early Warning System Bot 2013-01-25 01:26:30 PST
Comment on attachment 184695 [details]
WIP

Attachment 184695 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/16113305
Comment 9 Takashi Sakamoto 2013-01-25 02:00:39 PST
Created attachment 184702 [details]
WIP
Comment 10 Takashi Sakamoto 2013-01-25 03:10:47 PST
Created attachment 184714 [details]
WIP
Comment 11 Takashi Sakamoto 2013-01-25 03:14:52 PST
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.
Comment 12 WebKit Review Bot 2013-01-25 05:22:34 PST
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
Comment 13 WebKit Review Bot 2013-01-25 06:22:54 PST
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
Comment 14 Build Bot 2013-01-25 06:23:26 PST
Comment on attachment 184714 [details]
WIP

Attachment 184714 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/16124330
Comment 15 Build Bot 2013-01-25 06:37:10 PST
Comment on attachment 184714 [details]
WIP

Attachment 184714 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://queues.webkit.org/results/16117366
Comment 16 Build Bot 2013-01-25 07:21:52 PST
Comment on attachment 184714 [details]
WIP

Attachment 184714 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/16111592
Comment 17 Takashi Sakamoto 2013-01-27 18:25:59 PST
Created attachment 184927 [details]
WIP
Comment 18 WebKit Review Bot 2013-01-27 19:54:52 PST
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
Comment 19 WebKit Review Bot 2013-01-27 21:00:42 PST
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
Comment 20 Build Bot 2013-01-27 22:30:34 PST
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
Comment 21 Takashi Sakamoto 2013-01-28 03:37:24 PST
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
Comment 22 Build Bot 2013-01-28 03:48:22 PST
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
Comment 23 Dimitri Glazkov (Google) 2013-01-28 09:43:09 PST
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.
Comment 24 Antti Koivisto 2013-01-28 10:11:44 PST
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&
Comment 25 Takashi Sakamoto 2013-01-29 03:03:39 PST
Created attachment 185211 [details]
WIP
Comment 26 Takashi Sakamoto 2013-01-29 03:13:05 PST
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, ...
Comment 27 Takashi Sakamoto 2013-01-29 03:21:21 PST
(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.
Comment 28 Hajime Morrita 2013-01-29 03:23:50 PST
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.
Comment 29 WebKit Review Bot 2013-01-29 04:10:32 PST
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
Comment 30 Dimitri Glazkov (Google) 2013-01-29 09:47:20 PST
(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 :)
Comment 31 Build Bot 2013-01-29 09:52:09 PST
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
Comment 32 Build Bot 2013-01-29 20:37:16 PST
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
Comment 33 Takashi Sakamoto 2013-01-30 00:45:02 PST
Created attachment 185416 [details]
WIP
Comment 34 Build Bot 2013-01-30 00:50:37 PST
Comment on attachment 185416 [details]
WIP

Attachment 185416 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://queues.webkit.org/results/16217393
Comment 35 Build Bot 2013-01-30 00:58:46 PST
Comment on attachment 185416 [details]
WIP

Attachment 185416 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/16199552
Comment 36 Takashi Sakamoto 2013-01-30 01:15:14 PST
Created attachment 185427 [details]
WIP: added m_state
Comment 37 Takashi Sakamoto 2013-01-30 01:15:51 PST
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.
Comment 38 Build Bot 2013-01-30 01:55:59 PST
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
Comment 39 Eric Seidel (no email) 2013-01-30 02:13:59 PST
Death by a-thousand flaky tests.
Comment 40 Eric Seidel (no email) 2013-01-30 02:15:15 PST
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!
Comment 41 Antti Koivisto 2013-01-30 02:33:48 PST
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.
Comment 42 Antti Koivisto 2013-01-30 02:34:19 PST
"and is not supposed to have functions"
Comment 43 Antti Koivisto 2013-01-30 02:37:26 PST
m_state.m_parentStyle -> m_state.parentStyle etc.
Comment 44 Eric Seidel (no email) 2013-01-30 11:18:17 PST
(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)
Comment 45 Antti Koivisto 2013-01-30 11:36:32 PST
> 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.
Comment 46 Eric Seidel (no email) 2013-01-30 11:42:11 PST
Yup. :)  I believe we're in agreement. :)
Comment 47 Takashi Sakamoto 2013-01-31 00:06:09 PST
Created attachment 185696 [details]
Patch
Comment 48 Takashi Sakamoto 2013-01-31 00:07:16 PST
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.
Comment 49 Hajime Morrita 2013-01-31 00:17:00 PST
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.
Comment 50 Takashi Sakamoto 2013-01-31 21:03:15 PST
Created attachment 185930 [details]
Patch
Comment 51 Takashi Sakamoto 2013-01-31 21:05:17 PST
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.
Comment 52 Takashi Sakamoto 2013-01-31 21:14:44 PST
Created attachment 185933 [details]
Patch
Comment 53 Eric Seidel (no email) 2013-01-31 22:54:56 PST
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.
Comment 54 Takashi Sakamoto 2013-02-03 18:21:24 PST
Created attachment 186284 [details]
Patch
Comment 55 Takashi Sakamoto 2013-02-03 18:32:44 PST
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, ...)
Comment 56 Takashi Sakamoto 2013-02-03 18:35:26 PST
Antti, would you check the patch?
Comment 57 Build Bot 2013-02-03 19:45:36 PST
Comment on attachment 186284 [details]
Patch

Attachment 186284 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/16356899
Comment 58 Eric Seidel (no email) 2013-02-03 20:43:25 PST
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. :)
Comment 59 Takashi Sakamoto 2013-02-03 22:08:55 PST
Created attachment 186303 [details]
Patch
Comment 60 Takashi Sakamoto 2013-02-03 22:11:47 PST
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.
Comment 61 Takashi Sakamoto 2013-02-03 22:16:08 PST
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. :)
Comment 62 Eric Seidel (no email) 2013-02-03 22:22:43 PST
Comment on attachment 186303 [details]
Patch

LGTM.
Comment 63 Eric Seidel (no email) 2013-02-03 22:23:05 PST
I think we should land this and stat working on the next patch.  Thanks.
Comment 64 WebKit Review Bot 2013-02-03 23:04:31 PST
Comment on attachment 186303 [details]
Patch

Clearing flags on attachment: 186303

Committed r141742: <http://trac.webkit.org/changeset/141742>
Comment 65 WebKit Review Bot 2013-02-03 23:04:40 PST
All reviewed patches have been landed.  Closing bug.
Comment 66 Antti Koivisto 2013-02-04 03:34:56 PST
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.
Comment 67 Antti Koivisto 2013-02-04 03:38:22 PST
(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.
Comment 68 Antti Koivisto 2013-02-04 03:54:04 PST
(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.
Comment 69 Antti Koivisto 2013-02-04 04:06:38 PST
StyleResolver::Query?
StyleResolver::Request?
StyleResolver::ElementContext?
Comment 70 Antti Koivisto 2013-02-04 07:57:07 PST
StyleQuery?
Comment 71 Eric Seidel (no email) 2013-02-04 08:31:23 PST
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. :)
Comment 72 Hajime Morrita 2013-02-04 15:50:04 PST
(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.