spec change - option.label should be reflected like option.value

[Sachin Puranik]

There is a spec change in the below given revision for option element.

http://www.w3.org/Bugs/Public/show_bug.cgi?id=12255

Mozilla has already made this change.

IDL attribute "Label" should reflect content attribute "Label". If the content attribute is not specified return textContent

[Sachin Puranik]

Created attachment 108454 [details]
Spec change handling

[Sachin Puranik]

Hi,
Can you please review this change.

[Kent Tamura]

Comment on attachment 108454 [details]
Spec change handling

View in context: https://bugs.webkit.org/attachment.cgi?id=108454&action=review

> Source/WebCore/dom/OptionElement.cpp:136
> +String OptionElement::collectOptionLabel(const OptionElementData& data, const Element* element)
> +{
> +    String label = data.label();
> +    if (!label.isNull())
> +        return label;
> +
> +    // Use the text if the Label wasn't set.
> +    return collectOptionInnerText(element).stripWhiteSpace();
> +}

I don't think we need to introduce collectOptionLabel(). Making collectOptionInnerText() "protected" is enough.
Also, why do you call stripWhiteSpace()?

> LayoutTests/fast/forms/option-value-and-label.html:27
> -shouldBe('o1.label', '""');
> +shouldBe('o1.label', '"text"');
>  
>  var o2 = document.getElementById('o2');
>  shouldBe('o2.value', '"value"');
> -shouldBe('o2.label', '""');
> +shouldBe('o2.label', '"text"');

We need more test cases:
 - updating HTMLOptionElement::label
 - getting label when HTMLOptionElement::textContent has leading/trailing whitespaces

[Kent Tamura]

Comment on attachment 108454 [details]
Spec change handling

View in context: https://bugs.webkit.org/attachment.cgi?id=108454&action=review

> Source/WebCore/ChangeLog:4
> +        Implement the change in spec for option.label should be reflected like
> +        option.value

"like option.value" is unclear. Please write the behavior change concretely.

> Source/WebCore/ChangeLog:17
> +        * dom/OptionElement.cpp:
> +        (WebCore::OptionElement::normalizeText):
> +        (WebCore::OptionElement::collectOptionLabel):
> +        * dom/OptionElement.h:
> +        * html/HTMLOptionElement.cpp:
> +        (WebCore::HTMLOptionElement::label):
> +        (WebCore::HTMLOptionElement::setLabel):
> +        * html/HTMLOptionElement.h:
> +        * html/HTMLOptionElement.idl:

You had better write what is changed in each of functions.

[Sachin Puranik]

Created attachment 109001 [details]
updated patch - Incorporating review comments

[Sachin Puranik]

> why do you call stripWhiteSpace()?

Hi,
I am calling this as all the other browsers also return the striped string text value. Even webkit does the same thing for the options elements Value attribute.

[Darin Adler]

Comment on attachment 109001 [details]
updated patch - Incorporating review comments

View in context: https://bugs.webkit.org/attachment.cgi?id=109001&action=review

> Source/WebCore/html/HTMLOptionElement.cpp:224
> +    return collectOptionInnerText(this).stripWhiteSpace();

It's OK to strip whitespace, but you need to do it with the HTML definition of whitespace, not the Unicode definition. So you don’t want to use the String function stripWhiteSpace. You want to do more like what OptionalElement::normalizeText does or use the stripLeadingAndTrailingHTMLSpaces function from HTMLParserIdioms.h. It's also important to cover this in test cases, including at least one test case with Unicode whitespace that should not be stripped because it does not meet the HTML standard’s definition of a space.

Also, we should double check that this stripping requirement is clear in the specification.

[Sachin Puranik]

Created attachment 109284 [details]
updated patch - Incorporating review comments related to white space stripping

[WebKit Review Bot]

Attachment 109284 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1

Source/WebCore/html/HTMLOptionElement.cpp:228:  Should have a space between // and comment  [whitespace/comments] [4]
LayoutTests/ChangeLog:17:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
Total errors found: 2 in 13 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Sachin Puranik]

I also checked the spec at the link below.

http://www.whatwg.org/specs/web-apps/current-work/#dom-option-text

Spec says "The text IDL attribute, on getting, must return the value of the textContent IDL attribute on the element, with leading and trailing whitespace stripped, "

In our case we return "textContent IDL attribute" when label is not present. I think we should do the same thing what we do in case of text IDL attribute.

[Sachin Puranik]

Created attachment 109285 [details]
updated patch - Incorporating review comments related to white space stripping and fixing styling errors

updated patch - Incorporating review comments related to white space stripping and fixing styling errors

[Darin Adler]

Comment on attachment 109284 [details]
updated patch - Incorporating review comments related to white space stripping

View in context: https://bugs.webkit.org/attachment.cgi?id=109284&action=review

> Source/WebCore/html/HTMLOptionElement.cpp:225
> +    // Use the text if the Label wasn't set. Also strip leading and trailing html whitespace.

This comment says what we are doing, but not why. Comments need to say “why” or be omitted. The code already says what it does.

>> Source/WebCore/html/HTMLOptionElement.cpp:228
>> +    //we want to collapse our html whitespace within the text too.
> 
> Should have a space between // and comment  [whitespace/comments] [4]

Same problem. Another what comment.

[Darin Adler]

Comment on attachment 109285 [details]
updated patch - Incorporating review comments related to white space stripping and fixing styling errors

View in context: https://bugs.webkit.org/attachment.cgi?id=109285&action=review

commit-queue- because the comments still need a little work and the copyrights are a bit wrong too

> Source/WebCore/dom/OptionElement.cpp:3
> + * Portions Copyright (c) 2010 Motorola Mobility, Inc.  All rights reserved.

Should not add copyright since you are just re-ordering includes and removing a space in a comment.

> Source/WebCore/dom/OptionElement.h:3
> + * Portions Copyright (c) 2010 Motorola Mobility, Inc.  All rights reserved.

Doesn’t make sense to add copyright just for moving private down a line.

> Source/WebCore/html/HTMLOptionElement.cpp:8
> + * Portions Copyright (c) 2010 Motorola Mobility, Inc.  All rights reserved.

Would be better if your copyright matched the format of the others. Also, 2011 is the year of publication here, not 2010. Copyright should cite year of publication.

> Source/WebCore/html/HTMLOptionElement.cpp:225
> +    // Use the text if the Label wasn't set. Also strip leading and trailing html whitespace.

This comment says what we are doing, but not why. Comments need to say why we do things, not just repeat what the code does and say what we are doing.

Also, "label" should not be capitalized, "HTML" should be capitalized.

> Source/WebCore/html/HTMLOptionElement.cpp:228
> +    // we want to collapse our html whitespace within the text too.

This comment says what we are doing, but not why. Comments need to say why we do things, not just repeat what the code does and say what we are doing.

Also, "We" should be capitalized and "HTML" should be capitalized.

> Source/WebCore/html/HTMLOptionElement.h:7
> + * Portions Copyright (c) 2010 Motorola Mobility, Inc.  All rights reserved.

The year is 2011. Copyright dates are based on year of publication. Also I’m not sure that just adding a single-line function declaration justifies adding a copyright line here.

[Sachin Puranik]

Created attachment 109303 [details]
updated patch - removed the unnecessary comments from code.

[Sachin Puranik]

Created attachment 109305 [details]
incorporated all the comments related to copyrights.

Final Patch after changing the appropriate copyright information as per review comments. Also removed the unnecessary comments form the code. :)

[Darin Adler]

Comment on attachment 109305 [details]
incorporated all the comments related to copyrights.

View in context: https://bugs.webkit.org/attachment.cgi?id=109305&action=review

> Source/WebCore/html/HTMLOptionElement.idl:35
> -        attribute [Reflect] DOMString label;
> +        attribute [ConvertNullToNullString] DOMString label;

Is there a test that covers the [ConvertNullToNullString] behavior?

[WebKit Review Bot]

Comment on attachment 109305 [details]
incorporated all the comments related to copyrights.

Rejecting attachment 109305 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
302ab83511ddd2a57214c11d11b631afb27c4fda
r96405 = 7785babbe27465a907e4847bb05dd731fcabc3b3
Done rebuilding .git/svn/refs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc
First, rewinding head to replay your work on top of it...
Fast-forwarded master to refs/remotes/origin/master.
Updating chromium port dependencies using gclient...

________ running '/usr/bin/python gyp_webkit' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'
Updating webkit projects from gyp files...

Full output: http://queues.webkit.org/results/9900446

[Sachin Puranik]

Created attachment 109663 [details]
Because of one wrong line in the changelog - build bot rejected the patch , hence removed the line.

Hi,
This patch was done r+ and commit + earlir , but because of one wrong line in the patch , build bot rejected the final merge.Changed the patch again for merge.

[WebKit Review Bot]

Comment on attachment 109663 [details]
Because of one wrong line in the changelog - build bot rejected the patch , hence removed the line.

Rejecting attachment 109663 [details] from review queue.

[e-mail] does not have reviewer permissions according to http://trac.webkit.org/browser/trunk/Tools/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 Tools/Scripts/webkitpy/common/config/committers.py by adding yourself to the file (no review needed).  The commit-queue restarts itself every 2 hours.  After restart the commit-queue will correctly respect your reviewer rights.

[WebKit Review Bot]

Comment on attachment 109663 [details]
Because of one wrong line in the changelog - build bot rejected the patch , hence removed the line.

Rejecting attachment 109663 [details] from commit-queue.

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

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

- If you have committer rights please correct the error in Tools/Scripts/webkitpy/common/config/committers.py by adding yourself to the file (no review needed).  The commit-queue restarts itself every 2 hours.  After restart the commit-queue will correctly respect your committer rights.

[Sachin Puranik]

Created attachment 109665 [details]
Because of one wrong line in the changelog - build bot rejected the patch , hence removed the line.

[Sachin Puranik]

WebCore change log was wrong earlier
"Need a short description and bug URL (OOPS!)"

This line should be removed. This failed the patch from commit.

[WebKit Review Bot]

Comment on attachment 109665 [details]
Because of one wrong line in the changelog - build bot rejected the patch , hence removed the line. 

Attachment 109665 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9954151

New failing tests:
fast/forms/option-value-and-label-changed-by-js.html
fast/forms/option-label-trim-html-spaces.html

[Sachin Puranik]

Created attachment 109770 [details]
Fixed the layout test error.

[Darin Adler]

Comment on attachment 109770 [details]
Fixed the layout test error.

View in context: https://bugs.webkit.org/attachment.cgi?id=109770&action=review

> Source/WebCore/html/HTMLOptionElement.cpp:228
> +    label = collectOptionInnerText(this).stripWhiteSpace(isHTMLSpace);
> +    label = label.simplifyWhiteSpace(isHTMLSpace);
> +
> +    return label;

I think this would read better without the local variable:

    return collectOptionInnerText(this).stripWhiteSpace(isHTMLSpace).simplifyWhiteSpace(isHTMLSpace);

[WebKit Review Bot]

Comment on attachment 109770 [details]
Fixed the layout test error.

Clearing flags on attachment: 109770

Committed r96721: <http://trac.webkit.org/changeset/96721>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Sachin Puranik]

Thanks for quick review :)