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

Takashi Sakamoto
Reported 2013-01-31 18:49:02 PST
We need suitable methods to access StyleResolver::State's member variables.
Attachments
WIP (138.09 KB, patch)
2013-02-05 02:39 PST, Takashi Sakamoto 		no flags
DetailsFormatted DiffDiff
WIP (138.90 KB, patch)
2013-02-05 21:40 PST, Takashi Sakamoto 		no flags
DetailsFormatted DiffDiff
Patch (138.85 KB, patch)
2013-02-06 19:45 PST, Takashi Sakamoto 		no flags
DetailsFormatted DiffDiff
Patch (138.86 KB, patch)
2013-02-08 00:22 PST, Takashi Sakamoto 		no flags
DetailsFormatted DiffDiff
Patch (138.86 KB, patch)
2013-02-11 21:55 PST, Takashi Sakamoto 		no flags
DetailsFormatted DiffDiff
Patch (140.32 KB, patch)
2013-02-13 00:12 PST, Takashi Sakamoto 		no flags
DetailsFormatted DiffDiff
Patch (143.15 KB, patch)
2013-02-13 01:01 PST, Takashi Sakamoto 		no flags
DetailsFormatted DiffDiff
Show Obsolete (6) View All Add attachment proposed patch, testcase, etc.
Takashi Sakamoto
Comment 1 2013-02-05 02:39:47 PST
Created attachment 186583 [details] WIP
EFL EWS Bot
Comment 2 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
Early Warning System Bot
Comment 3 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
Early Warning System Bot
Comment 4 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
WebKit Review Bot
Comment 5 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
Build Bot
Comment 6 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
WebKit Review Bot
Comment 7 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
Peter Beverloo (cr-android ews)
Comment 8 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
Build Bot
Comment 9 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
kov's GTK+ EWS bot
Comment 10 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
Build Bot
Comment 11 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
Takashi Sakamoto
Comment 12 2013-02-05 21:40:18 PST
Created attachment 186753 [details] WIP
Takashi Sakamoto
Comment 13 2013-02-06 19:45:10 PST
Created attachment 186981 [details] Patch
Build Bot
Comment 14 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
Build Bot
Comment 15 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
Takashi Sakamoto
Comment 16 2013-02-08 00:22:06 PST
Created attachment 187254 [details] Patch
Takashi Sakamoto
Comment 17 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".
Build Bot
Comment 18 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
Build Bot
Comment 19 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
Build Bot
Comment 20 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
Dimitri Glazkov (Google)
Comment 21 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
Ryosuke Niwa
Comment 22 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().
Takashi Sakamoto
Comment 23 2013-02-11 21:55:49 PST
Created attachment 187770 [details] Patch
Takashi Sakamoto
Comment 24 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.
Antti Koivisto
Comment 25 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() {}
Takashi Sakamoto
Comment 26 2013-02-13 00:12:41 PST
Created attachment 188028 [details] Patch
Build Bot
Comment 27 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
Takashi Sakamoto
Comment 28 2013-02-13 01:01:26 PST
Created attachment 188031 [details] Patch
Takashi Sakamoto
Comment 29 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.
WebKit Review Bot
Comment 30 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>
WebKit Review Bot
Comment 31 2013-02-13 09:28:28 PST
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.