New issue
Advanced search Search tips

Issue 810579 link

Starred by 1 user

Issue metadata

Status: Available
Owner: ----
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Task

Blocking:
issue 707656



Sign in to add a comment

[LayoutNG] Parameterize editing unit tests with LayoutNG

Project Member Reported by xiaoche...@chromium.org, Feb 9 2018

Issue description

As we convert existing editing code to work with LayoutNG, we should also convert existing tests by parameterizing them, so that we get better test coverage.
 

Comment 1 by yosin@chromium.org, Feb 9 2018

It seems it is too early to do so... :-<

For VisibleUnitsWordTest, following tests are failed:
- All/ParameterizedVisibleUnitsWordTest.StartOfWordBasic/1
- All/ParameterizedVisibleUnitsWordTest.StartOfWordPreviousWordIfOnBoundaryBasic/1
- All/ParameterizedVisibleUnitsWordTest.StartOfWordFirstLetter/1

Test outputs:
../../third_party/WebKit/Source/core/editing/VisibleUnitsWordTest.cpp(78): error: Expected equality of these values:
  "<p> (|1) abc def</p>"
  DoStartOfWord("<p> (|1) abc def</p>")
    Which is: "<p> |(1) abc def</p>"
../../third_party/WebKit/Source/core/editing/VisibleUnitsWordTest.cpp(79): error: Expected equality of these values:
  "<p> (1|) abc def</p>"
  DoStartOfWord("<p> (1|) abc def</p>")
    Which is: "<p> |(1) abc def</p>"
../../third_party/WebKit/Source/core/editing/VisibleUnitsWordTest.cpp(80): error: Expected equality of these values:
  "<p> (1)| abc def</p>"
  DoStartOfWord("<p> (1)| abc def</p>")
    Which is: "<p> |(1) abc def</p>"
../../third_party/WebKit/Source/core/editing/VisibleUnitsWordTest.cpp(81): error: Expected equality of these values:
  "<p> (1) |abc def</p>"
  DoStartOfWord("<p> (1) |abc def</p>")
    Which is: "<p> |(1) abc def</p>"
../../third_party/WebKit/Source/core/editing/VisibleUnitsWordTest.cpp(82): error: Expected equality of these values:
  "<p> (1) |abc def</p>"
  DoStartOfWord("<p> (1) a|bc def</p>")
    Which is: "<p> |(1) abc def</p>"
../../third_party/WebKit/Source/core/editing/VisibleUnitsWordTest.cpp(83): error: Expected equality of these values:
  "<p> (1) |abc def</p>"
  DoStartOfWord("<p> (1) ab|c def</p>")
    Which is: "<p> |(1) abc def</p>"
../../third_party/WebKit/Source/core/editing/VisibleUnitsWordTest.cpp(84): error: Expected equality of these values:
  "<p> (1) abc| def</p>"
  DoStartOfWord("<p> (1) abc| def</p>")
    Which is: "<p> |(1) abc def</p>"
../../third_party/WebKit/Source/core/editing/VisibleUnitsWordTest.cpp(85): error: Expected equality of these values:
  "<p> (1) abc |def</p>"
  DoStartOfWord("<p> (1) abc |def</p>")
    Which is: "<p> |(1) abc def</p>"
../../third_party/WebKit/Source/core/editing/VisibleUnitsWordTest.cpp(86): error: Expected equality of these values:
  "<p> (1) abc |def</p>"
  DoStartOfWord("<p> (1) abc d|ef</p>")
    Which is: "<p> |(1) abc def</p>"
../../third_party/WebKit/Source/core/editing/VisibleUnitsWordTest.cpp(87): error: Expected equality of these values:
  "<p> (1) abc |def</p>"
  DoStartOfWord("<p> (1) abc de|f</p>")
    Which is: "<p> |(1) abc def</p>"
[  FAILED  ] All/ParameterizedVisibleUnitsWordTest.StartOfWordBasic/1, where GetParam() = true (77 ms)





../../third_party/WebKit/Source/core/editing/VisibleUnitsWordTest.cpp(104): error: Expected equality of these values:
  "<p> (|1) abc def</p>"
  DoStartOfWord("<p> (1|) abc def</p>", EWordSide::kPreviousWordIfOnBoundary)
    Which is: "<p> |(1) abc def</p>"
../../third_party/WebKit/Source/core/editing/VisibleUnitsWordTest.cpp(107): error: Expected equality of these values:
  "<p> (1|) abc def</p>"
  DoStartOfWord("<p> (1)| abc def</p>", EWordSide::kPreviousWordIfOnBoundary)
    Which is: "<p> |(1) abc def</p>"
../../third_party/WebKit/Source/core/editing/VisibleUnitsWordTest.cpp(110): error: Expected equality of these values:
  "<p> (1)| abc def</p>"
  DoStartOfWord("<p> (1) |abc def</p>", EWordSide::kPreviousWordIfOnBoundary)
    Which is: "<p> |(1) abc def</p>"
../../third_party/WebKit/Source/core/editing/VisibleUnitsWordTest.cpp(113): error: Expected equality of these values:
  "<p> (1) |abc def</p>"
  DoStartOfWord("<p> (1) a|bc def</p>", EWordSide::kPreviousWordIfOnBoundary)
    Which is: "<p> |(1) abc def</p>"
../../third_party/WebKit/Source/core/editing/VisibleUnitsWordTest.cpp(116): error: Expected equality of these values:
  "<p> (1) |abc def</p>"
  DoStartOfWord("<p> (1) ab|c def</p>", EWordSide::kPreviousWordIfOnBoundary)
    Which is: "<p> |(1) abc def</p>"
../../third_party/WebKit/Source/core/editing/VisibleUnitsWordTest.cpp(119): error: Expected equality of these values:
  "<p> (1) |abc def</p>"
  DoStartOfWord("<p> (1) abc| def</p>", EWordSide::kPreviousWordIfOnBoundary)
    Which is: "<p> |(1) abc def</p>"
../../third_party/WebKit/Source/core/editing/VisibleUnitsWordTest.cpp(122): error: Expected equality of these values:
  "<p> (1) abc| def</p>"
  DoStartOfWord("<p> (1) abc |def</p>", EWordSide::kPreviousWordIfOnBoundary)
    Which is: "<p> |(1) abc def</p>"
../../third_party/WebKit/Source/core/editing/VisibleUnitsWordTest.cpp(125): error: Expected equality of these values:
  "<p> (1) abc |def</p>"
  DoStartOfWord("<p> (1) abc d|ef</p>", EWordSide::kPreviousWordIfOnBoundary)
    Which is: "<p> |(1) abc def</p>"
../../third_party/WebKit/Source/core/editing/VisibleUnitsWordTest.cpp(128): error: Expected equality of these values:
  "<p> (1) abc |def</p>"
  DoStartOfWord("<p> (1) abc de|f</p>", EWordSide::kPreviousWordIfOnBoundary)
    Which is: "<p> |(1) abc def</p>"
../../third_party/WebKit/Source/core/editing/VisibleUnitsWordTest.cpp(131): error: Expected equality of these values:
  "<p> (1) abc |def</p>"
  DoStartOfWord("<p> (1) abc def|</p>", EWordSide::kPreviousWordIfOnBoundary)
    Which is: "<p> |(1) abc def</p>"
../../third_party/WebKit/Source/core/editing/VisibleUnitsWordTest.cpp(134): error: Expected equality of these values:
  "<p> (1) abc |def</p>"
  DoStartOfWord("<p> (1) abc def</p>|", EWordSide::kPreviousWordIfOnBoundary)
    Which is: "<p> |(1) abc def</p>"
[  FAILED  ] All/ParameterizedVisibleUnitsWordTest.StartOfWordPreviousWordIfOnBoundaryBasic/1, where GetParam() = true (78 ms)


../../third_party/WebKit/Source/core/editing/VisibleUnitsWordTest.cpp(147): error: Expected equality of these values:
  "<p> (|1) abc def</p>"
  DoStartOfWord("<p> (|1) abc def</p>")
    Which is: "<p> |(1) abc def</p>"
../../third_party/WebKit/Source/core/editing/VisibleUnitsWordTest.cpp(148): error: Expected equality of these values:
  "<p> (1|) abc def</p>"
  DoStartOfWord("<p> (1|) abc def</p>")
    Which is: "<p> |(1) abc def</p>"
../../third_party/WebKit/Source/core/editing/VisibleUnitsWordTest.cpp(149): error: Expected equality of these values:
  "<p> (1)| abc def</p>"
  DoStartOfWord("<p> (1)| abc def</p>")
    Which is: "<p> |(1) abc def</p>"
../../third_party/WebKit/Source/core/editing/VisibleUnitsWordTest.cpp(150): error: Expected equality of these values:
  "<p> (1) |abc def</p>"
  DoStartOfWord("<p> (1) |abc def</p>")
    Which is: "<p> |(1) abc def</p>"
../../third_party/WebKit/Source/core/editing/VisibleUnitsWordTest.cpp(151): error: Expected equality of these values:
  "<p> (1) |abc def</p>"
  DoStartOfWord("<p> (1) a|bc def</p>")
    Which is: "<p> |(1) abc def</p>"
../../third_party/WebKit/Source/core/editing/VisibleUnitsWordTest.cpp(152): error: Expected equality of these values:
  "<p> (1) |abc def</p>"
  DoStartOfWord("<p> (1) ab|c def</p>")
    Which is: "<p> |(1) abc def</p>"
../../third_party/WebKit/Source/core/editing/VisibleUnitsWordTest.cpp(153): error: Expected equality of these values:
  "<p> (1) abc| def</p>"
  DoStartOfWord("<p> (1) abc| def</p>")
    Which is: "<p> |(1) abc def</p>"
../../third_party/WebKit/Source/core/editing/VisibleUnitsWordTest.cpp(154): error: Expected equality of these values:
  "<p> (1) abc |def</p>"
  DoStartOfWord("<p> (1) abc |def</p>")
    Which is: "<p> |(1) abc def</p>"
../../third_party/WebKit/Source/core/editing/VisibleUnitsWordTest.cpp(155): error: Expected equality of these values:
  "<p> (1) abc |def</p>"
  DoStartOfWord("<p> (1) abc d|ef</p>")
    Which is: "<p> |(1) abc def</p>"
../../third_party/WebKit/Source/core/editing/VisibleUnitsWordTest.cpp(156): error: Expected equality of these values:
  "<p> (1) abc |def</p>"
  DoStartOfWord("<p> (1) abc de|f</p>")
    Which is: "<p> |(1) abc def</p>"
[  FAILED  ] All/ParameterizedVisibleUnitsWordTest.StartOfWordFirstLetter/1, where GetParam() = true (101 ms)


We may start to parameterize those passing.
Project Member

Comment 3 by bugdroid1@chromium.org, Apr 12 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/1324be868df000cb2e070199ce2d10cd0c5a2706

commit 1324be868df000cb2e070199ce2d10cd0c5a2706
Author: Yoshifumi Inoue <yosin@chromium.org>
Date: Thu Apr 12 08:30:26 2018

Make NextWordPosition() to utilize TextOffsetMapping

This patch changes |NextWordPosition()| to utilize |TextOffsetMapping| to make
|NextWordPosition()| to work with LayoutNG.

This patch is a part of the patch[1].

[1] http://crrev.com/c/737981 Simplify word granularity handling

Bug: 778507, 810579

Change-Id: I7e196b0ee390d68b93f2494333554279d6fbd42d
Reviewed-on: https://chromium-review.googlesource.com/1004536
Commit-Queue: Yoshifumi Inoue <yosin@chromium.org>
Reviewed-by: Yoichi Osato <yoichio@chromium.org>
Reviewed-by: Xiaocheng Hu <xiaochengh@chromium.org>
Cr-Commit-Position: refs/heads/master@{#550106}
[modify] https://crrev.com/1324be868df000cb2e070199ce2d10cd0c5a2706/third_party/blink/renderer/core/editing/visible_units_word.cc
[modify] https://crrev.com/1324be868df000cb2e070199ce2d10cd0c5a2706/third_party/blink/renderer/core/editing/visible_units_word_test.cc

Project Member

Comment 4 by bugdroid1@chromium.org, May 29 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/6535528f00c742b6cb9dcce8e46567048ac0dadc

commit 6535528f00c742b6cb9dcce8e46567048ac0dadc
Author: Yoshifumi Inoue <yosin@chromium.org>
Date: Tue May 29 04:05:29 2018

Make EndOfSentence() to utilize TextOffsetMapping

This patch changes |EndOfSentence()| to utilize |TextOffsetMapping| to make
|EndOfSentence()| to work with LayoutNG by introducing |FindBoundaryForward()|
to share logic with |NextWordPosition()|.

Because of |TextOffsetMapping| traverses flat tree, DOM tree version of
|EndOfSentence()| works as same as flat tree version.

Bug: 778507, 810579
Change-Id: Ib3e2ad8aa4a0e8b8657a0363c2f3e3da8355eae3
Reviewed-on: https://chromium-review.googlesource.com/1009346
Commit-Queue: Yoshifumi Inoue <yosin@chromium.org>
Reviewed-by: Xiaocheng Hu <xiaochengh@chromium.org>
Cr-Commit-Position: refs/heads/master@{#562334}
[modify] https://crrev.com/6535528f00c742b6cb9dcce8e46567048ac0dadc/third_party/blink/renderer/core/editing/visible_units.cc
[modify] https://crrev.com/6535528f00c742b6cb9dcce8e46567048ac0dadc/third_party/blink/renderer/core/editing/visible_units.h
[modify] https://crrev.com/6535528f00c742b6cb9dcce8e46567048ac0dadc/third_party/blink/renderer/core/editing/visible_units_sentence.cc
[modify] https://crrev.com/6535528f00c742b6cb9dcce8e46567048ac0dadc/third_party/blink/renderer/core/editing/visible_units_sentence_test.cc
[modify] https://crrev.com/6535528f00c742b6cb9dcce8e46567048ac0dadc/third_party/blink/renderer/core/editing/visible_units_word.cc

Project Member

Comment 5 by bugdroid1@chromium.org, Jun 1 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/6ea45774b43642488abee6688f0ac328903c2400

commit 6ea45774b43642488abee6688f0ac328903c2400
Author: Yoshifumi Inoue <yosin@chromium.org>
Date: Fri Jun 01 12:49:37 2018

Introduce TextSegments class with Finder interface

This patch introduces |TextSegments| class with |Finder| interface as
replacement of |FindBoundaryForwrad()| and |FindBoundaryBackward()| along with
call sites, to implementing sentence boundary function with state instead of
|std::function<>|.

This patch also changes |TextOffsetMapping::GetPositionAfter()| to return
correct value.

This patch is a preparation of the patch[1].

[1] http://crrev.com/c/1075918 Make NextSentencePosition() to utilize
TextOffsetMapping

Bug: 778507, 810579
Change-Id: I56c6d08589740709cfe1a4d7281274f2f56fbac6
Reviewed-on: https://chromium-review.googlesource.com/1078167
Commit-Queue: Yoshifumi Inoue <yosin@chromium.org>
Reviewed-by: Yoichi Osato <yoichio@chromium.org>
Cr-Commit-Position: refs/heads/master@{#563599}
[modify] https://crrev.com/6ea45774b43642488abee6688f0ac328903c2400/third_party/blink/renderer/core/editing/BUILD.gn
[modify] https://crrev.com/6ea45774b43642488abee6688f0ac328903c2400/third_party/blink/renderer/core/editing/text_offset_mapping.cc
[modify] https://crrev.com/6ea45774b43642488abee6688f0ac328903c2400/third_party/blink/renderer/core/editing/text_offset_mapping_test.cc
[add] https://crrev.com/6ea45774b43642488abee6688f0ac328903c2400/third_party/blink/renderer/core/editing/text_segments.cc
[add] https://crrev.com/6ea45774b43642488abee6688f0ac328903c2400/third_party/blink/renderer/core/editing/text_segments.h
[modify] https://crrev.com/6ea45774b43642488abee6688f0ac328903c2400/third_party/blink/renderer/core/editing/visible_units.cc
[modify] https://crrev.com/6ea45774b43642488abee6688f0ac328903c2400/third_party/blink/renderer/core/editing/visible_units.h
[modify] https://crrev.com/6ea45774b43642488abee6688f0ac328903c2400/third_party/blink/renderer/core/editing/visible_units_sentence.cc
[modify] https://crrev.com/6ea45774b43642488abee6688f0ac328903c2400/third_party/blink/renderer/core/editing/visible_units_word.cc

Project Member

Comment 6 by bugdroid1@chromium.org, Jun 4 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/4040a4d9004dd3258db0b9abea5c66757987bb5e

commit 4040a4d9004dd3258db0b9abea5c66757987bb5e
Author: Yoshifumi Inoue <yosin@chromium.org>
Date: Mon Jun 04 07:56:13 2018

Make NextSentencePosition() to utilize TextSegments class

This patch changes |NextSentencePosition()| to utilize |TextSegments| class
to make |NextSentencePosition()| to work with LayoutNG.

Bug: 778507, 810579
Change-Id: If750dec03584cd1e5631fd37d1ab6516d5963eb3
Reviewed-on: https://chromium-review.googlesource.com/1075918
Commit-Queue: Yoshifumi Inoue <yosin@chromium.org>
Reviewed-by: Yoichi Osato <yoichio@chromium.org>
Reviewed-by: Xiaocheng Hu <xiaochengh@chromium.org>
Cr-Commit-Position: refs/heads/master@{#564012}
[modify] https://crrev.com/4040a4d9004dd3258db0b9abea5c66757987bb5e/third_party/blink/renderer/core/editing/visible_units_sentence.cc

Project Member

Comment 7 by bugdroid1@chromium.org, Jun 6 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/c063974c76f9579fbccd11f6939dffaf8ca26055

commit c063974c76f9579fbccd11f6939dffaf8ca26055
Author: Yoshifumi Inoue <yosin@chromium.org>
Date: Wed Jun 06 09:52:11 2018

Make EndOfWordPosition() to utilize TextSegments class

This patch changes |EndOfWordPosition()| to utilize |TextSegments| class
to make |EndOfWordPosition()| to work with LayoutNG.

This patch also changes |GranularityAdjuster| to handle end of word position in
flat tree mapped before start of word position in DOM tree.

This patch changes following test expectations:
* third_party/WebKit/LayoutTests/fast/text/international/cjk-segmentation.html
|TextSegments| provides more context characters to ICU word breaker.
* VisibleSelectionTest.expandUsingGranularity
* ParameterizedVisibleUnitsWordTest.EndOfWordShadowDOM
Due by |PositionInFlatTree| to |Position| conversion.

Bug: 778507, 810579
Change-Id: Ie888f8b76e00386bc728a5c647aecf8f33ef7b6e
Reviewed-on: https://chromium-review.googlesource.com/1086811
Reviewed-by: Yoichi Osato <yoichio@chromium.org>
Commit-Queue: Yoshifumi Inoue <yosin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#564839}
[modify] https://crrev.com/c063974c76f9579fbccd11f6939dffaf8ca26055/third_party/WebKit/LayoutTests/fast/text/international/cjk-segmentation.html
[modify] https://crrev.com/c063974c76f9579fbccd11f6939dffaf8ca26055/third_party/blink/renderer/core/editing/selection_adjuster.cc
[modify] https://crrev.com/c063974c76f9579fbccd11f6939dffaf8ca26055/third_party/blink/renderer/core/editing/visible_selection_test.cc
[modify] https://crrev.com/c063974c76f9579fbccd11f6939dffaf8ca26055/third_party/blink/renderer/core/editing/visible_units.h
[modify] https://crrev.com/c063974c76f9579fbccd11f6939dffaf8ca26055/third_party/blink/renderer/core/editing/visible_units_word.cc
[modify] https://crrev.com/c063974c76f9579fbccd11f6939dffaf8ca26055/third_party/blink/renderer/core/editing/visible_units_word_test.cc

Project Member

Comment 8 by bugdroid1@chromium.org, Nov 16

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/4523b04c1c41cc3c76edb8313261707ca572fef0

commit 4523b04c1c41cc3c76edb8313261707ca572fef0
Author: Xiaocheng Hu <xiaochengh@chromium.org>
Date: Fri Nov 16 20:12:29 2018

Parameterized CharacterIteratorTest for LayoutNG

This patch parameterizes CharacterIteratorTest for LayoutNG to prevent
regressions when LayoutNG is enabled.

Bug: 810579
Change-Id: I160081c9389aed9ee8cb326be6e022d7b6fd375f
Reviewed-on: https://chromium-review.googlesource.com/c/1340271
Reviewed-by: Christian Biesinger <cbiesinger@chromium.org>
Commit-Queue: Xiaocheng Hu <xiaochengh@chromium.org>
Cr-Commit-Position: refs/heads/master@{#608916}
[modify] https://crrev.com/4523b04c1c41cc3c76edb8313261707ca572fef0/third_party/blink/renderer/core/editing/iterators/character_iterator_test.cc

Project Member

Comment 9 by bugdroid1@chromium.org, Nov 19

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/ad9bbcc0fcab5ecdc4fd58bb931b7fd1262fbf32

commit ad9bbcc0fcab5ecdc4fd58bb931b7fd1262fbf32
Author: Xiaocheng Hu <xiaochengh@chromium.org>
Date: Mon Nov 19 22:50:23 2018

[LayoutNG] Make all CharacterIteratorTest pass with LayoutNG

CharacterIteratorTest.GetPositionWithEmitChar16Before used to fail with
LayoutNG enabled, because it's actually a buggy case in legacy layout
where TextIterator emits an extra character.

This patch makes the test case skip the extra character when LayoutNG
is enabled, and hence allows it to pass.

Bug: 810579
Change-Id: Ie111735af3c94fe7b47eed7a993d1d374612caa6
Reviewed-on: https://chromium-review.googlesource.com/c/1340862
Reviewed-by: Yoichi Osato <yoichio@chromium.org>
Reviewed-by: Yoshifumi Inoue <yosin@chromium.org>
Commit-Queue: Xiaocheng Hu <xiaochengh@chromium.org>
Cr-Commit-Position: refs/heads/master@{#609478}
[modify] https://crrev.com/ad9bbcc0fcab5ecdc4fd58bb931b7fd1262fbf32/third_party/blink/renderer/core/editing/iterators/character_iterator_test.cc

Sign in to add a comment