Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2801483002:
Add DocumentMarker::MatchStatus enum for text match markers (Closed)
|
Created:
3 years, 8 months ago by rlanday Modified:
3 years, 8 months ago CC:
blink-reviews, blink-reviews-layout_chromium.org, chromium-reviews, eae+blinkwatch, jchaffraix+rendering, kinuko+watch, leviw+renderwatch, pdr+renderingwatchlist_chromium.org, szager+layoutwatch_chromium.org, zoltan1 Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd DocumentMarker::MatchStatus enum for text match markers
Suggested by @yosin here:
https://codereview.chromium.org/2801483002#msg20
Modify the DocumentMarker() constructor for text match markers and DocumentMarkerController::addTextMatchMarker() to take an enum for whether or not the text match is active instead of a bool. This will make it more clear in call sites what the parameter means.
BUG=707867
Review-Url: https://codereview.chromium.org/2801483002
Cr-Commit-Position: refs/heads/master@{#462303}
Committed: https://chromium.googlesource.com/chromium/src/+/c198feadcc54ea92134df2dc3a5f4f390fcd9493
Patch Set 1 #
Total comments: 2
Patch Set 2 : Use string in JavaScript API #
Total comments: 2
Patch Set 3 : Respond to comments #Patch Set 4 : Update tests #
Total comments: 5
Patch Set 5 : Split off Internals changes #Patch Set 6 : Only use enum in consturctor and DMC::addTextMatch() #Patch Set 7 : Rebase on renaming CLs that were landed #
Total comments: 1
Patch Set 8 : Add k prefix #Dependent Patchsets: Messages
Total messages: 54 (35 generated)
Description was changed from ========== Add DocumentMarker::MatchStatus enum for text match markers BUG= ========== to ========== Add DocumentMarker::MatchStatus enum for text match markers Suggested by @yosin here: https://codereview.chromium.org/2781623010#msg33 BUG=707867 ==========
rlanday@chromium.org changed reviewers: + xiaochengh@chromium.org, yosin@chromium.org
rlanday@chromium.org changed required reviewers: + 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...
https://codereview.chromium.org/2801483002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/testing/Internals.h (right): https://codereview.chromium.org/2801483002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/testing/Internals.h:196: bool isActive); Note: I didn't convert this to use an enum because this is used by IDL code (third_party/WebKit/Source/core/testing/Internals.idl) and I couldn't figure out if it's possible to use a C++ enum there
https://codereview.chromium.org/2801483002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/testing/Internals.h (right): https://codereview.chromium.org/2801483002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/testing/Internals.h:196: bool isActive); On 2017/04/04 at 20:48:23, rlanday wrote: > Note: I didn't convert this to use an enum because this is used by IDL code (third_party/WebKit/Source/core/testing/Internals.idl) and I couldn't figure out if it's possible to use a C++ enum there We just use string in IDLs. There's a lot of examples in Internals.
The CQ bit was checked by rlanday@chromium.org to run a CQ dry run
On 2017/04/04 at 21:11:16, xiaochengh wrote: > https://codereview.chromium.org/2801483002/diff/1/third_party/WebKit/Source/c... > File third_party/WebKit/Source/core/testing/Internals.h (right): > > https://codereview.chromium.org/2801483002/diff/1/third_party/WebKit/Source/c... > third_party/WebKit/Source/core/testing/Internals.h:196: bool isActive); > On 2017/04/04 at 20:48:23, rlanday wrote: > > Note: I didn't convert this to use an enum because this is used by IDL code (third_party/WebKit/Source/core/testing/Internals.idl) and I couldn't figure out if it's possible to use a C++ enum there > > We just use string in IDLs. There's a lot of examples in Internals. Ok, 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/2801483002/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/scrolling/scrollbar-tickmarks-styled.html (right): https://codereview.chromium.org/2801483002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/scrolling/scrollbar-tickmarks-styled.html:27: window.internals.addTextMatchMarker(range, "Active"); nit: use single quotes for JavaScript strings. https://codereview.chromium.org/2801483002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/testing/Internals.h (right): https://codereview.chromium.org/2801483002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/testing/Internals.h:198: bool isActive); Could you also change this one?
On 2017/04/04 at 21:48:38, xiaochengh wrote: > https://codereview.chromium.org/2801483002/diff/20001/third_party/WebKit/Layo... > File third_party/WebKit/LayoutTests/fast/scrolling/scrollbar-tickmarks-styled.html (right): > > https://codereview.chromium.org/2801483002/diff/20001/third_party/WebKit/Layo... > third_party/WebKit/LayoutTests/fast/scrolling/scrollbar-tickmarks-styled.html:27: window.internals.addTextMatchMarker(range, "Active"); > nit: use single quotes for JavaScript strings. > > https://codereview.chromium.org/2801483002/diff/20001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/testing/Internals.h (right): > > https://codereview.chromium.org/2801483002/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/testing/Internals.h:198: bool isActive); > Could you also change this one? Updated
All layout tests calling internals.addTextMarker or internals.setMarkersActive should be modified accordingly. Please run layout tests locally to ensure that all are changed. Another option is to leave internals untouched in this patch, since the priority is low. In this way, adding TODOs there is enough.
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...
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...
On 2017/04/04 at 23:20:41, xiaochengh wrote: > All layout tests calling internals.addTextMarker or internals.setMarkersActive should be modified accordingly. > > Please run layout tests locally to ensure that all are changed. > > Another option is to leave internals untouched in this patch, since the priority is low. In this way, adding TODOs there is enough. I updated the tests, let's see if the trybots all pass...
On 2017/04/04 at 23:20:41, xiaochengh wrote: > All layout tests calling internals.addTextMarker or internals.setMarkersActive should be modified accordingly. > > Please run layout tests locally to ensure that all are changed. > > Another option is to leave internals untouched in this patch, since the priority is low. In this way, adding TODOs there is enough. To make patch small, I like xiaochengh@'s suggestion to have TODO and move test changes to another patch.
Please keep in mind to make patch smaller. It seems we can have 1. Introduce enum class MatchStatus and use for ctor and DMC::addTextMatch() 2. Rename |bool activeMatch()| to |bool IsActiveMatch()| 3. Rename |void setActiveMatch(bool)| to |setIsActiveMatch(bool)| 4. Adopt Internals and layout tests The reason of avoiding |bool| parameter is it is hard to understand what |true| and |false| mean in call site. For setIsActive(true) and setIsActive(false), we can understand what |true| and |false| means since getter function IsActive() returns |bool|. In simplified rule, if getter returns |bool|, setter should take |bool|, assume getter name implies it returns bool, e.g. IsXXX. https://codereview.chromium.org/2801483002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/markers/DocumentMarker.cpp (left): https://codereview.chromium.org/2801483002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/markers/DocumentMarker.cpp:69: bool activeMatch() const { return m_match; } I think we can leave |bool activeMatch()| as is. We may want to rename this function to |isActiveMatch()| to better align with predicate naming. When we really, I think not, want to expose MatchState, we should have function |matchState()|. https://codereview.chromium.org/2801483002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/markers/DocumentMarker.cpp (right): https://codereview.chromium.org/2801483002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/markers/DocumentMarker.cpp:82: DocumentMarkerTextMatch, trueInstance, nit: s/trueInstance/activeInstance/ https://codereview.chromium.org/2801483002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/markers/DocumentMarker.cpp:85: DocumentMarkerTextMatch, falseInstance, nit: s/falseInstance/InactiveInstance/ https://codereview.chromium.org/2801483002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/markers/DocumentMarker.h (right): https://codereview.chromium.org/2801483002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/markers/DocumentMarker.h:164: I think we can use |bool| parameter for |setActiveMatch()|, because |bool| parameter implies active or inactive. Better name of this function is |setIsActiveMatch()|, and we can do in another patch. https://codereview.chromium.org/2801483002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/testing/Internals.idl (right): https://codereview.chromium.org/2801483002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/testing/Internals.idl:110: [RaisesException] void setMarkersActive(Node node, unsigned long startOffset, unsigned long endOffset, DOMString matchStatus); We don't need to change setMarkerActive() since we can read this boolean as active or inactive. If we want to pass |matchStatus|, it is better to call setMarkerMatchStatus().
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...
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...
On 2017/04/05 at 01:33:37, yosin wrote: > Please keep in mind to make patch smaller. > It seems we can have > > 1. Introduce enum class MatchStatus and use for ctor and DMC::addTextMatch() > 2. Rename |bool activeMatch()| to |bool IsActiveMatch()| > 3. Rename |void setActiveMatch(bool)| to |setIsActiveMatch(bool)| > 4. Adopt Internals and layout tests Ok, I updated this CL to be step 1, I'm going to create new CLs for the other three
Description was changed from ========== Add DocumentMarker::MatchStatus enum for text match markers Suggested by @yosin here: https://codereview.chromium.org/2781623010#msg33 BUG=707867 ========== to ========== Add DocumentMarker::MatchStatus enum for text match markers Suggested by @yosin here: https://codereview.chromium.org/2801483002#msg20 BUG=707867 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Add DocumentMarker::MatchStatus enum for text match markers Suggested by @yosin here: https://codereview.chromium.org/2801483002#msg20 BUG=707867 ========== to ========== Add DocumentMarker::MatchStatus enum for text match markers Suggested by @yosin here: https://codereview.chromium.org/2801483002#msg20 Modify the DocumentMarker() constructor for text match markers and DocumentMarkerController::addTextMatchMarker() to take an enum for whether or not the text match is active instead of a bool. This will make it more clear in call sites what the parameter means. BUG=707867 ==========
lgtm
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...
rlanday@chromium.org changed reviewers: + tkent@chromium.org
rlanday@chromium.org changed required reviewers: + tkent@chromium.org
Adding tkent for third_party/WebKit/Source/core/testing/Internals.cpp and third_party/WebKit/Source/web/TextFinder.cpp
https://codereview.chromium.org/2801483002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/markers/DocumentMarker.h (right): https://codereview.chromium.org/2801483002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/markers/DocumentMarker.h:137: enum class MatchStatus { Inactive, Active }; They should be "kInactive" and "kActive". https://www.chromium.org/blink/coding-style#TOC-Names > kInterCaps is preferable for new code. [names-enum-members]
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by rlanday@chromium.org to run a CQ dry run
On 2017/04/05 at 22:22:32, tkent wrote: > https://codereview.chromium.org/2801483002/diff/120001/third_party/WebKit/Sou... > File third_party/WebKit/Source/core/editing/markers/DocumentMarker.h (right): > > https://codereview.chromium.org/2801483002/diff/120001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/editing/markers/DocumentMarker.h:137: enum class MatchStatus { Inactive, Active }; > They should be "kInactive" and "kActive". > > https://www.chromium.org/blink/coding-style#TOC-Names > > kInterCaps is preferable for new code. [names-enum-members] Updated
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm
The CQ bit was unchecked by rlanday@chromium.org
The CQ bit was checked by rlanday@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from xiaochengh@chromium.org Link to the patchset: https://codereview.chromium.org/2801483002/#ps140001 (title: "Add k prefix")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
All required reviewers (with asterisk prefixes) have not yet approved this CL. No L-G-T-M from a valid reviewer yet. CQ run can only be started once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer,_not_ a full super star committer. Committers are members of the group "project-chromium-committers". Note that this has nothing to do with OWNERS files.
rlanday@chromium.org changed required reviewers: - yosin@chromium.org
The CQ bit was checked by rlanday@chromium.org
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": 140001, "attempt_start_ts": 1491439211766580,
"parent_rev": "a85021918767e9c6c8b1759ccb0e71e7b9c2a5db", "commit_rev":
"c198feadcc54ea92134df2dc3a5f4f390fcd9493"}
Message was sent while issue was closed.
Description was changed from ========== Add DocumentMarker::MatchStatus enum for text match markers Suggested by @yosin here: https://codereview.chromium.org/2801483002#msg20 Modify the DocumentMarker() constructor for text match markers and DocumentMarkerController::addTextMatchMarker() to take an enum for whether or not the text match is active instead of a bool. This will make it more clear in call sites what the parameter means. BUG=707867 ========== to ========== Add DocumentMarker::MatchStatus enum for text match markers Suggested by @yosin here: https://codereview.chromium.org/2801483002#msg20 Modify the DocumentMarker() constructor for text match markers and DocumentMarkerController::addTextMatchMarker() to take an enum for whether or not the text match is active instead of a bool. This will make it more clear in call sites what the parameter means. BUG=707867 Review-Url: https://codereview.chromium.org/2801483002 Cr-Commit-Position: refs/heads/master@{#462303} Committed: https://chromium.googlesource.com/chromium/src/+/c198feadcc54ea92134df2dc3a5f... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as https://chromium.googlesource.com/chromium/src/+/c198feadcc54ea92134df2dc3a5f... |
