Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1857083002:
Segment strings on dot and comma in addition to space and tab (Closed)
|
Created:
4 years, 8 months ago by eae Modified:
4 years, 8 months ago CC:
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, kinuko+watch, 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. |
DescriptionSegment strings on dot and comma in addition to space and tab
Add dot and comma do the list of word separators in an effort to prevent
hitting the long-word performance cliff when shaping JSON and JS source.
This is a speculative change based on reported observations as we've yet
been able to reproduce this problem. We do know that performance suffers
somewhat for very long words though, eminently for fonts with ligatures.
R=lushnikov@chromium.org
BUG=593679
Committed: https://crrev.com/63f4a420e42667f2167b7fcf4a9ac8e10f66e792
Cr-Commit-Position: refs/heads/master@{#386022}
Patch Set 1 #Patch Set 2 : #Patch Set 3 : #Patch Set 4 : #
Total comments: 2
Patch Set 5 : Rebase w/HEAD #Patch Set 6 : #
Messages
Total messages: 29 (15 generated)
Description was changed from ========== wip BUG= ========== to ========== Segment strings on dot and comma in addition to space and tab Add dot and comma do the list of word separators in an effort to prevent hitting the long-word performance cliff when shaping JSON and JS source. This is a speculative change based on reported observations as we've yet been able to reproduce this problem. We do know that performance suffers somewhat for very long words though, eminently for fonts with ligatures. ==========
Description was changed from ========== Segment strings on dot and comma in addition to space and tab Add dot and comma do the list of word separators in an effort to prevent hitting the long-word performance cliff when shaping JSON and JS source. This is a speculative change based on reported observations as we've yet been able to reproduce this problem. We do know that performance suffers somewhat for very long words though, eminently for fonts with ligatures. ========== to ========== Segment strings on dot and comma in addition to space and tab Add dot and comma do the list of word separators in an effort to prevent hitting the long-word performance cliff when shaping JSON and JS source. This is a speculative change based on reported observations as we've yet been able to reproduce this problem. We do know that performance suffers somewhat for very long words though, eminently for fonts with ligatures. BUG=593679 ==========
Description was changed from ========== Segment strings on dot and comma in addition to space and tab Add dot and comma do the list of word separators in an effort to prevent hitting the long-word performance cliff when shaping JSON and JS source. This is a speculative change based on reported observations as we've yet been able to reproduce this problem. We do know that performance suffers somewhat for very long words though, eminently for fonts with ligatures. BUG=593679 ========== to ========== Segment strings on dot and comma in addition to space and tab Add dot and comma do the list of word separators in an effort to prevent hitting the long-word performance cliff when shaping JSON and JS source. This is a speculative change based on reported observations as we've yet been able to reproduce this problem. We do know that performance suffers somewhat for very long words though, eminently for fonts with ligatures. R=lushnikov@chromium.org BUG=593679 ==========
eae@chromium.org changed reviewers: + lushnikov@chromium.org
Thanks, hope this will fix the issue! lgtm https://codereview.chromium.org/1857083002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/fonts/shaping/CachingWordShapeIterator.h (right): https://codereview.chromium.org/1857083002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/fonts/shaping/CachingWordShapeIterator.h:108: return ch == spaceCharacter || ch == tabulationCharacter I would add a semicolon here as well
https://codereview.chromium.org/1857083002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/fonts/shaping/CachingWordShapeIterator.h (right): https://codereview.chromium.org/1857083002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/fonts/shaping/CachingWordShapeIterator.h:108: return ch == spaceCharacter || ch == tabulationCharacter Please disregard. It turned out semicolon is a pretty rare character in minified js libs. Dot and comma are among the most popular ones
The CQ bit was checked by eae@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1857083002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1857083002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios_dbg_simulator_gn on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...) mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by eae@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from lushnikov@chromium.org Link to the patchset: https://codereview.chromium.org/1857083002/#ps80001 (title: "Rebase w/HEAD")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1857083002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1857083002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by eae@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from lushnikov@chromium.org Link to the patchset: https://codereview.chromium.org/1857083002/#ps100001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1857083002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1857083002/100001
Message was sent while issue was closed.
Description was changed from ========== Segment strings on dot and comma in addition to space and tab Add dot and comma do the list of word separators in an effort to prevent hitting the long-word performance cliff when shaping JSON and JS source. This is a speculative change based on reported observations as we've yet been able to reproduce this problem. We do know that performance suffers somewhat for very long words though, eminently for fonts with ligatures. R=lushnikov@chromium.org BUG=593679 ========== to ========== Segment strings on dot and comma in addition to space and tab Add dot and comma do the list of word separators in an effort to prevent hitting the long-word performance cliff when shaping JSON and JS source. This is a speculative change based on reported observations as we've yet been able to reproduce this problem. We do know that performance suffers somewhat for very long words though, eminently for fonts with ligatures. R=lushnikov@chromium.org BUG=593679 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Segment strings on dot and comma in addition to space and tab Add dot and comma do the list of word separators in an effort to prevent hitting the long-word performance cliff when shaping JSON and JS source. This is a speculative change based on reported observations as we've yet been able to reproduce this problem. We do know that performance suffers somewhat for very long words though, eminently for fonts with ligatures. R=lushnikov@chromium.org BUG=593679 ========== to ========== Segment strings on dot and comma in addition to space and tab Add dot and comma do the list of word separators in an effort to prevent hitting the long-word performance cliff when shaping JSON and JS source. This is a speculative change based on reported observations as we've yet been able to reproduce this problem. We do know that performance suffers somewhat for very long words though, eminently for fonts with ligatures. R=lushnikov@chromium.org BUG=593679 Committed: https://crrev.com/63f4a420e42667f2167b7fcf4a9ac8e10f66e792 Cr-Commit-Position: refs/heads/master@{#386022} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/63f4a420e42667f2167b7fcf4a9ac8e10f66e792 Cr-Commit-Position: refs/heads/master@{#386022}
Message was sent while issue was closed.
behdad@chromium.org changed reviewers: + behdad@chromium.org
Message was sent while issue was closed.
Doesn't the can-use-word-cache logic also need update?
Message was sent while issue was closed.
A revert of this CL (patchset #6 id:100001) has been created in https://codereview.chromium.org/1871653003/ by magjed@chromium.org. The reason for reverting is: Speculatively reverting. I think this CL caused these failures: Regressions: Unexpected image-only failures (3) fast/forms/color/color-suggestion-picker-appearance.html [ Failure ] fast/forms/color/color-suggestion-picker-one-row-appearance.html [ Failure ] fast/forms/color/color-suggestion-picker-two-row-appearance.html [ Failure ] on some bots, e.g. https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Mac10.10.
Message was sent while issue was closed.
phistuck@gmail.com changed reviewers: + phistuck@gmail.com
Message was sent while issue was closed.
Wow... Are you sure this should have the consequence of rebaselining over a thousand tests?...
Message was sent while issue was closed.
A revert of this CL (patchset #6 id:100001) has been created in https://codereview.chromium.org/1879483002/ by kjellander@chromium.org. The reason for reverting is: Several webkit_tests tests started failing on Mac with this. See http://crbug.com/602207 for details.. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
