Make InlineBoxPosition constructor not to accept null-InlineBox

Make InlineBoxPosition constructor not to accept null-InlineBox and an negative offset

This patch makes |InlineBoxPosition| not to accept null |InlineBox| and an
negative offset to avoid wrong usage of |InlineBoxPosition|.

Before this patch, |InlineBoxPosition.offset_in_box| holds an offset of
specified position event if |.layout_object| is null. After this patch,
|.offset_in_box| is zero if |.layout_object| is null.

Other than explicit test case and |LocalCaretRectOfPositionTemplate()|, we don't
use |offset_in_box| member when |InlineBox| is null. We've not seen wrong usage
so far and code reading doesn't find wrong usages. Once we find wrong call
sites, we'll fix them.

BUG=707656
TEST=run_webkit_unit_tests --gtest_filter=VisibleUnitsTest.ComputeInlineBoxPositionMixedEditable

Review-Url: https://codereview.chromium.org/2931673004
Cr-Commit-Position: refs/heads/master@{#479264}
Committed: https://chromium.googlesource.com/chromium/src/+/4a043e477b4857645c2f73583ea147b318fb40a4

[yosin_UTC9, 2017-06-09 05:28:53]

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

[commit-bot: I haz the power, 2017-06-09 05:29:06]

Dry run: CQ is trying da patch.

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

[yosin_UTC9, 2017-06-09 05:32:23]

Description was changed from

==========
2017-06-09T14:16:20

BUG=

2017-06-09T14:00:28
==========

to

==========
Make InlineBoxPosition constructor not to accept null-InlineBox and an negative
offset

This patch makes |InlineBoxPosition| not to accept null |InlineBox| and an
negative offset to avoid wrong usage of |InlineBoxPosition|.

Other than explicit test case of null |InlineBox|, we've not seen wrong usage
so far and code reading doesn't find wrong usages.

Once we find wrong call sites, we'll fix them.

BUG=707656
TEST=run_webkit_unit_tests
--gtest_filter=VisibleUnitsTest.ComputeInlineBoxPositionMixedEditable
==========

[commit-bot: I haz the power, 2017-06-09 06:24:04]

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

[commit-bot: I haz the power, 2017-06-09 06:24:05]

Dry run: Try jobs failed on following builders:
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[yosin_UTC9, 2017-06-12 05:52:43]

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

[yosin_UTC9, 2017-06-12 05:52:47]

Patchset #1 (id:1) has been deleted

[commit-bot: I haz the power, 2017-06-12 05:52:52]

Dry run: 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-06-12 07:28:06]

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

[commit-bot: I haz the power, 2017-06-12 07:28:07]

Dry run: This issue passed the CQ dry run.

[yosin_UTC9, 2017-06-12 08:19:03]

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

[yosin_UTC9, 2017-06-12 08:19:04]

PTAL

[Xiaocheng, 2017-06-12 18:53:41]

https://codereview.chromium.org/2931673004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/editing/VisibleUnits.cpp (right):

https://codereview.chromium.org/2931673004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/VisibleUnits.cpp:1033: nullptr,
position.GetPosition().ComputeEditingOffset()));
Could you clarify this change?

We should have |box_position.offset_in_box == 0| here. Why can't we use 0 offset
here, but have to use ComputeEditingOffset() instead?

[yosin_UTC9, 2017-06-13 06:54:37]

Description was changed from

==========
Make InlineBoxPosition constructor not to accept null-InlineBox and an negative
offset

This patch makes |InlineBoxPosition| not to accept null |InlineBox| and an
negative offset to avoid wrong usage of |InlineBoxPosition|.

Other than explicit test case of null |InlineBox|, we've not seen wrong usage
so far and code reading doesn't find wrong usages.

Once we find wrong call sites, we'll fix them.

BUG=707656
TEST=run_webkit_unit_tests
--gtest_filter=VisibleUnitsTest.ComputeInlineBoxPositionMixedEditable
==========

to

==========
Make InlineBoxPosition constructor not to accept null-InlineBox and an negative
offset

This patch makes |InlineBoxPosition| not to accept null |InlineBox| and an
negative offset to avoid wrong usage of |InlineBoxPosition|.

Before this patch, |InlineBoxPosition.offset_in_box| holds an offset of
specified position event if |.layout_object| is null. After this patch,
|.offset_in_box| is zero if |.layout_object| is null.

Other than explicit test case and |LocalCaretRectOfPositionTemplate()|, we don't
use |offset_in_box| member when |InlineBox| is null. We've not seen wrong usage
so far and code reading doesn't find wrong usages. Once we find wrong call
sites, we'll fix them.

BUG=707656
TEST=run_webkit_unit_tests
--gtest_filter=VisibleUnitsTest.ComputeInlineBoxPositionMixedEditable
==========

[yosin_UTC9, 2017-06-13 06:55:00]

PTAL

Reply question and update description.

https://codereview.chromium.org/2931673004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/editing/VisibleUnits.cpp (right):

https://codereview.chromium.org/2931673004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/VisibleUnits.cpp:1033: nullptr,
position.GetPosition().ComputeEditingOffset()));
On 2017/06/12 at 18:53:41, Xiaocheng wrote:
> Could you clarify this change?
> 
> We should have |box_position.offset_in_box == 0| here. Why can't we use 0
offset here, but have to use ComputeEditingOffset() instead?

Before this patch, we have box_positon.layout_object = nullptr and offset_in_box
= position.ComputeEditingOffset(), which is
in ComputeInlineBoxPositionTempalte() as int caret_offset =
position.ComputeEditingOffset() at L764.

After this patch, we ComputeInlineBox() returns |.offset_in_box == 0| for
|.layoyut_object == nullptr|, hence we need to
pass |.ComputeEditingOffset()| to |LayoutObject::LocalCaret()|.

I added an explanation to description.

[Xiaocheng, 2017-06-13 17:25:03]

lgtm

https://codereview.chromium.org/2931673004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/editing/VisibleUnits.cpp (right):

https://codereview.chromium.org/2931673004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/VisibleUnits.cpp:1033: nullptr,
position.GetPosition().ComputeEditingOffset()));
On 2017/06/13 at 06:55:00, yosin_UTC9 wrote:
> On 2017/06/12 at 18:53:41, Xiaocheng wrote:
> > Could you clarify this change?
> > 
> > We should have |box_position.offset_in_box == 0| here. Why can't we use 0
offset here, but have to use ComputeEditingOffset() instead?
> 
> Before this patch, we have box_positon.layout_object = nullptr and
offset_in_box = position.ComputeEditingOffset(), which is
> in ComputeInlineBoxPositionTempalte() as int caret_offset =
position.ComputeEditingOffset() at L764.
> 
> After this patch, we ComputeInlineBox() returns |.offset_in_box == 0| for
|.layoyut_object == nullptr|, hence we need to
> pass |.ComputeEditingOffset()| to |LayoutObject::LocalCaret()|.
> 
> I added an explanation to description.

Thanks!

[yosin_UTC9, 2017-06-14 01:04:01]

The CQ bit was checked by yosin@chromium.org

[commit-bot: I haz the power, 2017-06-14 01:04:28]

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-06-14 02:50:10]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1497402241127290,
"parent_rev": "e2fb02881eb16603ce55e9dbfdf73e1ae4daade3", "commit_rev":
"4a043e477b4857645c2f73583ea147b318fb40a4"}

[commit-bot: I haz the power, 2017-06-14 02:50:21]

Description was changed from

==========
Make InlineBoxPosition constructor not to accept null-InlineBox and an negative
offset

This patch makes |InlineBoxPosition| not to accept null |InlineBox| and an
negative offset to avoid wrong usage of |InlineBoxPosition|.

Before this patch, |InlineBoxPosition.offset_in_box| holds an offset of
specified position event if |.layout_object| is null. After this patch,
|.offset_in_box| is zero if |.layout_object| is null.

Other than explicit test case and |LocalCaretRectOfPositionTemplate()|, we don't
use |offset_in_box| member when |InlineBox| is null. We've not seen wrong usage
so far and code reading doesn't find wrong usages. Once we find wrong call
sites, we'll fix them.

BUG=707656
TEST=run_webkit_unit_tests
--gtest_filter=VisibleUnitsTest.ComputeInlineBoxPositionMixedEditable
==========

to

==========
Make InlineBoxPosition constructor not to accept null-InlineBox and an negative
offset

This patch makes |InlineBoxPosition| not to accept null |InlineBox| and an
negative offset to avoid wrong usage of |InlineBoxPosition|.

Before this patch, |InlineBoxPosition.offset_in_box| holds an offset of
specified position event if |.layout_object| is null. After this patch,
|.offset_in_box| is zero if |.layout_object| is null.

Other than explicit test case and |LocalCaretRectOfPositionTemplate()|, we don't
use |offset_in_box| member when |InlineBox| is null. We've not seen wrong usage
so far and code reading doesn't find wrong usages. Once we find wrong call
sites, we'll fix them.

BUG=707656
TEST=run_webkit_unit_tests
--gtest_filter=VisibleUnitsTest.ComputeInlineBoxPositionMixedEditable

Review-Url: https://codereview.chromium.org/2931673004
Cr-Commit-Position: refs/heads/master@{#479264}
Committed:
https://chromium.googlesource.com/chromium/src/+/4a043e477b4857645c2f73583ea1...
==========

[commit-bot: I haz the power, 2017-06-14 02:50:24]

Committed patchset #1 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/4a043e477b4857645c2f73583ea1...