99731 2012-10-18 10:06:49 -0700 Add 8-bit path to RenderBlock::handleTrailingSpaces() 2012-10-18 11:03:25 -0700 1 1 1 Unclassified WebKit Layout and Rendering 528+ (Nightly build) All All RESOLVED FIXED P2 Normal --- 1 msaboff msaboff eric mitz webkit.review.bot oldest_to_newest 745345 0 msaboff 2012-10-18 10:06:49 -0700 RenderBlock::handleTrailingSpaces() uses the characters() method to look for the range of space characters at the end of a text run. This should be updated to use characters8() or characters16() as appropriate. 745352 1 169430 msaboff 2012-10-18 10:13:55 -0700 Created attachment 169430 Patch 745359 2 169430 mitz 2012-10-18 10:20:25 -0700 Comment on attachment 169430 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=169430&action=review > Source/WebCore/rendering/RenderBlockLineLayout.cpp:1030 > + UChar current = characters[firstSpace - 1]; > + if (!isCollapsibleSpace(current, lastText)) Have you tried making current a CharacterType and making isCollapsibleSpace() a function template as well? 745361 3 msaboff 2012-10-18 10:25:15 -0700 (In reply to comment #2) > (From update of attachment 169430 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=169430&action=review > > > Source/WebCore/rendering/RenderBlockLineLayout.cpp:1030 > > + UChar current = characters[firstSpace - 1]; > > + if (!isCollapsibleSpace(current, lastText)) > > Have you tried making current a CharacterType and making isCollapsibleSpace() a function template as well? In most cases I haven't done this. The implicit conversion from an LChar (unsigned char) to a UChar (unsigned short) is typically a nop. If you think it is more readable I can make that change. 745362 4 169430 mitz 2012-10-18 10:27:19 -0700 Comment on attachment 169430 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=169430&action=review >>> Source/WebCore/rendering/RenderBlockLineLayout.cpp:1030 >>> + if (!isCollapsibleSpace(current, lastText)) >> >> Have you tried making current a CharacterType and making isCollapsibleSpace() a function template as well? > > In most cases I haven't done this. The implicit conversion from an LChar (unsigned char) to a UChar (unsigned short) is typically a nop. If you think it is more readable I can make that change. If it doesn’t change the generated code, then no, I don’t think it’s necessary. 745389 5 169430 webkit.review.bot 2012-10-18 11:03:21 -0700 Comment on attachment 169430 Patch Clearing flags on attachment: 169430 Committed r131776: <http://trac.webkit.org/changeset/131776> 745390 6 webkit.review.bot 2012-10-18 11:03:25 -0700 All reviewed patches have been landed. Closing bug. 169430 2012-10-18 10:13:55 -0700 2012-10-18 11:03:20 -0700 Patch 99731.patch text/plain 2718 msaboff SW5kZXg6IFNvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2Vi Q29yZS9DaGFuZ2VMb2cJKHJldmlzaW9uIDEzMTc2NSkKKysrIFNvdXJjZS9XZWJDb3JlL0NoYW5n ZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDE5IEBACisyMDEyLTEwLTE4ICBNaWNoYWVs IFNhYm9mZiAgPG1zYWJvZmZAYXBwbGUuY29tPgorCisgICAgICAgIEFkZCA4LWJpdCBwYXRoIHRv IFJlbmRlckJsb2NrOjpoYW5kbGVUcmFpbGluZ1NwYWNlcygpCisgICAgICAgIGh0dHBzOi8vYnVn cy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD05OTczMQorCisgICAgICAgIFJldmlld2VkIGJ5 IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgIEZhY3RvcmVkIG91dCBhbmQgYWRkZWQgZmluZEZp cnN0VHJhaWxpbmdTcGFjZSgpIHRlbXBsYXRlZCBoZWxwZXIgZnVuY3Rpb24gdGhhdCBpcyBjYWxs ZWQgd2l0aCB0aGUgCisgICAgICAgIGFwcHJvcmlhdGUgY2hhcmFjdGVyIHBvaW50ZXIgdHlwZS4K KworICAgICAgICBObyB0ZXN0cyBuZWVkZWQsIGNoYW5nZSBjb3ZlcmVkIGJ5IGV4aXN0aW5nIHRl c3RzLgorCisgICAgICAgICogcmVuZGVyaW5nL1JlbmRlckJsb2NrTGluZUxheW91dC5jcHA6Cisg ICAgICAgIChXZWJDb3JlOjpmaW5kRmlyc3RUcmFpbGluZ1NwYWNlKToKKyAgICAgICAgKFdlYkNv cmU6OlJlbmRlckJsb2NrOjpoYW5kbGVUcmFpbGluZ1NwYWNlcyk6CisKIDIwMTItMTAtMTggIFRh a2FzaGkgU2FrYW1vdG8gIDx0YXNha0Bnb29nbGUuY29tPgogCiAgICAgICAgIFJFR1JFU1NJT04o cjEzMTQ2NCk6IE51bGwtcG9pbnRlciBjcmFzaCBpbiBTdHlsZVJlc29sdmVyOjpzdHlsZUZvckVs ZW1lbnQKSW5kZXg6IFNvdXJjZS9XZWJDb3JlL3JlbmRlcmluZy9SZW5kZXJCbG9ja0xpbmVMYXlv dXQuY3BwCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT0KLS0tIFNvdXJjZS9XZWJDb3JlL3JlbmRlcmluZy9SZW5kZXJCbG9j a0xpbmVMYXlvdXQuY3BwCShyZXZpc2lvbiAxMzE2NTYpCisrKyBTb3VyY2UvV2ViQ29yZS9yZW5k ZXJpbmcvUmVuZGVyQmxvY2tMaW5lTGF5b3V0LmNwcAkod29ya2luZyBjb3B5KQpAQCAtMTAyMSw2 ICsxMDIxLDIwIEBAIHN0YXRpYyB2b2lkIHNldFN0YXRpY1Bvc2l0aW9ucyhSZW5kZXJCbG8KICAg ICBjaGlsZC0+bGF5ZXIoKS0+c2V0U3RhdGljQmxvY2tQb3NpdGlvbihibG9ja0hlaWdodCk7CiB9 CiAKK3RlbXBsYXRlIDx0eXBlbmFtZSBDaGFyYWN0ZXJUeXBlPgorc3RhdGljIGlubGluZSBpbnQg ZmluZEZpcnN0VHJhaWxpbmdTcGFjZShSZW5kZXJUZXh0KiBsYXN0VGV4dCwgY29uc3QgQ2hhcmFj dGVyVHlwZSogY2hhcmFjdGVycywgaW50IHN0YXJ0LCBpbnQgc3RvcCkKK3sKKyAgICBpbnQgZmly c3RTcGFjZSA9IHN0b3A7CisgICAgd2hpbGUgKGZpcnN0U3BhY2UgPiBzdGFydCkgeworICAgICAg ICBVQ2hhciBjdXJyZW50ID0gY2hhcmFjdGVyc1tmaXJzdFNwYWNlIC0gMV07CisgICAgICAgIGlm ICghaXNDb2xsYXBzaWJsZVNwYWNlKGN1cnJlbnQsIGxhc3RUZXh0KSkKKyAgICAgICAgICAgIGJy ZWFrOworICAgICAgICBmaXJzdFNwYWNlLS07CisgICAgfQorCisgICAgcmV0dXJuIGZpcnN0U3Bh Y2U7Cit9CisKIGlubGluZSBCaWRpUnVuKiBSZW5kZXJCbG9jazo6aGFuZGxlVHJhaWxpbmdTcGFj ZXMoQmlkaVJ1bkxpc3Q8QmlkaVJ1bj4mIGJpZGlSdW5zLCBCaWRpQ29udGV4dCogY3VycmVudENv bnRleHQpCiB7CiAgICAgaWYgKCFiaWRpUnVucy5ydW5Db3VudCgpCkBAIC0xMDM0LDE0ICsxMDQ4 LDEyIEBAIGlubGluZSBCaWRpUnVuKiBSZW5kZXJCbG9jazo6aGFuZGxlVHJhaWwKICAgICAgICAg cmV0dXJuIDA7CiAKICAgICBSZW5kZXJUZXh0KiBsYXN0VGV4dCA9IHRvUmVuZGVyVGV4dChsYXN0 T2JqZWN0KTsKLSAgICBjb25zdCBVQ2hhciogY2hhcmFjdGVycyA9IGxhc3RUZXh0LT5jaGFyYWN0 ZXJzKCk7Ci0gICAgaW50IGZpcnN0U3BhY2UgPSB0cmFpbGluZ1NwYWNlUnVuLT5zdG9wKCk7Ci0g ICAgd2hpbGUgKGZpcnN0U3BhY2UgPiB0cmFpbGluZ1NwYWNlUnVuLT5zdGFydCgpKSB7Ci0gICAg ICAgIFVDaGFyIGN1cnJlbnQgPSBjaGFyYWN0ZXJzW2ZpcnN0U3BhY2UgLSAxXTsKLSAgICAgICAg aWYgKCFpc0NvbGxhcHNpYmxlU3BhY2UoY3VycmVudCwgbGFzdFRleHQpKQotICAgICAgICAgICAg YnJlYWs7Ci0gICAgICAgIGZpcnN0U3BhY2UtLTsKLSAgICB9CisgICAgaW50IGZpcnN0U3BhY2U7 CisgICAgaWYgKGxhc3RUZXh0LT5pczhCaXQoKSkKKyAgICAgICAgZmlyc3RTcGFjZSA9IGZpbmRG aXJzdFRyYWlsaW5nU3BhY2UobGFzdFRleHQsIGxhc3RUZXh0LT5jaGFyYWN0ZXJzOCgpLCB0cmFp bGluZ1NwYWNlUnVuLT5zdGFydCgpLCB0cmFpbGluZ1NwYWNlUnVuLT5zdG9wKCkpOworICAgIGVs c2UKKyAgICAgICAgZmlyc3RTcGFjZSA9IGZpbmRGaXJzdFRyYWlsaW5nU3BhY2UobGFzdFRleHQs IGxhc3RUZXh0LT5jaGFyYWN0ZXJzMTYoKSwgdHJhaWxpbmdTcGFjZVJ1bi0+c3RhcnQoKSwgdHJh aWxpbmdTcGFjZVJ1bi0+c3RvcCgpKTsKKwogICAgIGlmIChmaXJzdFNwYWNlID09IHRyYWlsaW5n U3BhY2VSdW4tPnN0b3AoKSkKICAgICAgICAgcmV0dXJuIDA7CiAK