Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1530833002:
Refactor word segmentation in CachingWordShapeIterator (Closed)
|
Created:
5 years ago by kojii Modified:
5 years ago CC:
chromium-reviews, pdr+graphicswatchlist_chromium.org, drott+blinkwatch_chromium.org, Rik, blink-reviews-platform-graphics_chromium.org, dshwang, jbroman, Justin Novosad, danakj, krit, f(malita), blink-reviews, vmpstr+blinkwatch_chromium.org, Stephen Chennney, rwlbuis Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRefactor word segmentation in CachingWordShapeIterator
This patch refactors CachingWordShapeIterator to separate the word
segmentation logic for caching purpose from the word shaping code.
In preparation of adjusting the word segmentation for caching
purpose for CJK, separating these two code helps.
No behavior changes.
BUG=570229
Committed: https://crrev.com/f41398ad3df1c472237bfc07c9085d2d257bfd5e
Cr-Commit-Position: refs/heads/master@{#366390}
Patch Set 1 #Patch Set 2 : Cleanup #
Total comments: 8
Patch Set 3 : drott review #Patch Set 4 : Rebase and minor editorial changes #Patch Set 5 : Use endIndex instead of Range, and simplified further #Messages
Total messages: 21 (12 generated)
Patchset #2 (id:20001) has been deleted
Patchset #2 (id:40001) has been deleted
Description was changed from ========== WIP: Refactor word segmentation in CachingWordShapeIterator BUG= ========== to ========== Refactor word segmentation in CachingWordShapeIterator This patch refactors CachingWordShapeIterator to separate the word segmentation logic for caching purpose from the word shaping code. In preparation of adjusting the word segmentation for caching purpose for CJK, separating these two code helps. No behavior changes. BUG=570229 ==========
kojii@chromium.org changed reviewers: + drott@chromium.org, eae@chromium.org
drott@, PTAL, eae@ is ooo. The background is in bug 570229, but if this refactoring does not look an improvement to you, I can reconsider, please let me know in that case.
In general, this looks like a good clarification of the code to me - but I would want to wait for eae@'s opinion - he should be back soon. A few specific comments below. https://codereview.chromium.org/1530833002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/fonts/shaping/CachingWordShapeIterator.h (right): https://codereview.chromium.org/1530833002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/fonts/shaping/CachingWordShapeIterator.h:116: std::pair<unsigned, unsigned> nextWordRange() I'd suggest making std::pair<unsigned, unsigned> a struct, with a default constructor when returning it and use this consistently in every place where you use this type. Perhaps even declare it in TextRun and make the subRun() method take this type? https://codereview.chromium.org/1530833002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/fonts/shaping/CachingWordShapeIterator.h:129: for (unsigned i = start + 1; ; i++) { Can this loop and use nextRangeUntil as well? https://codereview.chromium.org/1530833002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/fonts/shaping/CachingWordShapeIterator.h:143: std::pair<unsigned, unsigned> nextRangeUntil(UChar ch) What's the difference between nextRangeUntil and nextRangeWhile? They look identical to me. https://codereview.chromium.org/1530833002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/fonts/shaping/CachingWordShapeIterator.h:186: ASSERT(*wordResult); What has changed to be able upgrade this to an assertion?
> In general, this looks like a good clarification of the code to me Thank you, I wanted to hear your general thoughts, so that's greatly appreciated. > - but I would want to wait for eae@'s opinion - he should be back soon. Me too, but his schedule looks like he's back on Monday, work for two days and then OOO again. I'd like to fix bug 565902 by Jan branch, but year end is hard; US/Europe people are mostly ooo on year end, and Asians tend to be ooo after new year. I'll prepare both short and long solutions, maybe I want to land the short one if this is going to take longer. https://codereview.chromium.org/1530833002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/fonts/shaping/CachingWordShapeIterator.h (right): https://codereview.chromium.org/1530833002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/fonts/shaping/CachingWordShapeIterator.h:116: std::pair<unsigned, unsigned> nextWordRange() On 2015/12/16 12:40:58, drott wrote: > I'd suggest making std::pair<unsigned, unsigned> a struct, with a default > constructor when returning it and use this consistently in every place where you > use this type. Perhaps even declare it in TextRun and make the subRun() method > take this type? That one I was wondering, thank you for pointing it out. I'll try that. https://codereview.chromium.org/1530833002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/fonts/shaping/CachingWordShapeIterator.h:129: for (unsigned i = start + 1; ; i++) { On 2015/12/16 12:40:59, drott wrote: > Can this loop and use nextRangeUntil as well? This needs two characters, and possibly more other space characters. We can split this out to yet another function to make it clearer. https://codereview.chromium.org/1530833002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/fonts/shaping/CachingWordShapeIterator.h:143: std::pair<unsigned, unsigned> nextRangeUntil(UChar ch) On 2015/12/16 12:40:59, drott wrote: > What's the difference between nextRangeUntil and nextRangeWhile? They look > identical to me. "Until" exists loop when "==" while "While" exists when "!=". https://codereview.chromium.org/1530833002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/fonts/shaping/CachingWordShapeIterator.h:186: ASSERT(*wordResult); On 2015/12/16 12:40:59, drott wrote: > What has changed to be able upgrade this to an assertion? Actually, nothing; even before the patch, this should never become nullptr, because we check above and returns false already if |*wordResult|. Figured out that this is not needed during the refactoring.
PTAL.
Patchset #4 (id:100001) has been deleted
Patchset #4 (id:120001) has been deleted
Patchset #4 (id:140001) has been deleted
Patchset #4 (id:160001) has been deleted
Patchset #5 (id:200001) has been deleted
PTAL. Refactored from the comment https://codereview.chromium.org/1525993005/#msg12 linux_chromium_gn_chromeos_rel is failing for ui/gfx/gfx/vector_icons.o, should be unrelated.
LGTM - similar here, I think we can land this and modify/revert if eae@ has concerns once he has a chance to take a look.
The CQ bit was checked by kojii@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1530833002/220001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1530833002/220001
Message was sent while issue was closed.
Description was changed from ========== Refactor word segmentation in CachingWordShapeIterator This patch refactors CachingWordShapeIterator to separate the word segmentation logic for caching purpose from the word shaping code. In preparation of adjusting the word segmentation for caching purpose for CJK, separating these two code helps. No behavior changes. BUG=570229 ========== to ========== Refactor word segmentation in CachingWordShapeIterator This patch refactors CachingWordShapeIterator to separate the word segmentation logic for caching purpose from the word shaping code. In preparation of adjusting the word segmentation for caching purpose for CJK, separating these two code helps. No behavior changes. BUG=570229 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:220001)
Message was sent while issue was closed.
Description was changed from ========== Refactor word segmentation in CachingWordShapeIterator This patch refactors CachingWordShapeIterator to separate the word segmentation logic for caching purpose from the word shaping code. In preparation of adjusting the word segmentation for caching purpose for CJK, separating these two code helps. No behavior changes. BUG=570229 ========== to ========== Refactor word segmentation in CachingWordShapeIterator This patch refactors CachingWordShapeIterator to separate the word segmentation logic for caching purpose from the word shaping code. In preparation of adjusting the word segmentation for caching purpose for CJK, separating these two code helps. No behavior changes. BUG=570229 Committed: https://crrev.com/f41398ad3df1c472237bfc07c9085d2d257bfd5e Cr-Commit-Position: refs/heads/master@{#366390} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/f41398ad3df1c472237bfc07c9085d2d257bfd5e Cr-Commit-Position: refs/heads/master@{#366390} |
