HTMLOptionsCollection remove() argument should be long, not unsigned long

HTMLOptionsCollection remove() argument should be long, not unsigned long

As per spec link: 
https://html.spec.whatwg.org/#the-htmloptionscollection-interface,
HTMLOptionsCollection remove() api argument should be
long, not unsigned long

This change cannot be tested since, we can only test with a
small number of options (certainly less than two billion)
and the difference is only observable around INT_MAX and
INT_MIN, also return value of remove() is always void, it
does not throw any exception. Implementation for remove()
is already using int.

BUG=496400
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=198149

[shiva.jm, 2015-06-29 08:39:09]

shiva.jm@samsung.com changed reviewers:
+ pdr@chromium.org, philipj@opera.com

[shiva.jm, 2015-06-29 08:39:23]

pls have a look.

[philipj_slow, 2015-06-29 09:21:44]

It looks like you accidentally removed the title "HTMLOptionsCollection remove()
api argument should be long, not unsigned long" from the description. When you
do this, the resulting commit will not have the title. (I know because I've done
it myself, oops.)

[shiva.jm, 2015-06-29 09:29:10]

On 2015/06/29 09:21:44, philipj wrote:
> It looks like you accidentally removed the title "HTMLOptionsCollection
remove()
> api argument should be long, not unsigned long" from the description. When you
> do this, the resulting commit will not have the title. (I know because I've
done
> it myself, oops.)

Added the Title in description again.

[philipj_slow, 2015-06-29 09:33:05]

Please also link to the tracking bug for signed/unsigned changes with BUG= in
the description.

https://codereview.chromium.org/1216933002/diff/1/LayoutTests/fast/js/resourc...
File LayoutTests/fast/js/resources/select-options-remove.js (right):

https://codereview.chromium.org/1216933002/diff/1/LayoutTests/fast/js/resourc...
LayoutTests/fast/js/resources/select-options-remove.js:245:
shouldBe("select2.options.remove(-2147483649)", "undefined");
Checking the return value of remove() doesn't seem meaningful, as it's always
void.

Is it possible to write a test that would fail with the old code and pass with
the new? It seems to me that there isn't, because we can only test with a small
number of options (certainly less than two billion) and the difference is only
observable around INT_MAX and INT_MIN. Can you confirm?

[shiva.jm, 2015-06-29 11:00:00]

On 2015/06/29 09:33:05, philipj wrote:
> Please also link to the tracking bug for signed/unsigned changes with BUG= in
> the description.
> 
>
https://codereview.chromium.org/1216933002/diff/1/LayoutTests/fast/js/resourc...
> File LayoutTests/fast/js/resources/select-options-remove.js (right):
> 
>
https://codereview.chromium.org/1216933002/diff/1/LayoutTests/fast/js/resourc...
> LayoutTests/fast/js/resources/select-options-remove.js:245:
> shouldBe("select2.options.remove(-2147483649)", "undefined");
> Checking the return value of remove() doesn't seem meaningful, as it's always
> void.
> 
> Is it possible to write a test that would fail with the old code and pass with
> the new? It seems to me that there isn't, because we can only test with a
small
> number of options (certainly less than two billion) and the difference is only
> observable around INT_MAX and INT_MIN. Can you confirm?

Updated the Bug= to track to master bug link, yes i think test is not possible
to check both old and new changes.
We can try using idlharness.js to check, but it asserts.
Gecko also suppourts long, but it throws exception also, Webkit still not
updated.
shall we remove the tests and push new patch ?

[philipj_slow, 2015-06-29 14:44:36]

On 2015/06/29 11:00:00, shiva.jm wrote:
> On 2015/06/29 09:33:05, philipj wrote:
> > Please also link to the tracking bug for signed/unsigned changes with BUG=
in
> > the description.
> > 
> >
>
https://codereview.chromium.org/1216933002/diff/1/LayoutTests/fast/js/resourc...
> > File LayoutTests/fast/js/resources/select-options-remove.js (right):
> > 
> >
>
https://codereview.chromium.org/1216933002/diff/1/LayoutTests/fast/js/resourc...
> > LayoutTests/fast/js/resources/select-options-remove.js:245:
> > shouldBe("select2.options.remove(-2147483649)", "undefined");
> > Checking the return value of remove() doesn't seem meaningful, as it's
always
> > void.
> > 
> > Is it possible to write a test that would fail with the old code and pass
with
> > the new? It seems to me that there isn't, because we can only test with a
> small
> > number of options (certainly less than two billion) and the difference is
only
> > observable around INT_MAX and INT_MIN. Can you confirm?
> 
> Updated the Bug= to track to master bug link, yes i think test is not possible
> to check both old and new changes.
> We can try using idlharness.js to check, but it asserts.
> Gecko also suppourts long, but it throws exception also, Webkit still not
> updated.
> shall we remove the tests and push new patch ?

Yes, I think just reverting the tests and explaining precisely why this cannot
be tested in the description would be OK. Also say that the implementation
already uses int.

[shiva.jm, 2015-06-30 05:33:23]

Updated the review comments and description, pls have a look.

https://codereview.chromium.org/1216933002/diff/1/LayoutTests/fast/js/resourc...
File LayoutTests/fast/js/resources/select-options-remove.js (right):

https://codereview.chromium.org/1216933002/diff/1/LayoutTests/fast/js/resourc...
LayoutTests/fast/js/resources/select-options-remove.js:245:
shouldBe("select2.options.remove(-2147483649)", "undefined");
On 2015/06/29 09:33:05, philipj wrote:
> Checking the return value of remove() doesn't seem meaningful, as it's always
> void.
> 
> Is it possible to write a test that would fail with the old code and pass with
> the new? It seems to me that there isn't, because we can only test with a
small
> number of options (certainly less than two billion) and the difference is only
> observable around INT_MAX and INT_MIN. Can you confirm?

Done.

[shiva.jm, 2015-07-01 12:02:54]

On 2015/06/29 14:44:36, philipj wrote:
> On 2015/06/29 11:00:00, shiva.jm wrote:
> > On 2015/06/29 09:33:05, philipj wrote:
> > > Please also link to the tracking bug for signed/unsigned changes with BUG=
> in
> > > the description.
> > > 
> > >
> >
>
https://codereview.chromium.org/1216933002/diff/1/LayoutTests/fast/js/resourc...
> > > File LayoutTests/fast/js/resources/select-options-remove.js (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1216933002/diff/1/LayoutTests/fast/js/resourc...
> > > LayoutTests/fast/js/resources/select-options-remove.js:245:
> > > shouldBe("select2.options.remove(-2147483649)", "undefined");
> > > Checking the return value of remove() doesn't seem meaningful, as it's
> always
> > > void.
> > > 
> > > Is it possible to write a test that would fail with the old code and pass
> with
> > > the new? It seems to me that there isn't, because we can only test with a
> > small
> > > number of options (certainly less than two billion) and the difference is
> only
> > > observable around INT_MAX and INT_MIN. Can you confirm?
> > 
> > Updated the Bug= to track to master bug link, yes i think test is not
possible
> > to check both old and new changes.
> > We can try using idlharness.js to check, but it asserts.
> > Gecko also suppourts long, but it throws exception also, Webkit still not
> > updated.
> > shall we remove the tests and push new patch ?
> 
> Yes, I think just reverting the tests and explaining precisely why this cannot
> be tested in the description would be OK. Also say that the implementation
> already uses int.

Updated the description for test behaviour and implementation.

[philipj_slow, 2015-07-01 14:47:32]

The CQ bit was checked by philipj@opera.com

[philipj_slow, 2015-07-01 14:47:32]

lgtm

[commit-bot: I haz the power, 2015-07-01 14:47:37]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1216933002/40001

[commit-bot: I haz the power, 2015-07-01 15:47:44]

Committed patchset #3 (id:40001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=198149