Bug 40966 – [chromium] overlapping characters in complex text

Bug 40966 - [chromium] overlapping characters in complex text


Summary:	[chromium] overlapping characters in complex text

Status:	RESOLVED FIXED

Product:	WebKit
Classification:	Unclassified
Component:	New Bugs
Version:	528+ (Nightly build)
Platform:	Other Mac OS X 10.5

Importance:	P2 Normal
Assigned To:	Evan Martin

URL:
Keywords:

Depends on:
Blocks:
	Show dependency tree / graph

Reported:	2010-06-21 20:18 PDT by Evan Martin
Modified:	2010-06-24 16:12 PDT (History)
CC List:	1 user (show)

See Also:

Attachments
Patch (40.84 KB, patch) 2010-06-21 20:19 PDT, Evan Martin	no flags	Details \| Formatted Diff \| Diff
Patch (41.15 KB, patch) 2010-06-21 20:21 PDT, Evan Martin	no flags	Details \| Formatted Diff \| Diff
Patch (117.91 KB, patch) 2010-06-22 13:30 PDT, Evan Martin	abarth: review+	Details \| Formatted Diff \| Diff
a simple test (611 bytes, text/html) 2010-06-23 15:25 PDT, Jungshik Shin	no flags	Details
Show Obsolete (2) 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 Evan Martin 2010-06-21 20:18:40 PDT

[chromium] overlapping characters in complex text

Comment 1 Evan Martin 2010-06-21 20:19:29 PDT

Created attachment 59328 [details]
Patch

Comment 2 Evan Martin 2010-06-21 20:21:29 PDT

Created attachment 59329 [details]
Patch

Comment 3 Adam Langley 2010-06-22 09:29:37 PDT

Code cleanup is good, but I'm not clear where the actual fix is. Is it the clearing of the arrays? If so, isn't that a Harfbuzz bug? Maybe add a comment when clearing them so that someone doesn't optimise it away in the future.

Comment 4 Evan Martin 2010-06-22 13:25:53 PDT

I spent a long time reverting and reapplying bits of this patch and I am actually kind of confused why this refactoring is making the test fail or pass.  However, the new code is clearly more correct than the prior code (instead of a looping reallocation we now just allocate the proper memory in the first go) I think the patch is ok anyway.  I imagine it's probably something using uninitialized memory that's causing the problem.

Comment 5 Evan Martin 2010-06-22 13:30:28 PDT

Created attachment 59412 [details]
Patch

Comment 6 Jungshik Shin 2010-06-23 15:25:22 PDT

Created attachment 59567 [details]
a simple test

I confirmed that the patch here fixes one of two issues reported against Chromium ( http://crbug.com/44646.html http://crbug.com/43951.html ).  I'm attaching a test html file illustrating the issue fixed by this patch.

Comment 7 Adam Barth 2010-06-24 15:55:03 PDT

Comment on attachment 59412 [details]
Patch

Not my strong suit, but looks reasonable.

WebCore/platform/graphics/chromium/FontLinux.cpp:421
 +          m_glyphs16 = new uint16_t[size];
This doesn't need to be zeroed?

Comment 8 Evan Martin 2010-06-24 15:57:49 PDT

That's an output, so it seems ok.

Comment 9 Evan Martin 2010-06-24 16:12:33 PDT

Committed r61795: <http://trac.webkit.org/changeset/61795>