[webkit-reviews] review granted: [Bug 135639] CSS: Refactor :visited handling in SelectorChecker : [Attachment 237356] Patch

[bugzilla-daemon at webkit.org]

Benjamin Poulain <benjamin at webkit.org> has granted Yusuke Suzuki
<utatane.tea at gmail.com>'s request for review:
Bug 135639: CSS: Refactor :visited handling in SelectorChecker
https://bugs.webkit.org/show_bug.cgi?id=135639

Attachment 237356: Patch
https://bugs.webkit.org/attachment.cgi?id=237356&action=review

------- Additional Comments from Benjamin Poulain <benjamin at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=237356&action=review


Some comments but it is minor. I trust you with the update, but if you want I
can also review it.

> Source/WebCore/ChangeLog:9
> +	   Fix :-webkit-any(:visited) consistency in linkMatchType and actual
selector matching.
> +	   And fix disabling :visited matching when the first link comes.

It would be good to have a more descriptive changelog.

A lot of important information is on the bug report itself. It is a good idea
to also repeat that in the changelog. With such information, someone can figure
out what your patch does and why you are taking this approach.

Changelog are especially useful when trying to find the reasoning behind a
change that was very far in the past. Sometime we have to investigate issue
related to code that is over 10 years old.

>> Source/WebCore/css/SelectorChecker.cpp:707
>> +		    return false;
> 
> This changes :-webkit-any(:visited). But :-webkit-any(:link) behavior is not
changed.
> What do you think?
> Since still we need to consider what is the best behavior about :-webkit-any
and :visited, :link, in this time, I don't change :-webkit-any(:link) behavior.


I am okay with this as a temporary measure. I think you should also disable
:link for consistency (:link would always match inside a functional pseudo
class).

We need big changes for :not() and :-webkit-any()/:matches(), that's gonna be
painful :(

> Source/WebCore/css/SelectorChecker.h:58
> -	   SelectorCheckingContext(const CSSSelector* selector, Element*
element, VisitedMatchType visitedMatchType)
> +	   SelectorCheckingContext(const CSSSelector* selector, Element*
element, Mode resolvingMode)

Can you move "VisitedMatchType" inside SelectorChecker.cpp now? Or is it still
referenced somewhere?

> LayoutTests/ChangeLog:9
> +	   Fix :-webkit-any(:visited) consistency in linkMatchType and actual
selector matching.
> +	   And fix disabling :visited matching when the first link comes.

You don't need to repeat the changelog for test. Usually here you describe the
tests when they are not obvious.

> LayoutTests/ChangeLog:16
> +	   * fast/history/nested-visited-test-override-expected.txt: Added.
> +	   * fast/history/nested-visited-test-override.html: Added.
> +	   * fast/history/visited-inside-any-expected.txt: Added.
> +	   * fast/history/visited-inside-any.html: Added.
> +	   * fast/history/visited-inside-not-expected.txt: Added.
> +	   * fast/history/visited-inside-not.html: Added.

It is very good to have this. I would duplicate all of that for :link since
they are opposite.

> LayoutTests/fast/history/nested-visited-test-override.html:12
> +    description(":visited is only effective when the target is first
encountered link element.");

We usually put the description() call at the beginning of the <script>, outside
the test function.