Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2263083002:
Refactor CachingWordShapeIterator::nextWordEndIndex (Closed)
|
Created:
4 years, 4 months ago by kojii Modified:
4 years, 3 months ago CC:
ajuma+watch_chromium.org, blink-reviews, blink-reviews-platform-graphics_chromium.org, Rik, chromium-reviews, danakj+watch_chromium.org, dshwang, drott+blinkwatch_chromium.org, krit, f(malita), jbroman, Justin Novosad, pdr+graphicswatchlist_chromium.org, rwlbuis, Stephen Chennney Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRefactor CachingWordShapeIterator::nextWordEndIndex
This patch refactors CachingWordShapeIterator::nextWordEndIndex so that:
1. Better readability.
2. Split 3 cases into 3 different loops to make it possible to identify
the loop from the stack trace.
BUG=639085
Committed: https://crrev.com/103f7f70a9162ce1946ee09b08a1ee7bb6352d17
Cr-Commit-Position: refs/heads/master@{#413463}
Patch Set 1 #Patch Set 2 : Comment editorial #
Messages
Total messages: 26 (17 generated)
The CQ bit was checked by kojii@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 ========== shape BUG= ========== to ========== Refactor CachingWordShapeIterator::nextWordEndIndex This patch refactors CachingWordShapeIterator::nextWordEndIndex so that: 1. Better readability. 2. Split 3 cases into 3 different loops so that stack trace can identify which loop it was running. BUG=639085 ==========
kojii@chromium.org changed reviewers: + drott@chromium.org, eae@chromium.org
PTAL. Reviewed the functions but can't find any cases where m_textRun[] can be index-out-of-range. Hopefully this gives better readability and also stack trace can give us which case it is failing.
LGTM with some nits on comment wording. For testing, I sometimes need to disable CJK segmentation (emoji issues debugging for example) - which lines would I need to modify, making line 132 always true? Anything else? Perhaps making line 152 always false?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/08/22 at 10:16:14, drott wrote: > For testing, I sometimes need to disable CJK segmentation (emoji issues debugging for example) - which lines would I need to modify, making line 132 always true? Anything else? Perhaps making line 152 always false? Yeah, line 132, and maybe line 135 too for a non-CJK word to stop at CJK/emoji. Maybe I should add comments for each block?
On 2016/08/22 at 13:17:46, kojii wrote: > On 2016/08/22 at 10:16:14, drott wrote: > > For testing, I sometimes need to disable CJK segmentation (emoji issues debugging for example) - which lines would I need to modify, making line 132 always true? Anything else? Perhaps making line 152 always false? > > Yeah, line 132, and maybe line 135 too for a non-CJK word to stop at CJK/emoji. Ok. > Maybe I should add comments for each block? Sounds good to me.
The CQ bit was checked by kojii@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...
Comments updated.
The CQ bit was checked by kojii@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:20001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Refactor CachingWordShapeIterator::nextWordEndIndex This patch refactors CachingWordShapeIterator::nextWordEndIndex so that: 1. Better readability. 2. Split 3 cases into 3 different loops so that stack trace can identify which loop it was running. BUG=639085 ========== to ========== Refactor CachingWordShapeIterator::nextWordEndIndex This patch refactors CachingWordShapeIterator::nextWordEndIndex so that: 1. Better readability. 2. Split 3 cases into 3 different loops to make it possible to identify the loop from the stack trace. BUG=639085 ==========
The CQ bit was checked by kojii@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from drott@chromium.org Link to the patchset: https://codereview.chromium.org/2263083002/#ps40001 (title: "Comment editorial")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #2 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Refactor CachingWordShapeIterator::nextWordEndIndex This patch refactors CachingWordShapeIterator::nextWordEndIndex so that: 1. Better readability. 2. Split 3 cases into 3 different loops to make it possible to identify the loop from the stack trace. BUG=639085 ========== to ========== Refactor CachingWordShapeIterator::nextWordEndIndex This patch refactors CachingWordShapeIterator::nextWordEndIndex so that: 1. Better readability. 2. Split 3 cases into 3 different loops to make it possible to identify the loop from the stack trace. BUG=639085 Committed: https://crrev.com/103f7f70a9162ce1946ee09b08a1ee7bb6352d17 Cr-Commit-Position: refs/heads/master@{#413463} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/103f7f70a9162ce1946ee09b08a1ee7bb6352d17 Cr-Commit-Position: refs/heads/master@{#413463}
Message was sent while issue was closed.
LGTM |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
