Bug 153205 - Selector checker should not mutate document and style
Summary: Selector checker should not mutate document and style
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-01-18 06:16 PST by Antti Koivisto
Modified: 2016-01-19 09:39 PST (History)
4 users (show)

See Also:


Attachments
WIP (93.99 KB, patch)
2016-01-18 06:17 PST, Antti Koivisto
no flags Details | Formatted Diff | Diff
WIP (94.62 KB, patch)
2016-01-18 07:01 PST, Antti Koivisto
no flags Details | Formatted Diff | Diff
patch (106.10 KB, patch)
2016-01-18 07:48 PST, Antti Koivisto
no flags Details | Formatted Diff | Diff
patch (106.07 KB, patch)
2016-01-18 07:55 PST, Antti Koivisto
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Antti Koivisto 2016-01-18 06:16:42 PST
Selector checker currently writes affected-by bits directly to the document and style during selector matching. This is confusing, complicated and wrong.
Comment 1 Antti Koivisto 2016-01-18 06:17:22 PST
Created attachment 269217 [details]
WIP
Comment 2 WebKit Commit Bot 2016-01-18 06:27:07 PST
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.
Comment 3 Antti Koivisto 2016-01-18 07:01:33 PST
Created attachment 269220 [details]
WIP
Comment 4 Antti Koivisto 2016-01-18 07:48:23 PST
Created attachment 269221 [details]
patch
Comment 5 Antti Koivisto 2016-01-18 07:55:07 PST
Created attachment 269223 [details]
patch
Comment 6 Darin Adler 2016-01-18 15:18:23 PST
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.
Comment 7 Antti Koivisto 2016-01-19 02:54:58 PST
> 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.
Comment 8 Antti Koivisto 2016-01-19 09:39:56 PST
https://trac.webkit.org/r195293