[webkit-reviews] review denied: [Bug 38731] Make CSS Parser properly handle only-for-pages pseudo-classes : [Attachment 55541] Make CSS parser properly handle page-only pseudo-classes.

[bugzilla-daemon at webkit.org]

Shinichiro Hamaji <hamaji at chromium.org> has denied Yuzo Fujishima
<yuzo at google.com>'s request for review:
Bug 38731: Make CSS Parser properly handle only-for-pages pseudo-classes
https://bugs.webkit.org/show_bug.cgi?id=38731

Attachment 55541: Make CSS parser properly handle page-only pseudo-classes.
https://bugs.webkit.org/attachment.cgi?id=55541&action=review

------- Additional Comments from Shinichiro Hamaji <hamaji at chromium.org>
A few more comments on the test case.

> + at page world:right { color: red; }

We may want no reds in expectations, even though this won't show any red in
rendering result.

> + at page a_page_name:visited { /* :visited is invalid for @page */
> +    color: blue;
> +}

I think this should not be shown, right? If so, I think red would be better.

> +	   try {
> +	       document.querySelectorAll(".login-popup:first");
> +	       document.getElementById("test2").innerHTML = "Test 2: FAIL";
> +	   } catch (e) {
> +	       document.getElementById("test2").style.display = "none";
> +	   }
> +	   try {
> +	       document.querySelectorAll("::first");
> +	       document.getElementById("test3").innerHTML = "Test 3: FAIL";
> +	   } catch (e) {
> +	       document.getElementById("test3").style.display = "none";
> +	   }
> +
> +	   try {
> +	       document.querySelectorAll(".login-popup:left");
> +	       document.getElementById("test4").innerHTML = "Test 4: FAIL";
> +	   } catch (e) {
> +	       document.getElementById("test4").style.display = "none";
> +	   }
> +	   try {
> +	       document.querySelectorAll("::left");
> +	       document.getElementById("test5").innerHTML = "Test 5: FAIL";
> +	   } catch (e) {
> +	       document.getElementById("test5").style.display = "none";
> +	   }
> +
> +	   try {
> +	       document.querySelectorAll(".login-popup:right");
> +	       document.getElementById("test6").innerHTML = "Test 6: FAIL";
> +	   } catch (e) {
> +	       document.getElementById("test6").style.display = "none";
> +	   }
> +	   try {
> +	       document.querySelectorAll("::right");
> +	       document.getElementById("test7").innerHTML = "Test 7: FAIL";
> +	   } catch (e) {
> +	       document.getElementById("test7").style.display = "none";
> +	   }

I'd write this with a loop. Also, as Alexey suggested, it would be better to
have strings like "PASS" when a test passes. Like,

document.getElementById("test7").innerHTML = "Test 7: PASS";

Are there any reason we should remove these divs?

> +    bool matchPagePseudoClass = (m_match == PagePseudoClass);
> +
> +    if (matchPagePseudoClass != isPagePseudoClass)

I'm not sure if we need a blank line here.