Bug 37848 – WCSS: -wap-input-format and -wap-input-required not supported.

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Charles Wei]

WebKit has partial WCSS support, but doesn't support -wap-input-format  and -wap-input-required.

[Charles Wei]

Created attachment 53794 [details]
add WCSS -wap-input-format and -wap-input-required support

This patch adds -wap-input-format and -wap-input-required support to WebKit.

WebKit already has partial WCSS support,  but lack the support for -wap-input-format and -wap-input-required.

-wap-input-format  defines a mask of value space an input can take, like "*N" indicates only numbers will be accepted.

-wap-input-required specifies if an input control is requied.  if true,  the form can't be submitted without having a valid value for the input control.

[Charles Wei]

Created attachment 53930 [details]
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

[George Staikos]

Comment on attachment 53930 [details]
resubmit of the patch after addressing reviewer's comments


> +#if ENABLE(WCSS)
> +void HTMLInputElement::setWapInputFormat(String& mask)
> +{
> +    String validateMask = validateInputMask(m_data, mask);
> +    if (!validateMask.isEmpty())
> +        m_data.setInputFormatMask(validateMask);
> +}
> +#endif

I think the argument to this function should be const.

> +    virtual InputElementData data() { return m_data; }

 These functions should probably be const?



The rest looks good.  r- for those two minor fixes, then r+ if no concerns from others between now and morning.

[Charles Wei]

Created attachment 54050 [details]
Resubmission after addressing reviewer's comments

Resubmission after addressing the reviewer's comments  -- adding const for some functions which are const functions.

[Charles Wei]

(In reply to comment #3)
> (From update of attachment 53930 [details])
> 
> > +#if ENABLE(WCSS)
> > +void HTMLInputElement::setWapInputFormat(String& mask)
> > +{
> > +    String validateMask = validateInputMask(m_data, mask);
> > +    if (!validateMask.isEmpty())
> > +        m_data.setInputFormatMask(validateMask);
> > +}
> > +#endif
> 
> I think the argument to this function should be const.

====> mask can't be const, because we need to change it inside the function 

> 
> > +    virtual InputElementData data() { return m_data; }
> 
>  These functions should probably be const?
> 
====> Fixed with the new patch.
> 
> 
> The rest looks good.  r- for those two minor fixes, then r+ if no concerns from
> others between now and morning.

[WebKit Commit Bot]

Comment on attachment 54050 [details]
Resubmission after addressing reviewer's comments

Rejecting patch 54050 from commit-queue.

Failed to run "['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--exit-after-n-failures=1', '--quiet']" exit_code: 1
Running build-dumprendertree
Compiling Java tests
make: Nothing to be done for `default'.
Running tests from /Users/eseidel/Projects/CommitQueue/LayoutTests
Testing 12725 test cases.
fast/wcss/wap-input-format.xhtml -> failed

Exiting early after 1 failures. 8914 tests run.
158.44s total testing time

8913 test cases (99%) succeeded
1 test case (<1%) had incorrect layout
4 test cases (<1%) had stderr output

Full output: http://webkit-commit-queue.appspot.com/results/1803086

[Charles Wei]

(In reply to comment #6)
> (From update of attachment 54050 [details])
> Rejecting patch 54050 from commit-queue.
> 
> Failed to run "['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari',
> '--exit-after-n-failures=1', '--quiet']" exit_code: 1
> Running build-dumprendertree
> Compiling Java tests
> make: Nothing to be done for `default'.
> Running tests from /Users/eseidel/Projects/CommitQueue/LayoutTests
> Testing 12725 test cases.
> fast/wcss/wap-input-format.xhtml -> failed
> 
> Exiting early after 1 failures. 8914 tests run.
> 158.44s total testing time
> 
> 8913 test cases (99%) succeeded
> 1 test case (<1%) had incorrect layout
> 4 test cases (<1%) had stderr output
> 
> Full output: http://webkit-commit-queue.appspot.com/results/1803086


This test case is only for WCSS , you need to to enable WCSS to pass.  In this case, how to make this test  case conditional (for WCSS)  ?

[Eric Seidel (no email)]

We can make directories conditional with hacks inside run-webkit-tests (old-run-webkit-tests specifically).

[Eric Seidel (no email)]

See how this is done for SVG, WML, etc. by searching in old-run-webkit-tests.

[Eric Seidel (no email)]

An example from WML:
http://trac.webkit.org/browser/trunk/WebKitTools/Scripts/old-run-webkit-tests#L496
http://trac.webkit.org/browser/trunk/WebKitTools/Scripts/webkitperl/features.pm

I've not yet added this support to new-run-webkit-tests, so you're off the hook there.

[Charles Wei]

Created attachment 54685 [details]
make the test cases conditional for WCSS

Make the test cases conditional for WCSS .

[George Staikos]

Comment on attachment 54685 [details]
make the test cases conditional for WCSS

Seems to do as requested.

[WebKit Commit Bot]

Comment on attachment 54685 [details]
make the test cases conditional for WCSS

Rejecting patch 54685 from commit-queue.

Failed to run "[u'git', u'svn', u'dcommit']" exit_code: 1
Last 500 characters of output:
ment.h
	M	WebKitTools/ChangeLog
	M	WebKitTools/Scripts/old-run-webkit-tests
	M	WebKitTools/Scripts/webkitperl/features.pm
A repository hook failed: MERGE request failed on '/repository/webkit/trunk': Commit blocked by pre-commit hook (exit code 1) with output:
svnlook: Can't write to stream: Broken pipe

    The following ChangeLog files contain OOPS:

        trunk/WebKitTools/ChangeLog

    Please don't ever say "OOPS" in a ChangeLog file.
 at /usr/local/git/libexec/git-core/git-svn line 570


Full output: http://webkit-commit-queue.appspot.com/results/2040001

[Eric Seidel (no email)]

Comment on attachment 54050 [details]
Resubmission after addressing reviewer's comments

Cleared George Staikos's review+ from obsolete attachment 54050 [details] so that this bug does not appear in http://webkit.org/pending-commit.

[Charles Wei]

Created attachment 54991 [details]
patch resubmission

Change Log update to have the reviewer instead of "Reviewed by nobody(OOPs)"

[Charles Wei]

Created attachment 54992 [details]
Resubmission fixing the reviewer info

resubmit fixing the reviewer info , and obsolete the old  patches.

[WebKit Commit Bot]

Comment on attachment 54992 [details]
Resubmission fixing the reviewer info

Rejecting patch 54992 from review queue.

[e-mail] does not have reviewer permissions according to http://trac.webkit.org/browser/trunk/WebKitTools/Scripts/webkitpy/common/config/committers.py.

- If you do not have reviewer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

- If you have reviewer rights please correct the error in WebKitTools/Scripts/webkitpy/common/config/committers.py by adding yourself to the file (no review needed).  Due to bug 30084 the commit-queue will require a restart after your change.  Please contact [e-mail] to request a commit-queue restart.  After restart the commit-queue will correctly respect your reviewer rights.

[George Staikos]

Comment on attachment 54992 [details]
Resubmission fixing the reviewer info

This was already r+.  Not sure how this happened.

[WebKit Commit Bot]

Comment on attachment 54992 [details]
Resubmission fixing the reviewer info

Rejecting patch 54992 from commit-queue.

Failed to run "[u'git', u'svn', u'dcommit']" exit_code: 1
Last 500 characters of output:
ment.h
	M	WebKitTools/ChangeLog
	M	WebKitTools/Scripts/old-run-webkit-tests
	M	WebKitTools/Scripts/webkitperl/features.pm
A repository hook failed: MERGE request failed on '/repository/webkit/trunk': Commit blocked by pre-commit hook (exit code 1) with output:
svnlook: Can't write to stream: Broken pipe

    The following ChangeLog files contain OOPS:

        trunk/WebKitTools/ChangeLog

    Please don't ever say "OOPS" in a ChangeLog file.
 at /usr/local/git/libexec/git-core/git-svn line 570


Full output: http://webkit-commit-queue.appspot.com/results/2109031

[Charles Wei]

(In reply to comment #19)
> (From update of attachment 54992 [details])
> Rejecting patch 54992 from commit-queue.
> 
> Failed to run "[u'git', u'svn', u'dcommit']" exit_code: 1
> Last 500 characters of output:
> ment.h
>     M    WebKitTools/ChangeLog
>     M    WebKitTools/Scripts/old-run-webkit-tests
>     M    WebKitTools/Scripts/webkitperl/features.pm
> A repository hook failed: MERGE request failed on '/repository/webkit/trunk':
> Commit blocked by pre-commit hook (exit code 1) with output:
> svnlook: Can't write to stream: Broken pipe
> 
>     The following ChangeLog files contain OOPS:
> 
>         trunk/WebKitTools/ChangeLog
> 
>     Please don't ever say "OOPS" in a ChangeLog file.
>  at /usr/local/git/libexec/git-core/git-svn line 570
> 
> 
> Full output: http://webkit-commit-queue.appspot.com/results/2109031



That's really frustrating.  I submitted many patches before, all with "Reviewed by NOBODY(OOPS!)",  and they were landed.    When I submit my patch , I don't know who is going to review it and land it , so I have to leave it there.

Here's what the it say at http://webkit.org/coding/contributing.html:

ChangeLog files
ChangeLogs are simple text files which provide historical documentation for all changes to the WebKit project. ...  Don't worry about the "Reviewed by NOBODY (OOPS!)" line, the person landing your patch will fill this in.


I suppose the submitter is not supposed to change that ?  Please elaborate who is going to replace the "OOPs".

Thanks 

Charles

[Eric Seidel (no email)]

(In reply to comment #18)
> (From update of attachment 54992 [details])
> This was already r+.  Not sure how this happened.

If you look at the history (link on the top right of the page):
https://bugs.webkit.org/show_activity.cgi?id=37848

Then you see that Charles set r+.  The commit-queue of course rejected the r+.  It's possible to use the commit-queue without setting r+ and just setting cq+.  However then the commit-queue will not attempt to fill in the reviewer line for you.

[Eric Seidel (no email)]

(In reply to comment #20)
> >     The following ChangeLog files contain OOPS:
> > 
> >         trunk/WebKitTools/ChangeLog
> > 
> >     Please don't ever say "OOPS" in a ChangeLog file.
> >  at /usr/local/git/libexec/git-core/git-svn line 570

If you look at the file in question (WebKitTools/ChangeLog) in the patch (attachment 54992 [details]), note that it has OOPS twice:

+        Reviewed by NOBODY (OOPS!).
+        Need a short description and bug URL (OOPS!)

The reviewed by OOPS is expected.  If you leave that line as-is, the commit-queue will fill in the reviewer with whoever set r+ on the patch.

The second one is NOT expected.  You should fill that line in with a short description of the patch as well as explanations of testing, etc.

> That's really frustrating.  I submitted many patches before, all with "Reviewed
> by NOBODY(OOPS!)",  and they were landed.    When I submit my patch , I don't
> know who is going to review it and land it , so I have to leave it there.
> 
> Here's what the it say at http://webkit.org/coding/contributing.html:
> 
> ChangeLog files
> ChangeLogs are simple text files which provide historical documentation for all
> changes to the WebKit project. ...  Don't worry about the "Reviewed by NOBODY
> (OOPS!)" line, the person landing your patch will fill this in.

I think that our current behavior of using OOPS lines to mean two different things is bad and should be changed.  However, it is what it is for now. Hopefully the above explanation helped.

[Charles Wei]

Created attachment 55198 [details]
fixes changelog problem

This patch fixes all the ChangeLog problems in last submission.

[WebKit Commit Bot]

Comment on attachment 55198 [details]
fixes changelog problem

Clearing flags on attachment: 55198

Committed r58867: <http://trac.webkit.org/changeset/58867>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.