The start and end arguments should be unsigned long in setSelectionRange() function

The start and end arguments should be unsigned long in setSelectionRange() function

as per specs @ https://html.spec.whatwg.org/#the-textarea-element

BUG=460722

[tanay.c, 2015-08-05 13:00:54]

tanay.c@samsung.com changed reviewers:
+ philipj@opera.com

[tanay.c, 2015-08-05 13:00:55]

PTAL

[philipj_slow, 2015-08-05 14:04:46]

The implementation still uses int, can you investigate the impact of making the
change throughout?

In HTMLTextFormControlElement::setSelectionRange there is some clamping which
should make it possible to write tests for this, previously passing -1 would
behave as if you passed 0, but after this change it will do something different.

[tanay.c, 2015-08-07 05:56:53]

On 2015/08/05 14:04:46, philipj wrote:
> The implementation still uses int, can you investigate the impact of making
the
> change throughout?
> 
> In HTMLTextFormControlElement::setSelectionRange there is some clamping which
> should make it possible to write tests for this, previously passing -1 would
> behave as if you passed 0, but after this change it will do something
different.

Thanks for the comments. 
With the implementation changes,  some existing tests results are effected. 
When -1 is passed the value defaults to unsigned maximum (4294967295 - after
unsigned int conversion) 
and this will change the range expected.
e.g 
start = -1 end = 5
after function call
start = 5 end = 5

If this is expected then the changed cases can be uploaded along with the
function signature
changes, in addition to IDL changes. Please suggest.

[philipj_slow, 2015-08-10 12:22:31]

On 2015/08/07 05:56:53, tanay.c wrote:
> On 2015/08/05 14:04:46, philipj wrote:
> > The implementation still uses int, can you investigate the impact of making
> the
> > change throughout?
> > 
> > In HTMLTextFormControlElement::setSelectionRange there is some clamping
which
> > should make it possible to write tests for this, previously passing -1 would
> > behave as if you passed 0, but after this change it will do something
> different.
> 
> Thanks for the comments. 
> With the implementation changes,  some existing tests results are effected. 
> When -1 is passed the value defaults to unsigned maximum (4294967295 - after
> unsigned int conversion) 
> and this will change the range expected.
> e.g 
> start = -1 end = 5
> after function call
> start = 5 end = 5
> 
> If this is expected then the changed cases can be uploaded along with the
> function signature
> changes, in addition to IDL changes. Please suggest.

The spec says "Arguments greater than the length of the value of the text field
must be treated as pointing at the end of the text field."

[philipj_slow, 2015-08-10 12:23:17]

More generally, just make sure that the behavior is exactly what the spec says
and that any change made is covered by an existing or new test case.

[tanay.c, 2015-08-12 12:50:05]

On 2015/08/10 12:23:17, philipj wrote:
> More generally, just make sure that the behavior is exactly what the spec says
> and that any change made is covered by an existing or new test case.

Uploaded a patch with the changes. Please take a look.

[philipj_slow, 2015-08-12 13:20:04]

https://codereview.chromium.org/1269263003/diff/120001/Source/core/html/HTMLT...
File Source/core/html/HTMLTextFormControlElement.cpp (left):

https://codereview.chromium.org/1269263003/diff/120001/Source/core/html/HTMLT...
Source/core/html/HTMLTextFormControlElement.cpp:354: const int editorValueLength
= static_cast<int>(innerEditorValue().length());
Making this unsigned now would make more sense.

https://codereview.chromium.org/1269263003/diff/120001/Source/core/html/HTMLT...
File Source/core/html/HTMLTextFormControlElement.cpp (right):

https://codereview.chromium.org/1269263003/diff/120001/Source/core/html/HTMLT...
Source/core/html/HTMLTextFormControlElement.cpp:374: ASSERT(start ==
(unsigned)indexForPosition(innerEditor, startPosition));
So indexForPosition() still returns int. It looks to me like it's int all the
way down here, for example the methods on Position return int and not unsigned.
Is there a clean cut that can be made at some point, without getting some
nonsensical behavior for the range INT_MAX..UINT_MAX?

[tanay.c, 2015-08-14 05:39:12]

Thanks for the comments. PTAL.

https://codereview.chromium.org/1269263003/diff/120001/Source/core/html/HTMLT...
File Source/core/html/HTMLTextFormControlElement.cpp (left):

https://codereview.chromium.org/1269263003/diff/120001/Source/core/html/HTMLT...
Source/core/html/HTMLTextFormControlElement.cpp:354: const int editorValueLength
= static_cast<int>(innerEditorValue().length());
On 2015/08/12 13:20:03, philipj wrote:
> Making this unsigned now would make more sense.

Done.

https://codereview.chromium.org/1269263003/diff/120001/Source/core/html/HTMLT...
File Source/core/html/HTMLTextFormControlElement.cpp (right):

https://codereview.chromium.org/1269263003/diff/120001/Source/core/html/HTMLT...
Source/core/html/HTMLTextFormControlElement.cpp:374: ASSERT(start ==
(unsigned)indexForPosition(innerEditor, startPosition));
On 2015/08/12 13:20:03, philipj wrote:
> So indexForPosition() still returns int. It looks to me like it's int all the
> way down here, for example the methods on Position return int and not
unsigned.
> Is there a clean cut that can be made at some point, without getting some
> nonsensical behavior for the range INT_MAX..UINT_MAX?

It may not be possible to find a clean cut boundary. In that case, it would be
better to change the caller and callee function definitions to unsigned. I will
investigate the same.

[philipj_slow, 2015-08-14 07:45:53]

As this goes deeper, it becomes harder to see where there may be
signed<->unsigned conversions. I think it would be very helpful to try to
compile everything with compiler warnings enabled for conversions, and make sure
that no new such warnings are introduced. Otherwise there's a big risk that the
observable behavior has changed but isn't actually more correct, as the invalid
conversion has just moved elsewhere.

https://codereview.chromium.org/1269263003/diff/160001/Source/core/html/HTMLT...
File Source/core/html/HTMLTextFormControlElement.cpp (right):

https://codereview.chromium.org/1269263003/diff/160001/Source/core/html/HTMLT...
Source/core/html/HTMLTextFormControlElement.cpp:336: index +=
std::min<unsigned>(length, passedPosition.offsetInContainerNode());
offsetInContainerNode() returns int. Maybe assign it to a local variable and
assert that it is >= 0?

https://codereview.chromium.org/1269263003/diff/160001/Source/core/html/HTMLT...
Source/core/html/HTMLTextFormControlElement.cpp:352: const unsigned
editorValueLength = static_cast<unsigned>(innerEditorValue().length());
String::length() is already unsigned.

https://codereview.chromium.org/1269263003/diff/160001/Source/core/html/HTMLT...
Source/core/html/HTMLTextFormControlElement.cpp:371: ASSERT(start ==
(unsigned)indexForPosition(innerEditor, startPosition));
Don't use C-style casts.

https://codereview.chromium.org/1269263003/diff/160001/Source/core/html/HTMLT...
File Source/core/html/HTMLTextFormControlElement.h (right):

https://codereview.chromium.org/1269263003/diff/160001/Source/core/html/HTMLT...
Source/core/html/HTMLTextFormControlElement.h:71: unsigned selectionStart()
const;
If you're changing this you should change it in the IDL as well, and make sure
that any internal calls to these handle them as unsigned or do some reasonable
conversion to int.

[tanay.c, 2015-08-17 12:06:32]

Thanks for the comments. However, on further analysis it looks like there will
be some changes which will effect both the chromium side and the blink side.
This is due to signature changes in the following functions:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
&
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

Hence, we might need to do this change in 5 steps:
1) Blink side with 2 functions 
          int afunc()
          unsigned afunc(bool unused_param)
2) Chromium side changes with afunc(unused_param)
3) Blink side changes 
          remove int afunc()
          add    unsigned afunc()
4) Chromium side changes 
          use unsigned afunc()
5) Blink side remove afunc(bool unsued_param)

Please let me know your comments.

https://codereview.chromium.org/1269263003/diff/160001/Source/core/html/HTMLT...
File Source/core/html/HTMLTextFormControlElement.cpp (right):

https://codereview.chromium.org/1269263003/diff/160001/Source/core/html/HTMLT...
Source/core/html/HTMLTextFormControlElement.cpp:352: const unsigned
editorValueLength = static_cast<unsigned>(innerEditorValue().length());
On 2015/08/14 07:45:52, philipj wrote:
> String::length() is already unsigned.

Done.

https://codereview.chromium.org/1269263003/diff/160001/Source/core/html/HTMLT...
Source/core/html/HTMLTextFormControlElement.cpp:371: ASSERT(start ==
(unsigned)indexForPosition(innerEditor, startPosition));
On 2015/08/14 07:45:52, philipj wrote:
> Don't use C-style casts.

Done.

https://codereview.chromium.org/1269263003/diff/160001/Source/core/html/HTMLT...
File Source/core/html/HTMLTextFormControlElement.h (right):

https://codereview.chromium.org/1269263003/diff/160001/Source/core/html/HTMLT...
Source/core/html/HTMLTextFormControlElement.h:71: unsigned selectionStart()
const;
On 2015/08/14 07:45:53, philipj wrote:
> If you're changing this you should change it in the IDL as well, and make sure
> that any internal calls to these handle them as unsigned or do some reasonable
> conversion to int.

Modified the IDL and the handled the internal calls. However, these changes will
require some of the exposed API signatures to change.

[philipj_slow, 2015-08-18 07:47:33]

On 2015/08/17 12:06:32, tanay.c wrote:
> Thanks for the comments. However, on further analysis it looks like there will
> be some changes which will effect both the chromium side and the blink side.
> This is due to signature changes in the following functions:
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> &
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> 
> Hence, we might need to do this change in 5 steps:
> 1) Blink side with 2 functions 
>           int afunc()
>           unsigned afunc(bool unused_param)
> 2) Chromium side changes with afunc(unused_param)
> 3) Blink side changes 
>           remove int afunc()
>           add    unsigned afunc()
> 4) Chromium side changes 
>           use unsigned afunc()
> 5) Blink side remove afunc(bool unsued_param)
> 
> Please let me know your comments.

Hmm, carrying this over to the Chromium side as well does not sound great.
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Integer_Types
says to not use unsigned, so the more it can be limited the better I think.

Did you try the compiler warning trick to see how much else code is affected? If
there is no clean cut to be made, it's probably best to just leave it as signed
everywhere.

[tanay.c, 2015-08-20 05:58:27]

On 2015/08/18 07:47:33, philipj wrote:
> On 2015/08/17 12:06:32, tanay.c wrote:
> > Thanks for the comments. However, on further analysis it looks like there
will
> > be some changes which will effect both the chromium side and the blink side.
> > This is due to signature changes in the following functions:
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> > &
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> > 
> > Hence, we might need to do this change in 5 steps:
> > 1) Blink side with 2 functions 
> >           int afunc()
> >           unsigned afunc(bool unused_param)
> > 2) Chromium side changes with afunc(unused_param)
> > 3) Blink side changes 
> >           remove int afunc()
> >           add    unsigned afunc()
> > 4) Chromium side changes 
> >           use unsigned afunc()
> > 5) Blink side remove afunc(bool unsued_param)
> > 
> > Please let me know your comments.
> 
> Hmm, carrying this over to the Chromium side as well does not sound great.
> http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Integer_Types
> says to not use unsigned, so the more it can be limited the better I think.
> 
> Did you try the compiler warning trick to see how much else code is affected?
If
> there is no clean cut to be made, it's probably best to just leave it as
signed
> everywhere.

I have tried with another patch which seems to crash test cases related to
Position offset in the bots. 
Even if compiler warnings are eliminated, it does not guarantee that all
implicit conversions
are handled. 
Issue is under discussion @ https://github.com/w3c/selection-api/issues/33
Awaiting further inputs.