Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2925063002:
ALL-IN-ONE: Refactor ComputeInlineBoxPosition() (Closed)
|
Created:
3 years, 6 months ago by yosin_UTC9 Modified:
3 years, 5 months ago CC:
blink-reviews, chromium-reviews Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionRefactor ComputeInlineBoxPosition()
This patch splits |ComputeInlineBoxPositionTemplate()| into
- processing Text node
- processing InlineBox
to reduce size of template with utilizing const-variable for improving code
health.
BUG=707656
TEST=n/a; no behavior changes
Patch Set 1 : 2017-06-08T19:15:47 #
Total comments: 5
Messages
Total messages: 32 (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-07T18:43:20 BUG=707656 2017-06-07T18:43:14 ========== to ========== Refactor ComputeInlineBoxPosition() This patch splits |ComputeInlineBoxPositionTemplate()| into - processing Text node - processing InlineBox to reduce size of template with utilizing const-variable for improving code health. 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: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
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 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_...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_tsan_rel_ng 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
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.
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...
yosin@chromium.org changed reviewers: + xiaochengh@chromium.org, yoichio@chromium.org
PTAL https://codereview.chromium.org/2925063002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/VisibleUnits.cpp (right): https://codereview.chromium.org/2925063002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/VisibleUnits.cpp:1327: return layout_object->LocalCaretRect( Since ComputeInlineBoxPosition() returns offset zero when there are no inline box for position, we should compute offset from position.
Patchset #1 (id:40001) has been deleted
Patchset #1 (id:60001) has been deleted
PTAL https://codereview.chromium.org/2925063002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/VisibleUnits.cpp (right): https://codereview.chromium.org/2925063002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/VisibleUnits.cpp:1173: return layout_object->LocalCaretRect( Since ComputeInlineBoxPosition() returns offset zero when there are no inline box for position, we should compute offset from position.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Sorry I don't understand this patch. The code flow in ComputeInlineBoxPositionTemplate is too complicated. Could you first simplify it somehow? The patch also changes behavior. https://codereview.chromium.org/2925063002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/VisibleUnits.cpp (left): https://codereview.chromium.org/2925063002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/VisibleUnits.cpp:958: inline_box = SearchAheadForBetterMatch(text_layout_object); I don't understand this part. It seems to be setting the value of |inline_box| repeatedly in the loop without returning. Why it can be changed to an early return? https://codereview.chromium.org/2925063002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/VisibleUnitsTest.cpp (left): https://codereview.chromium.org/2925063002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/VisibleUnitsTest.cpp:308: EXPECT_EQ(2, actual.offset_in_box); This is behavioral change.
On 2017/06/08 at 21:34:12, xiaochengh wrote: > Sorry I don't understand this patch. > > The code flow in ComputeInlineBoxPositionTemplate is too complicated. Could you first simplify it somehow? This is "simplification" somehow... Anyway try to split this patch.
https://codereview.chromium.org/2925063002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/VisibleUnits.cpp (left): https://codereview.chromium.org/2925063002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/VisibleUnits.cpp:958: inline_box = SearchAheadForBetterMatch(text_layout_object); On 2017/06/08 at 21:34:12, Xiaocheng wrote: > I don't understand this part. > > It seems to be setting the value of |inline_box| repeatedly in the loop without returning. Why it can be changed to an early return? Setting |inline_box| is one in the loop at L976. After setting |iline_box|, we break loop. This loop is can be represents as: (is_early_return, inline_box, candidate) = this_loop(...); if (is_eraly_return) return InlineBoxPosition(inline_box, caret_offset); // as L968 ... https://codereview.chromium.org/2925063002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/VisibleUnitsTest.cpp (left): https://codereview.chromium.org/2925063002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/VisibleUnitsTest.cpp:308: EXPECT_EQ(2, actual.offset_in_box); On 2017/06/08 at 21:34:12, Xiaocheng wrote: > This is behavioral change. This comes from here: ORIGINAL: inline_box = ToLayoutBox(layout_object)->InlineBoxWrapper(); if (!inline_box || (caret_offset > inline_box->CaretMinOffset() && caret_offset < inline_box->CaretMaxOffset())) return InlineBoxPosition(inline_box, caret_offset); if (!inline_box) return InlineBoxPosition(inline_box, caret_offset); Anyway, I'll move this change into another patch.
On 2017/06/09 at 01:55:55, yosin wrote: > https://codereview.chromium.org/2925063002/diff/80001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/editing/VisibleUnits.cpp (left): > > https://codereview.chromium.org/2925063002/diff/80001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/editing/VisibleUnits.cpp:958: inline_box = SearchAheadForBetterMatch(text_layout_object); > On 2017/06/08 at 21:34:12, Xiaocheng wrote: > > I don't understand this part. > > > > It seems to be setting the value of |inline_box| repeatedly in the loop without returning. Why it can be changed to an early return? > > Setting |inline_box| is one in the loop at L976. After setting |iline_box|, we break loop. > > This loop is can be represents as: > > (is_early_return, inline_box, candidate) = this_loop(...); > if (is_eraly_return) > return InlineBoxPosition(inline_box, caret_offset); // as L968 > ... > > https://codereview.chromium.org/2925063002/diff/80001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/editing/VisibleUnitsTest.cpp (left): > > https://codereview.chromium.org/2925063002/diff/80001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/editing/VisibleUnitsTest.cpp:308: EXPECT_EQ(2, actual.offset_in_box); > On 2017/06/08 at 21:34:12, Xiaocheng wrote: > > This is behavioral change. > > This comes from here: > > ORIGINAL: > inline_box = ToLayoutBox(layout_object)->InlineBoxWrapper(); > if (!inline_box || (caret_offset > inline_box->CaretMinOffset() && > caret_offset < inline_box->CaretMaxOffset())) > return InlineBoxPosition(inline_box, caret_offset); > > if (!inline_box) > return InlineBoxPosition(inline_box, caret_offset); > > Anyway, I'll move this change into another patch. Thanks. I'll wait for the split patches. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
