[webkit-reviews] review denied: [Bug 22431] Implement <access> support in WML : [Attachment 25384] Updated patch

[bugzilla-daemon at webkit.org]

Eric Seidel <eric at webkit.org> has denied Nikolas Zimmermann
<zimmermann at kde.org>'s request for review:
Bug 22431: Implement <access> support in WML
https://bugs.webkit.org/show_bug.cgi?id=22431

Attachment 25384: Updated patch
https://bugs.webkit.org/attachment.cgi?id=25384&action=review

------- Additional Comments from Eric Seidel <eric at webkit.org>

> +	   if (Page* page = document()->page())
> +	       page->wmlPageState()->setDeckAccessDomain(attr->value());

> +	   if (Page* page = document()->page()) 
> +	       page->wmlPageState()->setDeckAccessPath(attr->value());

I think these calls would be better named "restrictAccessToDeckToDomain" and
"restrictAccessToDeckToPath"

identifying that adding a new restriction clears all others, and that this is
not what domains or paths the current deck can acess, but rather, what domains
and paths can acesss the deck.	The names are still not perfect, but I think
they're better than what were there before.

That should probably be "containsWMLVariableReference()"
 
> +bool valueContainsVariableReference(const String& text)
> +{
> +    int start = text.find('$');
> +    if (start == -1)
> +	   return false;
> +
> +    int end = start + 1;
> +    while (text[end] == '$')
> +	   ++end;
> +
> +    return ((end-start) % 2) != 0;
> +}

Also, that function will do the following:
"$" true
"$$" false
"$$$" true
"$$ foobar $bar" false

I think it needs more testing...

>      enum WMLVariableEscapingMode {
>	   WMLVariableEscaping_None = 0,
> -	   WMLVariableEscaping_Escape = 1,
> -	   WMLVariableEscaping_Unescape = 2
> +	   WMLVariableEscaping_Escape,
> +	   WMLVariableEscaping_Unescape
>      };

WebKit style would say those should be WMLVariableEscapingNone, no _ I think.

r- for the lack of variable testing (and thus incorrectness of said function). 
I"m not sure it's really a big deal that it's wrong.  Otherwise the patch
looked fine.  Please rename the functions noted to something better. :)