Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2932283002:
Introduce IsCaretAtEdgeOfInlineTextBox() to simplify ComputeInlineBoxPosition() (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. |
DescriptionIntroduce IsCaretAtEdgeOfInlineTextBox() to simplify ComputeInlineBoxPosition()
This patch introduces |IsCaretAtEdgeOfInlineTextBox()| to simplify loop body and
simplify condition expression in |ComputeInlineBoxPosition()| for improving
code health.
The implementation of |IsCaretAtEdgeOfInlineTextBox()| based on following
reasoning:
Table of exclusive-or expressions:
1 (caret_offset == caret_max_offset) ^ (affinity == Downstream)
2 (caret_offset == caret_min_offset) ^ (affinity == Upstream)
Results of expression 1 and 2 are same for combinations of |caret_offset| and
|affinity|.
caret_offset|affinity |result
------------+----------+-------
max_offset |Downstream|false
max_offset |Upstream |true
min_offset |Downstream|true
min_offset |Upstream |false
BUG=707656
TEST=n/a; no behavior changes
Review-Url: https://codereview.chromium.org/2932283002
Cr-Commit-Position: refs/heads/master@{#479236}
Committed: https://chromium.googlesource.com/chromium/src/+/736123357fcf3d8bf78af92fdd24efa81333117e
Patch Set 1 : 2017-06-12T15:57:21 #
Total comments: 2
Patch Set 2 : 2017-06-13T16:36:07 #Messages
Total messages: 30 (23 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...
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 #1 (id:1) has been deleted
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...)
Description was changed from ========== 2017-06-12T15:08:07 BUG= 2017-06-12T15:06:41 ========== to ========== Simplify condition expression in ComputeInlineBoxPosition() This patch simplifies condition expression in |ComputeInlineBoxPosition()| by decomposing into if-statements with early-break/continue style for improving code health. Table of exclusive-or expressions: 1 (caret_offset == caret_max_offset) ^ (affinity == Downstream) 2 (caret_offset == caret_min_offset) ^ (affinity == Upstream) Results of expression 1 and 2 are same or combinations of |caret_offset| and |affinity|. caret_offset|affinity |result ------------+----------+------- max_offset |Downstream|false max_offset |Upstream |true min_offset |Downstream|true min_offset |Upstream |false BUG=707656 TEST=n/a; no behavior changes ==========
Description was changed from ========== Simplify condition expression in ComputeInlineBoxPosition() This patch simplifies condition expression in |ComputeInlineBoxPosition()| by decomposing into if-statements with early-break/continue style for improving code health. Table of exclusive-or expressions: 1 (caret_offset == caret_max_offset) ^ (affinity == Downstream) 2 (caret_offset == caret_min_offset) ^ (affinity == Upstream) Results of expression 1 and 2 are same or combinations of |caret_offset| and |affinity|. caret_offset|affinity |result ------------+----------+------- max_offset |Downstream|false max_offset |Upstream |true min_offset |Downstream|true min_offset |Upstream |false BUG=707656 TEST=n/a; no behavior changes ========== to ========== Simplify condition expression in ComputeInlineBoxPosition() This patch simplifies condition expression in |ComputeInlineBoxPosition()| by decomposing into if-statements with early-break/continue style for improving code health. Table of exclusive-or expressions: 1 (caret_offset == caret_max_offset) ^ (affinity == Downstream) 2 (caret_offset == caret_min_offset) ^ (affinity == Upstream) Results of expression 1 and 2 are same for combinations of |caret_offset| and |affinity|. caret_offset|affinity |result ------------+----------+------- max_offset |Downstream|false max_offset |Upstream |true min_offset |Downstream|true min_offset |Upstream |false BUG=707656 TEST=n/a; no behavior changes ==========
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 #1 (id:20001) has been deleted
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.
https://codereview.chromium.org/2932283002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/VisibleUnits.cpp (right): https://codereview.chromium.org/2932283002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/VisibleUnits.cpp:820: if (caret_offset == caret_min_offset) { I think it's better to use a wrapper function that evaluates the condition, so that we don't have complicated control flow with interleaving breaks and continues.
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 ========== Simplify condition expression in ComputeInlineBoxPosition() This patch simplifies condition expression in |ComputeInlineBoxPosition()| by decomposing into if-statements with early-break/continue style for improving code health. Table of exclusive-or expressions: 1 (caret_offset == caret_max_offset) ^ (affinity == Downstream) 2 (caret_offset == caret_min_offset) ^ (affinity == Upstream) Results of expression 1 and 2 are same for combinations of |caret_offset| and |affinity|. caret_offset|affinity |result ------------+----------+------- max_offset |Downstream|false max_offset |Upstream |true min_offset |Downstream|true min_offset |Upstream |false BUG=707656 TEST=n/a; no behavior changes ========== to ========== Introduce IsCaretAtEdgeOfInlineTextBox() to simplify ComputeInlineBoxPosition() This patch introduces |IsCaretAtEdgeOfInlineTextBox()| to simplify loop body and simplify condition expression in |ComputeInlineBoxPosition()| for improving code health. The implementation of |IsCaretAtEdgeOfInlineTextBox()| based on following reasoning: Table of exclusive-or expressions: 1 (caret_offset == caret_max_offset) ^ (affinity == Downstream) 2 (caret_offset == caret_min_offset) ^ (affinity == Upstream) Results of expression 1 and 2 are same for combinations of |caret_offset| and |affinity|. caret_offset|affinity |result ------------+----------+------- max_offset |Downstream|false max_offset |Upstream |true min_offset |Downstream|true min_offset |Upstream |false BUG=707656 TEST=n/a; no behavior changes ==========
PTAL - Introduce a new function IsCaretAtEdgeOfInlineTextBox - Update description https://codereview.chromium.org/2932283002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/VisibleUnits.cpp (right): https://codereview.chromium.org/2932283002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/VisibleUnits.cpp:820: if (caret_offset == caret_min_offset) { On 2017/06/12 at 18:35:12, Xiaocheng wrote: > I think it's better to use a wrapper function that evaluates the condition, so that we don't have complicated control flow with interleaving breaks and continues. You're right. Move the condition to another function.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm with nit: please update patch title accordingly.
On 2017/06/13 at 17:20:37, xiaochengh wrote: > lgtm with nit: please update patch title accordingly. Good catch! I update patch title. I updated title in description but forgot to update title...
The CQ bit was checked by yosin@chromium.org
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": 1497402219231690,
"parent_rev": "bff5dcfbfafd7a80aedbfebb97161d9bbeedcf2f", "commit_rev":
"736123357fcf3d8bf78af92fdd24efa81333117e"}
Message was sent while issue was closed.
Description was changed from ========== Introduce IsCaretAtEdgeOfInlineTextBox() to simplify ComputeInlineBoxPosition() This patch introduces |IsCaretAtEdgeOfInlineTextBox()| to simplify loop body and simplify condition expression in |ComputeInlineBoxPosition()| for improving code health. The implementation of |IsCaretAtEdgeOfInlineTextBox()| based on following reasoning: Table of exclusive-or expressions: 1 (caret_offset == caret_max_offset) ^ (affinity == Downstream) 2 (caret_offset == caret_min_offset) ^ (affinity == Upstream) Results of expression 1 and 2 are same for combinations of |caret_offset| and |affinity|. caret_offset|affinity |result ------------+----------+------- max_offset |Downstream|false max_offset |Upstream |true min_offset |Downstream|true min_offset |Upstream |false BUG=707656 TEST=n/a; no behavior changes ========== to ========== Introduce IsCaretAtEdgeOfInlineTextBox() to simplify ComputeInlineBoxPosition() This patch introduces |IsCaretAtEdgeOfInlineTextBox()| to simplify loop body and simplify condition expression in |ComputeInlineBoxPosition()| for improving code health. The implementation of |IsCaretAtEdgeOfInlineTextBox()| based on following reasoning: Table of exclusive-or expressions: 1 (caret_offset == caret_max_offset) ^ (affinity == Downstream) 2 (caret_offset == caret_min_offset) ^ (affinity == Upstream) Results of expression 1 and 2 are same for combinations of |caret_offset| and |affinity|. caret_offset|affinity |result ------------+----------+------- max_offset |Downstream|false max_offset |Upstream |true min_offset |Downstream|true min_offset |Upstream |false BUG=707656 TEST=n/a; no behavior changes Review-Url: https://codereview.chromium.org/2932283002 Cr-Commit-Position: refs/heads/master@{#479236} Committed: https://chromium.googlesource.com/chromium/src/+/736123357fcf3d8bf78af92fdd24... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:60001) as https://chromium.googlesource.com/chromium/src/+/736123357fcf3d8bf78af92fdd24... |
