Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1897853004:
Fix InlineTextBox::characterWidths() not to measure every substring (Closed)
|
Created:
4 years, 8 months ago by kojii Modified:
4 years, 8 months ago CC:
blink-reviews, blink-reviews-layout_chromium.org, chromium-reviews, eae+blinkwatch, jchaffraix+rendering, leviw+renderwatch, pdr+renderingwatchlist_chromium.org, szager+layoutwatch_chromium.org, zoltan1 Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix InlineTextBox::characterWidths() not to measure every substring
This patch changes InlineTextBox::characterWidths() to use
Font::individualCharacterRanges() rather than to measure every
substring of InlineTextBox. Measuring every substring without context
is very expensive and also is not correct.
When dev tools have long strings without spaces such as minified JS and
accessbility is turned on, the measuring of substrings can almost hang
it up.
Currently, WebAXObject::characterOffsets() is the only user of this
function.
BUG=593679, 568032
TEST=Existing tests cover, such as AutomationApiTest.BoundsForRange
Committed: https://crrev.com/11f1c680cca2250e01121b9bddd7bc6f8fb28a96
Cr-Commit-Position: refs/heads/master@{#388162}
Patch Set 1 #Patch Set 2 : Fix AutomationApiTest.BoundsForRange, use width() to simplify the logic #Messages
Total messages: 18 (8 generated)
Description was changed from ========== characterWidths BUG=593679 ========== to ========== Fix InlineTextBox::characterWidths() not to measure every substring This patch changes InlineTextBox::characterWidths() to use Font::individualCharacterRanges() rather than to measure every substring of InlineTextBox. Measuring every substring without context is very expensive and also is not correct. BUG=593679 ==========
kojii@chromium.org changed reviewers: + drott@chromium.org, eae@chromium.org
PTAL. Looks like urgent, crbug.com/593679#c106 wants the fix to land by 2am PST today.
Description was changed from ========== Fix InlineTextBox::characterWidths() not to measure every substring This patch changes InlineTextBox::characterWidths() to use Font::individualCharacterRanges() rather than to measure every substring of InlineTextBox. Measuring every substring without context is very expensive and also is not correct. BUG=593679 ========== to ========== Fix InlineTextBox::characterWidths() not to measure every substring This patch changes InlineTextBox::characterWidths() to use Font::individualCharacterRanges() rather than to measure every substring of InlineTextBox. Measuring every substring without context is very expensive and also is not correct. Currently, WebAXObject::characterOffsets() is the only user of this function. BUG=593679 ==========
Description was changed from ========== Fix InlineTextBox::characterWidths() not to measure every substring This patch changes InlineTextBox::characterWidths() to use Font::individualCharacterRanges() rather than to measure every substring of InlineTextBox. Measuring every substring without context is very expensive and also is not correct. Currently, WebAXObject::characterOffsets() is the only user of this function. BUG=593679 ========== to ========== Fix InlineTextBox::characterWidths() not to measure every substring This patch changes InlineTextBox::characterWidths() to use Font::individualCharacterRanges() rather than to measure every substring of InlineTextBox. Measuring every substring without context is very expensive and also is not correct. Currently, WebAXObject::characterOffsets() is the only user of this function. BUG=593679 TEST=Existing tests cover, such as AutomationApiTest.BoundsForRange ==========
kojii@chromium.org changed reviewers: + tkent@chromium.org
+tkent@
looks ok. lgtm
Change LGTM. Maybe you could explain why is this under BUG=593679 if > Currently, WebAXObject::characterOffsets() is the only user of this function. in the change log? Perhaps you could add issue 568032 as well. Thanks for addressing this!
Description was changed from ========== Fix InlineTextBox::characterWidths() not to measure every substring This patch changes InlineTextBox::characterWidths() to use Font::individualCharacterRanges() rather than to measure every substring of InlineTextBox. Measuring every substring without context is very expensive and also is not correct. Currently, WebAXObject::characterOffsets() is the only user of this function. BUG=593679 TEST=Existing tests cover, such as AutomationApiTest.BoundsForRange ========== to ========== Fix InlineTextBox::characterWidths() not to measure every substring This patch changes InlineTextBox::characterWidths() to use Font::individualCharacterRanges() rather than to measure every substring of InlineTextBox. Measuring every substring without context is very expensive and also is not correct. When dev tools have long strings without spaces such as minified JS and accessbility is turned on, the measuring of substrings can almost hang it up. Currently, WebAXObject::characterOffsets() is the only user of this function. BUG=593679, 568032 TEST=Existing tests cover, such as AutomationApiTest.BoundsForRange ==========
On 2016/04/19 at 07:48:01, drott wrote: > Change LGTM. Maybe you could explain why is this under BUG=593679 if > > Currently, WebAXObject::characterOffsets() is the only user of this > function. > in the change log? Perhaps you could add issue 568032 as well. > > Thanks for addressing this! Thank you, added to the change log, and confirmed 'show accessibility tree' is improved.
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/1897853004/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1897853004/20001
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Fix InlineTextBox::characterWidths() not to measure every substring This patch changes InlineTextBox::characterWidths() to use Font::individualCharacterRanges() rather than to measure every substring of InlineTextBox. Measuring every substring without context is very expensive and also is not correct. When dev tools have long strings without spaces such as minified JS and accessbility is turned on, the measuring of substrings can almost hang it up. Currently, WebAXObject::characterOffsets() is the only user of this function. BUG=593679, 568032 TEST=Existing tests cover, such as AutomationApiTest.BoundsForRange ========== to ========== Fix InlineTextBox::characterWidths() not to measure every substring This patch changes InlineTextBox::characterWidths() to use Font::individualCharacterRanges() rather than to measure every substring of InlineTextBox. Measuring every substring without context is very expensive and also is not correct. When dev tools have long strings without spaces such as minified JS and accessbility is turned on, the measuring of substrings can almost hang it up. Currently, WebAXObject::characterOffsets() is the only user of this function. BUG=593679, 568032 TEST=Existing tests cover, such as AutomationApiTest.BoundsForRange Committed: https://crrev.com/11f1c680cca2250e01121b9bddd7bc6f8fb28a96 Cr-Commit-Position: refs/heads/master@{#388162} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/11f1c680cca2250e01121b9bddd7bc6f8fb28a96 Cr-Commit-Position: refs/heads/master@{#388162}
Message was sent while issue was closed.
LGTM, thank you!
Message was sent while issue was closed.
LGTM, thank you! |
