Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2918783002:
Extract InlineBox handling from MostForwardCaretPosition() (Closed)
|
Created:
3 years, 6 months ago by yosin_UTC9 Modified:
3 years, 6 months ago CC:
blink-reviews, chromium-reviews Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionExtract InlineBox handling from MostForwardCaretPosition()
This patch extracts |InlineBox| handling code fragment into
|IsOffsetRenderedForForward()| for ease of adopting
|MostForwardCaretPosition()| to Layout NG.
Note: The patch[1] does same thing for |MostBackwardCaretPosition()|
[1] http://crrev.com/2919503003: Extract InlineBox handling from
MostBackwardCaretPosition()
BUG=707656
TEST=n/a; no behavior changes
Review-Url: https://codereview.chromium.org/2918783002
Cr-Commit-Position: refs/heads/master@{#477207}
Committed: https://chromium.googlesource.com/chromium/src/+/436a66696c7fe35e921076cf22d2bc35ffb2c7f6
Patch Set 1 : 2017-06-01T18:38:06 #
Total comments: 8
Patch Set 2 : 2017-06-05T18:17:50 #
Total comments: 4
Patch Set 3 : 2017-06-06T13:19:51 #Messages
Total messages: 33 (26 generated)
The CQ bit was checked by yosin@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...
Description was changed from ========== 2017-06-01T17:03:59 BUG=707656 2017-06-01T17:03:47 ========== to ========== Extract InlineBox handling from MostForwardCaretPosition() This patch extracts |InlineBox| handling code fragment into |IsOffsetRenderedForForward()| for ease of adopting |MostForwardCaretPosition()| to Layout NG. Note: The patch[1] does same thing for |MostBackwardCaretPosition()| [1] http://crrev.com/2919503003: Extract InlineBox handling from MostBackwardCaretPosition() BUG=707656 TEST=n/a; no behavior changes ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_headless_rel on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by yosin@chromium.org to run a CQ dry run
Patchset #1 (id:1) has been deleted
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 yosin@chromium.org to run a CQ dry run
Patchset #1 (id:20001) has been deleted
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
yosin@chromium.org changed reviewers: + xiaochengh@chromium.org, yoichio@chromium.org
PTAL
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The comments basically also apply to https://codereview.chromium.org/2919503003. Btw, the two new functions are almost identical. We should deduplicate their code in some follow-up patch. https://codereview.chromium.org/2918783002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/VisibleUnits.cpp (right): https://codereview.chromium.org/2918783002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/VisibleUnits.cpp:2834: // Returns true if |offset| is rendered text of |layout_text_object| The purpose of the function isn't clear. An offset is a number, not text, so the proposition "|offset| is rendered text" is ill-formed. https://codereview.chromium.org/2918783002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/VisibleUnits.cpp:2845: return true; It returns true here if an adjacent character at either side of |offset_in_node| is rendered. https://codereview.chromium.org/2918783002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/VisibleUnits.cpp:2852: // The text continues on the next line only if the last text box is not nit: We can save one line here. https://codereview.chromium.org/2918783002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/VisibleUnits.cpp:2882: return true; I haven't understood in what situation |continues_on_next_line| remains true. Do you have an idea?
PTAL I also updated http://crrev.com/2919503003, which does same thing for MostBackwardCaretPosition() https://codereview.chromium.org/2918783002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/VisibleUnits.cpp (right): https://codereview.chromium.org/2918783002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/VisibleUnits.cpp:2834: // Returns true if |offset| is rendered text of |layout_text_object| On 2017/06/01 at 22:19:47, Xiaocheng wrote: > The purpose of the function isn't clear. > > An offset is a number, not text, so the proposition "|offset| is rendered text" is ill-formed. Oops, sorry for confusion. |offset| should be |offset_in_node|. As talked offline, I don't understand this part fully, I put TODO to say we need to study more. https://codereview.chromium.org/2918783002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/VisibleUnits.cpp:2845: return true; On 2017/06/01 at 22:19:48, Xiaocheng wrote: > It returns true here if an adjacent character at either side of |offset_in_node| is rendered. My interpretation is that if |text_offset| is in |box|, we return true. For |return true| case, |box->Start() <= text_offset && text_offset < box->end()|. https://codereview.chromium.org/2918783002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/VisibleUnits.cpp:2852: // The text continues on the next line only if the last text box is not On 2017/06/01 at 22:19:48, Xiaocheng wrote: > nit: We can save one line here. Done. https://codereview.chromium.org/2918783002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/VisibleUnits.cpp:2882: return true; On 2017/06/01 at 22:19:47, Xiaocheng wrote: > I haven't understood in what situation |continues_on_next_line| remains true. > > Do you have an idea? It seems most of case, |continues_on_next_line| remains true. e.g. "abc$def" is a simple case, where "$" denotes line break, It seems |continues_on_next_line| is false, is rare. When I change |c_on_next_line = false| to |c_on_next_line = true|, following 5 test are failed: editing/selection/modify_extend/extend_by_character.html [ Failure ] editing/selection/modify_move/move-by-character-001.html [ Failure ] editing/selection/modify_move/move-by-character-003.html [ Failure ] editing/selection/modify_move/move-by-word-visually-multi-space.html [ Failure ] editing/selection/move-left-right.html [ Failure ] One case is assert_selection( '<div contenteditable>f |oo bar baz</div>', selection => { for (var i = 0; i < 7; ++i) selection.modify('extend', 'forward', 'character'); }, '<div contenteditable>f ^oo bar | baz</div>'), + expected <div contenteditable>f ^oo bar | baz</div>, + but got <div contenteditable>f ^oo bar| baz</div>, + sameupto <div contenteditable>f ^oo bar When I set breakpoint to |c_on_next_line = false|, 32 test hits. This is another example of |c_on_next_line = false|. Text node: "abc $def" |text_offset| is 3, a space before $. |first_text_box| is "abc" (no trailing space) |last_text_box| is "def". This function was introduced on April 2008 by the patch[1]. Note: When I set breakpoint at |c_on_next_line| to false, the test "move-past-trailing-space.html", added by the patch[1], doesn't hit the breakpoint. :-< [1] https://trac.webkit.org/changeset/32438/webkit
The CQ bit was checked by yosin@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...
Patchset #2 (id:60001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/2918783002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/VisibleUnits.cpp (right): https://codereview.chromium.org/2918783002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/VisibleUnits.cpp:2838: static bool CanForwardCaretPosition(const LayoutText* text_layout_object, nit: |CanBeForwardCaretPosition| seems a better name. https://codereview.chromium.org/2918783002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/VisibleUnits.cpp:2856: // |DoesContinueOnNextLine()|. Note: |MostForwardCaretPosition()| has nit: s/Forward/Backward/
The CQ bit was checked by yosin@chromium.org to run a CQ dry run
The CQ bit was unchecked by yosin@chromium.org
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 yosin@chromium.org
Thanks for reviewing! Committing... https://codereview.chromium.org/2918783002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/VisibleUnits.cpp (right): https://codereview.chromium.org/2918783002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/VisibleUnits.cpp:2838: static bool CanForwardCaretPosition(const LayoutText* text_layout_object, On 2017/06/05 at 19:35:53, Xiaocheng wrote: > nit: |CanBeForwardCaretPosition| seems a better name. Done. Thanks for providing good name. https://codereview.chromium.org/2918783002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/VisibleUnits.cpp:2856: // |DoesContinueOnNextLine()|. Note: |MostForwardCaretPosition()| has On 2017/06/05 at 19:35:53, Xiaocheng wrote: > nit: s/Forward/Backward/ Done. Thanks for catching this.
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/2918783002/#ps100001 (title: "2017-06-06T13:19:51")
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": 100001, "attempt_start_ts": 1496722874094810,
"parent_rev": "ba2889de7991c1f5162cd12dd6cc6e4de3bc3ac0", "commit_rev":
"436a66696c7fe35e921076cf22d2bc35ffb2c7f6"}
Message was sent while issue was closed.
Description was changed from ========== Extract InlineBox handling from MostForwardCaretPosition() This patch extracts |InlineBox| handling code fragment into |IsOffsetRenderedForForward()| for ease of adopting |MostForwardCaretPosition()| to Layout NG. Note: The patch[1] does same thing for |MostBackwardCaretPosition()| [1] http://crrev.com/2919503003: Extract InlineBox handling from MostBackwardCaretPosition() BUG=707656 TEST=n/a; no behavior changes ========== to ========== Extract InlineBox handling from MostForwardCaretPosition() This patch extracts |InlineBox| handling code fragment into |IsOffsetRenderedForForward()| for ease of adopting |MostForwardCaretPosition()| to Layout NG. Note: The patch[1] does same thing for |MostBackwardCaretPosition()| [1] http://crrev.com/2919503003: Extract InlineBox handling from MostBackwardCaretPosition() BUG=707656 TEST=n/a; no behavior changes Review-Url: https://codereview.chromium.org/2918783002 Cr-Commit-Position: refs/heads/master@{#477207} Committed: https://chromium.googlesource.com/chromium/src/+/436a66696c7fe35e921076cf22d2... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:100001) as https://chromium.googlesource.com/chromium/src/+/436a66696c7fe35e921076cf22d2... |
