[webkit-reviews] review denied: [Bug 23758] WCSS input extension is not supported : [Attachment 30443] Latest Updated patch for code change

[bugzilla-daemon at webkit.org]

Sam Weinig <sam at webkit.org> has denied yichao.yin
<yichao.yin at torchmobile.com.cn>'s request for review:
Bug 23758: WCSS input extension is not supported
https://bugs.webkit.org/show_bug.cgi?id=23758

Attachment 30443: Latest Updated patch for code change
https://bugs.webkit.org/attachment.cgi?id=30443&action=review

------- Additional Comments from Sam Weinig <sam at webkit.org>
> +	   This change depends on https://bugs.webkit.org/show_bug.cgi?id=23452


This doesn't make sense to have in a changelog.  Either the change works or it
doesn't.

> +
> +	   Tests: fast/wcss/inputelement-inputformat.xhtml
> +		  fast/wcss/inputelement-inputrequired.xhtml
> +		  fast/wcss/textarea-inputformat.xhtml
> +		  fast/wcss/textarea-inputrequired.xhtml

These tests don't seem to be included.

> +    bool ok = true;
> +    UChar mask = inputFormatMask[maskIndex];
> +    // Match the inputed character with input mask
> +    switch (mask) {
> +    case 'A':
> +	   ok = !WTF::isASCIIDigit(inChar) && !WTF::isASCIILower(inChar) &&
WTF::isASCIIPrintable(inChar);
> +	   break;
> +    case 'a':
> +	   ok = !WTF::isASCIIDigit(inChar) && !WTF::isASCIIUpper(inChar) &&
WTF::isASCIIPrintable(inChar);
> +	   break;
> +    case 'N':
> +	   ok = WTF::isASCIIDigit(inChar);
> +	   break;
> +    case 'n':
> +	   ok = !WTF::isASCIIAlpha(inChar) && WTF::isASCIIPrintable(inChar);
> +	   break;
> +    case 'X':
> +	   ok = !WTF::isASCIILower(inChar) && WTF::isASCIIPrintable(inChar);
> +	   break;
> +    case 'x':
> +	   ok = !WTF::isASCIIUpper(inChar) && WTF::isASCIIPrintable(inChar);
> +	   break;
> +    case 'M':
> +	   ok = WTF::isASCIIPrintable(inChar);
> +	   break;
> +    case 'm':
> +	   ok = WTF::isASCIIPrintable(inChar);

The WTF should not be needed for these (or at the very least, you should add
using namespace WTF at the top of file).  The cases for 'M' and 'm' can be
combined.
> Index: WebCore/html/HTMLTextAreaElement.h
> ===================================================================
> --- WebCore/html/HTMLTextAreaElement.h	(revision 43681)
> +++ WebCore/html/HTMLTextAreaElement.h	(working copy)
> @@ -3,6 +3,7 @@
>   *		(C) 1999 Antti Koivisto (koivisto at kde.org)
>   *		(C) 2000 Dirk Mueller (mueller at kde.org)
>   * Copyright (C) 2004, 2005, 2006, 2007 Apple Inc. All rights reserved.
> + * Copyright (C) 2009 Torch Mobile Inc. All rights reserved.
(http://www.torchmobile.com/)
>   *
>   * This library is free software; you can redistribute it and/or
>   * modify it under the terms of the GNU Library General Public
> @@ -26,11 +27,19 @@
>  
>  #include "HTMLFormControlElement.h"
>  
> +#if ENABLE(WCSS)
> +#include "InputElement.h"
> +#endif
> +
>  namespace WebCore {
>  
>  class VisibleSelection;
>  
> +#if ENABLE(WCSS)
> +class HTMLTextAreaElement : public HTMLFormControlElementWithState, public
InputElement {
> +#else
>  class HTMLTextAreaElement : public HTMLFormControlElementWithState {
> +#endif

This seems wrong to me.  An HTMLTextAreaElement is not an InputElement and
therefore should never inherit from InputElement.  The changes made to this
class seem too invasive and I think a cleaner approach is necessary.  

r-