Bug 111252 – Merge SelectorCheckingContext into SelectorChecker.

• Status: RESOLVED
• Resolution: WONTFIX
• Component: New Bugs

[Dimitri Glazkov (Google)]

Merge SelectorCheckingContext into SelectorChecker.

[Dimitri Glazkov (Google)]

Created attachment 191108 [details]
WIP

[Dimitri Glazkov (Google)]

Created attachment 191117 [details]
Patch

[Build Bot]

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

[Eric Seidel (no email)]

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?

[Antti Koivisto]

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?

[Dimitri Glazkov (Google)]

(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 :)

[Dimitri Glazkov (Google)]

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.

[Dimitri Glazkov (Google)]

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 :)

[Antti Koivisto]

(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.

[Dimitri Glazkov (Google)]

(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.

[Benjamin Poulain]

Closing, SelectorCheckingContext is irrelevant now.