Summary: | [Refactoring] StyleResolver::State should have methods to access its member variables. | ||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Takashi Sakamoto <tasak> | ||||||||||||||||
Component: | CSS | Assignee: | 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
Takashi Sakamoto
2013-01-31 18:49:02 PST
Created attachment 186583 [details]
WIP
Comment on attachment 186583 [details] WIP Attachment 186583 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/16367684 Comment on attachment 186583 [details] WIP Attachment 186583 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/16371689 Comment on attachment 186583 [details] WIP Attachment 186583 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/16374620 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 on attachment 186583 [details] WIP Attachment 186583 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://queues.webkit.org/results/16376619 Comment on attachment 186583 [details] WIP Attachment 186583 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/16369684 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 on attachment 186583 [details] WIP Attachment 186583 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/16378551 Comment on attachment 186583 [details] WIP Attachment 186583 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/16384001 Comment on attachment 186583 [details] WIP Attachment 186583 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/16365817 Created attachment 186753 [details]
WIP
Created attachment 186981 [details]
Patch
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 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 Created attachment 187254 [details]
Patch
I filed https://bugs.webkit.org/show_bug.cgi?id=109258 to fix "New failing: inspector/styles/region-style-crash.html". 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 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 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 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 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(). Created attachment 187770 [details]
Patch
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 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() {} Created attachment 188028 [details]
Patch
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 Created attachment 188031 [details]
Patch
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 on attachment 188031 [details] Patch Clearing flags on attachment: 188031 Committed r142757: <http://trac.webkit.org/changeset/142757> All reviewed patches have been landed. Closing bug. |