Do not create child node for Option constructor with no arguments/undefined text

Do not create child node for Option constructor with no arguments/undefined/empty text.

Avoid Option constructor to append child, if its having
no arguments/undefined/empty text.

spec:
https://html.spec.whatwg.org/#the-option-element

BUG=723006
Review-Url: https://codereview.chromium.org/2891723002
Cr-Commit-Position: refs/heads/master@{#473523}
Committed: https://chromium.googlesource.com/chromium/src/+/850a8677a6589a4c223c4a05793435e70109fd55

[Shanmuga Pandi, 2017-05-17 09:17:58]

shanmuga.m@samsung.com changed reviewers:
+ foolip@chromium.org, fs@opera.com, tkent@chromium.org

[Shanmuga Pandi, 2017-05-17 09:17:59]

PTAL!!

Thanks

[Shanmuga Pandi, 2017-05-17 09:18:12]

The CQ bit was checked by shanmuga.m@samsung.com to run a CQ dry run

[Shanmuga Pandi, 2017-05-17 09:18:26]

Description was changed from

==========
Do not create child node for Option constructor with no arguments/undefined text

Avoid Option constuctor appending child, if its having
no arguments/undefined text.

BUG=723006
==========

to

==========
Do not create child node for Option constructor with no arguments/undefined
text.

Avoid Option constuctor appending child, if its having
no arguments/undefined text.

BUG=723006
==========

[Shanmuga Pandi, 2017-05-17 09:18:46]

Description was changed from

==========
Do not create child node for Option constructor with no arguments/undefined
text.

Avoid Option constuctor appending child, if its having
no arguments/undefined text.

BUG=723006
==========

to

==========
Do not create child node for Option constructor with no arguments/undefined
text.

Avoid Option constuctor to append child, if its having
no arguments/undefined text.

BUG=723006
==========

[commit-bot: I haz the power, 2017-05-17 09:18:55]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Shanmuga Pandi, 2017-05-17 09:18:59]

Description was changed from

==========
Do not create child node for Option constructor with no arguments/undefined
text.

Avoid Option constuctor to append child, if its having
no arguments/undefined text.

BUG=723006
==========

to

==========
Do not create child node for Option constructor with no arguments/undefined
text.

Avoid Option constructor to append child, if its having
no arguments/undefined text.

BUG=723006
==========

[commit-bot: I haz the power, 2017-05-17 11:18:39]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-17 11:18:40]

Dry run: This issue passed the CQ dry run.

[foolip, 2017-05-17 11:23:12]

https://codereview.chromium.org/2891723002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/html/HTMLOptionElement.cpp (right):

https://codereview.chromium.org/2891723002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/HTMLOptionElement.cpp:75: if
(!data.IsNull())
The spec says "If text is not the empty string, then append to option a new Text
node whose data is text."

This only handles the argument being omitted, not when it is the empty string,
so it should still be possible to write a per-spec test that fails.

I think that the right thing here is to:
1. Import the test from wpt (or wait for an automatic import)
2. Use data.IsEmpty() here and in the same CL update the test to cover that
case.

[Shanmuga Pandi, 2017-05-17 11:29:36]

https://codereview.chromium.org/2891723002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/html/HTMLOptionElement.cpp (right):

https://codereview.chromium.org/2891723002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/HTMLOptionElement.cpp:75: if
(!data.IsNull())
On 2017/05/17 11:23:12, foolip wrote:
> The spec says "If text is not the empty string, then append to option a new
Text
> node whose data is text."
> 
> This only handles the argument being omitted, not when it is the empty string,
> so it should still be possible to write a per-spec test that fails.
> 
> I think that the right thing here is to:
> 1. Import the test from wpt (or wait for an automatic import)
> 2. Use data.IsEmpty() here and in the same CL update the test to cover that
> case.

I understood. I will wait for the automatic import. I am not sure about how to
import test from wpt.

[Shanmuga Pandi, 2017-05-18 05:47:58]

> 2. Use data.IsEmpty() here and in the same CL update the test to cover that
> case.

 @foolip,
   Can we add the test case in same imported file in
external/<>/option-element-constructor.html after importing ? or will create a
new test file for that test case ?

  Thanks.

[foolip, 2017-05-18 21:03:00]

On 2017/05/18 05:47:58, Shanmuga Pandi wrote:
> > 2. Use data.IsEmpty() here and in the same CL update the test to cover that
> > case.
> 
>  @foolip,
>    Can we add the test case in same imported file in
> external/<>/option-element-constructor.html after importing ? or will create a
> new test file for that test case ?
> 
>   Thanks.

Yep, you can edit the imported files directly, and the changes will be
upstreamed to web-platform-tests automatically.

[tkent, 2017-05-22 00:03:15]

The test was imported.  Please rebase.

[Shanmuga Pandi, 2017-05-22 05:55:26]

Description was changed from

==========
Do not create child node for Option constructor with no arguments/undefined
text.

Avoid Option constructor to append child, if its having
no arguments/undefined text.

BUG=723006
==========

to

==========
Do not create child node for Option constructor with no
arguments/undefined/empty text.

Avoid Option constructor to append child, if its having
no arguments/undefined/empty text.

BUG=723006
==========

[Shanmuga Pandi, 2017-05-22 05:55:51]

Description was changed from

==========
Do not create child node for Option constructor with no
arguments/undefined/empty text.

Avoid Option constructor to append child, if its having
no arguments/undefined/empty text.

BUG=723006
==========

to

==========
Do not create child node for Option constructor with no
arguments/undefined/empty text.

Avoid Option constructor to append child, if its having
no arguments/undefined/empty text.

spec:
https://html.spec.whatwg.org/#the-option-element

BUG=723006
==========

[Shanmuga Pandi, 2017-05-22 05:58:35]

On 2017/05/22 00:03:15, tkent wrote:
> The test was imported.  Please rebase.

Rebased and updated the test.

[tkent, 2017-05-22 06:06:11]

The CQ bit was checked by tkent@chromium.org

[tkent, 2017-05-22 06:06:11]

lgtm

[commit-bot: I haz the power, 2017-05-22 06:06:27]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-22 08:08:50]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1495433171074990,
"parent_rev": "c9f9768b2d76c8f09ffb715f5ba6eaa67ee243a7", "commit_rev":
"850a8677a6589a4c223c4a05793435e70109fd55"}

[commit-bot: I haz the power, 2017-05-22 08:09:02]

Description was changed from

==========
Do not create child node for Option constructor with no
arguments/undefined/empty text.

Avoid Option constructor to append child, if its having
no arguments/undefined/empty text.

spec:
https://html.spec.whatwg.org/#the-option-element

BUG=723006
==========

to

==========
Do not create child node for Option constructor with no
arguments/undefined/empty text.

Avoid Option constructor to append child, if its having
no arguments/undefined/empty text.

spec:
https://html.spec.whatwg.org/#the-option-element

BUG=723006

Review-Url: https://codereview.chromium.org/2891723002
Cr-Commit-Position: refs/heads/master@{#473523}
Committed:
https://chromium.googlesource.com/chromium/src/+/850a8677a6589a4c223c4a057934...
==========

[commit-bot: I haz the power, 2017-05-22 08:09:03]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/850a8677a6589a4c223c4a057934...