Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
675413003:
WebLocalFrameImpl::selectWordAroundPosition should select a word before space. (Closed)
DescriptionWebLocalFrameImpl::selectWordAroundPosition should select a word before space.
If you have a "foo| ", WebLocalFrameImpl::selectWordAroundPosition
selects space after the caret.
This is from the Selection::expandUsingGranularity behavior.
It is used widely including double-click word selection on desktop.
Thus we can not change the behavior.
I changed the implementation to use VisibleUnit functions directly.
New implementation is that we try to find a word on right side and left side.
In addition, move implement from WebLocalFrameImpl to FrameSelection.
BUG=397696
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=185359
Patch Set 1 : update #
Total comments: 8
Patch Set 2 : Update #
Total comments: 1
Patch Set 3 : Use VisibleUnit functions and add test cases #
Total comments: 6
Patch Set 4 : nit picks #
Total comments: 10
Patch Set 5 : Adapt to selectWordAroundPosition too #
Messages
Total messages: 25 (9 generated)
Patchset #1 (id:1) has been deleted
yoichio@chromium.org changed reviewers: + donnd@chromium.org, yosin@chromium.org
Since this patch uses |isSeparator()|, this patch select a word before separator. https://codereview.chromium.org/675413003/diff/20001/Source/core/editing/Fram... File Source/core/editing/FrameSelection.cpp (right): https://codereview.chromium.org/675413003/diff/20001/Source/core/editing/Fram... Source/core/editing/FrameSelection.cpp:1934: void FrameSelection::selectWordAroundPosition(const VisiblePosition& position) Can we use |Position| rather than |VisiblePosition| for ease of use of this function? It seems this function can work with |Position|. https://codereview.chromium.org/675413003/diff/20001/Source/core/editing/Fram... Source/core/editing/FrameSelection.cpp:1938: if (isSeparator(position.characterAfter()) && !isSeparator(position.characterBefore())) { How about this case "foo |,"? https://codereview.chromium.org/675413003/diff/20001/Source/core/editing/Fram... Source/core/editing/FrameSelection.cpp:1945: String text = plainText(selection.start(), selection.end()); Should we check |isSeparator(c)| for character in |text|? https://codereview.chromium.org/675413003/diff/20001/Source/core/editing/Fram... Source/core/editing/FrameSelection.cpp:1949: setSelection(selection); Should we pass |WordGranuaryity| to |setSelection()| as original |selectWordAroundPosition()|?
https://codereview.chromium.org/675413003/diff/20001/Source/core/editing/Fram... File Source/core/editing/FrameSelection.cpp (right): https://codereview.chromium.org/675413003/diff/20001/Source/core/editing/Fram... Source/core/editing/FrameSelection.cpp:1934: void FrameSelection::selectWordAroundPosition(const VisiblePosition& position) On 2014/10/28 06:11:46, Yosi_UTC9 wrote: > Can we use |Position| rather than |VisiblePosition| for ease of use of this > function? It seems this function can work with |Position|. No, characterAfter and Before are VisiblePosition functions. https://codereview.chromium.org/675413003/diff/20001/Source/core/editing/Fram... Source/core/editing/FrameSelection.cpp:1938: if (isSeparator(position.characterAfter()) && !isSeparator(position.characterBefore())) { On 2014/10/28 06:11:46, Yosi_UTC9 wrote: > How about this case "foo |,"? To search far characters are too aggressive, I think. https://codereview.chromium.org/675413003/diff/20001/Source/core/editing/Fram... Source/core/editing/FrameSelection.cpp:1945: String text = plainText(selection.start(), selection.end()); On 2014/10/28 06:11:46, Yosi_UTC9 wrote: > Should we check |isSeparator(c)| for character in |text|? Done. https://codereview.chromium.org/675413003/diff/20001/Source/core/editing/Fram... Source/core/editing/FrameSelection.cpp:1949: setSelection(selection); On 2014/10/28 06:11:46, Yosi_UTC9 wrote: > Should we pass |WordGranuaryity| to |setSelection()| as original > |selectWordAroundPosition()|? Done.
donnd@google.com changed reviewers: + donnd@google.com
LGTM, with just a suggestion about more tests. Thanks for doing this Yoichi! https://codereview.chromium.org/675413003/diff/40001/Source/core/editing/Fram... File Source/core/editing/FrameSelectionTest.cpp (right): https://codereview.chromium.org/675413003/diff/40001/Source/core/editing/Fram... Source/core/editing/FrameSelectionTest.cpp:141: TEST_F(FrameSelectionTest, SelectWordAroundPosition) Maybe add a couple more tests, like the normal case (e.g. selects World at position 6) and the case where nothing can be selected? Or a TODO for this?
Since we make selectWordAroundPosition to be smart, I would like to see design doc for rationale of algorithm to show evidence the algorithm work all cases, e.g. "| foo", "foo| ", "fo|o", " |foo", and so on. Also, I recommend to use word handling functions in "VisibleUnits.cpp" for BIDI and other details about word. FrameSelection should not know details of word.
Patchset #3 (id:60001) has been deleted
On 2014/10/30 02:15:32, Yosi_UTC9 wrote: > Since we make selectWordAroundPosition to be smart, I would like to see design > doc for rationale of algorithm to show evidence the algorithm work all cases, > e.g. "| foo", "foo| ", "fo|o", " |foo", and so on. > Added test cases. > Also, I recommend to use word handling functions in "VisibleUnits.cpp" for BIDI > and other details about word. FrameSelection should not know details of word. Used those functions.
Ping.
yosin@chromium.org changed reviewers: - donnd@chromium.org
LGTM Please ask OWNERS review. Thanks! https://codereview.chromium.org/675413003/diff/80001/Source/core/editing/Fram... File Source/core/editing/FrameSelection.cpp (right): https://codereview.chromium.org/675413003/diff/80001/Source/core/editing/Fram... Source/core/editing/FrameSelection.cpp:1937: for (auto& wordSide : wordSideList) { Q: Do we need |&|? https://codereview.chromium.org/675413003/diff/80001/Source/core/editing/Fram... Source/core/editing/FrameSelection.cpp:1940: nit: We don't need to have a blank line. https://codereview.chromium.org/675413003/diff/80001/Source/core/editing/Fram... Source/core/editing/FrameSelection.cpp:1942: nit: We don't need to have a blank line.
yoichio@chromium.org changed reviewers: + yutak@chromium.org
Add yutak@, an owner. https://codereview.chromium.org/675413003/diff/80001/Source/core/editing/Fram... File Source/core/editing/FrameSelection.cpp (right): https://codereview.chromium.org/675413003/diff/80001/Source/core/editing/Fram... Source/core/editing/FrameSelection.cpp:1937: for (auto& wordSide : wordSideList) { On 2014/11/14 04:19:51, Yosi_UTC9 wrote: > Q: Do we need |&|? Used EWordSide instead. http://chromium-cpp.appspot.com/ https://codereview.chromium.org/675413003/diff/80001/Source/core/editing/Fram... Source/core/editing/FrameSelection.cpp:1940: On 2014/11/14 04:19:51, Yosi_UTC9 wrote: > nit: We don't need to have a blank line. Done. https://codereview.chromium.org/675413003/diff/80001/Source/core/editing/Fram... Source/core/editing/FrameSelection.cpp:1942: On 2014/11/14 04:19:51, Yosi_UTC9 wrote: > nit: We don't need to have a blank line. Done.
Just have one question, aside from style issues. BTW, I'm not an OWNER of web/, so I don't have enough power to approve the whole change. https://codereview.chromium.org/675413003/diff/100001/Source/core/editing/Fra... File Source/core/editing/FrameSelection.cpp (right): https://codereview.chromium.org/675413003/diff/100001/Source/core/editing/Fra... Source/core/editing/FrameSelection.cpp:1936: EWordSide wordSideList[2] = { RightWordIfOnBoundary, LeftWordIfOnBoundary }; static const https://codereview.chromium.org/675413003/diff/100001/Source/core/editing/Fra... File Source/core/editing/FrameSelection.h (right): https://codereview.chromium.org/675413003/diff/100001/Source/core/editing/Fra... Source/core/editing/FrameSelection.h:187: // Returnes true if a word is select. Returnes -> Returns or Return select -> selected https://codereview.chromium.org/675413003/diff/100001/Source/core/editing/Fra... File Source/core/editing/FrameSelectionTest.cpp (right): https://codereview.chromium.org/675413003/diff/100001/Source/core/editing/Fra... Source/core/editing/FrameSelectionTest.cpp:141: #define EXPECT_EQ_SELECTEDTEXT(text) \ SELECTED_TEXT https://codereview.chromium.org/675413003/diff/100001/Source/core/editing/Fra... Source/core/editing/FrameSelectionTest.cpp:142: EXPECT_EQ(text, WebString(selection().selectedText()).utf8()); 1. Semicolon is unnecessary. 2. Usually, macros like this are defined as: #define FOO(bar) do { baz(bar); } while (false) ... but in this case that may be a little bit overkill, so I'm fine with your version (modulo semicolon). https://codereview.chromium.org/675413003/diff/100001/Source/web/WebLocalFram... File Source/web/WebLocalFrameImpl.cpp (right): https://codereview.chromium.org/675413003/diff/100001/Source/web/WebLocalFram... Source/web/WebLocalFrameImpl.cpp:1163: frame->selection().setSelection(selection, granularity); Why don't you update this function as well?
yoichio@chromium.org changed reviewers: + tkent@chromium.org
yoichio@chromium.org changed reviewers: + tkent@chromium.org
Add tkent@, a web/ owner. https://codereview.chromium.org/675413003/diff/100001/Source/core/editing/Fra... File Source/core/editing/FrameSelection.cpp (right): https://codereview.chromium.org/675413003/diff/100001/Source/core/editing/Fra... Source/core/editing/FrameSelection.cpp:1936: EWordSide wordSideList[2] = { RightWordIfOnBoundary, LeftWordIfOnBoundary }; On 2014/11/14 05:14:56, Yuta Kitamura wrote: > static const Done. https://codereview.chromium.org/675413003/diff/100001/Source/core/editing/Fra... File Source/core/editing/FrameSelection.h (right): https://codereview.chromium.org/675413003/diff/100001/Source/core/editing/Fra... Source/core/editing/FrameSelection.h:187: // Returnes true if a word is select. On 2014/11/14 05:14:56, Yuta Kitamura wrote: > Returnes -> Returns or Return > select -> selected Done. https://codereview.chromium.org/675413003/diff/100001/Source/core/editing/Fra... File Source/core/editing/FrameSelectionTest.cpp (right): https://codereview.chromium.org/675413003/diff/100001/Source/core/editing/Fra... Source/core/editing/FrameSelectionTest.cpp:141: #define EXPECT_EQ_SELECTEDTEXT(text) \ On 2014/11/14 05:14:56, Yuta Kitamura wrote: > SELECTED_TEXT Done. https://codereview.chromium.org/675413003/diff/100001/Source/core/editing/Fra... Source/core/editing/FrameSelectionTest.cpp:142: EXPECT_EQ(text, WebString(selection().selectedText()).utf8()); On 2014/11/14 05:14:56, Yuta Kitamura wrote: > 1. Semicolon is unnecessary. > > 2. Usually, macros like this are defined as: > > #define FOO(bar) do { baz(bar); } while (false) > > ... but in this case that may be a little bit overkill, so I'm fine > with your version (modulo semicolon). Anyway, we need something bridges WTFString to utf8 for test output :) https://codereview.chromium.org/675413003/diff/100001/Source/web/WebLocalFram... File Source/web/WebLocalFrameImpl.cpp (right): https://codereview.chromium.org/675413003/diff/100001/Source/web/WebLocalFram... Source/web/WebLocalFrameImpl.cpp:1163: frame->selection().setSelection(selection, granularity); On 2014/11/14 05:14:57, Yuta Kitamura wrote: > Why don't you update this function as well? I was not sure but confirmed that selectWordAroundPosition is used from selectMisspelledWord which gets a text under mouse cursor thus it is OK.
Add tkent@, a web/ owner. https://codereview.chromium.org/675413003/diff/100001/Source/core/editing/Fra... File Source/core/editing/FrameSelection.cpp (right): https://codereview.chromium.org/675413003/diff/100001/Source/core/editing/Fra... Source/core/editing/FrameSelection.cpp:1936: EWordSide wordSideList[2] = { RightWordIfOnBoundary, LeftWordIfOnBoundary }; On 2014/11/14 05:14:56, Yuta Kitamura wrote: > static const Done. https://codereview.chromium.org/675413003/diff/100001/Source/core/editing/Fra... File Source/core/editing/FrameSelection.h (right): https://codereview.chromium.org/675413003/diff/100001/Source/core/editing/Fra... Source/core/editing/FrameSelection.h:187: // Returnes true if a word is select. On 2014/11/14 05:14:56, Yuta Kitamura wrote: > Returnes -> Returns or Return > select -> selected Done. https://codereview.chromium.org/675413003/diff/100001/Source/core/editing/Fra... File Source/core/editing/FrameSelectionTest.cpp (right): https://codereview.chromium.org/675413003/diff/100001/Source/core/editing/Fra... Source/core/editing/FrameSelectionTest.cpp:141: #define EXPECT_EQ_SELECTEDTEXT(text) \ On 2014/11/14 05:14:56, Yuta Kitamura wrote: > SELECTED_TEXT Done. https://codereview.chromium.org/675413003/diff/100001/Source/core/editing/Fra... Source/core/editing/FrameSelectionTest.cpp:142: EXPECT_EQ(text, WebString(selection().selectedText()).utf8()); On 2014/11/14 05:14:56, Yuta Kitamura wrote: > 1. Semicolon is unnecessary. > > 2. Usually, macros like this are defined as: > > #define FOO(bar) do { baz(bar); } while (false) > > ... but in this case that may be a little bit overkill, so I'm fine > with your version (modulo semicolon). Anyway, we need something bridges WTFString to utf8 for test output :) https://codereview.chromium.org/675413003/diff/100001/Source/web/WebLocalFram... File Source/web/WebLocalFrameImpl.cpp (right): https://codereview.chromium.org/675413003/diff/100001/Source/web/WebLocalFram... Source/web/WebLocalFrameImpl.cpp:1163: frame->selection().setSelection(selection, granularity); On 2014/11/14 05:14:57, Yuta Kitamura wrote: > Why don't you update this function as well? I was not sure but confirmed that selectWordAroundPosition is used from selectMisspelledWord which gets a text under mouse cursor thus it is OK.
lgtm
lgtm
The CQ bit was checked by yoichio@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/675413003/120001
Message was sent while issue was closed.
Committed patchset #5 (id:120001) as 185359 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
