[webkit-reviews] review denied: [Bug 38731] Make CSS Parser properly handle only-for-pages pseudo-classes : [Attachment 55355] 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 55355: Make CSS parser properly handle page-only pseudo-classes.
https://bugs.webkit.org/attachment.cgi?id=55355&action=review

------- Additional Comments from Shinichiro Hamaji <hamaji at chromium.org>
The C++ code change looks sane to me but I'm putting r- for this patch per
Alexey's comment (thanks!).

> @@ -431,6 +431,7 @@ void CSSSelector::extractPseudoType() const
>  
>      bool element = false; // pseudo-element
>      bool compat = false; // single colon compatbility mode
> +    bool page = false; // pseudo-page
>  
>      switch (m_pseudoType) {
>      case PseudoAfter:

I slightly prefer more descriptive name like isPseudoPage.

> @@ -529,12 +530,13 @@ void CSSSelector::extractPseudoType() const
>      case PseudoFirstPage:
>      case PseudoLeftPage:
>      case PseudoRightPage:
> -	   // FIXME: These should only be allowed in @page rules. Disabled them
altogether until that's implemented correctly.
> -	   m_pseudoType = PseudoUnknown;
> -	   return;
> +	   page = true;
> +	   break;
>      }
>  
> -    if (m_match == PseudoClass && element) {
> +    if ((m_match == PseudoPage && !page) || (m_match != PseudoPage && page))

> +	   m_pseudoType = PseudoUnknown;
> +    else if (m_match == PseudoClass && element) {
>	   if (!compat)
>	       m_pseudoType = PseudoUnknown;
>	   else

I'd write

bool inPage = (m_match == PseudoPage);
if (inPage != isPseudoPage)
    m_pseudoType = PseudoUnknown;

but I'm not sure if this is the preferred style in WebKit.