Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(269)

Issues Search
    My Issues | Starred     Open | Closed | All

Issue 655073002: Fix trailing space handling for complex text (Closed)

Created:
6 years, 2 months ago by eae
Modified:
6 years, 2 months ago
Reviewers:
chrishtr, pdr.
CC:
blink-reviews, blink-reviews-rendering, eae+blinkwatch, jchaffraix+rendering, leviw+renderwatch, pdr+renderingwatchlist_chromium.org, rune+blink, zoltan1
Base URL:
https://chromium.googlesource.com/chromium/blink.git@master
Project:
blink
Visibility:
Public.

Description

Fix trailing space handling for complex text Fix bug in line layout where it was incorrectly assumed that trailing spaces would always be measured with the simple font code path. R=chrishtr@chromium.org BUG=423274 TEST=fast/text/international/complex-text-trailing-space.html Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=183716

Patch Set 1 #

Total comments: 9

Patch Set 2 : #

Unified diffs Side-by-side diffs Delta from patch set Stats (+69 lines, -1 line) Patch
A LayoutTests/fast/text/international/complex-text-trailing-space.html View 1 1 chunk +30 lines, -0 lines 0 comments Download
A LayoutTests/fast/text/international/complex-text-trailing-space-expected.html View 1 1 chunk +33 lines, -0 lines 0 comments Download
M Source/core/rendering/RenderBlockLineLayout.cpp View 1 chunk +6 lines, -1 line 0 comments Download

Messages

Total messages: 14 (2 generated)
eae
6 years, 2 months ago (2014-10-14 23:15:08 UTC) #1
pdr.
https://codereview.chromium.org/655073002/diff/1/Source/core/rendering/RenderBlockLineLayout.cpp File Source/core/rendering/RenderBlockLineLayout.cpp (right): https://codereview.chromium.org/655073002/diff/1/Source/core/rendering/RenderBlockLineLayout.cpp#newcode1177 Source/core/rendering/RenderBlockLineLayout.cpp:1177: if (useComplexCodePath) Why doesn't constructTextRun handle setting whether it ...
6 years, 2 months ago (2014-10-14 23:21:21 UTC) #3
eae
On 2014/10/14 23:21:21, pdr wrote: > https://codereview.chromium.org/655073002/diff/1/Source/core/rendering/RenderBlockLineLayout.cpp > File Source/core/rendering/RenderBlockLineLayout.cpp (right): > > https://codereview.chromium.org/655073002/diff/1/Source/core/rendering/RenderBlockLineLayout.cpp#newcode1177 > ...
6 years, 2 months ago (2014-10-14 23:45:14 UTC) #4
chrishtr
https://codereview.chromium.org/655073002/diff/1/LayoutTests/fast/text/international/complex-text-trailing-space.html File LayoutTests/fast/text/international/complex-text-trailing-space.html (right): https://codereview.chromium.org/655073002/diff/1/LayoutTests/fast/text/international/complex-text-trailing-space.html#newcode27 LayoutTests/fast/text/international/complex-text-trailing-space.html:27: forced to use the complex path. Please document which ...
6 years, 2 months ago (2014-10-14 23:50:10 UTC) #5
pdr.
LGTM Is there a bug/fixme/mentalnote for fixing the complex text bit on runs? I completely ...
6 years, 2 months ago (2014-10-14 23:57:00 UTC) #6
eae
On Tue, Oct 14, 2014 at 4:56 PM, <pdr@chromium.org> wrote: > LGTM > > Is ...
6 years, 2 months ago (2014-10-14 23:59:57 UTC) #7
eae
https://codereview.chromium.org/655073002/diff/1/LayoutTests/fast/text/international/complex-text-trailing-space-expected.html File LayoutTests/fast/text/international/complex-text-trailing-space-expected.html (right): https://codereview.chromium.org/655073002/diff/1/LayoutTests/fast/text/international/complex-text-trailing-space-expected.html#newcode12 LayoutTests/fast/text/international/complex-text-trailing-space-expected.html:12: text-rendering: optimizeLegibility; In this file this style rule forces ...
6 years, 2 months ago (2014-10-15 00:30:37 UTC) #8
chrishtr
lgtm
6 years, 2 months ago (2014-10-15 00:49:05 UTC) #9
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/655073002/20001
6 years, 2 months ago (2014-10-15 01:42:28 UTC) #11
commit-bot: I haz the power
Committed patchset #2 (id:20001) as 183716
6 years, 2 months ago (2014-10-15 04:26:54 UTC) #12
pdr.
On 2014/10/14 at 23:57:00, pdr wrote: > https://codereview.chromium.org/655073002/diff/1/Source/core/rendering/RenderBlockLineLayout.cpp#newcode1170 > Source/core/rendering/RenderBlockLineLayout.cpp:1170: bool useComplexCodePath = !toRenderText(trailingSpaceChild)-> > ...
6 years, 2 months ago (2014-10-15 06:15:08 UTC) #13
eae
6 years, 2 months ago (2014-10-15 15:11:15 UTC) #14
Message was sent while issue was closed. 
On 2014/10/15 06:15:08, pdr wrote:
> On 2014/10/14 at 23:57:00, pdr wrote:
> >
>
https://codereview.chromium.org/655073002/diff/1/Source/core/rendering/Render...
> > Source/core/rendering/RenderBlockLineLayout.cpp:1170: bool
useComplexCodePath
> = !toRenderText(trailingSpaceChild)->
> > Can you move this below and use the (poorly-named) 't' RenderText?
> 
> 
> Was there a reason for not doing this?
> 
> I was suggesting the following:
> static inline void stripTrailingSpace(float& inlineMax, float& inlineMin,
> RenderObject* trailingSpaceChild)
> {
>     if (trailingSpaceChild && trailingSpaceChild->isText()) {
>         // Collapse away the trailing space at the end of a block.
>         RenderText* renderText = toRenderText(trailingSpaceChild);
>         const UChar space = ' ';
>         const Font& font = renderText->style()->font(); // FIXME: This ignores
> first-line.
>         TextRun run = constructTextRun(renderText, font, &space, 1,
> renderText->style(), LTR);
>         bool useComplexCodePath = !renderText->canUseSimpleFontCodePath();
>         if (useComplexCodePath)
>             run.setUseComplexCodePath(true);
>         float spaceWidth = font.width(run);
>         inlineMax -= spaceWidth + font.fontDescription().wordSpacing();
>         if (inlineMin > inlineMax)
>             inlineMin = inlineMax;
>     }
> }

Missed that suggestion, will do it in a follow up CL.

Powered by Google App Engine
This is Rietveld 408576698