[webkit-reviews] review canceled: [Bug 37848] WCSS: -wap-input-format and -wap-input-required not supported. : [Attachment 53794] add WCSS -wap-input-format and -wap-input-required support

[bugzilla-daemon at webkit.org]

Charles Wei <charles.wei at torchmobile.com.cn> has canceled Charles Wei
<charles.wei at torchmobile.com.cn>'s request for review:
Bug 37848: WCSS: -wap-input-format  and -wap-input-required not supported.
https://bugs.webkit.org/show_bug.cgi?id=37848

Attachment 53794: add WCSS -wap-input-format and -wap-input-required support
https://bugs.webkit.org/attachment.cgi?id=53794&action=review

------- Additional Comments from Charles Wei <charles.wei at torchmobile.com.cn>
Resubmit the patch after addressing the reviewer's comments as below : 

1730	#if ENABLE(WCSS)
1731	PassRefPtr<CSSValue> CSSParser::parseWCSSInputProperty()
1732	{
1733	    RefPtr<CSSValue> parsedValue = 0;
1734	    CSSParserValue* value = m_valueList->current();
1735	    String inputProperty;
1736	    while (value) {
1737		if (value->unit == CSSPrimitiveValue::CSS_STRING || value->unit
== CSSPrimitiveValue::CSS_IDENT)
1738		    inputProperty = String(value->string);
1739	
1740		if (!inputProperty.isEmpty())
1741		   parsedValue = CSSPrimitiveValue::create(inputProperty,
CSSPrimitiveValue::CSS_STRING);
1742	
1743		value = m_valueList->next();
1744	    }
1745	
1746	    return parsedValue;
1747	}
1748	#endif

In this block, is it expected that only the last value will be the parsed
value?	On my sample page there is only one, but if only one is allowed,
there's no need for a while loop right?  Just want to make sure that this won't
cause unexpected behavior.

====> Fixed 

205	Range* range =
element->document()->frame()->selection()->selection().toNormalizedRange().get(
);

This would be better as a PassRef<Range>.

====> Fixed 

Some minor nits that I've seen so far (writing here so I don't forget):
[3:28:43 PM] Dan Bates:  5322	      if
(m_element->hasTagName(WebCore::inputTag) && primitiveValue) {
[3:28:43 PM] Dan Bates: And
[3:28:44 PM] Dan Bates: 5329	     if (m_element->isFormControlElement() &&
primitiveValue) {
[3:28:53 PM] Dan Bates: Why not check primitiveValue first?
[3:28:57 PM] Dan Bates:  5331		  bool required =
(primitiveValue->getStringValue() == "true") ? true : false;
[3:29:11 PM] Dan Bates: Can be rewritten:  bool required =
primitiveValue->getStringValue() == "true";

====> Fixed 


To factor it out since it is shared by WMLInputElement
====> Fixed

#include "Chrome.h"
>  #include "ChromeClient.h"
> +
> +#if ENABLE(WCSS)
> +#include "CSSPropertyNames.h"
> +#include "CSSRule.h"
> +#include "CSSRuleList.h"
> +#include "CSSStyleRule.h"
> +#include "CSSStyleSelector.h"
> +#endif
> +
>
> These should be before the Chrome.h to be sorted alphabetically.

====> Fixed



>Also, I would expect the reviewer to ask you to remove inputCharsCount if it
is always cursorPosition + 1 and the way it's used could just be updated to be
>
> if (cursorPosition >= data.maxInputCharsAllowed())
>
> I will try to apply the patch shortly, just hoping to get it reviewed & in
quickly.

====> Fixed