Bug 108563

Summary: [Refactoring] StyleResolver::State should have methods to access its member variables.
Product: WebKit Reporter: Takashi Sakamoto <tasak>
Component: CSSAssignee: Takashi Sakamoto <tasak>
Status: RESOLVED FIXED    
Severity: Normal CC: allan.jensen, buildbot, cmarcelo, dglazkov, gtk-ews, koivisto, macpherson, menard, ojan.autocc, peter+ews, rniwa, webcomponents-bugzilla, WebkitBugTracker, webkit-ews, webkit.review.bot, xan.lopez
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 89879    
Attachments:
Description Flags
WIP
none
WIP
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Takashi Sakamoto 2013-01-31 18:49:02 PST
We need suitable methods to access StyleResolver::State's member variables.
Comment 1 Takashi Sakamoto 2013-02-05 02:39:47 PST
Created attachment 186583 [details]
WIP
Comment 2 EFL EWS Bot 2013-02-05 02:48:46 PST
Comment on attachment 186583 [details]
WIP

Attachment 186583 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/16367684
Comment 3 Early Warning System Bot 2013-02-05 02:49:00 PST
Comment on attachment 186583 [details]
WIP

Attachment 186583 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/16371689
Comment 4 Early Warning System Bot 2013-02-05 02:49:11 PST
Comment on attachment 186583 [details]
WIP

Attachment 186583 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/16374620
Comment 5 WebKit Review Bot 2013-02-05 02:52:34 PST
Comment on attachment 186583 [details]
WIP

Attachment 186583 [details] did not pass cr-linux-debug-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16366720
Comment 6 Build Bot 2013-02-05 03:10:57 PST
Comment on attachment 186583 [details]
WIP

Attachment 186583 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://queues.webkit.org/results/16376619
Comment 7 WebKit Review Bot 2013-02-05 03:18:15 PST
Comment on attachment 186583 [details]
WIP

Attachment 186583 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16369684
Comment 8 Peter Beverloo (cr-android ews) 2013-02-05 03:27:10 PST
Comment on attachment 186583 [details]
WIP

Attachment 186583 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/16379540
Comment 9 Build Bot 2013-02-05 03:36:40 PST
Comment on attachment 186583 [details]
WIP

Attachment 186583 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/16378551
Comment 10 kov's GTK+ EWS bot 2013-02-05 03:41:05 PST
Comment on attachment 186583 [details]
WIP

Attachment 186583 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/16384001
Comment 11 Build Bot 2013-02-05 06:58:28 PST
Comment on attachment 186583 [details]
WIP

Attachment 186583 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/16365817
Comment 12 Takashi Sakamoto 2013-02-05 21:40:18 PST
Created attachment 186753 [details]
WIP
Comment 13 Takashi Sakamoto 2013-02-06 19:45:10 PST
Created attachment 186981 [details]
Patch
Comment 14 Build Bot 2013-02-07 02:43:48 PST
Comment on attachment 186981 [details]
Patch

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

New failing tests:
inspector/styles/region-style-crash.html
Comment 15 Build Bot 2013-02-07 02:57:51 PST
Comment on attachment 186981 [details]
Patch

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

New failing tests:
inspector/styles/region-style-crash.html
Comment 16 Takashi Sakamoto 2013-02-08 00:22:06 PST
Created attachment 187254 [details]
Patch
Comment 17 Takashi Sakamoto 2013-02-08 00:23:39 PST
I filed https://bugs.webkit.org/show_bug.cgi?id=109258 to fix "New failing: inspector/styles/region-style-crash.html".
Comment 18 Build Bot 2013-02-08 03:00:57 PST
Comment on attachment 187254 [details]
Patch

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

New failing tests:
inspector/styles/region-style-crash.html
Comment 19 Build Bot 2013-02-08 03:13:12 PST
Comment on attachment 187254 [details]
Patch

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

New failing tests:
inspector/styles/region-style-crash.html
Comment 20 Build Bot 2013-02-08 20:32:11 PST
Comment on attachment 187254 [details]
Patch

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

New failing tests:
inspector/styles/region-style-crash.html
Comment 21 Dimitri Glazkov (Google) 2013-02-11 11:18:28 PST
Comment on attachment 187254 [details]
Patch

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

This looks fairly mechanical and straightforward. Are you blocked on landing a fix for bug 108563?

> Source/WebCore/ChangeLog:3
> +        [Refectoring] StyleResolver::State should have methods to access its member variables.

fect -> fact
Comment 22 Ryosuke Niwa 2013-02-11 11:46:55 PST
Comment on attachment 187254 [details]
Patch

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

> Source/WebCore/css/StyleResolver.cpp:543
> -    if (state.matchedRules.isEmpty())
> +    Vector<const RuleData*, 32>& matchedRules = state.matchedRules();
> +    if (matchedRules.isEmpty())
>          return;
>  
>      sortMatchedRules();

It's counter intuitive to have a reference to matchedRules, and the call sortMatchedRules() that takes no argument to sort it.
It would have been better if the state object had isMatchedRulesEmpty() function and sortMatchedRules().
Also, this appears the only place where the matchedRules is ever meaningfully used, and we can probably get rid of it if we added clearMatchesRules().
Comment 23 Takashi Sakamoto 2013-02-11 21:55:49 PST
Created attachment 187770 [details]
Patch
Comment 24 Takashi Sakamoto 2013-02-11 21:56:31 PST
Comment on attachment 187254 [details]
Patch

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

Thank you for reviewing.

> Are you blocked on landing a fix for bug 108563?

If possible, I would like to rebase layout test results. Because inspector/styles/region-style-crash.html doesn't crash.

>> Source/WebCore/ChangeLog:3
>> +        [Refectoring] StyleResolver::State should have methods to access its member variables.
> 
> fect -> fact

Done.

>> Source/WebCore/css/StyleResolver.cpp:543
>>      sortMatchedRules();
> 
> It's counter intuitive to have a reference to matchedRules, and the call sortMatchedRules() that takes no argument to sort it.
> It would have been better if the state object had isMatchedRulesEmpty() function and sortMatchedRules().
> Also, this appears the only place where the matchedRules is ever meaningfully used, and we can probably get rid of it if we added clearMatchesRules().

I would like to try such refactoring in another patch, because this patch is large, i.e. about 139kb. I think, it would be better to keep this patch mechanical and straightforward.
Comment 25 Antti Koivisto 2013-02-12 09:28:40 PST
Comment on attachment 187770 [details]
Patch

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

> Source/WebCore/css/StyleResolver.h:464
>          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)
> +        : 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)
> +        , m_distributedToInsertionPoint(false)
> +        , m_elementAffectedByClassRules(false)
> +        , m_applyPropertyToRegularStyle(true)
> +        , m_applyPropertyToVisitedLinkStyle(false)
> +#if ENABLE(CSS_SHADERS)
> +        , m_hasPendingShaders(false)
> +#endif
> +        , m_lineHeightValue(0)
> +        , m_fontDirty(false)
> +        , m_hasUAAppearance(false)
> +        , m_backgroundData(BackgroundFillLayer) { }
> +
> +    public:
> +        void initElement(Element*);
> +        void initForStyleResolve(Document*, Element*, RenderStyle* parentStyle = 0, PseudoId = NOPSEUDO, RenderRegion* regionForStyling = 0);
> +        void clear()
> +        {
> +            m_element = 0;
> +            m_styledElement = 0;
> +            m_parentStyle = 0;
> +            m_parentNode = 0;
> +            m_regionForStyling = 0;
> +            m_ruleList = 0;
> +            m_matchedRules.clear();
> +            m_pendingImageProperties.clear();
>  #if ENABLE(CSS_SHADERS)
> -        , hasPendingShaders(false)
> +            m_hasPendingShaders = false;
>  #endif
> -        , lineHeightValue(0)
> -        , fontDirty(false)
> -        , hasUAAppearance(false)
> -        , backgroundData(BackgroundFillLayer) { }
> +#if ENABLE(CSS_FILTERS) && ENABLE(SVG)
> +            m_pendingSVGDocuments.clear();
> +#endif
> +        }

You should move functions with more than one line to cpp (constructor, clear(), ensureRuleList(), cacheBorderAndBackground(),..).

> Source/WebCore/css/StyleResolver.h:470
> +        Document* document() const { return m_element->document(); }
> +        Element* element() const { return m_element; }
> +        StyledElement* styledElement() const { return m_styledElement; }
> +        void setStyle(PassRefPtr<RenderStyle> style) { m_style = style; }
> +        RenderStyle* style() const { return m_style.get(); }

Would it be possible to follow proper const here? 
const Foo* foo() const
or/and
Foo* foo() {}
Comment 26 Takashi Sakamoto 2013-02-13 00:12:41 PST
Created attachment 188028 [details]
Patch
Comment 27 Build Bot 2013-02-13 00:55:26 PST
Comment on attachment 188028 [details]
Patch

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

New failing tests:
inspector/styles/region-style-crash.html
Comment 28 Takashi Sakamoto 2013-02-13 01:01:26 PST
Created attachment 188031 [details]
Patch
Comment 29 Takashi Sakamoto 2013-02-13 01:05:00 PST
Comment on attachment 187770 [details]
Patch

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

Thank you for reviewing.

I rebaselined inspector/styles/region-style-crash.html, because, I think, it will take much time to make inspector support CSS regions.

>> Source/WebCore/css/StyleResolver.h:464
>> +        }
> 
> You should move functions with more than one line to cpp (constructor, clear(), ensureRuleList(), cacheBorderAndBackground(),..).

I see. Done.

>> Source/WebCore/css/StyleResolver.h:470
>> +        RenderStyle* style() const { return m_style.get(); }
> 
> Would it be possible to follow proper const here? 
> const Foo* foo() const
> or/and
> Foo* foo() {}

I tried... but I found that I need much more refactoring to follow proper const. I will try this in another patch.
Comment 30 WebKit Review Bot 2013-02-13 09:28:21 PST
Comment on attachment 188031 [details]
Patch

Clearing flags on attachment: 188031

Committed r142757: <http://trac.webkit.org/changeset/142757>
Comment 31 WebKit Review Bot 2013-02-13 09:28:28 PST
All reviewed patches have been landed.  Closing bug.