Put unqualified pseudos into the more specific rulesets.

Put unqualified pseudos into the more specific rulesets.

We were incorrectly putting unqualified pseudo classes for :focus,
:visited, :-webkit-any-link and :link into the universal RuleSet which
meant we would match them for every element defeating the optimization
of having the separate rulesets.

This patch starts tracking the pseudos inside the extraction step to
make sure we put them into the right ruleset. It also adds some asserts
that the default UA sheet never adds any universal rules.

This removes one rule (:focus) from the set of rules for every element.

[esprehn, 2017-05-10 00:19:42]

Description was changed from

==========
Put :focus rule in the UA sheet into the focus rule set.

We were incorrectly putting unqualified pseudo classes for :focus,
:visited, :-webkit-any-link and :link into the universal RuleSet which
meant we would match them for every element defeating the optimization
of having the separate rulesets.

This patch starts tracking the pseudos inside the extraction step to
make sure we put them into the right ruleset. It also adds some asserts
that the default UA sheet never adds any universal rules.

This removes one rule (:focus) from the set of rules for every element.
==========

to

==========
Put unqualified pseudos into the more specific rulesets.

We were incorrectly putting unqualified pseudo classes for :focus,
:visited, :-webkit-any-link and :link into the universal RuleSet which
meant we would match them for every element defeating the optimization
of having the separate rulesets.

This patch starts tracking the pseudos inside the extraction step to
make sure we put them into the right ruleset. It also adds some asserts
that the default UA sheet never adds any universal rules.

This removes one rule (:focus) from the set of rules for every element.
==========

[esprehn, 2017-05-10 00:19:58]

The CQ bit was checked by esprehn@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-10 00:20:58]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[esprehn, 2017-05-10 00:24:04]

The CQ bit was checked by esprehn@chromium.org to run a CQ dry run

[esprehn, 2017-05-10 00:24:44]

esprehn@chromium.org changed reviewers:
+ meade@chromium.org, nainar@chromium.org, rune@opera.com

[commit-bot: I haz the power, 2017-05-10 00:25:30]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[nainar, 2017-05-10 02:23:16]

lgtm

[commit-bot: I haz the power, 2017-05-10 02:27:54]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-10 02:27:55]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, build has
not started yet; builder either lacks capacity or does not exist (misspelled?))
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_TIMED_OUT, build has not started yet;
builder either lacks capacity or does not exist (misspelled?))
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does
not exist (misspelled?))
  chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, build
has not started yet; builder either lacks capacity or does not exist
(misspelled?))
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT,
build has not started yet; builder either lacks capacity or does not exist
(misspelled?))
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does
not exist (misspelled?))
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does
not exist (misspelled?))
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does
not exist (misspelled?))
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, build
has not started yet; builder either lacks capacity or does not exist
(misspelled?))
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT,
build has not started yet; builder either lacks capacity or does not exist
(misspelled?))

[rune, 2017-05-10 08:46:02]

https://codereview.chromium.org/2873123002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/RuleSet.cpp (right):

https://codereview.chromium.org/2873123002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/RuleSet.cpp:155: CSSSelector::PseudoType&
pseudoType) {
camelCase?

https://codereview.chromium.org/2873123002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/RuleSet.cpp:200: CSSSelector::PseudoType
pseudoType = CSSSelector::kPseudoUnknown;
camelCase?

https://codereview.chromium.org/2873123002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/RuleSet.cpp:240: switch (pseudoType) {
So, if I read this correctly:

"[attr]:visited" will now end up in link_pseudo_class_rules_, while it used to
end up in universal, while ":visited" ended up in link_pseudo_class_rules_ both
before and after this change?

Could you add a unit test for the former case?

https://codereview.chromium.org/2873123002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/RuleSet.cpp:250:
focus_pseudo_class_rules_.push_back(rule_data);
You say that you remove the html.css ":focus" rule from the universal bucket. I
was wondering why this didn't happen without this patch since ":focus" is a
single simple selector and would have been matched by the
component.GetPseudoType() switch?

Is it because the default @namespace in html.css generates an implicit type
selector?

Anyway, I think this means affected-by-focus will not be set for the focus-ring
rule in html.css which explains your test failures.

Should we just drop the affected-by-focus flag assuming it's always true?

Thinking about the default namespace, that means the universal :focus in
html.css only applies to elements in the html namespace. Is that correct?

[esprehn, 2017-05-10 18:07:18]

On 2017/05/10 at 08:46:02, rune wrote:
>
https://codereview.chromium.org/2873123002/diff/40001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/css/RuleSet.cpp (right):
> 
>
https://codereview.chromium.org/2873123002/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/css/RuleSet.cpp:155: CSSSelector::PseudoType&
pseudoType) {
> camelCase?
> 
>
https://codereview.chromium.org/2873123002/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/css/RuleSet.cpp:200: CSSSelector::PseudoType
pseudoType = CSSSelector::kPseudoUnknown;
> camelCase?

Heh yeah...

> 
>
https://codereview.chromium.org/2873123002/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/css/RuleSet.cpp:240: switch (pseudoType) {
> So, if I read this correctly:
> 
> "[attr]:visited" will now end up in link_pseudo_class_rules_, while it used to
end up in universal, while ":visited" ended up in link_pseudo_class_rules_ both
before and after this change?
> 
> Could you add a unit test for the former case?

Yeah we should have better tests.

> 
>
https://codereview.chromium.org/2873123002/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/css/RuleSet.cpp:250:
focus_pseudo_class_rules_.push_back(rule_data);
> You say that you remove the html.css ":focus" rule from the universal bucket.
I was wondering why this didn't happen without this patch since ":focus" is a
single simple selector and would have been matched by the
component.GetPseudoType() switch?
> 
> Is it because the default @namespace in html.css generates an implicit type
selector?

Yeah that makes sense, I was trying to figure out why the :focus rule in the
html.css file is defeating the ruleset optimization. Today the rules are going
into the universal set.

> 
> Anyway, I think this means affected-by-focus will not be set for the
focus-ring rule in html.css which explains your test failures.
> 
> Should we just drop the affected-by-focus flag assuming it's always true?

Probably? Every element could match :focus.

> 
> Thinking about the default namespace, that means the universal :focus in
html.css only applies to elements in the html namespace. Is that correct?

I guess so, today unqualified CSS rules in html.css like :focus or :visited end
up in the universal set.

[meade_UTC10, 2017-05-11 01:59:40]

It looks like maybe you forgot to upload a patchset? This lgtm assuming there
were actually changes along with the replies to the comments :)

[meade_UTC10, 2017-05-11 02:00:01]

On 2017/05/11 01:59:40, meade_UTC10 wrote:
> It looks like maybe you forgot to upload a patchset? This lgtm assuming there
> were actually changes along with the replies to the comments :)

Oh, and the tests being fixed...