108563 – [Refactoring] StyleResolver::State should have methods to access its member variables.

RESOLVED FIXED 108563

[Refactoring] StyleResolver::State should have methods to access its member variables.

https://bugs.webkit.org/show_bug.cgi?id=108563

Summary [Refactoring] StyleResolver::State should have methods to access its member v...

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	Details Formatted Diff Diff
WIP (138.90 KB, patch) 2013-02-05 21:40 PST, Takashi Sakamoto	no flags	Details Formatted Diff Diff
Patch (138.85 KB, patch) 2013-02-06 19:45 PST, Takashi Sakamoto	no flags	Details Formatted Diff Diff
Patch (138.86 KB, patch) 2013-02-08 00:22 PST, Takashi Sakamoto	no flags	Details Formatted Diff Diff
Patch (138.86 KB, patch) 2013-02-11 21:55 PST, Takashi Sakamoto	no flags	Details Formatted Diff Diff
Patch (140.32 KB, patch) 2013-02-13 00:12 PST, Takashi Sakamoto	no flags	Details Formatted Diff Diff
Patch (143.15 KB, patch) 2013-02-13 01:01 PST, Takashi Sakamoto	no flags	Details Formatted Diff Diff
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.

Status RESOLVED

Resolution FIXED

Priority P2

Severity Normal

Classification Unclassified

Version 528+ (Nightly build)

Hardware Unspecified

OS Unspecified

Product WebKit

Component CSS

Assignee

Takashi Sakamoto

Reported

2013-01-31 18:49 PST

Modified

2013-02-13 09:28 PST History

CC List

16 users Show

URL

Keywords

Depends on

Blocks

89879

Dependencies

tree graph