Selector checker should not mutate document and style

• Component: CSS
• Status: RESOLVED FIXED

[Antti Koivisto]

Selector checker currently writes affected-by bits directly to the document and style during selector matching. This is confusing, complicated and wrong.

[Antti Koivisto]

Created attachment 269217 [details]
WIP

[WebKit Commit Bot]

Attachment 269217 [details] did not pass style-queue:


ERROR: Source/WebCore/cssjit/SelectorCompiler.cpp:327:  Missing space inside { }.  [whitespace/braces] [5]
Total errors found: 1 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Antti Koivisto]

Created attachment 269220 [details]
WIP

[Antti Koivisto]

Created attachment 269221 [details]
patch

[Antti Koivisto]

Created attachment 269223 [details]
patch

[Darin Adler]

Comment on attachment 269223 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=269223&action=review

> Source/WebCore/css/SelectorChecker.cpp:91
> +    checkingContext.styleRelations.append({ const_cast<Element&>(element), type, value });

A little strange that we end up using the const_cast here. Any way to avoid it in future? I guess you are focusing on using const here more to express the fact that we won’t be setting flags and such on the Element.

> Source/WebCore/css/SelectorChecker.cpp:1014
> +        LocalContext subContext(context);

The coined word “subcontext” is a single word and it should not have a capital "C" in it.

> Source/WebCore/css/SelectorCheckerTestFunctions.h:66
> +ALWAYS_INLINE bool isMediaDocument(const Element* element)

This requires a non-null pointer, so I would have expected it to take a reference. Then I thought there was some assembly-language-related reason these have to take pointers, but I noticed that isChecked takes a reference.

> Source/WebCore/css/SelectorCheckerTestFunctions.h:86
> +ALWAYS_INLINE bool isInRange(const Element* element)

Ditto.

> Source/WebCore/css/SelectorCheckerTestFunctions.h:91
> +ALWAYS_INLINE bool isOutOfRange(const Element* element)

Ditto.

[Antti Koivisto]

> A little strange that we end up using the const_cast here. Any way to avoid
> it in future? I guess you are focusing on using const here more to express
> the fact that we won’t be setting flags and such on the Element.

Right, this structure is used by the client (which presumably has non-const access to the DOM) to mutate the Element later. Selector checker never reads style relations, just writes them out. To apply the data someone will need to const_cast at some point, I thought better do it here.

[Antti Koivisto]

https://trac.webkit.org/r195293