16981 2008-01-22 21:02:56 -0800 Thai word break support for platforms which don't use ICU 2012-08-02 04:17:20 -0700 1 1 1 Unclassified WebKit Layout and Rendering 528+ (Nightly build) PC Linux RESOLVED WONTFIX http://linux.thai.net/viewvc/viewvc.cgi/software/webkit/webkit-0svn25144/webkit_wordbreak.diff?revision=1.4&view=markup P2 Normal --- 60786 1 pattara webkit-unassigned alp ap hausmann lars.knoll mitz oldest_to_newest 68007 0 pattara 2008-01-22 21:02:56 -0800 In Thai, a sentence is composed of several words without space in between like in English. Therefore it is neccessary for the software to know word boundary locations so that it can wrap the line properly (i.e., wrap between words, not in the middle of the word). This patch adds Thai word break support for WebKit using libthai library (http://sourceforge.net/projects/libthai). The algorithm/code is very similar to the one in KDE's KHTML. As a Thai, I think it's best to link WebKit with libthai since the compile time so that webkit will always come with full Thai support, but I understand the rest of the world who don't read Thai might not want to install libthai library when they don't really use it. Therefore the patch checks to load libthai dynamically (using dlopen) if libthai is not linked. http://linux.thai.net/viewvc/viewvc.cgi/software/webkit/webkit-0svn25144/webkit_wordbreak.diff?revision=1.4&view=markup 68008 1 18616 pattara 2008-01-22 21:04:32 -0800 Created attachment 18616 Patch to enable Thai wordbreaking in WebKit Tested with Ubuntu libqtwebkit0d package version 0~svn27674-1~gutsy1 68012 2 ap 2008-01-23 00:16:09 -0800 I'm not sure about the scope of this bug. Does this work on Mac/Windows WebKit already, thanks to ICU? If not, could you please provide a test case showing the problems? 68016 3 mitz 2008-01-23 00:26:35 -0800 (In reply to comment #2) > I'm not sure about the scope of this bug. Does this work on Mac/Windows WebKit > already, thanks to ICU? On Mac OS X WebKit uses UCCreateTextBreakLocator precisely because the version of ICU that ships on the system does not support Thai (see bug 12175). 68022 4 pattara 2008-01-23 03:44:22 -0800 Test page: http://www.mm.co.th/~pattara/thai/wbrtest-short.html In a working system, sentences should be wrapped at the boundaries of words, not in between. But I think in MAC and Windows, if it doesn't work already, the solution should be different as those systems already have API for Thai word breaking, e.g., ATSUI, UniScribe, we should just use them, no need to use libthai)? 68024 5 ap 2008-01-23 04:15:31 -0800 To have this patch reviewed, please mark it review? by clicking Edit link to the right of the patch. There are a number of coding style issues that would definitely make it rejected though, so you may want to read <http://webkit.org/coding/contributing.html>. It's also OK to request review if you'd like to get feedback on the substance of the patch first, and fix style later. Sounds like this fix will be very important for platforms that can not use existing Thai support from ICU - thank you very much for working on it! 68043 6 pattara 2008-01-23 10:12:56 -0800 I think ICU is another possible option. I saw <a href=http://bugs.webkit.org/show_bug.cgi?id=4628>Bug 4628</a>. We could add a check -- if LINUX then use ICU for Thai word breaking instead of Carbon? But then we need an explicit dependency between ICU and WebKit (in Ubuntu where I tested there seems none though). The above patch will look for libthai and use it dynamicially if exists, without forcing users to install it. For Thais, compile-time linking is of course better but for the rest non-Thai-speaking users, this dynamic method may save their bandwidth and disk space. 68049 7 hyatt 2008-01-23 12:17:37 -0800 The patch mentions "libkhtml." Was this code lifted from KHTML and moved into WebKit? If so, please make sure to include the appropriate copyright attributions at the top of break_lines.cpp. 68086 8 pattara 2008-01-23 19:00:46 -0800 It was lifted from KHTML and it was us who submitted to KHTML. Anyway, will add attributions as you suggested. 68318 9 18616 eric 2008-01-26 14:18:22 -0800 Comment on attachment 18616 Patch to enable Thai wordbreaking in WebKit First, let me thank you VERY MUCH for taking the time to contribute to WebKit. We strongly value new contributions, and I would like to encourage you to contribute more in the future. That said, we're not going to take this patch. More information below: Ok, so it looks like ICU does support Thai breaking. Safari 3.0.4 on Mac OS X already gets this test case correct: http://www.mm.co.th/~pattara/thai/wbrtest-short.html (as far as I can tell) So this bug only applies to platforms which are not able to use ICU. Both WebKitWinApple and WebKitMac use ICU. I'm not certain about WebKitGtk or WebKitQt. In order for this bug to move forward, we would need an example of a platform combination under active development which did not support proper Thai word breaks. In that case, we would need to re-write this patch to apply to the TextIterator platform abstraction model (which looks to be possible. This patch has a raft of style issues, as previously mentioned, as well as memory leaks (the entire thai cached leaks). I'm marking this r-, and I recommend with close the bug as "INVALID" unless we can come up with a paltform combination which doesn't already have thai word break support. 68319 10 eric 2008-01-26 14:22:00 -0800 I meant "TextBreakIterator" when I said TextIterator. The files in question can be found under: WebCore/platform/text 68320 11 mitz 2008-01-26 14:26:38 -0800 (In reply to comment #9) > Ok, so it looks like ICU does support Thai breaking. Safari 3.0.4 on Mac OS X > already gets this test case correct: > http://www.mm.co.th/~pattara/thai/wbrtest-short.html > (as far as I can tell) The Mac OS X port of WebKit does not use ICU for word-breaking because the version of ICU that ships with Tiger does not support Thai. Instead, WebKit uses Carbon's Unicode Utilities on both Tiger and Leopard. 68342 12 pattara 2008-01-26 22:17:56 -0800 (In reply to comment #9) > (From update of attachment 18616 [edit]) > First, let me thank you VERY MUCH for taking the time to contribute to WebKit. > We strongly value new contributions, and I would like to encourage you to > contribute more in the future. > That said, we're not going to take this patch. More information below: > Ok, so it looks like ICU does support Thai breaking. Safari 3.0.4 on Mac OS X > already gets this test case correct: > http://www.mm.co.th/~pattara/thai/wbrtest-short.html > (as far as I can tell) > So this bug only applies to platforms which are not able to use ICU. Both > WebKitWinApple and WebKitMac use ICU. I'm not certain about WebKitGtk or > WebKitQt. Safari on Mac works ok (thanks to Carbon). But as far as we have tested, Thai word breaking is NOT working in WebKitQt on Linux. As I mentioned in #6, maybe the problem is better solved using ICU provided that WebKit and ICU is already connected. But as I see on my Linux box that the WebKitQt package doesn't depend on libicu. So we guess maybe WebKit is not using libicu and hence propose this patch which uses libthai as a possible solution for word breaking, similar to in KHTML and Firefox. The bottom line is just to have propert Thai word breaking in WebKit. Any solution is actually fine. Please let us know if we can be of any further help regarding this (testing, etc..). 68343 13 ap 2008-01-26 23:30:11 -0800 Many non-Apple ports do not want to use ICU (because it's too large), so I think that this work is of good value. CC'ing some people that would know for sure. 112182 14 hausmann 2009-03-04 04:18:50 -0800 I think for non-ICU builds, such as the Qt one, the TextBreakIterator should continue to work correctly for Thai. The TextBreakIterator for the Qt build uses QTextBoundaryFinder. That's the class that may need some additional fixing for Thai. I'm working with Pattara on that :-) 685215 15 hausmann 2012-08-02 04:17:20 -0700 For the Qt port we have chosen to use ICU, which I think means now that all ports are using ICU and this bug can be closed :) 18616 2008-01-22 21:04:32 -0800 2010-06-10 16:41:40 -0700 Patch to enable Thai wordbreaking in WebKit webkit_wordbreak.diff text/plain 5492 pattara ZGlmZiAtdU5yIHdlYmtpdC0wfnN2bjI1MTQ0Lm9yaWcvV2ViQ29yZS9yZW5kZXJpbmcvYnJlYWtf bGluZXMuY3BwIHdlYmtpdC0wfnN2bjI1MTQ0L1dlYkNvcmUvcmVuZGVyaW5nL2JyZWFrX2xpbmVz LmNwcAotLS0gd2Via2l0LTB+c3ZuMjUxNDQub3JpZy9XZWJDb3JlL3JlbmRlcmluZy9icmVha19s aW5lcy5jcHAJMjAwNy0wOC0xOSAxMjoxNToxNS4wMDAwMDAwMDAgKzA3MDAKKysrIHdlYmtpdC0w fnN2bjI1MTQ0L1dlYkNvcmUvcmVuZGVyaW5nL2JyZWFrX2xpbmVzLmNwcAkyMDA4LTAxLTIyIDA5 OjMzOjA0LjAwMDAwMDAwMCArMDcwMApAQCAtMTExLDQgKzExMSwxMzggQEAKICAgICByZXR1cm4g bGVuOwogfQogCisjaWYgUExBVEZPUk0oVU5JWCkKKy8qIFRoYWkgU3VwcG9ydCAKKyAqIElmIEhB VkVfTElCVEhBSSBpcyBkZWZpbmVkLCBsaWJraHRtbCB3aWxsIGxpbmsgYWdhaW5zdAorICogbGli dGhhaSBzaW5jZSBjb21waWxlIHRpbWUuIE90aGVyd2lzZSBpdCB3aWxsIHRyeSB0bworICogZGxv cGVuIGF0IHJ1bi10aW1lCisgKgorICogT3R0IFBhdHRhcmEsIEpha2thcHVuIEt3YW5yb2VuZ2ph aSAxNiBKYW4gMjAwOAorICovCisjaWZuZGVmIEhBVkVfTElCVEhBSQordHlwZWRlZiBpbnQgKCp0 aF9icmtfZGVmKShjb25zdCB1bnNpZ25lZCBjaGFyKiwgaW50W10sIGludCk7CitzdGF0aWMgdGhf YnJrX2RlZiB0aF9icmsgPSAwOwordHlwZWRlZiBpbnQgKCp0aF91bmkydGlzX2xpbmVfZGVmKShj b25zdCB3Y2hhcl90KiwgdW5zaWduZWQgY2hhciosIGludCk7CitzdGF0aWMgdGhfdW5pMnRpc19s aW5lX2RlZiB0aF91bmkydGlzX2xpbmUgPSAwOworI2Vsc2UKKyNpbmNsdWRlIDx0aGFpL3RoYWls aWIuaD4KKyNpbmNsdWRlIDx0aGFpL3RoYnJrLmg+CisjZW5kaWYKKworc3RydWN0IFRoYWlDYWNo ZQoreworICAgIFRoYWlDYWNoZSgpIHsKKyAgICAgICAgc3RyaW5nID0gMDsKKyAgICAgICAgYWxs b2NhdGVkID0gMHg0MDA7CisgICAgICAgIHdicnBvcyA9IChpbnQgKikgbWFsbG9jKGFsbG9jYXRl ZCpzaXplb2YoaW50KSk7CisgICAgICAgIG51bXdicnBvcyA9IDA7CisgICAgICAgIG51bWlzYnJl YWthYmxlID0gMHg0MDA7CisgICAgICAgIGlzYnJlYWthYmxlID0gKGludCAqKSBtYWxsb2MobnVt aXNicmVha2FibGUqc2l6ZW9mKGludCkpOworICAgIH0KKyAgICB+VGhhaUNhY2hlKCkgeworICAg ICAgICBmcmVlKHdicnBvcyk7CisgICAgICAgIGZyZWUoaXNicmVha2FibGUpOworICAgIH0KKyAg ICAvL2NvbnN0IHVuc2lnbmVkIGNoYXIgKnN0cmluZzsKKwljb25zdCBVQ2hhciogc3RyaW5nOwor ICAgIGludCAqd2JycG9zOworICAgIGludCAqaXNicmVha2FibGU7CisgICAgaW50IGFsbG9jYXRl ZDsKKyAgICBpbnQgbnVtd2JycG9zLG51bWlzYnJlYWthYmxlOworfTsKK3N0YXRpYyBUaGFpQ2Fj aGUgKmNhY2hlID0gMDsKKwordm9pZCBjbGVhbnVwX3RoYWlicmVha3MoKQoreworICAgIGRlbGV0 ZSBjYWNoZTsKKyAgICBjYWNoZSA9IDA7CisjaWZuZGVmIEhBVkVfTElCVEhBSQorICAgIHRoX2Jy ayA9IDA7CisgICAgdGhfdW5pMnRpc19saW5lID0gMDsKKyNlbmRpZgorfQorCisvKiBOZWVkIGRs ZmNuLmggZm9yIHRoZSByb3V0aW5lcyB0byBkeW5hbWljYWxseSBsb2FkCisgKiB0aGFpIHdvcmQg Y3V0IGxpYnJhcnkgc28gdGhhdCB0aGVyZSBpcyBubyBuZWVkIHRvCisgKiBpbmNsdWRlIHRoZSBo ZWFkZXIgZmlsZSB3aGVuIGNvbXBpbG5nCisqLworI2luY2x1ZGUgPGRsZmNuLmg+CisjaW5jbHVk ZSA8c3RkaW8uaD4KK2Jvb2wgaXNCcmVha2FibGVUaGFpKCBjb25zdCBVQ2hhciAqc3RyaW5nLCBj b25zdCBpbnQgcG9zLCBjb25zdCBpbnQgbGVuKQoreworCXVuc2lnbmVkIGNoYXIqIHRpc19jaGFy OworCXZvaWQqIG1vZHVsZTsKKwlzdGF0aWMgaW50IHRyeW1vcmUgPSAxOworCWNvbnN0IGNoYXIg KmVycm9yOworCisgICAgLyogbG9hZCBsaWJ0aGFpIGR5bmFtaWNhbGx5ICovCisgICAgaWYgKCF0 aF9icmsgJiYgIXRoX3VuaTJ0aXNfbGluZSAmJiAhdHJ5bW9yZSkgeworICAgICAgcmV0dXJuIDA7 CisgICAgfSBlbHNlIGlmICggIXRoX2JyayAmJiB0cnltb3JlICkgeworICAgIAltb2R1bGUgPSBk bG9wZW4oImxpYnRoYWkuc28uMCIsIFJUTERfTEFaWSk7CisgICAgCWlmICghbW9kdWxlKSB7Cisg ICAgICAJICBwcmludGYoIldhcm5pbmc6IGNvdWxkbid0IG9wZW4gbGlidGhhaS5zbzogJXMsIHRo YWkgd29yZCBicmVhayBmZWF0dXJlIGRpc2FibGVkXG4iLCBkbGVycm9yKCkpOworICAgICAgICAg IC8vIG5vIHdvcmRjdXQgbGlicmFyeSwgYnJlYWsgYW55d2hlcmUgaXMgb2sKKyAgICAgICAgICB0 cnltb3JlID0gMDsKKyAgICAgICAgICByZXR1cm4gMDsKKyAgICAgICAgfSBlbHNlIHsKKyAgICAg ICAgICAvKiBHZXQgc3ltYm9sICovCisgICAgICAgICAgZGxlcnJvcigpOworICAgICAgICAgIHRo X2JyayA9ICh0aF9icmtfZGVmKSBkbHN5bShtb2R1bGUsICJ0aF9icmsiKTsKKwkJICB0aF91bmky dGlzX2xpbmUgPSAodGhfdW5pMnRpc19saW5lX2RlZikgZGxzeW0obW9kdWxlLCAidGhfdW5pMnRp c19saW5lIik7CisKKyAgICAgICAgICBpZiAoZXJyb3IgPSBkbGVycm9yKCkpIHsKKyAgICAgICAg ICAgIHByaW50ZigiV2FybmluZzogY291bGRuJ3QgZmluZCBsaWJ0aGFpOiAlc1xuIixlcnJvcik7 CisgICAgICAgICAgICAvLyBubyB3b3JraW5nIHdvcmRjdXQgbGlicmFyeSwgYnJlYWsgYW55d2hl cmUgaXMgb2sKKyAgICAgICAgICAgIHRyeW1vcmUgPSAwOworICAgICAgICAgICAgcmV0dXJuIDA7 CisgICAgICAgICAgfSBlbHNlIHsKKyAgICAgICAgICAgIHByaW50ZigiTGlidGhhaSBsb2FkZWQg c3VjY2Vzc2Z1bGx5Li5cbiIpOworCSAgCSAgfQorICAgICAgICB9CisgICAgfQorCisgICAgaWYg KCF0aF9icmsgfHwgIXRoX3VuaTJ0aXNfbGluZSkKKyAgICAgICAgcmV0dXJuIDA7CisJaWYgKCFj YWNoZSkKKyAgICAgICAgY2FjaGUgPSBuZXcgVGhhaUNhY2hlOworIAlpZiAoc3RyaW5nICE9IGNh Y2hlLT5zdHJpbmcpIHsKKyAJLy9Db252ZXJ0IGZyb20gVW5pY29kZSB0byBUSVMKKwkgIHdjaGFy X3QgKndfY2hhciA9ICh3Y2hhcl90KikgbWFsbG9jKGxlbipzaXplb2Yod2NoYXJfdCkpOworCSAg Zm9yIChpbnQgaSA9IDA7aSA8IGxlbjsgaSsrKSB7CisJICAgIHdfY2hhcltpXSA9IHN0cmluZ1tp XTsKKwkgIH0KKwkgIHRpc19jaGFyID0gKHVuc2lnbmVkIGNoYXIqKSBtYWxsb2MobGVuKnNpemVv Zih1bnNpZ25lZCBjaGFyKSk7CisJICB0aF91bmkydGlzX2xpbmUod19jaGFyLCB0aXNfY2hhciwg bGVuKTsKKy8vICAgCQlmb3IgKGludCBpID0gMDsgaSA8IGxlbjsgaSsrKQorLy8gICAJCQlwcmlu dGYoInRpc19jaGFyWyVkXSA9ICVkIFxuIiwgaSwgdGlzX2NoYXJbaV0pOworCisgICAgICAgICAg Y2FjaGUtPm51bXdicnBvcyA9IHRoX2JyaygoY29uc3QgdW5zaWduZWQgY2hhciopdGlzX2NoYXIs IGNhY2hlLT53YnJwb3MsIGNhY2hlLT5hbGxvY2F0ZWQpOworICAgICAgICAgLy9mcHJpbnRmKHN0 ZGVyciwibGlidGhhaSByZXR1cm5zIHdpdGggdmFsdWUgJWRcbiIsY2FjaGUtPm51bXdicnBvcyk7 CisKKwkgIGlmIChjYWNoZS0+bnVtd2JycG9zID49IGNhY2hlLT5hbGxvY2F0ZWQpIHsKKwkgICAg Y2FjaGUtPmFsbG9jYXRlZCA9IGxlbjsKKyAgICAgICAgICAgIGNhY2hlLT53YnJwb3MgPSAoaW50 ICopcmVhbGxvYyhjYWNoZS0+d2JycG9zLCBjYWNoZS0+YWxsb2NhdGVkKnNpemVvZihpbnQpKTsK KyAgICAgICAgICAgIGNhY2hlLT5udW13YnJwb3MgPSB0aF9icmsodGlzX2NoYXIsIGNhY2hlLT53 YnJwb3MsIGNhY2hlLT5hbGxvY2F0ZWQpOworICAJICB9CisgIAkgIGlmICggbGVuID4gY2FjaGUt Pm51bWlzYnJlYWthYmxlICkgeworICAJICAgIGNhY2hlLT5udW1pc2JyZWFrYWJsZT1sZW47Cisg ICAgICAgICAgIGNhY2hlLT5pc2JyZWFrYWJsZSA9IChpbnQgKilyZWFsbG9jKGNhY2hlLT5pc2Jy ZWFrYWJsZSwgY2FjaGUtPm51bWlzYnJlYWthYmxlKnNpemVvZihpbnQpKTsKKyAgCSAgfQorICAJ ICBmb3IgKGludCBpID0gMCA7IGkgPCBsZW4gOyArK2kpIHsKKyAgCSAgICBjYWNoZS0+aXNicmVh a2FibGVbaV0gPSAwOworICAJICB9CisgICAgICAJICBpZiAoIGNhY2hlLT5udW13YnJwb3MgPiAw ICkgeworICAgICAgICAgICAgZm9yIChpbnQgaSA9IGNhY2hlLT5udW13YnJwb3MtMTsgaSA+PSAw OyAtLWkpIHsKKyAgICAgICAgICAgICAgY2FjaGUtPmlzYnJlYWthYmxlW2NhY2hlLT53YnJwb3Nb aV1dID0gMTsKKyAgCSAgICB9CisgIAkgIH0KKyAgICAgICAgICBjYWNoZS0+c3RyaW5nID0gc3Ry aW5nOworCSAgZGVsZXRlKHRpc19jaGFyKTsKKwkgIGRlbGV0ZSh3X2NoYXIpOworIAl9CisKKwly ZXR1cm4gY2FjaGUtPmlzYnJlYWthYmxlW3Bvc107Cit9CisjZW5kaWYKIH0gLy8gbmFtZXNwYWNl IFdlYkNvcmUKZGlmZiAtdU5yIHdlYmtpdC0wfnN2bjI1MTQ0Lm9yaWcvV2ViQ29yZS9yZW5kZXJp bmcvYnJlYWtfbGluZXMuaCB3ZWJraXQtMH5zdm4yNTE0NC9XZWJDb3JlL3JlbmRlcmluZy9icmVh a19saW5lcy5oCi0tLSB3ZWJraXQtMH5zdm4yNTE0NC5vcmlnL1dlYkNvcmUvcmVuZGVyaW5nL2Jy ZWFrX2xpbmVzLmgJMjAwNy0wOC0xOSAxMjoxNToxNS4wMDAwMDAwMDAgKzA3MDAKKysrIHdlYmtp dC0wfnN2bjI1MTQ0L1dlYkNvcmUvcmVuZGVyaW5nL2JyZWFrX2xpbmVzLmgJMjAwOC0wMS0yMiAw OTozMTo0MC4wMDAwMDAwMDAgKzA3MDAKQEAgLTI4LDkgKzI4LDE5IEBACiBuYW1lc3BhY2UgV2Vi Q29yZSB7CiAKICAgICBpbnQgbmV4dEJyZWFrYWJsZVBvc2l0aW9uKGNvbnN0IFVDaGFyKiwgaW50 IHBvcywgaW50IGxlbiwgYm9vbCBicmVha05CU1AgPSBmYWxzZSk7CisJI2lmIFBMQVRGT1JNKFVO SVgpCisJYm9vbCBpc0JyZWFrYWJsZVRoYWkoIGNvbnN0IFVDaGFyICpzdHJpbmcsIGNvbnN0IGlu dCBwb3MsIGNvbnN0IGludCBsZW4pOworCXZvaWQgY2xlYW51cF90aGFpYnJlYWtzKCk7CisJI2Vu ZGlmCiAKICAgICBpbmxpbmUgYm9vbCBpc0JyZWFrYWJsZShjb25zdCBVQ2hhciogc3RyLCBpbnQg cG9zLCBpbnQgbGVuLCBpbnQmIG5leHRCcmVha2FibGUsIGJvb2wgYnJlYWtOQlNQID0gZmFsc2Up CiAgICAgeworCSNpZiBQTEFURk9STShVTklYKQorCWNvbnN0IFVDaGFyKiB1Y2ggPSBzdHIgKyBw b3M7CisJaWYgKCh1bnNpZ25lZCBzaG9ydCkqdWNoID49IDB4MGUwMCAmJiAodW5zaWduZWQgc2hv cnQpKnVjaCA8IDB4MGU4MCkgeyAvLyAwZTAwIC0gMGU3ZiA9PSBUaGFpCisJCXJldHVybiBpc0Jy ZWFrYWJsZVRoYWkoc3RyLCBwb3MsIGxlbik7CisJfQorCSNlbmRpZgogICAgICAgICBpZiAocG9z ID4gbmV4dEJyZWFrYWJsZSkKICAgICAgICAgICAgIG5leHRCcmVha2FibGUgPSBuZXh0QnJlYWth YmxlUG9zaXRpb24oc3RyLCBwb3MsIGxlbiwgYnJlYWtOQlNQKTsKICAgICAgICAgcmV0dXJuIHBv cyA9PSBuZXh0QnJlYWthYmxlOwo=