Bug 111252

Created attachment 191108 [details] WIP

Created attachment 191117 [details] Patch

Comment on attachment 191117 [details] Patch Attachment 191117 [details] did not pass mac-ews (mac): Output: http://webkit-commit-queue.appspot.com/results/16797731 New failing tests: editing/selection/selection-invalid-offset.html

I thought I wrote up comments for this patch, but they must have not sent. I'm slightly surprised to see this, since SelecorChecker (and many of the style classes) could just be static and operate on context objets. That appears to be someone's eariler goal here. That said, It's OK for the logic and state to live in a single stack allocated class. I assume you were the person who did the split before?

Comment on attachment 191117 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=191117&action=review > Source/WebCore/css/StyleResolver.cpp:2118 > - SelectorChecker selectorChecker(document(), state.mode()); > - SelectorChecker::SelectorCheckingContext context(ruleData.selector(), state.element(), SelectorChecker::VisitedMatchEnabled); > - context.elementStyle = state.style(); > - context.scope = scope; > - context.pseudoId = state.pseudoStyleRequest().pseudoId; > - context.scrollbar = state.pseudoStyleRequest().scrollbar; > - context.scrollbarPart = state.pseudoStyleRequest().scrollbarPart; > - context.behaviorAtBoundary = behaviorAtBoundary; > - SelectorChecker::Match match = selectorChecker.match(context, dynamicPseudo, DOMSiblingTraversalStrategy()); > + SelectorChecker selectorChecker(ruleData.selector(), state.element(), state.mode(), SelectorChecker::VisitedMatchEnabled, state.style(), scope, state.pseudoStyleRequest().pseudoId, state.pseudoStyleRequest().scrollbar, state.pseudoStyleRequest().scrollbarPart, behaviorAtBoundary); Wasn't the point of having a context object to avoid confusing and inflexible argument lists like this?

(In reply to comment #4) > I thought I wrote up comments for this patch, but they must have not sent. > > I'm slightly surprised to see this, since SelecorChecker (and many of the style classes) could just be static and operate on context objets. That appears to be someone's eariler goal here. > > That said, It's OK for the logic and state to live in a single stack allocated class. I assume you were the person who did the split before? Roland did it a while back in bug 77525. Back then, SelectorChecker was a lot more than just an on-stack class. Since then, I whittled it down, and it seems to make little sense to have both of these. The const SelectorCheckerContext& param in front of all of SelectorChecker members even looks like a "this" ptr (if you pretend it's Python :)

Comment on attachment 191117 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=191117&action=review >> Source/WebCore/css/StyleResolver.cpp:2118 >> + SelectorChecker selectorChecker(ruleData.selector(), state.element(), state.mode(), SelectorChecker::VisitedMatchEnabled, state.style(), scope, state.pseudoStyleRequest().pseudoId, state.pseudoStyleRequest().scrollbar, state.pseudoStyleRequest().scrollbarPart, behaviorAtBoundary); > > Wasn't the point of having a context object to avoid confusing and inflexible argument lists like this? Ideally, SelectorChecker should just take State as a constructor: SelectorCheckt(ruleData.selector(), SelectorChecker::VisitedMatchEnabled, state); It's pretty nice logically -- convey the current resolver state into the selector checker. However, the way they are defined is a bit messy. StyleResolver.h needs SelectorChecker.h to pull in some constants. I was thinking of moving them into a separate header, and then it'll be pretty.

One other thing that might be useful: I was hoping to study the places where nested SelectorCheckers are created next, and: 1) defining better separation between nesting checkers and nested checkers (like, often the nesting checker messes with the nested checker, and that seems bad) 2) exploring selector checker only doing one thing (checking selectors) and some other class doing the thing of traversing the selector chain. If you have other cool ideas, let me know. This is my hobby hacking anyway :)

(In reply to comment #7) > Ideally, SelectorChecker should just take State as a constructor: You mean StyleResolver::State? That doesn't seem correct. We don't want to have two-way dependency between these. SelectorChecker has clients that have nothing to do with StyleResolver.

(In reply to comment #9) > (In reply to comment #7) > > Ideally, SelectorChecker should just take State as a constructor: > > You mean StyleResolver::State? That doesn't seem correct. We don't want to have two-way dependency between these. SelectorChecker has clients that have nothing to do with StyleResolver. Right (see the second half of comment 7). StyleResolver doesn't need to depend on SelectorChecker, so by moving a few enums, I can switch the dependency and have SelectorChecker depend on StyleResolver.

Closing, SelectorCheckingContext is irrelevant now.