New issue
Advanced search Search tips

Issue 757719 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner: ----
Closed: Jan 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Task

Blocking:
issue 388681



Sign in to add a comment

Re-architecture of SurroundingText

Project Member Reported by yosin@chromium.org, Aug 22 2017

Issue description

Since usages of SurroundingText doesn't involve DOM mutation, we don't need
to use Range object in SurroundingText[1].

[1] https://goo.gl/SXGdrk

 

Comment 1 by yosin@chromium.org, Aug 22 2017

Blocking: 388681
Project Member

Comment 2 by bugdroid1@chromium.org, Aug 22 2017

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

commit 6ccf0038cbd54aad63df7d7753431d85962b6fb5
Author: Yoshifumi Inoue <yosin@chromium.org>
Date: Tue Aug 22 09:59:37 2017

Get rid of an unused function WebSurroundingText::Initialize()

This patch gets rid of an unused function |WebSurroundingText::Initialize()| to
reduce size of source code for improving code health.

Note: Following patch will get rid of |Position| version of |SurroundingText|
which is used only by |WebSurroundingText::Initialize()|.

Bug:  757719 
Change-Id: Ia51290011586f59dcde48b5b23c60df83aa02369
Reviewed-on: https://chromium-review.googlesource.com/625543
Reviewed-by: Kent Tamura <tkent@chromium.org>
Commit-Queue: Yoshifumi Inoue <yosin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#496266}
[modify] https://crrev.com/6ccf0038cbd54aad63df7d7753431d85962b6fb5/third_party/WebKit/Source/core/exported/WebSurroundingText.cpp
[modify] https://crrev.com/6ccf0038cbd54aad63df7d7753431d85962b6fb5/third_party/WebKit/public/web/WebSurroundingText.h

Project Member

Comment 3 by bugdroid1@chromium.org, Aug 24 2017

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

commit 95197c5f5f6e6b9a5f0df63b3c332d9ee5b0c031
Author: Yoshifumi Inoue <yosin@chromium.org>
Date: Thu Aug 24 08:31:54 2017

Change Range version of SurroundingText constructor to take EphemeralRange

This patch changes |Range| version of |SurroundingText| constructor to take
|EphemeralRange| to avoid using temporary |Range| object for Olipan-GC friendly.

This patch is also a preparation of getting rid of |Position| version of
|SurroundingText| constructor which isun't used.

Bug: 388681,  757719 
Change-Id: I845c6244225cbe4f6d8bf410af317440ea36c0bd
Reviewed-on: https://chromium-review.googlesource.com/631081
Reviewed-by: Kent Tamura <tkent@chromium.org>
Commit-Queue: Yoshifumi Inoue <yosin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#496983}
[modify] https://crrev.com/95197c5f5f6e6b9a5f0df63b3c332d9ee5b0c031/third_party/WebKit/Source/core/editing/SurroundingText.cpp
[modify] https://crrev.com/95197c5f5f6e6b9a5f0df63b3c332d9ee5b0c031/third_party/WebKit/Source/core/editing/SurroundingText.h
[modify] https://crrev.com/95197c5f5f6e6b9a5f0df63b3c332d9ee5b0c031/third_party/WebKit/Source/core/editing/SurroundingTextTest.cpp
[modify] https://crrev.com/95197c5f5f6e6b9a5f0df63b3c332d9ee5b0c031/third_party/WebKit/Source/core/exported/WebSurroundingText.cpp

Project Member

Comment 4 by bugdroid1@chromium.org, Aug 25 2017

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

commit 9fd708d148b048b1afd47168cbffc3494597471b
Author: Yoshifumi Inoue <yosin@chromium.org>
Date: Fri Aug 25 07:48:17 2017

Get rid of Position version of constructor from SurroundingText class

This patch gets rid of |Position| version of constructor from |SurroundingText|
class to simplify |SurroundingText| class for improving code health.

Bug:  757719 
Change-Id: Iebecd83a5cd5117a5201bb14e88619b588bde2a2
Reviewed-on: https://chromium-review.googlesource.com/634166
Reviewed-by: Kent Tamura <tkent@chromium.org>
Commit-Queue: Yoshifumi Inoue <yosin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#497349}
[modify] https://crrev.com/9fd708d148b048b1afd47168cbffc3494597471b/third_party/WebKit/Source/core/editing/SurroundingText.cpp
[modify] https://crrev.com/9fd708d148b048b1afd47168cbffc3494597471b/third_party/WebKit/Source/core/editing/SurroundingText.h
[modify] https://crrev.com/9fd708d148b048b1afd47168cbffc3494597471b/third_party/WebKit/Source/core/editing/SurroundingTextTest.cpp
[modify] https://crrev.com/9fd708d148b048b1afd47168cbffc3494597471b/third_party/WebKit/Source/core/testing/Internals.cpp

Project Member

Comment 5 by bugdroid1@chromium.org, Dec 23 2017

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

commit da8f9ea66e37e7f32f092afb9c3fb133bbdf4e1b
Author: Xiaocheng Hu <xiaochengh@chromium.org>
Date: Sat Dec 23 00:40:10 2017

Change SurroundingText into POD type

Since no DOM/style mutation is supposed to happen during the life time
of SurroundingText, there is no need to store a Range in it. As the only
purpose of the Range is for extracting plain text in it, this patch
removes the Range, and precomputes and stores the plain text instead.

This also changes SurroundingText into POD so that we can remove the
class wrapping with WebSurroundingText later.

Bug:  757719 
Change-Id: I18bc05dfcde62b0c24d086227cc6586678263449
Reviewed-on: https://chromium-review.googlesource.com/843227
Reviewed-by: Ryan Landay <rlanday@chromium.org>
Commit-Queue: Xiaocheng Hu <xiaochengh@chromium.org>
Cr-Commit-Position: refs/heads/master@{#526110}
[modify] https://crrev.com/da8f9ea66e37e7f32f092afb9c3fb133bbdf4e1b/third_party/WebKit/Source/core/editing/SurroundingText.cpp
[modify] https://crrev.com/da8f9ea66e37e7f32f092afb9c3fb133bbdf4e1b/third_party/WebKit/Source/core/editing/SurroundingText.h

Project Member

Comment 6 by bugdroid1@chromium.org, Dec 23 2017

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

commit 6fdf4287485fa9a60ef9e07db99cd38d23d00d0c
Author: Xiaocheng Hu <xiaochengh@chromium.org>
Date: Sat Dec 23 00:51:05 2017

Convert SurroundingText-related layout tests into gtests

Since SurroundingText API is not directly exposed to Web, it's better to
test it with gtest instead of layout test.

This patch:
- Removes the layout test API from Internals
- Removes surrounding-text-detached-no-crash.html, which is only for
  testing the robustness of the removed Internals API ( crbug.com/509860 )
- Converts surrounding-text.html into SurroundingTextTest unit tests.

This is also a preparation patch for removing the wrapping class
WebSurroundingText (crrev.com/c/841629).

Bug:  757719 
Cq-Include-Trybots: master.tryserver.chromium.linux:linux_layout_tests_layout_ng
Change-Id: Ica6003efd856156a7525239b10cc3988d08be8aa
Reviewed-on: https://chromium-review.googlesource.com/843056
Reviewed-by: Emil A Eklund <eae@chromium.org>
Reviewed-by: Ryan Landay <rlanday@chromium.org>
Commit-Queue: Xiaocheng Hu <xiaochengh@chromium.org>
Cr-Commit-Position: refs/heads/master@{#526113}
[modify] https://crrev.com/6fdf4287485fa9a60ef9e07db99cd38d23d00d0c/third_party/WebKit/LayoutTests/FlagExpectations/enable-blink-features=LayoutNG
[delete] https://crrev.com/1508f4a32a3fb19d6e3599acc0bcc817ebe6c5be/third_party/WebKit/LayoutTests/editing/surrounding-text/surrounding-text-detached-no-crash-expected.txt
[delete] https://crrev.com/1508f4a32a3fb19d6e3599acc0bcc817ebe6c5be/third_party/WebKit/LayoutTests/editing/surrounding-text/surrounding-text-detached-no-crash.html
[delete] https://crrev.com/1508f4a32a3fb19d6e3599acc0bcc817ebe6c5be/third_party/WebKit/LayoutTests/editing/surrounding-text/surrounding-text-expected.txt
[delete] https://crrev.com/1508f4a32a3fb19d6e3599acc0bcc817ebe6c5be/third_party/WebKit/LayoutTests/editing/surrounding-text/surrounding-text.html
[modify] https://crrev.com/6fdf4287485fa9a60ef9e07db99cd38d23d00d0c/third_party/WebKit/Source/core/editing/SurroundingTextTest.cpp
[modify] https://crrev.com/6fdf4287485fa9a60ef9e07db99cd38d23d00d0c/third_party/WebKit/Source/core/testing/Internals.cpp
[modify] https://crrev.com/6fdf4287485fa9a60ef9e07db99cd38d23d00d0c/third_party/WebKit/Source/core/testing/Internals.h
[modify] https://crrev.com/6fdf4287485fa9a60ef9e07db99cd38d23d00d0c/third_party/WebKit/Source/core/testing/Internals.idl

Project Member

Comment 7 by bugdroid1@chromium.org, Dec 28 2017

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

commit d8f75673c207f2717d14eb60e6f952a89e7c0d76
Author: Xiaocheng Hu <xiaochengh@chromium.org>
Date: Thu Dec 28 07:28:26 2017

Merge SurroundingText into WebSurroundingText

SurroundingText is exposed to outside Blink using a wrapper class
WebSurroundingText. With SurroundingText already changed into POD
type by crrev.com/c/843227, and there is no other usage of
SurroundingText, such wrapping is no longer necessary.

Hence, this patch merges SurroundingText into WebSurroundingText
to simplify the code base.

Bug:  757719 , 731490
Change-Id: Ia1aef602feee3f0e12da828657f183a1621aaa44
Reviewed-on: https://chromium-review.googlesource.com/841629
Commit-Queue: Xiaocheng Hu <xiaochengh@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Cr-Commit-Position: refs/heads/master@{#526280}
[modify] https://crrev.com/d8f75673c207f2717d14eb60e6f952a89e7c0d76/third_party/WebKit/Source/core/editing/BUILD.gn
[delete] https://crrev.com/ccf8a33bef7c4736b3bfb98e742debf71fd3cfc4/third_party/WebKit/Source/core/editing/SurroundingText.cpp
[delete] https://crrev.com/ccf8a33bef7c4736b3bfb98e742debf71fd3cfc4/third_party/WebKit/Source/core/editing/SurroundingText.h
[modify] https://crrev.com/d8f75673c207f2717d14eb60e6f952a89e7c0d76/third_party/WebKit/Source/core/editing/SurroundingTextTest.cpp
[modify] https://crrev.com/d8f75673c207f2717d14eb60e6f952a89e7c0d76/third_party/WebKit/Source/core/exported/WebSurroundingText.cpp
[modify] https://crrev.com/d8f75673c207f2717d14eb60e6f952a89e7c0d76/third_party/WebKit/public/web/WebSurroundingText.h

Project Member

Comment 8 by bugdroid1@chromium.org, Dec 30 2017

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

commit 97c55c70870d9b2c24742d3ea5ccc8eaa1102123
Author: Xiaocheng Hu <xiaochengh@chromium.org>
Date: Sat Dec 30 02:07:43 2017

Move editing/SurroundingTextTest.cpp to core/exported

This patch finishes a TODO left in a previous patch crrev.com/c/841629,
which only modified the file without renaming it in order not to break
the git revision log.

Bug:  757719 
Change-Id: If3aa6a7b650f90133ffe0bcca20ee0ac05f01422
Reviewed-on: https://chromium-review.googlesource.com/845075
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Commit-Queue: Xiaocheng Hu <xiaochengh@chromium.org>
Cr-Commit-Position: refs/heads/master@{#526404}
[modify] https://crrev.com/97c55c70870d9b2c24742d3ea5ccc8eaa1102123/third_party/WebKit/Source/core/BUILD.gn
[modify] https://crrev.com/97c55c70870d9b2c24742d3ea5ccc8eaa1102123/third_party/WebKit/Source/core/editing/BUILD.gn
[rename] https://crrev.com/97c55c70870d9b2c24742d3ea5ccc8eaa1102123/third_party/WebKit/Source/core/exported/WebSurroundingTextTest.cpp

Project Member

Comment 9 by bugdroid1@chromium.org, Jan 4 2018

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

commit 0bee1776aaaf2ca018f926cd1f880af172969e45
Author: Xiaocheng Hu <xiaochengh@chromium.org>
Date: Thu Jan 04 17:48:33 2018

Clean up WebSurroundingText public interface

This patch cleans up the public interface of WebSurroundingText that:
- IsNull() is renamed to IsEmpty() to be more intuitive;
- InitializeFromCurrentSelection() and InitializeFromRange() are
  changed into constructors to simplify instance creation, and also
  prevent the initialize functions from being called more than once.

It also cleans up the variable name |surroundingText| to cope with
the Chromium coding style.

Bug:  757719 
Change-Id: I5d0248704b46dab3adbad33f1353d5bd709560c7
Reviewed-on: https://chromium-review.googlesource.com/845359
Reviewed-by: Chris Harrelson <chrishtr@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Commit-Queue: Xiaocheng Hu <xiaochengh@chromium.org>
Cr-Commit-Position: refs/heads/master@{#527030}
[modify] https://crrev.com/0bee1776aaaf2ca018f926cd1f880af172969e45/content/renderer/render_frame_impl.cc
[modify] https://crrev.com/0bee1776aaaf2ca018f926cd1f880af172969e45/third_party/WebKit/Source/core/exported/WebSurroundingText.cpp
[modify] https://crrev.com/0bee1776aaaf2ca018f926cd1f880af172969e45/third_party/WebKit/Source/core/exported/WebSurroundingTextTest.cpp
[modify] https://crrev.com/0bee1776aaaf2ca018f926cd1f880af172969e45/third_party/WebKit/public/web/WebSurroundingText.h

Status: Fixed (was: Available)

Sign in to add a comment