Bug 170245 – Long Arabic text in ContentEditable with css white-space=pre hangs Safari

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[alan]

rdar://problem/22127615

[alan]

Created attachment 305760 [details]
Patch

[Myles C. Maxfield]

Comment on attachment 305760 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=305760&action=review

Where's your performance test?

> Source/WebCore/rendering/line/BreakingContext.h:798
> +    float wrapW = wrapWidthOffset;

Can we give this a better name?

> Source/WebCore/rendering/line/BreakingContext.h:1065
> +            // Measuring complex text with ligature by individual codepoints and by complete words could produce considerably different width values.

I'd instead say "Measuring the width of complex text character-by-character, rather than measuring it all together, could produce considerably different width values."

> Source/WebCore/rendering/line/BreakingContext.h:1069
> +                wrapW = wrapWidthOffset + additionalTempWidth;

This doesn't seem right. Shouldn't this get reset from the uncommitted width of the LineWidth object?

[alan]

(In reply to Myles C. Maxfield from comment #2)
> Comment on attachment 305760 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=305760&action=review
> 
> Where's your performance test?
https://trac.webkit.org/changeset/214557/webkit


> > Source/WebCore/rendering/line/BreakingContext.h:798
> > +    float wrapW = wrapWidthOffset;
> 
> Can we give this a better name?
Sure.

> 
> > Source/WebCore/rendering/line/BreakingContext.h:1065
> > +            // Measuring complex text with ligature by individual codepoints and by complete words could produce considerably different width values.
> 
> I'd instead say "Measuring the width of complex text character-by-character,
> rather than measuring it all together, could produce considerably different
> width values."
ok.

> 
> > Source/WebCore/rendering/line/BreakingContext.h:1069
> > +                wrapW = wrapWidthOffset + additionalTempWidth;
> 
> This doesn't seem right. Shouldn't this get reset from the uncommitted width
> of the LineWidth object?
At this point wrapWidthOffset + additionalTempWidth == uncommitted width, but I agree, using the uncommitted width is a good idea.

[alan]

> > This doesn't seem right. Shouldn't this get reset from the uncommitted width
> > of the LineWidth object?
> At this point wrapWidthOffset + additionalTempWidth == uncommitted width,
> but I agree, using the uncommitted width is a good idea.
Hang on, that's incorrect.

[alan]

Created attachment 306048 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 306048 [details]
Patch

Clearing flags on attachment: 306048

Committed r214726: <http://trac.webkit.org/changeset/214726>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.