Bug 63363 - Add an option to enable legacy rounding hacks
Summary: Add an option to enable legacy rounding hacks
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Text (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: mitz
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2011-06-24 16:20 PDT by mitz
Modified: 2011-09-07 12:06 PDT (History)
5 users (show)

See Also:


Attachments
Re-add most rounding hack code and add SPI for enabling it on OS X (161.90 KB, patch)
2011-06-24 16:38 PDT, mitz
andersca: review+
Details | Formatted Diff | Diff
Re-add most rounding hack code and add SPI for enabling it on OS X (54.84 KB, patch)
2011-06-24 21:45 PDT, mitz
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-02 (1.29 MB, application/zip)
2011-06-24 22:07 PDT, WebKit Review Bot
no flags Details
Re-add most rounding hack code and add SPI for enabling it on OS X (162.06 KB, patch)
2011-06-24 22:36 PDT, mitz
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description mitz 2011-06-24 16:20:36 PDT
Add an option to enable legacy rounding hacks
Comment 1 mitz 2011-06-24 16:38:37 PDT
Created attachment 98561 [details]
Re-add most rounding hack code and add SPI for enabling it on OS X
Comment 2 mitz 2011-06-24 16:39:24 PDT
<rdar://problem/9385928>
Comment 3 Darin Adler 2011-06-24 17:04:31 PDT
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.
Comment 4 mitz 2011-06-24 20:40:42 PDT
(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.
Comment 5 mitz 2011-06-24 21:45:57 PDT
Created attachment 98575 [details]
Re-add most rounding hack code and add SPI for enabling it on OS X

Updated after r89690.
Comment 6 WebKit Review Bot 2011-06-24 22:07:20 PDT
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
Comment 7 WebKit Review Bot 2011-06-24 22:07:25 PDT
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
Comment 8 mitz 2011-06-24 22:36:25 PDT
Created attachment 98580 [details]
Re-add most rounding hack code and add SPI for enabling it on OS X
Comment 9 mitz 2011-06-24 23:49:43 PDT
Landed in r89733. <http://trac.webkit.org/r89733>
Comment 10 James Robinson 2011-06-27 13:49:59 PDT
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!
Comment 11 mitz 2011-06-27 14:31:48 PDT
There are no known website compatibility issues.
Comment 12 Ryosuke Niwa 2011-06-27 21:00:44 PDT
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.
Comment 13 Ryosuke Niwa 2011-06-27 21:01:18 PDT
(In reply to comment #12)
> The size of box is 0x80000000 instead of 0.

I mean the width.
Comment 14 mitz 2011-06-27 21:21:08 PDT
(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.
Comment 15 mitz 2011-06-27 23:01:01 PDT
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.
Comment 16 mitz 2011-06-27 23:05:57 PDT
Reverting just WidthIterator changes the results from wkGetGlyphTransformedAdvances(). I don’t know how to explain this.
Comment 17 mitz 2011-06-27 23:14:29 PDT
Filed bug 63512.