98876 2012-10-10 01:02:29 -0700 [Qt] REGRESSION (r130851): fast/text/word-space-with-kerning.html fails 2012-10-23 05:12:14 -0700 1 1 1 Unclassified WebKit Tools / Tests 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 79666 98317 1 zarvai allan.jensen allan.jensen enrica hausmann mitz noam ossy pierre.rossi webkit.review.bot oldest_to_newest 738702 0 zarvai 2012-10-10 01:02:29 -0700 fast/text/word-space-with-kerning.html looks differently from reference html after r130851: http://build.webkit.org/results/Qt%20Linux%20Release/r130865%20%2853124%29/results.html 738716 1 zarvai 2012-10-10 01:13:02 -0700 An other bot that stores results for longer time: http://build.webkit.sed.hu/results/x86-64%20Linux%20Qt%20Release/r130851%20%2843632%29/results.html 738738 2 zarvai 2012-10-10 01:36:15 -0700 Skipped on Qt in https://trac.webkit.org/changeset/130873. Please unskip it with the proper fix. 748417 3 allan.jensen 2012-10-23 02:39:18 -0700 It seems the problem is that our version of Font::floatWidthForComplexText, when used on a string such as " b ", applies word-spacing both before and after the word, while the layout assumes we only apply word-spacing once. 748441 4 170104 allan.jensen 2012-10-23 03:04:27 -0700 Created attachment 170104 Patch 748447 5 allan.jensen 2012-10-23 03:11:38 -0700 Btw, quick note. The problem is only related to kerning because kerning enables complex font-path. The problem happens in many more similar cases if we disable simple font-path completely and always choose complex. This means more tests are likely to have been influence by this bug if they caused complex font-path due to some other mechanism. 748483 6 170104 hausmann 2012-10-23 04:21:09 -0700 Comment on attachment 170104 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=170104&action=review > Source/WebCore/platform/graphics/qt/FontQt.cpp:216 > + // RenderBlockLineLayout expects us to only add word-spacing for trailing spaces, not for leading spaces. > + if (treatAsSpace(run[0])) > + return width + run.expansion() - m_wordSpacing; Where do you find that logic in the simple text code path? 748505 7 allan.jensen 2012-10-23 04:52:56 -0700 (In reply to comment #6) > (From update of attachment 170104 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=170104&action=review > > > Source/WebCore/platform/graphics/qt/FontQt.cpp:216 > > + // RenderBlockLineLayout expects us to only add word-spacing for trailing spaces, not for leading spaces. > > + if (treatAsSpace(run[0])) > > + return width + run.expansion() - m_wordSpacing; > > Where do you find that logic in the simple text code path? I didn't find it, I just instrumented the code to call both and dumped the results. "A" was 17px wide by both " A" was 83px wide by complex, and 23px by simple " A " was 149px wide by complex, and 89px by simple. 748508 8 170104 hausmann 2012-10-23 04:59:01 -0700 Comment on attachment 170104 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=170104&action=review >>> Source/WebCore/platform/graphics/qt/FontQt.cpp:216 >>> + return width + run.expansion() - m_wordSpacing; >> >> Where do you find that logic in the simple text code path? > > I didn't find it, I just instrumented the code to call both and dumped the results. > > "A" was 17px wide by both > " A" was 83px wide by complex, and 23px by simple > " A " was 149px wide by complex, and 89px by simple. Indeed, that logic existed before r113968 and in that revision was moved into FontQt4.cpp, but it's missing here. I personally would prefer if it was written like before also: if (treatAsSpace(run[0])) width -= m_wordSpacing and then share the return: return width + run.expansion(); But the change seems correct. 748521 9 allan.jensen 2012-10-23 05:12:14 -0700 Committed r132205: <http://trac.webkit.org/changeset/132205> 170104 2012-10-23 03:04:27 -0700 2012-10-23 04:59:00 -0700 Patch bug-98876-20121023120244.patch text/plain 3489 allan.jensen U3VidmVyc2lvbiBSZXZpc2lvbjogMTMyMDg2CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggNzZhYThjNDEwM2JkZDc5 MDkwZjA4NzM4NDgxMjU3Mjg0NDU4ZDA5Mi4uZmUzNDU2YTdmNTUwMDMyNjQ1ZTZlYjYxODVkYjAw NmQyYWQzMzkxMCAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDE3IEBACisyMDEyLTEwLTIzICBBbGxh biBTYW5kZmVsZCBKZW5zZW4gIDxhbGxhbi5qZW5zZW5AZGlnaWEuY29tPgorCisgICAgICAgIFtR dF0gUkVHUkVTU0lPTiAocjEzMDg1MSk6IGZhc3QvdGV4dC93b3JkLXNwYWNlLXdpdGgta2Vybmlu Zy5odG1sIGZhaWxzCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNn aT9pZD05ODg3NgorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAg ICAgIERvIG5vdCBhZGQgd29yZC1zcGFjaW5nIGZvciBsZWFkaW5nIHNwYWNlLiBUaGlzIG1hdGNo ZXMgd2hhdCBzaW1wbGUgcGF0aCBmb250LXdpZHRoIGRvZXMuCisKKyAgICAgICAgVGVzdGVkIGJ5 IGV4aXN0aW5nIHRlc3RzLgorCisgICAgICAgICogcGxhdGZvcm0vZ3JhcGhpY3MvcXQvRm9udFF0 LmNwcDoKKyAgICAgICAgKFdlYkNvcmU6OkZvbnQ6OmZsb2F0V2lkdGhGb3JDb21wbGV4VGV4dCk6 CisKIDIwMTItMTAtMjIgIFZzZXZvbG9kIFZsYXNvdiAgPHZzZXZpa0BjaHJvbWl1bS5vcmc+CiAK ICAgICAgICAgV2ViIEluc3BlY3RvcjogUHJlcGFyZSBtYXBwaW5ncyB0byBtb3ZpbmcgdWlTb3Vy Y2VDb2RlcyBjcmVhdGlvbiBvdXQgb2YgdGhlbSB0byB3b3Jrc3BhY2UuCmRpZmYgLS1naXQgYS9T b3VyY2UvV2ViQ29yZS9wbGF0Zm9ybS9ncmFwaGljcy9xdC9Gb250UXQuY3BwIGIvU291cmNlL1dl YkNvcmUvcGxhdGZvcm0vZ3JhcGhpY3MvcXQvRm9udFF0LmNwcAppbmRleCBmNWJhY2RkY2MzNWU2 ZTU5ODAzMmY0M2I2NjQwZGE0ODU4ZDM5Y2ZlLi41YjE4ZmJhYmY4N2RlOWIxN2MzMjdlMzQ0YzFh YzU5MTc0MjJjZGM1IDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViQ29yZS9wbGF0Zm9ybS9ncmFwaGlj cy9xdC9Gb250UXQuY3BwCisrKyBiL1NvdXJjZS9XZWJDb3JlL3BsYXRmb3JtL2dyYXBoaWNzL3F0 L0ZvbnRRdC5jcHAKQEAgLTIxMSw2ICsyMTEsOSBAQCBmbG9hdCBGb250OjpmbG9hdFdpZHRoRm9y Q29tcGxleFRleHQoY29uc3QgVGV4dFJ1biYgcnVuLCBIYXNoU2V0PGNvbnN0IFNpbXBsZUZvbgog ICAgIGZsb2F0IHgxID0gbGluZS5jdXJzb3JUb1goMCk7CiAgICAgZmxvYXQgeDIgPSBsaW5lLmN1 cnNvclRvWChydW4ubGVuZ3RoKCkpOwogICAgIGNvbnN0IGZsb2F0IHdpZHRoID0gcUFicyh4MiAt IHgxKTsKKyAgICAvLyBSZW5kZXJCbG9ja0xpbmVMYXlvdXQgZXhwZWN0cyB1cyB0byBvbmx5IGFk ZCB3b3JkLXNwYWNpbmcgZm9yIHRyYWlsaW5nIHNwYWNlcywgbm90IGZvciBsZWFkaW5nIHNwYWNl cy4KKyAgICBpZiAodHJlYXRBc1NwYWNlKHJ1blswXSkpCisgICAgICAgIHJldHVybiB3aWR0aCAr IHJ1bi5leHBhbnNpb24oKSAtIG1fd29yZFNwYWNpbmc7CiAKICAgICByZXR1cm4gd2lkdGggKyBy dW4uZXhwYW5zaW9uKCk7CiB9CmRpZmYgLS1naXQgYS9MYXlvdXRUZXN0cy9DaGFuZ2VMb2cgYi9M YXlvdXRUZXN0cy9DaGFuZ2VMb2cKaW5kZXggM2EyMzg2MTJmMTk4MmIwZTI0NmEzNjg2ZDE2NjVm NjAwM2U5ODhiOS4uNDk2ODVmNGM5ZDYwNGQ0OTk5OWFmMjEzMWI3NzM0ZmNlODAxZjJhOCAxMDA2 NDQKLS0tIGEvTGF5b3V0VGVzdHMvQ2hhbmdlTG9nCisrKyBiL0xheW91dFRlc3RzL0NoYW5nZUxv ZwpAQCAtMSwzICsxLDE0IEBACisyMDEyLTEwLTIzICBBbGxhbiBTYW5kZmVsZCBKZW5zZW4gIDxh bGxhbi5qZW5zZW5AZGlnaWEuY29tPgorCisgICAgICAgIFtRdF0gUkVHUkVTU0lPTiAocjEzMDg1 MSk6IGZhc3QvdGV4dC93b3JkLXNwYWNlLXdpdGgta2VybmluZy5odG1sIGZhaWxzCisgICAgICAg IGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD05ODg3NgorCisgICAgICAg IFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgIFVuc2tpcCBwYXNzaW5nIHdv cmQtd2lkdGggdGVzdHMuCisKKyAgICAgICAgKiBwbGF0Zm9ybS9xdC9UZXN0RXhwZWN0YXRpb25z OgorCiAyMDEyLTEwLTIyICBWc2V2b2xvZCBWbGFzb3YgIDx2c2V2aWtAY2hyb21pdW0ub3JnPgog CiAgICAgICAgIFdlYiBJbnNwZWN0b3I6IFByZXBhcmUgbWFwcGluZ3MgdG8gbW92aW5nIHVpU291 cmNlQ29kZXMgY3JlYXRpb24gb3V0IG9mIHRoZW0gdG8gd29ya3NwYWNlLgpkaWZmIC0tZ2l0IGEv TGF5b3V0VGVzdHMvcGxhdGZvcm0vcXQvVGVzdEV4cGVjdGF0aW9ucyBiL0xheW91dFRlc3RzL3Bs YXRmb3JtL3F0L1Rlc3RFeHBlY3RhdGlvbnMKaW5kZXggNjczMzVkOGQzNzI5ZGQ5ZjYyNDYyMzY4 OTA4ODUwMzBhOGQ2MmVkZi4uY2NiOTA4Mjk5MTJjNGEyNzM0NDg0YzA0MjhmZjc5ZWFiZDg4NTE4 MyAxMDA2NDQKLS0tIGEvTGF5b3V0VGVzdHMvcGxhdGZvcm0vcXQvVGVzdEV4cGVjdGF0aW9ucwor KysgYi9MYXlvdXRUZXN0cy9wbGF0Zm9ybS9xdC9UZXN0RXhwZWN0YXRpb25zCkBAIC0xNzQwLDcg KzE3NDAsNiBAQCBmYXN0L3RleHQvd2hpdGVzcGFjZS8wMTYuaHRtbAogZmFzdC90ZXh0L3doaXRl c3BhY2UvMDI0Lmh0bWwKIGZhc3QvdGV4dC93aWRlLXplcm8td2lkdGgtc3BhY2UuaHRtbAogZmFz dC90ZXh0L3dvcmQtYnJlYWstc29mdC1oeXBoZW4uaHRtbAotZmFzdC90ZXh0L3dvcmQtc3BhY2Uu aHRtbAogCiAjIC0tLS0tLS0gZmFpbHVyZXMgZHVlIHRvIG1pc3Npbmcgc3VwcG9ydCBmb3IgcGFy dGljdWxhciBYU0xUIGZlYXR1cmVzCiAjIHhzbDpvdXRwdXQKQEAgLTE4NjAsMTAgKzE4NTksNiBA QCBmYXN0L3dyaXRpbmctbW9kZS92ZXJ0aWNhbC1zdWJzdC1mb250LXZlcnQtbm8tZGZsdC5odG1s CiAjIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD05ODc1MQogZmFzdC94 bWxodHRwcmVxdWVzdC94bWxodHRwcmVxdWVzdC1ub25leGlzdGVudC1maWxlLmh0bWwKIAotIyBb UXRdIFJFR1JFU1NJT04gKHIxMzA4NTEpOiBmYXN0L3RleHQvd29yZC1zcGFjZS13aXRoLWtlcm5p bmcuaHRtbCBmYWlscwotIyBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9 OTg4NzYKLWZhc3QvdGV4dC93b3JkLXNwYWNlLXdpdGgta2VybmluZy5odG1sCi0KICMgPT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT0gIwogIyBmYWlsaW5nIHNlY3VyaXR5IHRlc3RzCiAjID09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09ICMK