Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2919603005:
[DMC #27] Move TextMatchMarker method implementations to TextMatchMarker.cpp (Closed)
DescriptionMove TextMatchMarker method implementations to TextMatchMarker.cpp
TextMatchMarker used to be RenderedDocumentMarker, which didn't have a .cpp
file, and had all its method implementations in a header file. This CL moves
TextMatchMarker's method implementations to its .cpp file for cleanliness, as
well as consistency with the classes for the other marker types.
BUG=707867
Review-Url: https://codereview.chromium.org/2919603005
Cr-Commit-Position: refs/heads/master@{#476550}
Committed: https://chromium.googlesource.com/chromium/src/+/bb2881f8de65ec1c7d64d92ce414a5f108a86270
Patch Set 1 #
Total comments: 4
Patch Set 2 : Remove blank lines #Patch Set 3 : Move layout_state_ initialization inline #
Total comments: 1
Patch Set 4 : Update comment #
Depends on Patchset: Messages
Total messages: 24 (14 generated)
rlanday@chromium.org changed reviewers: + xiaochengh@chromium.org, yosin@chromium.org
The CQ bit was checked by rlanday@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm https://codereview.chromium.org/2919603005/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/editing/markers/TextMatchMarker.cpp (right): https://codereview.chromium.org/2919603005/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/markers/TextMatchMarker.cpp:13: layout_state_ = State::kInvalid; nit: This should be in the member initialization list. https://codereview.chromium.org/2919603005/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/editing/markers/TextMatchMarker.h (right): https://codereview.chromium.org/2919603005/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/markers/TextMatchMarker.h:58: nit: Could you remove the empty lines at L58, L60 and L62?
https://codereview.chromium.org/2919603005/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/editing/markers/TextMatchMarker.cpp (right): https://codereview.chromium.org/2919603005/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/markers/TextMatchMarker.cpp:13: layout_state_ = State::kInvalid; On 2017/06/01 at 21:42:49, Xiaocheng wrote: > nit: This should be in the member initialization list. yosin previously requested initializing this outside the list: https://codereview.chromium.org/2904093002#msg14
The CQ bit was checked by rlanday@chromium.org to run a CQ dry run
On 2017/06/01 at 21:42:50, xiaochengh wrote: > https://codereview.chromium.org/2919603005/diff/1/third_party/WebKit/Source/c... > third_party/WebKit/Source/core/editing/markers/TextMatchMarker.h:58: > nit: Could you remove the empty lines at L58, L60 and L62? Updated
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2919603005/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/editing/markers/TextMatchMarker.cpp (right): https://codereview.chromium.org/2919603005/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/markers/TextMatchMarker.cpp:13: layout_state_ = State::kInvalid; On 2017/06/01 at 21:48:13, rlanday wrote: > On 2017/06/01 at 21:42:49, Xiaocheng wrote: > > nit: This should be in the member initialization list. > > yosin previously requested initializing this outside the list: > https://codereview.chromium.org/2904093002#msg14 I think what he meant is to put the initialization directly in class declaration.
The CQ bit was checked by rlanday@chromium.org to run a CQ dry run
On 2017/06/01 at 22:12:49, xiaochengh wrote: > https://codereview.chromium.org/2919603005/diff/1/third_party/WebKit/Source/c... > File third_party/WebKit/Source/core/editing/markers/TextMatchMarker.cpp (right): > > https://codereview.chromium.org/2919603005/diff/1/third_party/WebKit/Source/c... > third_party/WebKit/Source/core/editing/markers/TextMatchMarker.cpp:13: layout_state_ = State::kInvalid; > On 2017/06/01 at 21:48:13, rlanday wrote: > > On 2017/06/01 at 21:42:49, Xiaocheng wrote: > > > nit: This should be in the member initialization list. > > > > yosin previously requested initializing this outside the list: > > https://codereview.chromium.org/2904093002#msg14 > > I think what he meant is to put the initialization directly in class declaration. Updated
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
still lgtm, thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
lgtm w/ nits Thanks for doing this! https://codereview.chromium.org/2919603005/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/markers/TextMatchMarker.cpp (right): https://codereview.chromium.org/2919603005/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/markers/TextMatchMarker.cpp:49: // Now |m_renderedRect| can not be accessed until |setRenderedRect| is nit: s/m_renderedRect/rendered_rect_/ nit: s/setRenderedRect/SetRenderedRect/
The CQ bit was checked by rlanday@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from yosin@chromium.org, xiaochengh@chromium.org Link to the patchset: https://codereview.chromium.org/2919603005/#ps60001 (title: "Update comment")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1496366539143090,
"parent_rev": "2bf40442439ae96e02cd774b986327f694bfcce8", "commit_rev":
"c2d516efeed215f859a7fa31c6c989423840cb59"}
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1496366539143090,
"parent_rev": "bce5642dabf9a4f18cdcc7b88d7dce9a1eaae4de", "commit_rev":
"bb2881f8de65ec1c7d64d92ce414a5f108a86270"}
Message was sent while issue was closed.
Description was changed from ========== Move TextMatchMarker method implementations to TextMatchMarker.cpp TextMatchMarker used to be RenderedDocumentMarker, which didn't have a .cpp file, and had all its method implementations in a header file. This CL moves TextMatchMarker's method implementations to its .cpp file for cleanliness, as well as consistency with the classes for the other marker types. BUG=707867 ========== to ========== Move TextMatchMarker method implementations to TextMatchMarker.cpp TextMatchMarker used to be RenderedDocumentMarker, which didn't have a .cpp file, and had all its method implementations in a header file. This CL moves TextMatchMarker's method implementations to its .cpp file for cleanliness, as well as consistency with the classes for the other marker types. BUG=707867 Review-Url: https://codereview.chromium.org/2919603005 Cr-Commit-Position: refs/heads/master@{#476550} Committed: https://chromium.googlesource.com/chromium/src/+/bb2881f8de65ec1c7d64d92ce414... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/bb2881f8de65ec1c7d64d92ce414... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
