Make most{Backward,Forward}CaretPosition() to handle first-letter pseudo element

Make most{Backward,Forward}CaretPosition() to handle first-letter pseudo element

Before this patch, Blink doesn't work well for first-letter pseudo element
by two reasons:
 1. Blink doesn't use first-letter text and uses only remaining text.
 2. Scanning |InlineTextBox| with DOM offset rather than offset of
    |LayoutText::m_text|

This patch makes |most{Backward,Forward}CaretPosition()| and associated
functions to handle first-letter pseudo element with considering above.

This patch also updates layout test expectation texts as result of correct
handling of first-letter pseudo element.

Background:
In layout tree, |Text| node having first-letter style represented by two
|LayoutTextFragment| objects, one for first-letter text and another for
remaining text. |LayoutTextFragment| is derived from |LayutText|.

Note: first-letter text can be multiple characters, which
can contain leading whitespaces of |Text| node and punctuation characters
and a letter character.

|LayoutText::m_text| holds a text for layout/paint, which is different from
text in DOM node, e.g. applying CSS text-transform.

BUG=174349, 545520
TEST=webkit_unit_tests --gtest_filter=VisibleUnits.mostBackwardCaretPositionFirstLetter
TEST=webkit_unit_tests --gtest_filter=VisibleUnits.mostForwardCaretPositionFirstLetter

Committed: https://crrev.com/183a61e97bbb014b3b0a55e92cca03151632d1fe
Cr-Commit-Position: refs/heads/master@{#358020}

[yosin_UTC9, 2015-10-22 09:12:44]

Description was changed from

==========
2015-10-21T16:58:05

BUG=
==========

to

==========
2015-10-21T16:58:05

BUG=545520
==========

[yosin_UTC9, 2015-10-22 09:12:59]

Description was changed from

==========
2015-10-21T16:58:05

BUG=545520
==========

to

==========
2015-10-21T16:58:05

BUG=174349, 545520
==========

[yosin_UTC9, 2015-10-22 09:27:59]

Patchset #2 (id:20001) has been deleted

[yosin_UTC9, 2015-10-30 07:38:57]

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

[yosin_UTC9, 2015-10-30 07:38:58]

Make most{Backward,Forward}CaretPosition() to handle first-letter pseudo element

This patch makes |most{Backward,Forward}CaretPosition()| and associated
functions to handle first-letter pseudo element when these functions using
|InlineTextBox|.

In layout tree, |Text| node having first-letter style represented by two
|LayoutTextFrgment| objects, one for first-letter part and another for remaining
part, which is subclass of |LayoutText| rather than single |LayoutText|.

Before this patch, |most{Backward,Forward}CaretPosition()| use |LayoutTextFrame|
object for part only, and accessing it with offset in |Text| node rather than
offset in |InlineTextBox|, e.g. they handles offset zero as start of remaining
text instead of start of |Text| node.

Layout test expectation texts are updated as result of correct handling of
first-letter pseudo element.

BUG=174349, 545520
TEST=webkit_unit_tests
--gtest_filter=VisibleUnits.mostBackwardCaretPositionFirstLetter
TEST=webkit_unit_tests
--gtest_filter=VisibleUnits.mostForwardCaretPositionFirstLetter

[yosin_UTC9, 2015-10-30 07:40:27]

Description was changed from

==========
2015-10-21T16:58:05

BUG=174349, 545520
==========

to

==========
Make most{Backward,Forward}CaretPosition() to handle first-letter pseudo element

This patch makes |most{Backward,Forward}CaretPosition()| and associated
functions to handle first-letter pseudo element when these functions using
|InlineTextBox|.

In layout tree, |Text| node having first-letter style represented by two
|LayoutTextFrgment| objects, one for first-letter part and another for remaining
part, which is subclass of |LayoutText| rather than single |LayoutText|.

Before this patch, |most{Backward,Forward}CaretPosition()| use |LayoutTextFrame|
object for part only, and accessing it with offset in |Text| node rather than
offset in |InlineTextBox|, e.g. they handles offset zero as start of remaining
text instead of start of |Text| node.

Layout test expectation texts are updated as result of correct handling of
first-letter pseudo element.

BUG=174349, 545520
TEST=webkit_unit_tests
--gtest_filter=VisibleUnits.mostBackwardCaretPositionFirstLetter
TEST=webkit_unit_tests
--gtest_filter=VisibleUnits.mostForwardCaretPositionFirstLetter
==========

[hajimehoshi, 2015-10-30 08:36:59]

sgtm, but I don't have enough knowledge about this fix. Please wait for another
reviewer.

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

https://codereview.chromium.org/1404423005/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/VisibleUnits.cpp:2156: static
LayoutObject* associatedLayoutObjetOf(const Node& node, int offsetInNode)
nit: Objet -> Object

[yoichio, 2015-11-04 02:22:51]

Could you make the layouttests testharness and remove -expected.txt?

[yosin_UTC9, 2015-11-04 07:39:54]

On 2015/11/04 at 02:22:51, yoichio wrote:
> Could you make the layouttests testharness and remove -expected.txt?

Since, this is Pri-1 security bug, heap-buffer-overflow, and these tests
are copy of cluster fuzz's test case, I would like keep them as is to
have dump of current behavior.

[yoichio, 2015-11-05 04:00:07]

On 2015/11/04 at 07:39:54, yosin wrote:
> On 2015/11/04 at 02:22:51, yoichio wrote:
> > Could you make the layouttests testharness and remove -expected.txt?
> 
> Since, this is Pri-1 security bug, heap-buffer-overflow, and these tests
> are copy of cluster fuzz's test case, I would like keep them as is to
> have dump of current behavior.

O.K.

Description doesn't make sense. Please update.

>In layout tree, |Text| node having first-letter style represented by two
is represented by?
>|LayoutTextFrgment| objects, one for first-letter part and another for
remaining
>part, which is subclass of |LayoutText| rather than single |LayoutText|.
What is the subclass?

>Before this patch, |most{Backward,Forward}CaretPosition()| use
|LayoutTextFrame|
>object for part only, and accessing it with offset in |Text| node rather than
What does "part only" mean?
and access it with?

>offset in |InlineTextBox|, e.g. they handles offset zero as start of remaining
>text instead of start of |Text| node.
Doesn't make sense.

[yoichio, 2015-11-05 04:00:15]

https://codereview.chromium.org/1404423005/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/editing/VisibleUnits.cpp (right):

https://codereview.chromium.org/1404423005/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/VisibleUnits.cpp:2186: 
Could you use |Text* const anchorText = toText(anchorNode)| for readbility?

[yosin_UTC9, 2015-11-05 04:22:47]

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

https://codereview.chromium.org/1404423005/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/VisibleUnits.cpp:2156: static
LayoutObject* associatedLayoutObjetOf(const Node& node, int offsetInNode)
On 2015/10/30 at 08:36:59, hajimehoshi wrote:
> nit: Objet -> Object

Done.

https://codereview.chromium.org/1404423005/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/editing/VisibleUnits.cpp (right):

https://codereview.chromium.org/1404423005/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/VisibleUnits.cpp:2186: 
On 2015/11/05 at 04:00:15, yoichio wrote:
> Could you use |Text* const anchorText = toText(anchorNode)| for readbility?

No, since it is used only one place, L2188, introducing another variable is
confusing.

[yosin_UTC9, 2015-11-05 06:51:02]

Description was changed from

==========
Make most{Backward,Forward}CaretPosition() to handle first-letter pseudo element

This patch makes |most{Backward,Forward}CaretPosition()| and associated
functions to handle first-letter pseudo element when these functions using
|InlineTextBox|.

In layout tree, |Text| node having first-letter style represented by two
|LayoutTextFrgment| objects, one for first-letter part and another for remaining
part, which is subclass of |LayoutText| rather than single |LayoutText|.

Before this patch, |most{Backward,Forward}CaretPosition()| use |LayoutTextFrame|
object for part only, and accessing it with offset in |Text| node rather than
offset in |InlineTextBox|, e.g. they handles offset zero as start of remaining
text instead of start of |Text| node.

Layout test expectation texts are updated as result of correct handling of
first-letter pseudo element.

BUG=174349, 545520
TEST=webkit_unit_tests
--gtest_filter=VisibleUnits.mostBackwardCaretPositionFirstLetter
TEST=webkit_unit_tests
--gtest_filter=VisibleUnits.mostForwardCaretPositionFirstLetter
==========

to

==========
Make most{Backward,Forward}CaretPosition() to handle first-letter pseudo element

Before this patch, Blink doesn't work well for first-letter pseudo element
by two reasons:
 1. Blink doesn't use first-letter text and uses only remaining text.
 2. Scanning |InlineTextBox| with DOM offset rather than offset of
    |LayoutText::m_text|

This patch makes |most{Backward,Forward}CaretPosition()| and associated
functions to handle first-letter pseudo element with considering above.

This patch also updates layout test expectation texts as result of correct
handling of first-letter pseudo element.

Background:
In layout tree, |Text| node having first-letter style represented by two
|LayoutTextFragment| objects, one for first-letter text and another for
remaining text. |LayoutTextFragment| is derived from |LayutText|.

Note: first-letter text can be multiple characters, which
can contain leading whitespaces of |Text| node and punctuation characters
and a letter character.

|LayoutText::m_text| holds a text for layout/paint, which is different from
text in DOM node, e.g. applying CSS text-transform.

BUG=174349, 545520
TEST=webkit_unit_tests
--gtest_filter=VisibleUnits.mostBackwardCaretPositionFirstLetter
TEST=webkit_unit_tests
--gtest_filter=VisibleUnits.mostForwardCaretPositionFirstLetter
==========

[yosin_UTC9, 2015-11-05 06:51:24]

PTAL

Update description.

[yoichio, 2015-11-05 08:01:11]

lgtm

[yosin_UTC9, 2015-11-05 08:38:23]

yosin@chromium.org changed reviewers:
+ kouhei@chromium.org

[yosin_UTC9, 2015-11-05 08:38:24]

Could you take look for LayoutFragmentText.h change?
Just expose member function length().
Thanks!

[kouhei (in TOK), 2015-11-05 08:41:05]

On 2015/11/05 08:38:24, Yosi_OOO_til_Oct_30 wrote:
> Could you take look for LayoutFragmentText.h change?
> Just expose member function length().
> Thanks!

core/layout OWNER lgtm

[yosin_UTC9, 2015-11-05 09:02:29]

The CQ bit was checked by yosin@chromium.org

[commit-bot: I haz the power, 2015-11-05 09:03:02]

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

[commit-bot: I haz the power, 2015-11-05 10:05:59]

Committed patchset #5 (id:100001)

[commit-bot: I haz the power, 2015-11-05 10:06:41]

Patchset 5 (id:??) landed as
https://crrev.com/183a61e97bbb014b3b0a55e92cca03151632d1fe
Cr-Commit-Position: refs/heads/master@{#358020}