Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2750833002:
TextControlElement::selection() should return SelectionInDOMTree. (Closed)
DescriptionTextControlElement::selection() should return SelectionInDOMTree.
Currently |TextControlElement::selection()| is called
only by Editor::selectionForCommand()
and returned |Range| is used for constructing |EphemeralRange|,
which again is used for constructing |SelectionInDOMTree|.
This CL makes |TextControlElement::selection()|
to return |SelectionInDOMTree| and avoid creation of Range.
BUG=691193
Review-Url: https://codereview.chromium.org/2750833002
Cr-Commit-Position: refs/heads/master@{#457367}
Committed: https://chromium.googlesource.com/chromium/src/+/e3ae803e5499bf5cc3104f06152b491b59668f88
Patch Set 1 #
Total comments: 4
Patch Set 2 : Y #Patch Set 3 : Y #
Total comments: 7
Patch Set 4 : Y #
Messages
Total messages: 25 (16 generated)
Description was changed from ========== Update TextControlSelection selection API TextControlSelection should is currently returning Range, changed it to return SelectionInDOMTree as Range creation is not required. BUG=691193 ========== to ========== Update TextControlSelection selection API TextControlSelection should is currently returning Range, changed it to return SelectionInDOMTree as Range creation is not required. BUG=691193 R=SamsungPeerReview ==========
tanvir.rizvi@samsung.com changed reviewers: + shanmuga.m@samsung.com
Description was changed from ========== Update TextControlSelection selection API TextControlSelection should is currently returning Range, changed it to return SelectionInDOMTree as Range creation is not required. BUG=691193 R=SamsungPeerReview ========== to ========== Update TextControlSelection selection API TextControlSelection is currently returning Range, changed it to return SelectionInDOMTree as Range creation is not required. BUG=691193 R=SamsungPeerReview ==========
https://codereview.chromium.org/2750833002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/html/TextControlElement.cpp (right): https://codereview.chromium.org/2750833002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/html/TextControlElement.cpp:648: We need to keep. if (!startNode || !endNode) return SelectionInDOMTree(); https://codereview.chromium.org/2750833002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/html/TextControlElement.h (right): https://codereview.chromium.org/2750833002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/html/TextControlElement.h:34: namespace blink { Keep empty line as it is.
https://codereview.chromium.org/2750833002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/html/TextControlElement.h (right): https://codereview.chromium.org/2750833002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/html/TextControlElement.h:34: namespace blink { On 2017/03/14 14:31:48, Shanmuga Pandi wrote: > Keep empty line as it is. Done.
https://codereview.chromium.org/2750833002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/html/TextControlElement.cpp (right): https://codereview.chromium.org/2750833002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/html/TextControlElement.cpp:648: On 2017/03/14 14:31:48, Shanmuga Pandi wrote: > We need to keep. > > if (!startNode || !endNode) > return SelectionInDOMTree(); Done.
Peer review looks good to me
Description was changed from ========== Update TextControlSelection selection API TextControlSelection is currently returning Range, changed it to return SelectionInDOMTree as Range creation is not required. BUG=691193 R=SamsungPeerReview ========== to ========== Update TextControlSelection selection API TextControlSelection is currently returning Range, changed it to return SelectionInDOMTree as Range creation is not required. BUG=691193 ==========
tanvir.rizvi@samsung.com changed reviewers: + yosin@chromium.org
tanvir.rizvi@samsung.com changed reviewers: + xiaochengh@chromium.org
yosin@chromium.org changed reviewers: + tkent@chromium.org
lgtm +tkent@ for core/html changes. https://codereview.chromium.org/2750833002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/TextControlElement.cpp (right): https://codereview.chromium.org/2750833002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/TextControlElement.cpp:626: .setIsDirectional(selectionDirection() != "none" ? true : false) nit: No need to have |? true : false| https://codereview.chromium.org/2750833002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/TextControlElement.cpp:654: .setIsDirectional(selectionDirection() != "none" ? true : false) nit: No need to have |? true : false| https://codereview.chromium.org/2750833002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/TextControlElement.h (right): https://codereview.chromium.org/2750833002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/TextControlElement.h:103: const SelectionInDOMTree selection() const; nit: s/const SelecitonInDOMTree/SelectionInDOMTree/
Updated for nit. PTAL https://codereview.chromium.org/2750833002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/TextControlElement.cpp (right): https://codereview.chromium.org/2750833002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/TextControlElement.cpp:626: .setIsDirectional(selectionDirection() != "none" ? true : false) On 2017/03/15 09:29:28, yosin_UTC9 wrote: > nit: No need to have |? true : false| Done. https://codereview.chromium.org/2750833002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/TextControlElement.cpp:626: .setIsDirectional(selectionDirection() != "none" ? true : false) On 2017/03/15 09:29:28, yosin_UTC9 wrote: > nit: No need to have |? true : false| Done. https://codereview.chromium.org/2750833002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/TextControlElement.cpp:654: .setIsDirectional(selectionDirection() != "none" ? true : false) On 2017/03/15 09:29:28, yosin_UTC9 wrote: > nit: No need to have |? true : false| Done. https://codereview.chromium.org/2750833002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/TextControlElement.h (right): https://codereview.chromium.org/2750833002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/TextControlElement.h:103: const SelectionInDOMTree selection() const; On 2017/03/15 09:29:28, yosin_UTC9 wrote: > nit: s/const SelecitonInDOMTree/SelectionInDOMTree/ Done.
The CQ bit was checked by shanmuga.m@samsung.com 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 unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm CL description: > Update TextControlSelection selection API > > TextControlSelection is currently returning Range, The description isn't precise. What's 'TextControlSelection'? I suggest something like the following: ------------ TextControlElement::selection() should return SelectionInDOMTree. Currently it returns Range, ... ------------
Description was changed from ========== Update TextControlSelection selection API TextControlSelection is currently returning Range, changed it to return SelectionInDOMTree as Range creation is not required. BUG=691193 ========== to ========== TextControlElement::selection() should return SelectionInDOMTree. Currently |TextControlElement::selection()| is called only by Editor::selectionForCommand() and returned |Range| is used for constructing |EphemeralRange|, which again is used for constructing |SelectionInDOMTree|. This CL makes |TextControlElement::selection()| to return |SelectionInDOMTree| and avoid creation of Range. BUG=691193 ==========
The CQ bit was checked by tanvir.rizvi@samsung.com
The patchset sent to the CQ was uploaded after l-g-t-m from yosin@chromium.org Link to the patchset: https://codereview.chromium.org/2750833002/#ps60001 (title: "Y")
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": 1489646723442960,
"parent_rev": "2d05f4450c89d4ff77990fc736637e9a21cf90a4", "commit_rev":
"e3ae803e5499bf5cc3104f06152b491b59668f88"}
Message was sent while issue was closed.
Description was changed from ========== TextControlElement::selection() should return SelectionInDOMTree. Currently |TextControlElement::selection()| is called only by Editor::selectionForCommand() and returned |Range| is used for constructing |EphemeralRange|, which again is used for constructing |SelectionInDOMTree|. This CL makes |TextControlElement::selection()| to return |SelectionInDOMTree| and avoid creation of Range. BUG=691193 ========== to ========== TextControlElement::selection() should return SelectionInDOMTree. Currently |TextControlElement::selection()| is called only by Editor::selectionForCommand() and returned |Range| is used for constructing |EphemeralRange|, which again is used for constructing |SelectionInDOMTree|. This CL makes |TextControlElement::selection()| to return |SelectionInDOMTree| and avoid creation of Range. BUG=691193 Review-Url: https://codereview.chromium.org/2750833002 Cr-Commit-Position: refs/heads/master@{#457367} Committed: https://chromium.googlesource.com/chromium/src/+/e3ae803e5499bf5cc3104f06152b... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/e3ae803e5499bf5cc3104f06152b... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
