Range: node offsets should be unsigned.

Range: node offsets should be unsigned.

BUG=658716
Review-Url: https://codereview.chromium.org/2701413003
Cr-Commit-Position: refs/heads/master@{#451739}
Committed: https://chromium.googlesource.com/chromium/src/+/f5558c7534d4d34c0848d692432e7f0a919eb247

[tkent, 2017-02-21 05:08:55]

The CQ bit was checked by tkent@chromium.org to run a CQ dry run

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

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tkent, 2017-02-21 06:37:21]

Patchset #2 (id:20001) has been deleted

[tkent, 2017-02-21 06:37:25]

Patchset #1 (id:1) has been deleted

[tkent, 2017-02-21 06:37:38]

The CQ bit was checked by tkent@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-21 06:38:03]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tkent, 2017-02-21 06:43:57]

tkent@chromium.org changed reviewers:
+ yoichio@chromium.org, yosin@chromium.org

[tkent, 2017-02-21 06:43:58]

yosin@, yoichio@, would you review this please?

[yosin_UTC9, 2017-02-21 06:55:04]

lgtm

Thanks!

[tkent, 2017-02-21 07:52:22]

The CQ bit was unchecked by tkent@chromium.org

[tkent, 2017-02-21 07:52:25]

The CQ bit was checked by tkent@chromium.org

[commit-bot: I haz the power, 2017-02-21 07:52:57]

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

[tkent, 2017-02-21 08:14:37]

The CQ bit was unchecked by tkent@chromium.org

[tkent, 2017-02-21 08:15:55]

The CQ bit was checked by tkent@chromium.org

[tkent, 2017-02-21 08:15:55]

The patchset sent to the CQ was uploaded after l-g-t-m from yosin@chromium.org
Link to the patchset: https://codereview.chromium.org/2701413003/#ps60001
(title: "Resolve std::numeric_limits<int>::max() leftover")

[commit-bot: I haz the power, 2017-02-21 08:16:24]

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-02-21 10:50:04]

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

[commit-bot: I haz the power, 2017-02-21 10:50:05]

Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[tkent, 2017-02-21 11:48:44]

The CQ bit was checked by tkent@chromium.org

[commit-bot: I haz the power, 2017-02-21 11:48:59]

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-02-21 12:19:42]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1487677724595040,
"parent_rev": "533f8432f864440ea23933591911b93390f1130d", "commit_rev":
"f5558c7534d4d34c0848d692432e7f0a919eb247"}

[commit-bot: I haz the power, 2017-02-21 12:20:22]

Description was changed from

==========
Range: node offsets should be unsigned.

BUG=658716
==========

to

==========
Range: node offsets should be unsigned.

BUG=658716

Review-Url: https://codereview.chromium.org/2701413003
Cr-Commit-Position: refs/heads/master@{#451739}
Committed:
https://chromium.googlesource.com/chromium/src/+/f5558c7534d4d34c0848d692432e...
==========

[commit-bot: I haz the power, 2017-02-21 12:20:23]

Committed patchset #2 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/f5558c7534d4d34c0848d692432e...

[Sunny, 2017-02-21 12:32:04]

ratsunny@gmail.com changed reviewers:
+ ratsunny@gmail.com

[Sunny, 2017-02-21 12:32:05]

https://codereview.chromium.org/2701413003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/RangeBoundaryPoint.h (right):

https://codereview.chromium.org/2701413003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/RangeBoundaryPoint.h:73: static const
unsigned invalidOffset = static_cast<unsigned>(-1);
Hi tkent@, I've tried working on this issue before and have a question.

In this file do you think it's acceptable to cast -1 to unsigned as a indicator
of invalid offset?

My solution is use a boolean as indicator:
https://codereview.chromium.org/2673303002/diff/20001/third_party/WebKit/Sour...

Do you think it's unnecessary?

[tkent, 2017-02-22 02:08:43]

https://codereview.chromium.org/2701413003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/RangeBoundaryPoint.h (right):

https://codereview.chromium.org/2701413003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/RangeBoundaryPoint.h:73: static const
unsigned invalidOffset = static_cast<unsigned>(-1);
On 2017/02/21 at 12:32:05, Sunny wrote:
> Hi tkent@, I've tried working on this issue before and have a question.
> 
> In this file do you think it's acceptable to cast -1 to unsigned as a
indicator of invalid offset?
> 
> My solution is use a boolean as indicator:
https://codereview.chromium.org/2673303002/diff/20001/third_party/WebKit/Sour...

Adding a boolean flag is a more graceful way.
However, static_cast<unsigned>(-1) as an invalid value is more efficient than
the boolean flag in terms of memory consumption.
With the static_cast<unsigned>(-1), we can't handle offset=4294967295 correctly.
 But it's not a practical problem because Blink can't handle such large nodes
anyway.

[Sunny, 2017-02-22 02:45:46]

On 2017/02/22 02:08:43, tkent wrote:
>
https://codereview.chromium.org/2701413003/diff/60001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/dom/RangeBoundaryPoint.h (right):
> 
>
https://codereview.chromium.org/2701413003/diff/60001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/dom/RangeBoundaryPoint.h:73: static const
> unsigned invalidOffset = static_cast<unsigned>(-1);
> On 2017/02/21 at 12:32:05, Sunny wrote:
> > Hi tkent@, I've tried working on this issue before and have a question.
> > 
> > In this file do you think it's acceptable to cast -1 to unsigned as a
> indicator of invalid offset?
> > 
> > My solution is use a boolean as indicator:
>
https://codereview.chromium.org/2673303002/diff/20001/third_party/WebKit/Sour...
> 
> Adding a boolean flag is a more graceful way.
> However, static_cast<unsigned>(-1) as an invalid value is more efficient than
> the boolean flag in terms of memory consumption.
> With the static_cast<unsigned>(-1), we can't handle offset=4294967295
correctly.
>  But it's not a practical problem because Blink can't handle such large nodes
> anyway.

Ah. Got it, thanks for your guiding ~