Bug 9808 - REGRESSION: Incorrect layout (and ERROR) when forcing ATSU For All Text
Summary: REGRESSION: Incorrect layout (and ERROR) when forcing ATSU For All Text
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 420+
Hardware: Mac OS X 10.4
: P1 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2006-07-09 11:02 PDT by mitz
Modified: 2006-07-10 09:13 PDT (History)
0 users

See Also:

Attachments
Test case (126 bytes, text/html)
2006-07-09 11:02 PDT, mitz 		no flags Details
Patch with manual test and changelog (2.36 KB, patch)
2006-07-09 11:18 PDT, mitz 		no flags Details | Formatted Diff | Diff
Patch (2.30 KB, patch)
2006-07-10 07:46 PDT, mitz 		darin: review+
Details | Formatted Diff | Diff
Show Obsolete (1) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description mitz 2006-07-09 11:02:11 PDT 
To reproduce this bug, open the attached test case, choose Use ATSU For All Text from Safari's Debug menu and reload the page. Notice that the layout changes (the line breaks after Lorem) and if you are using a debug build, an error will be printed:
ERROR: unexpected result from ATSUGetGlyphBounds(): actualNumBounds(389998136) != 1
Comment 1 mitz 2006-07-09 11:02:47 PDT 
Created attachment 9299 [details]
Test case
Comment 2 mitz 2006-07-09 11:18:33 PDT 
Created attachment 9300 [details]
Patch with manual test and changelog
Comment 3 Darin Adler 2006-07-09 17:37:24 PDT 
Comment on attachment 9300 [details]
Patch with manual test and changelog

Will this extra check have a measurable performance impact?

If not, then r=me, but how can we be sure?
Comment 4 mitz 2006-07-09 23:26:58 PDT 
Comment on attachment 9300 [details]
Patch with manual test and changelog

Going to play it safe performance-wise and move the check into the complex path where it matters.
Comment 5 mitz 2006-07-10 07:46:26 PDT 
Created attachment 9341 [details]
Patch

Move the empty run check down into floatWidthForComplexText()
Comment 6 Darin Adler 2006-07-10 08:29:23 PDT 
Comment on attachment 9341 [details]
Patch

r=me

Even better if there was a comment explaining that this is not just an optimization, but required for correct results, but the layout test should suffice.
Comment 7 Darin Adler 2006-07-10 09:13:38 PDT 
Committed revision 15305.