Bug 77525

Created attachment 125283 [details] Patch Patch comments: Passing the struct SelectorCheckingContext by value in checkSelector: this corresponds to the previous parameter-passing and the internals are also modified rather heavily. In comparison, I pass them by const reference to checkOneSelector, because only :not and :-webkit-any need to modify anything.

Addendum: I'm also using references in checkOneSelector to avoid replacing 'e' and 'sel' with the much longer 'context.m_element' and 'context.m_selector' everywhere. Not sure if that's groovy.

Comment on attachment 125283 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=125283&action=review > Source/WebCore/css/CSSStyleSelector.cpp:2057 > + SelectorChecker::SelectorCheckingContext context = { > + ruleData.selector(), // m_selector > + m_element, // m_element > + false, // m_isSubSelector > + SelectorChecker::VisitedMatchEnabled, // m_visitedMatchType > + style(), // m_elementStyle > + m_parentNode ? m_parentNode->renderStyle() : 0 // m_elementParentStyle > + }; You should initialize using dot notation so yo don't need these comments context.foo = bar; SelectorCheckingContext should have constructor that initializes the mandatory stuff (selector and element at least) and sets the rest to defaults. Drop the m_ prefixes from the field names (those are not used with public structs). > Source/WebCore/css/SelectorChecker.cpp:454 > +SelectorChecker::SelectorMatch SelectorChecker::checkSelector(SelectorCheckingContext context, PseudoId& dynamicPseudo) const This should use const SelectorCheckingContext&. To modify the context you should make a copy explicitly.

Comment on attachment 125283 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=125283&action=review > Source/WebCore/css/SelectorChecker.cpp:1004 > + SelectorChecker::SelectorCheckingContext subContext = { SelectorChecker:: scope is implied, you don't need to use it explicitly.

Created attachment 125595 [details] Patch New patch, using const references as discussed. Also added a constructor to SelectorCheckingCOntext.

Comment on attachment 125595 [details] Patch Attachment 125595 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11430332 New failing tests: scrollbars/disabled-scrollbar.html scrollbars/scrollbar-orientation.html scrollbars/scrollbar-buttons.html scrollbars/overflow-scrollbar-combinations.html scrollbars/listbox-scrollbar-combinations.html

Comment on attachment 125595 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=125595&action=review looks pretty ok now > Source/WebCore/css/SelectorChecker.cpp:507 > + historyContext.element = context.element->previousElementSibling(); > + if (!historyContext.element) > + return SelectorFailsAllSiblings; > + historyContext.setCompoundSelector(); This still somehow seems bit messy. > Source/WebCore/css/SelectorChecker.h:56 > + SelectorCheckingContext(CSSSelector* selector, Element* element, bool isSubSelector, VisitedMatchType visitedMatchType, RenderStyle* elementStyle, RenderStyle* elementParentStyle) You are never setting isSubSelector to any other value than false so the parameter can be removed.

Created attachment 126011 [details] Patch New patch, removed isSubSelector parameter to constructor, removed setCompoundSelector & replaced with sub-selector and compound selector 'copy' constructors. Note: the compound selector constructor ignores the old context for now, however, the signature will come in handy when being extended for <style scoped>. Also updated variable names 'e'->'element' and 'sel'->'selector' in checkOneSelector() as requested. The only real changes in this function concern pseudoAny and pseudoNot.

Created attachment 126213 [details] Patch Patch, small readability update. Also poke EWS bots.

Comment on attachment 126213 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=126213&action=review r=me, a few comments to consider > Source/WebCore/css/SelectorChecker.cpp:697 > + Element* const & element = context.element; > + CSSSelector* const & selector = context.selector; I think our coding style would have const& here (without space). Not entirely sure... > Source/WebCore/css/SelectorChecker.h:77 > + > + inline SelectorCheckingContext createSubContext(CSSSelector* subSelector) const > + { > + return SelectorCheckingContext(subSelector, element, visitedMatchType, elementStyle, elementParentStyle, true); > + } > + inline SelectorCheckingContext createSubContext(CSSSelector* subSelector, VisitedMatchType newMatchType) const > + { > + return SelectorCheckingContext(subSelector, element, newMatchType, elementStyle, elementParentStyle, true); > + } > + inline SelectorCheckingContext createCompoundContext(CSSSelector* compoundSelector, Element* newElement, VisitedMatchType newMatchType) const > + { > + return SelectorCheckingContext(compoundSelector, newElement, newMatchType); > + } inline keywords here do nothing and should be removed. I feel that while these functions make the code more compact they also make it somewhat more magical. A better pattern would be to just make a copy and explicitly set the fields that you are altering.

Created attachment 126239 [details] patch for EWS testing patch for EWS, incl. the last changes discussed on IRC.

Comment on attachment 126239 [details] patch for EWS Attachment 126239 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11449792 New failing tests: platform/chromium/compositing/layout-width-change.html scrollbars/scrollbar-orientation.html scrollbars/disabled-scrollbar.html scrollbars/scrollbar-buttons.html scrollbars/listbox-scrollbar-combinations.html scrollbars/overflow-scrollbar-combinations.html

Reverted r107197 for reason: broke Committed r107205: <http://trac.webkit.org/changeset/107205>

Created attachment 126268 [details] patch for EWS Argh, must have pressed Undo once too many - that's what I get for hurrying... :P One mor round of EWS to make sure I got it right this time.

Committed in r107212 with a bungled commit message (fixed in r107214). 'Dem bots and scripts really hated me today... :P