[Webkit-unassigned] [Bug 37848] WCSS: -wap-input-format and -wap-input-required not supported.

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=37848


Charles Wei <charles.wei at torchmobile.com.cn> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #53794|0                           |1
        is obsolete|                            |
  Attachment #53794|review?                     |
               Flag|                            |
  Attachment #53930|                            |review?
               Flag|                            |




--- Comment #2 from Charles Wei <charles.wei at torchmobile.com.cn>  2010-04-21 01:29:04 PST ---
Created an attachment (id=53930)
 --> (https://bugs.webkit.org/attachment.cgi?id=53930)
resubmit of the patch after addressing reviewer's comments

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

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.