Measuring the width of a space character is redundant

• Component: New Bugs
• Status: NEW ---

[Myles C. Maxfield]

Measuring the width of a space character is redundant

[Myles C. Maxfield]

Created attachment 252486 [details]
Patch

[Myles C. Maxfield]

Comment on attachment 252486 [details]
Patch

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

> Source/WebCore/rendering/RenderBlockFlow.cpp:3994
>          const UChar space = ' ';

Unused.

[Myles C. Maxfield]

Created attachment 252497 [details]
Patch for landing

[Build Bot]

Comment on attachment 252497 [details]
Patch for landing

Attachment 252497 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5588724582711296

New failing tests:
fast/ruby/ruby-expansion-cjk-3.html
fast/ruby/ruby-expansion-cjk.html
http/tests/webfont/fallback-font-while-loading.html
fast/ruby/ruby-expansion-cjk-5.html
platform/mac/svg/fonts/svg-font-general.html
platform/mac/fast/text/rounding-hacks.html
fast/ruby/ruby-expansion-cjk-2.html
fast/ruby/ruby-expansion-cjk-4.html
fast/writing-mode/vertical-font-vmtx-units-per-em.html
fast/text/text-combine-different-fonts.html
fast/writing-mode/text-orientation-basic.html

[Build Bot]

Created attachment 252503 [details]
Archive of layout-test-results from ews102 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-mavericks  Platform: Mac OS X 10.9.5

[Build Bot]

Comment on attachment 252497 [details]
Patch for landing

Attachment 252497 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/5326887270219776

New failing tests:
fast/ruby/ruby-expansion-cjk-3.html
fast/ruby/ruby-expansion-cjk.html
http/tests/webfont/fallback-font-while-loading.html
fast/ruby/ruby-expansion-cjk-5.html
platform/mac/svg/fonts/svg-font-general.html
platform/mac/fast/text/rounding-hacks.html
fast/ruby/ruby-expansion-cjk-2.html
fast/ruby/ruby-expansion-cjk-4.html
fast/writing-mode/vertical-font-vmtx-units-per-em.html
fast/text/text-combine-different-fonts.html
fast/writing-mode/text-orientation-basic.html

[Build Bot]

Created attachment 252507 [details]
Archive of layout-test-results from ews107 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5

[Myles C. Maxfield]

Comment on attachment 252497 [details]
Patch for landing

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

> Source/WebCore/rendering/RenderText.cpp:549
> +            maxW -= font.spaceWidth();

This caused this test to fail:

fast/writing-mode/vertical-font-vmtx-units-per-em.html

> Source/WebCore/rendering/RenderText.cpp:715
> +    float wordTrailingSpaceWidth = font.typesettingFeatures() & Kerning ? font.spaceWidth() + wordSpacing : 0;

This caused this test to fail:

fast/writing-mode/text-orientation-basic.html

> Source/WebCore/rendering/SimpleLineLayoutTextFragmentIterator.cpp:184
> +        width = m_style.font.spaceWidth();

This caused this test to fail:

platform/mac/fast/text/rounding-hacks.html

> Source/WebCore/rendering/line/BreakingContext.h:743
> +    float wordTrailingSpaceWidth = (font.typesettingFeatures() & Kerning) && !textLayout ? font.spaceWidth() + wordSpacing : 0;

This caused these tests to fail:

fast/ruby/ruby-expansion-cjk-2.html
fast/ruby/ruby-expansion-cjk-3.html
fast/ruby/ruby-expansion-cjk-4.html
fast/ruby/ruby-expansion-cjk-5.html
fast/ruby/ruby-expansion-cjk.html
fast/text/text-combine-different-fonts.html
http/tests/webfont/fallback-font-while-loading.html
platform/mac/svg/fonts/svg-font-general.html

[Myles C. Maxfield]

http/tests/webfont/fallback-font-while-loading.html shows that this patch is fundamentally incorrect. In particular, the implementation of FontCacade::spaceWidth() is this:

float spaceWidth() const { return primaryFont().spaceWidth() + m_letterSpacing; }

The fact that this disregards fallback fonts means that this function really shouldn't be used as-is. Instead, this functions should have the behavior of the replaced code in this patch.

[Myles C. Maxfield]

(In reply to comment #9)
> http/tests/webfont/fallback-font-while-loading.html shows that this patch is
> fundamentally incorrect. In particular, the implementation of
> FontCacade::spaceWidth() is this:
> 
> float spaceWidth() const { return primaryFont().spaceWidth() +
> m_letterSpacing; }
> 
> The fact that this disregards fallback fonts means that this function really
> shouldn't be used as-is. Instead, this functions should have the behavior of
> the replaced code in this patch.

...Aaaaand we can't use this approach in FontCascade because SVG fonts require a RenderObject for context. This requirements goes away as soon as SVG fonts move entirely to the SVG -> OTF Font Converter.

[Myles C. Maxfield]

Blocked by https://bugs.webkit.org/show_bug.cgi?id=143402