Bug 63363 – Add an option to enable legacy rounding hacks

• Status: RESOLVED
• Resolution: FIXED
• Component: Text

Add an option to enable legacy rounding hacks

Created attachment 98561 [details]
Re-add most rounding hack code and add SPI for enabling it on OS X

<rdar://problem/9385928>

[Darin Adler]

Comment on attachment 98561 [details]
Re-add most rounding hack code and add SPI for enabling it on OS X

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

Looks like the EWS patch did not apply.

I’m a little unclear on the importance of the multiple levels of allowing/disallowing rounding hacks, but I trust they are all needed.

> Source/WebCore/platform/graphics/StringTruncator.cpp:95
> -static float stringWidth(const Font& renderer, const UChar* characters, unsigned length)
> +static float stringWidth(const Font& renderer, const UChar* characters, unsigned length, bool disableRoundingHacks)

Seems potentially confusing that half this file uses the enum and half this file uses a bool. Any reason to not just use the enum all the way down?

> Source/WebCore/platform/graphics/StringTruncator.h:40
> +

Extra blank line here.

(In reply to comment #3)
> (From update of attachment 98561 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=98561&action=review
> 
> Looks like the EWS patch did not apply.

That's unfortunate. I can try uploading just the code changes, in case the test is the problem.  

> I’m a little unclear on the importance of the multiple levels of allowing/disallowing rounding hacks, but I trust they are all needed.

I think there is only one level that controls whether rounding hacks are allowed, then there are all the call sites that either ask for the hacks or don't. This is similar to the Core Graphics notions of "allow" and "should". 
> 
> > Source/WebCore/platform/graphics/StringTruncator.cpp:95
> > -static float stringWidth(const Font& renderer, const UChar* characters, unsigned length)
> > +static float stringWidth(const Font& renderer, const UChar* characters, unsigned length, bool disableRoundingHacks)
> 
> Seems potentially confusing that half this file uses the enum and half this file uses a bool. Any reason to not just use the enum all the way down?
>

Only historical reasons. 
 
> > Source/WebCore/platform/graphics/StringTruncator.h:40
> > +
> 
> Extra blank line here.

Created attachment 98575 [details]
Re-add most rounding hack code and add SPI for enabling it on OS X

Updated after r89690.

[WebKit Review Bot]

Comment on attachment 98575 [details]
Re-add most rounding hack code and add SPI for enabling it on OS X

Attachment 98575 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8937643

New failing tests:
css1/basic/comments.html
css1/basic/grouping.html
css1/box_properties/border_bottom_width.html
css1/box_properties/border_bottom_width_inline.html
css1/box_properties/border_left_width.html
css1/basic/inheritance.html
css1/box_properties/border_color_inline.html
css1/box_properties/border_inline.html
css1/basic/class_as_selector.html
css1/box_properties/border.html
css1/box_properties/acid_test.html
css1/box_properties/border_color.html
css1/box_properties/border_left_width_inline.html
css1/basic/id_as_selector.html
css1/box_properties/border_left.html
css1/box_properties/border_bottom_inline.html
css1/basic/containment.html
css1/box_properties/border_bottom.html
css1/box_properties/border_left_inline.html
css1/basic/contextual_selectors.html

[WebKit Review Bot]

Created attachment 98578 [details]
Archive of layout-test-results from ec2-cr-linux-02

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-02  Port: Chromium  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

Created attachment 98580 [details]
Re-add most rounding hack code and add SPI for enabling it on OS X

Landed in r89733. <http://trac.webkit.org/r89733>

[James Robinson]

Are y'all free to share why the rounding hacks had to be added back?  Are there site compatibility issues we should be aware of, or was this just about non-web content?  Thanks!

There are no known website compatibility issues.

[Ryosuke Niwa]

According to http://build.webkit.org/TestFailures/#/Leopard Intel Debug (Tests), fast/text/zero-font-size.html started failing on Leopard Intel Debug (Tests) after this patch was landed:

http://build.webkit.org/results/Leopard%20Intel%20Debug%20(Tests)/r89733%20(31715)/fast/text/zero-font-size-pretty-diff.html

The size of box is 0x80000000 instead of 0.

[Ryosuke Niwa]

(In reply to comment #12)
> The size of box is 0x80000000 instead of 0.

I mean the width.

(In reply to comment #12)
> According to http://build.webkit.org/TestFailures/#/Leopard Intel Debug (Tests), fast/text/zero-font-size.html started failing on Leopard Intel Debug (Tests) after this patch was landed:
> 
> http://build.webkit.org/results/Leopard%20Intel%20Debug%20(Tests)/r89733%20(31715)/fast/text/zero-font-size-pretty-diff.html
> 
> The size of box is 0x80000000 instead of 0.

I am looking into it.

wkGetGlyphTransformedAdvances() is sometimes returning a huge horizontal advance for some glyphs in Times under the 0 transform. That code hasn’t changed in r89733. I am going to check if the r89732 WidthIterator was somehow masking the incorrect values.

Reverting just WidthIterator changes the results from wkGetGlyphTransformedAdvances(). I don’t know how to explain this.

Filed bug 63512.