Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2493873002:
Introduce Internals#replaceMisspelled() and Internals#setMarker() (Closed)
DescriptionIntroduce Internals#replaceMisspelled() and Internals#setMarker()
This patch introduce setMarker for ease of writing test case with document markers
and replaceMisspelled for replace misspelled.
This patch is a preparation of http://crrev.com/2457523003 which uses
Internals#replaceMisspelled() and Internals#setMarker()
BUG=664231
TEST=n/a; no behavior changes.
Committed: https://crrev.com/af7933e7d353a66f958961bc8f1b2f398378d84c
Cr-Commit-Position: refs/heads/master@{#432123}
Patch Set 1 #Patch Set 2 : add layout test #
Total comments: 6
Patch Set 3 : yosin's comments addressed #
Total comments: 1
Patch Set 4 : change to optional #
Total comments: 3
Patch Set 5 : for yosin's nit #
Messages
Total messages: 27 (13 generated)
Description was changed from ========== done done BUG= ========== to ========== add setMarker and replaceMisspelled to Internals, also add exception in spell-check related. BUG=664231 ==========
chaopeng@chromium.org changed reviewers: + xiaochengh@chromium.org, yosin@chromium.org
Separate the internals changes from this patch https://codereview.chromium.org/2457523003 to here. PTAL. Thank you.
Code changes is good. Could you split this pathc into two patches: - introducing setMarker and replaceMIspelled - Changing IDL to add RaiseException attribute Also please refer https://codereview.chromium.org/2457523003 in description as use case.
Description was changed from ========== add setMarker and replaceMisspelled to Internals, also add exception in spell-check related. BUG=664231 ========== to ========== add setMarker and replaceMisspelled to Internals. Use cas: https://codereview.chromium.org/2457523003 BUG=664231 ==========
Description was changed from ========== add setMarker and replaceMisspelled to Internals. Use cas: https://codereview.chromium.org/2457523003 BUG=664231 ========== to ========== add setMarker and replaceMisspelled to Internals. Use case: https://codereview.chromium.org/2457523003 BUG=664231 ==========
On 2016/11/11 01:44:00, Yosi_UTC9 wrote: > Code changes is good. > > Could you split this pathc into two patches: > - introducing setMarker and replaceMIspelled > - Changing IDL to add RaiseException attribute > > > Also please refer https://codereview.chromium.org/2457523003 in description as > use case. This patch now only include setMarker and replaceMIspelled. Also I added a layout test for it. PTAL. Thank you.
Please update summary and description: - summary should not include URL - description should explain what, why, e.g. this patch introduce XXX for ease of writing test case with document markers. https://codereview.chromium.org/2493873002/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/editing/spelling/spellcheck-marker.html (right): https://codereview.chromium.org/2493873002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/editing/spelling/spellcheck-marker.html:11: '<div contenteditable="true">^appla| </div>', nit: we don't need to have |="true"| https://codereview.chromium.org/2493873002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/editing/spelling/spellcheck-marker.html:13: let document = selection.document; nit: s/let/const/ https://codereview.chromium.org/2493873002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/editing/spelling/spellcheck-marker.html:14: let begin = selection.anchorOffset; nit: s/let/const/ nit: s/begin/start/ We usually use |start| instead of |begin|. https://codereview.chromium.org/2493873002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/editing/spelling/spellcheck-marker.html:15: let end = selection.focusOffset; nit: s/let/const/ https://codereview.chromium.org/2493873002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/testing/Internals.cpp (right): https://codereview.chromium.org/2493873002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/testing/Internals.cpp:189: static bool markerTypesFrom(const String& markerType, Please add a comment for meaning of |bool| return value. https://codereview.chromium.org/2493873002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/testing/Internals.cpp:192: if (markerType.isEmpty() || equalIgnoringCase(markerType, "all")) should be early return style if (markerType.isEmpty() || equalIgnoringCase(markerType, "all")) return DocumentMarker::AllMarkers(); DocumentMarker::MarkerType type; if (markerTypeFrom(markerType, type)) return true return false;
Description was changed from ========== add setMarker and replaceMisspelled to Internals. Use case: https://codereview.chromium.org/2457523003 BUG=664231 ========== to ========== This patch introduce setMarker for ease of writing test case with document markers and replaceMisspelled for replace misspelled. BUG=664231 ==========
Description was changed from ========== This patch introduce setMarker for ease of writing test case with document markers and replaceMisspelled for replace misspelled. BUG=664231 ========== to ========== This patch introduce setMarker for ease of writing test case with document markers and replaceMisspelled for replace misspelled. BUG=664231 ==========
On 2016/11/14 02:28:18, Yosi_UTC9 wrote: > Please update summary and description: > - summary should not include URL > - description should explain what, why, e.g. this patch introduce XXX for ease > of writing test case with document markers. > > https://codereview.chromium.org/2493873002/diff/20001/third_party/WebKit/Layo... > File third_party/WebKit/LayoutTests/editing/spelling/spellcheck-marker.html > (right): > > https://codereview.chromium.org/2493873002/diff/20001/third_party/WebKit/Layo... > third_party/WebKit/LayoutTests/editing/spelling/spellcheck-marker.html:11: '<div > contenteditable="true">^appla| </div>', > nit: we don't need to have |="true"| > > https://codereview.chromium.org/2493873002/diff/20001/third_party/WebKit/Layo... > third_party/WebKit/LayoutTests/editing/spelling/spellcheck-marker.html:13: let > document = selection.document; > nit: s/let/const/ > > https://codereview.chromium.org/2493873002/diff/20001/third_party/WebKit/Layo... > third_party/WebKit/LayoutTests/editing/spelling/spellcheck-marker.html:14: let > begin = selection.anchorOffset; > nit: s/let/const/ > nit: s/begin/start/ > > We usually use |start| instead of |begin|. > > https://codereview.chromium.org/2493873002/diff/20001/third_party/WebKit/Layo... > third_party/WebKit/LayoutTests/editing/spelling/spellcheck-marker.html:15: let > end = selection.focusOffset; > nit: s/let/const/ > > https://codereview.chromium.org/2493873002/diff/20001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/testing/Internals.cpp (right): > > https://codereview.chromium.org/2493873002/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/testing/Internals.cpp:189: static bool > markerTypesFrom(const String& markerType, > Please add a comment for meaning of |bool| return value. > > https://codereview.chromium.org/2493873002/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/testing/Internals.cpp:192: if > (markerType.isEmpty() || equalIgnoringCase(markerType, "all")) > should be early return style > > if (markerType.isEmpty() || equalIgnoringCase(markerType, "all")) > return DocumentMarker::AllMarkers(); > DocumentMarker::MarkerType type; > if (markerTypeFrom(markerType, type)) > return true > return false; Updated PTAL. Thank you.
Please make description line as following format: Title\n Description I could write: Introduce Internals#replaceMisspelled() and Internals#setMarker() This patch blah blah blah. This patch is a preparation of http://crrev.com/2457523003 which uses Internals#replaceMisspelled() and Internals#setMarker() BUG=664231 TEST=n/a; no behavior changes. https://codereview.chromium.org/2493873002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/testing/Internals.cpp (right): https://codereview.chromium.org/2493873002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/testing/Internals.cpp:176: static bool markerTypeFrom(const String& markerType, How about returning |WTF::Optional<DocumetnMarker::MarkerType>|, == std::optional<T> in C++17? https://chromium.googlesource.com/chromium/src/+/master/third_party/WebKit/So...
Description was changed from ========== This patch introduce setMarker for ease of writing test case with document markers and replaceMisspelled for replace misspelled. BUG=664231 ========== to ========== Introduce Internals#replaceMisspelled() and Internals#setMarker() This patch introduce setMarker for ease of writing test case with document markers and replaceMisspelled for replace misspelled. This patch is a preparation of http://crrev.com/2457523003 which uses Internals#replaceMisspelled() and Internals#setMarker() BUG=664231 TEST=n/a; no behavior changes. ==========
On 2016/11/14 05:18:59, Yosi_UTC9 wrote: > Please make description line as following format: > > Title\n > Description > > I could write: > > Introduce Internals#replaceMisspelled() and Internals#setMarker() > > This patch blah blah blah. > > This patch is a preparation of http://crrev.com/2457523003 which uses > Internals#replaceMisspelled() and Internals#setMarker() > > BUG=664231 > TEST=n/a; no behavior changes. > > https://codereview.chromium.org/2493873002/diff/40001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/testing/Internals.cpp (right): > > https://codereview.chromium.org/2493873002/diff/40001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/testing/Internals.cpp:176: static bool > markerTypeFrom(const String& markerType, > How about returning |WTF::Optional<DocumetnMarker::MarkerType>|, == > std::optional<T> in C++17? > https://chromium.googlesource.com/chromium/src/+/master/third_party/WebKit/So... PTAL. Thank you.
On 2016/11/14 at 06:00:34, chaopeng wrote: > On 2016/11/14 05:18:59, Yosi_UTC9 wrote: > > Please make description line as following format: > > > > Title\n > > Description > > > > I could write: > > > > Introduce Internals#replaceMisspelled() and Internals#setMarker() > > > > This patch blah blah blah. > > > > This patch is a preparation of http://crrev.com/2457523003 which uses > > Internals#replaceMisspelled() and Internals#setMarker() > > > > BUG=664231 > > TEST=n/a; no behavior changes. > > > > https://codereview.chromium.org/2493873002/diff/40001/third_party/WebKit/Sour... > > File third_party/WebKit/Source/core/testing/Internals.cpp (right): > > > > https://codereview.chromium.org/2493873002/diff/40001/third_party/WebKit/Sour... > > third_party/WebKit/Source/core/testing/Internals.cpp:176: static bool > > markerTypeFrom(const String& markerType, > > How about returning |WTF::Optional<DocumetnMarker::MarkerType>|, == > > std::optional<T> in C++17? > > https://chromium.googlesource.com/chromium/src/+/master/third_party/WebKit/So... > > PTAL. Thank you. Let's use WTF::Optional.
On 2016/11/14 06:06:37, Yosi_UTC9 wrote: > On 2016/11/14 at 06:00:34, chaopeng wrote: > > On 2016/11/14 05:18:59, Yosi_UTC9 wrote: > > > Please make description line as following format: > > > > > > Title\n > > > Description > > > > > > I could write: > > > > > > Introduce Internals#replaceMisspelled() and Internals#setMarker() > > > > > > This patch blah blah blah. > > > > > > This patch is a preparation of http://crrev.com/2457523003 which uses > > > Internals#replaceMisspelled() and Internals#setMarker() > > > > > > BUG=664231 > > > TEST=n/a; no behavior changes. > > > > > > > https://codereview.chromium.org/2493873002/diff/40001/third_party/WebKit/Sour... > > > File third_party/WebKit/Source/core/testing/Internals.cpp (right): > > > > > > > https://codereview.chromium.org/2493873002/diff/40001/third_party/WebKit/Sour... > > > third_party/WebKit/Source/core/testing/Internals.cpp:176: static bool > > > markerTypeFrom(const String& markerType, > > > How about returning |WTF::Optional<DocumetnMarker::MarkerType>|, == > > > std::optional<T> in C++17? > > > > https://chromium.googlesource.com/chromium/src/+/master/third_party/WebKit/So... > > > > PTAL. Thank you. > > Let's use WTF::Optional. Yes, the patch is using Optional
yosin@chromium.org changed reviewers: + tkent@chromium.org
lgtm w/ nits +tkent@ for OWNERS review https://codereview.chromium.org/2493873002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/testing/Internals.cpp (right): https://codereview.chromium.org/2493873002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/testing/Internals.cpp:184: nit: Please get rid of an extra blank line. https://codereview.chromium.org/2493873002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/testing/Internals.cpp:194: return static_cast<DocumentMarker::MarkerTypes>(type.value()); Can we use |DocumentMarker::MarkerTypes(type.value())|? https://codereview.chromium.org/2493873002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/testing/Internals.cpp:196: return WTF::nullopt; It is more readable switching if-condition: if (!type) return WTF::nullopt; return DocumentMarker::MarkerTypes(type.value());
lgtm
chaopeng@chromium.org changed reviewers: - xiaochengh@chromium.org
The CQ bit was checked by chaopeng@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tkent@chromium.org, yosin@chromium.org Link to the patchset: https://codereview.chromium.org/2493873002/#ps80001 (title: "for yosin's nit")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Introduce Internals#replaceMisspelled() and Internals#setMarker() This patch introduce setMarker for ease of writing test case with document markers and replaceMisspelled for replace misspelled. This patch is a preparation of http://crrev.com/2457523003 which uses Internals#replaceMisspelled() and Internals#setMarker() BUG=664231 TEST=n/a; no behavior changes. ========== to ========== Introduce Internals#replaceMisspelled() and Internals#setMarker() This patch introduce setMarker for ease of writing test case with document markers and replaceMisspelled for replace misspelled. This patch is a preparation of http://crrev.com/2457523003 which uses Internals#replaceMisspelled() and Internals#setMarker() BUG=664231 TEST=n/a; no behavior changes. ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Introduce Internals#replaceMisspelled() and Internals#setMarker() This patch introduce setMarker for ease of writing test case with document markers and replaceMisspelled for replace misspelled. This patch is a preparation of http://crrev.com/2457523003 which uses Internals#replaceMisspelled() and Internals#setMarker() BUG=664231 TEST=n/a; no behavior changes. ========== to ========== Introduce Internals#replaceMisspelled() and Internals#setMarker() This patch introduce setMarker for ease of writing test case with document markers and replaceMisspelled for replace misspelled. This patch is a preparation of http://crrev.com/2457523003 which uses Internals#replaceMisspelled() and Internals#setMarker() BUG=664231 TEST=n/a; no behavior changes. Committed: https://crrev.com/af7933e7d353a66f958961bc8f1b2f398378d84c Cr-Commit-Position: refs/heads/master@{#432123} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/af7933e7d353a66f958961bc8f1b2f398378d84c Cr-Commit-Position: refs/heads/master@{#432123} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
