[bindings] Avoid using custom binding for HTMLOptionsCollection.length attribute

[bindings] Avoid using custom binding for HTMLOptionsCollection.length attribute

As per the specification, https://html.spec.whatwg.org/#the-htmloptionscollection-interface,
there is no exception throwing mechanism defined when negative values are passed. In such case
we should just ignore the values and avoid using the custom binding for such usage.

Trying to set any value greater than 10000 will be ignored with the existing
length unchanged.

BUG=345519
R=haraken@chromium.org

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

[vivekg, 2015-08-18 10:25:09]

vivekg@chromium.org changed reviewers:
+ vivekg@chromium.org

[vivekg, 2015-08-18 10:25:09]

PTAL thanks.

[davve, 2015-08-18 12:04:29]

davve@opera.com changed reviewers:
+ davve@opera.com

[davve, 2015-08-18 12:04:30]

https://codereview.chromium.org/1292653003/diff/20001/Source/core/html/HTMLOp...
File Source/core/html/HTMLOptionsCollection.cpp (right):

https://codereview.chromium.org/1292653003/diff/20001/Source/core/html/HTMLOp...
Source/core/html/HTMLOptionsCollection.cpp:94: if (length > 0x7fffffffu)
Will this mean we'll get the following behavior?

'.options.length = a' with a in [0,10000] will set length to 'a'.
'.options.length = a' with a in [10001,2147483647] will set length to 10000*.
'.options.length = a' with a in [2147483648,inf) will leave length unchanged.

Would it be worthwhile to align the two latter cases somehow?

*Since we have the maxSelectItems limit in HTMLSelectElement.cpp

[vivekg, 2015-08-18 12:30:33]

https://codereview.chromium.org/1292653003/diff/20001/Source/core/html/HTMLOp...
File Source/core/html/HTMLOptionsCollection.cpp (right):

https://codereview.chromium.org/1292653003/diff/20001/Source/core/html/HTMLOp...
Source/core/html/HTMLOptionsCollection.cpp:94: if (length > 0x7fffffffu)
On 2015/08/18 at 12:04:30, David Vest wrote:
> Will this mean we'll get the following behavior?
> 
> '.options.length = a' with a in [0,10000] will set length to 'a'.
> '.options.length = a' with a in [10001,2147483647] will set length to 10000*.
> '.options.length = a' with a in [2147483648,inf) will leave length unchanged.
> 
> Would it be worthwhile to align the two latter cases somehow?
> 

Yes makes sense. So the simplified condition would be just:

if (length > maxSelectItems)
   return;

> *Since we have the maxSelectItems limit in HTMLSelectElement.cpp

[vivekg, 2015-08-18 13:16:49]

Done the changes in the latest patch. PTAL.

[haraken, 2015-08-18 13:27:12]

haraken@chromium.org changed reviewers:
+ tkent@chromium.org

[haraken, 2015-08-18 13:27:12]

LGTM

I want to have tkent take another look at the behavior change.

https://codereview.chromium.org/1292653003/diff/40001/Source/core/html/HTMLOp...
File Source/core/html/HTMLOptionsCollection.cpp (right):

https://codereview.chromium.org/1292653003/diff/40001/Source/core/html/HTMLOp...
Source/core/html/HTMLOptionsCollection.cpp:94: if (length >
HTMLSelectElement::maxSelectItems)

Can we add a test case for this branch?

[vivekg, 2015-08-18 13:30:44]

On 2015/08/18 at 13:27:12, haraken wrote:
> LGTM
> 
> I want to have tkent take another look at the behavior change.
> 
>
https://codereview.chromium.org/1292653003/diff/40001/Source/core/html/HTMLOp...
> File Source/core/html/HTMLOptionsCollection.cpp (right):
> 
>
https://codereview.chromium.org/1292653003/diff/40001/Source/core/html/HTMLOp...
> Source/core/html/HTMLOptionsCollection.cpp:94: if (length >
HTMLSelectElement::maxSelectItems)
> 
> Can we add a test case for this branch?

Setting options.length = -1 (translated to 2147483647) kind of covers this
branch although we can add a specific case i.e. options.length = 10001. WDYT?

[haraken, 2015-08-18 13:34:06]

On 2015/08/18 13:30:44, vivekg_ wrote:
> On 2015/08/18 at 13:27:12, haraken wrote:
> > LGTM
> > 
> > I want to have tkent take another look at the behavior change.
> > 
> >
>
https://codereview.chromium.org/1292653003/diff/40001/Source/core/html/HTMLOp...
> > File Source/core/html/HTMLOptionsCollection.cpp (right):
> > 
> >
>
https://codereview.chromium.org/1292653003/diff/40001/Source/core/html/HTMLOp...
> > Source/core/html/HTMLOptionsCollection.cpp:94: if (length >
> HTMLSelectElement::maxSelectItems)
> > 
> > Can we add a test case for this branch?
> 
> Setting options.length = -1 (translated to 2147483647) kind of covers this
> branch although we can add a specific case i.e. options.length = 10001. WDYT?

Sounds nice to have.

[vivekg, 2015-08-18 13:46:52]

On 2015/08/18 at 13:34:06, haraken wrote:
> On 2015/08/18 13:30:44, vivekg_ wrote:
> > On 2015/08/18 at 13:27:12, haraken wrote:
> > > LGTM
> > > 
> > > I want to have tkent take another look at the behavior change.
> > > 
> > >
> >
https://codereview.chromium.org/1292653003/diff/40001/Source/core/html/HTMLOp...
> > > File Source/core/html/HTMLOptionsCollection.cpp (right):
> > > 
> > >
> >
https://codereview.chromium.org/1292653003/diff/40001/Source/core/html/HTMLOp...
> > > Source/core/html/HTMLOptionsCollection.cpp:94: if (length >
> > HTMLSelectElement::maxSelectItems)
> > > 
> > > Can we add a test case for this branch?
> > 
> > Setting options.length = -1 (translated to 2147483647) kind of covers this
> > branch although we can add a specific case i.e. options.length = 10001.
WDYT?
> 
> Sounds nice to have.

Done.

[tkent, 2015-08-19 01:20:24]

https://codereview.chromium.org/1292653003/diff/60001/Source/core/html/HTMLOp...
File Source/core/html/HTMLOptionsCollection.cpp (right):

https://codereview.chromium.org/1292653003/diff/60001/Source/core/html/HTMLOp...
Source/core/html/HTMLOptionsCollection.cpp:95: return;
It's inconsistent with the current behavior of HTMLSelectElement::length setter,
which clamps on the value to maxSelectItems.

Does Firefox still have the limitation of the number of items?  If so, how does
it work for length setters?

[vivekg, 2015-08-19 01:53:35]

On 2015/08/19 01:20:24, tkent wrote:
>
https://codereview.chromium.org/1292653003/diff/60001/Source/core/html/HTMLOp...
> File Source/core/html/HTMLOptionsCollection.cpp (right):
> 
>
https://codereview.chromium.org/1292653003/diff/60001/Source/core/html/HTMLOp...
> Source/core/html/HTMLOptionsCollection.cpp:95: return;
> It's inconsistent with the current behavior of HTMLSelectElement::length
setter,
> which clamps on the value to maxSelectItems.
> 
> Does Firefox still have the limitation of the number of items?  If so, how
does
> it work for length setters?

Yes the limitation is still valid in FF. I saw this inconsistent behavior only
in Chrome whereas FF doesn't clamp to max value when given value is greater than
the max value.

[tkent, 2015-08-19 05:09:59]

On 2015/08/19 01:53:35, vivekg_ wrote:
> > Does Firefox still have the limitation of the number of items?  If so, how
> does
> > it work for length setters?
> 
> Yes the limitation is still valid in FF. I saw this inconsistent behavior only
> in Chrome whereas FF doesn't clamp to max value when given value is greater
than
> the max value.

Firefox behaves so for both of HTMLOptionCollection length setter and
HTMLSelectElement length setter, right?
Then, let's change the behavior of our HTMLSelectElement length setter too.

[vivekg, 2015-08-19 06:03:07]

On 2015/08/19 at 05:09:59, tkent wrote:
> On 2015/08/19 01:53:35, vivekg_ wrote:
> > > Does Firefox still have the limitation of the number of items?  If so, how
> > does
> > > it work for length setters?
> > 
> > Yes the limitation is still valid in FF. I saw this inconsistent behavior
only
> > in Chrome whereas FF doesn't clamp to max value when given value is greater
than
> > the max value.
> 
> Firefox behaves so for both of HTMLOptionCollection length setter and
HTMLSelectElement length setter, right?
> Then, let's change the behavior of our HTMLSelectElement length setter too.

FF behaves differently. For HTMLOptionCollection, it doesn't clamp the value and
ignores it without any exception. 
Whereas for HTMLSelectElement, it throws the exception as: "NotSupportedError:
Operation is not supported"

Sample JSFiddle here http://jsfiddle.net/vp40rxdk/show/ has the test case for
setting HTMLSelectElement's length attribute greater than 10000.

And here are the results

Chrome:
Test 1 10000
Test 2 10000

FF:
Test 1 10000
Operation is not supported

Opera(pre chromium):
Test 1 10000
Test 2 10001

IE (older version 9):
Test 1 10000 
Test 2 10001


As the results are not consistent, what should be our stand?

[tkent, 2015-08-19 06:45:02]

On 2015/08/19 06:03:07, vivekg_ wrote:
> FF behaves differently. For HTMLOptionCollection, it doesn't clamp the value
and
> ignores it without any exception. 
> Whereas for HTMLSelectElement, it throws the exception as: "NotSupportedError:
> Operation is not supported"
> 
> Sample JSFiddle here http://jsfiddle.net/vp40rxdk/show/ has the test case for
> setting HTMLSelectElement's length attribute greater than 10000.
> 
> And here are the results
> 
> Chrome:
> Test 1 10000
> Test 2 10000
> 
> FF:
> Test 1 10000
> Operation is not supported
> 
> Opera(pre chromium):
> Test 1 10000
> Test 2 10001
> 
> IE (older version 9):
> Test 1 10000 
> Test 2 10001
> 
> 
> As the results are not consistent, what should be our stand?

Thank you for the investigation.
I thought following FIrefox behavior was nice for interoperability, but the
Firefox behavior is not acceptable because the specification says
HTMLSelectElement length is equivalent to HTMLOptionsCollection length.
IMO, we should ignore length value greater than 10000 in both of
HTMLOptionsCollection length and HTMLSelectElement length.  So we need to update
only HTMLSelectElement::setLength().

[vivekg, 2015-08-19 06:53:35]

On 2015/08/19 at 06:45:02, tkent wrote:
> On 2015/08/19 06:03:07, vivekg_ wrote:
> > FF behaves differently. For HTMLOptionCollection, it doesn't clamp the value
and
> > ignores it without any exception. 
> > Whereas for HTMLSelectElement, it throws the exception as:
"NotSupportedError:
> > Operation is not supported"
> > 
> > Sample JSFiddle here http://jsfiddle.net/vp40rxdk/show/ has the test case
for
> > setting HTMLSelectElement's length attribute greater than 10000.
> > 
> > And here are the results
> > 
> > Chrome:
> > Test 1 10000
> > Test 2 10000
> > 
> > FF:
> > Test 1 10000
> > Operation is not supported
> > 
> > Opera(pre chromium):
> > Test 1 10000
> > Test 2 10001
> > 
> > IE (older version 9):
> > Test 1 10000 
> > Test 2 10001
> > 
> > 
> > As the results are not consistent, what should be our stand?
> 
> Thank you for the investigation.
> I thought following FIrefox behavior was nice for interoperability, but the
Firefox behavior is not acceptable because the specification says
HTMLSelectElement length is equivalent to HTMLOptionsCollection length.
> IMO, we should ignore length value greater than 10000 in both of
HTMLOptionsCollection length and HTMLSelectElement length.  So we need to update
only HTMLSelectElement::setLength().

Done. PTAL thanks!

[tkent, 2015-08-19 09:04:42]

The CQ bit was checked by tkent@chromium.org

[tkent, 2015-08-19 09:04:42]

lgtm

[tkent, 2015-08-19 09:04:42]

The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org
Link to the patchset: https://codereview.chromium.org/1292653003/#ps100001
(title: "Missed to fix the test case!")

[commit-bot: I haz the power, 2015-08-19 09:04:55]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1292653003/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1292653003/100001

[davve, 2015-08-19 09:58:35]

On 2015/08/19 06:45:02, tkent wrote:

> Thank you for the investigation.
> I thought following FIrefox behavior was nice for interoperability, but the
> Firefox behavior is not acceptable because the specification says
> HTMLSelectElement length is equivalent to HTMLOptionsCollection length.
> IMO, we should ignore length value greater than 10000 in both of
> HTMLOptionsCollection length and HTMLSelectElement length.  So we need to
update
> only HTMLSelectElement::setLength().

What do you think about the discrepancy between setting length and using the
setter?

For example, with patch set #6,

<select></select>
<script>
  var select = document.querySelector('select');
  select.options.length = 15000;
  // still no options.
  select.options[15000] = document.createElement('option');
  // Clamped to 10000 options.
</script>

since there is still clamping in HTMLSelectElement::setOption().

[tkent, 2015-08-19 10:14:42]

On 2015/08/19 09:58:35, David Vest wrote:
> What do you think about the discrepancy between setting length and using the
> setter?
> 
> For example, with patch set #6,
> 
> <select></select>
> <script>
>   var select = document.querySelector('select');
>   select.options.length = 15000;
>   // still no options.
>   select.options[15000] = document.createElement('option');
>   // Clamped to 10000 options.
> </script>
> 
> since there is still clamping in HTMLSelectElement::setOption().

Anonymous index setters should have the consistent behavior.  We should update
HTMLSelectElement::setOption().
We might want to show console warnings when ignoring large index.

[commit-bot: I haz the power, 2015-08-19 10:34:08]

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

[commit-bot: I haz the power, 2015-08-19 10:34:08]

Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[vivekg, 2015-08-19 10:35:49]

On 2015/08/19 at 10:14:42, tkent wrote:
> On 2015/08/19 09:58:35, David Vest wrote:
> > What do you think about the discrepancy between setting length and using the
> > setter?
> > 
> > For example, with patch set #6,
> > 
> > <select></select>
> > <script>
> >   var select = document.querySelector('select');
> >   select.options.length = 15000;
> >   // still no options.
> >   select.options[15000] = document.createElement('option');
> >   // Clamped to 10000 options.
> > </script>
> > 
> > since there is still clamping in HTMLSelectElement::setOption().
> 
> Anonymous index setters should have the consistent behavior.  We should update
HTMLSelectElement::setOption().
> We might want to show console warnings when ignoring large index.

Should we take that as a separate patch? WDYT?

[tkent, 2015-08-19 10:45:21]

On 2015/08/19 10:35:49, vivekg_ wrote:
> Should we take that as a separate patch? WDYT?

A separated patch is ok.  The main purpose of this CL is to remove custom
binding code :)

BTW, the CL description should mention the behavior change.

[vivekg, 2015-08-19 10:48:40]

On 2015/08/19 at 10:45:21, tkent wrote:
> On 2015/08/19 10:35:49, vivekg_ wrote:
> > Should we take that as a separate patch? WDYT?
> 
> A separated patch is ok.  The main purpose of this CL is to remove custom
binding code :)
> 
> BTW, the CL description should mention the behavior change.

Done.

[vivekg, 2015-08-19 11:03:07]

The CQ bit was checked by vivekg@chromium.org

[commit-bot: I haz the power, 2015-08-19 11:03:25]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1292653003/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1292653003/100001

[commit-bot: I haz the power, 2015-08-19 12:22:08]

Committed patchset #6 (id:100001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=200819

[tkent, 2015-08-20 05:16:17]

On 2015/08/19 10:45:21, tkent wrote:
> On 2015/08/19 10:35:49, vivekg_ wrote:
> > Should we take that as a separate patch? WDYT?
> 
> A separated patch is ok.  The main purpose of this CL is to remove custom
> binding code :)

I filed a bug for this. 
https://code.google.com/p/chromium/issues/detail?id=522802