73849 2011-12-05 11:29:47 -0800 In FontCacheAndroid.cpp should keep the pointer valid returned from CString::data() 2011-12-07 11:22:07 -0800 1 1 1 Unclassified WebKit Platform 528+ (Nightly build) Android Unspecified RESOLVED FIXED P2 Normal --- 73672 1 wangxianzhu wangxianzhu abarth cc-bugs jamesr peter webkit.review.bot oldest_to_newest 515243 0 wangxianzhu 2011-12-05 11:29:47 -0800 In FontCacheAndroid.cpp, name will be invalid after the following piece of code if name is got from String::utf8().data(), because the temporary CString returned from String::utf8() has been destructed after that statement. FontPlatformData* FontCache::createFontPlatformData(const FontDescription& fontDescription, const AtomicString& family) { const char* name = 0; // If a fallback font is being created (e.g. "-webkit-monospace"), convert // it in to the fallback name (e.g. "monospace"). if (!family.length() || family.startsWith("-webkit-")) name = getFallbackFontName(fontDescription); else name = family.string().utf8().data(); 515245 1 117908 wangxianzhu 2011-12-05 11:35:49 -0800 Created attachment 117908 patch 515274 2 117908 jamesr 2011-12-05 12:18:09 -0800 Comment on attachment 117908 patch Is this code testable in any way? 515340 3 117908 abarth 2011-12-05 13:17:51 -0800 Comment on attachment 117908 patch View in context: https://bugs.webkit.org/attachment.cgi?id=117908&action=review I agree with James that this should be testable. Is this covered by an existing LayoutTests on Android? > Source/WebCore/platform/graphics/chromium/FontCacheAndroid.cpp:150 > + CString nameString; // Keeps name valid within scope of this function in case that name is got from family. I'm not sure "got" is grammatically correct in this comment. Maybe remplace "got from family" with "from a family"? 515836 4 peter 2011-12-06 05:39:50 -0800 Thank you! This change lgtm and actually is how it's being used downstream. The nameString variable got lost in a clean-up, for which I'm to blame. In order to properly run tests we need to be able to link and run, but as of this morning there still are ~30 unique linking errors present. Downstream the code is being tested using regular layout tests. 515985 5 117908 abarth 2011-12-06 09:55:50 -0800 Comment on attachment 117908 patch I'm not as concerned about whether the tests are currently being run upstream but rather whether we will have test coverage for this change when we do start running tests. If I understood you correctly, you're saying that we will. 516582 6 peter 2011-12-07 03:45:42 -0800 Indeed, the font cache will be exercised by most layout tests as it's used in the CSS Font Selector. Xianzhu Wang, do we have any layout tests specifically testing the fallback behavior? 516731 7 abarth 2011-12-07 10:37:37 -0800 Ok. Looks like we just need to fix the typo in the comment and we're ready to land. 516744 8 118231 wangxianzhu 2011-12-07 10:51:40 -0800 Created attachment 118231 updated patch View in context: https://bugs.webkit.org/attachment.cgi?id=117908&action=review >> Source/WebCore/platform/graphics/chromium/FontCacheAndroid.cpp:150 >> + CString nameString; // Keeps name valid within scope of this function in case that name is got from family. > > I'm not sure "got" is grammatically correct in this comment. Maybe remplace "got from family" with "from a family"? Done. (In reply to comment #6) > Indeed, the font cache will be exercised by most layout tests as it's used in the CSS Font Selector. Xianzhu Wang, do we have any layout tests specifically testing the fallback behavior? I've that the changed code has already been covered by existing layout tests. I'm running layout tests to see if the fallback behavior is covered. If not, will file another bug for adding a layout test for it. 516759 9 wangxianzhu 2011-12-07 11:06:28 -0800 Verified that the fallback behavior is also covered by at least fast/css/font-family-pictograph.html. 516762 10 abarth 2011-12-07 11:09:08 -0800 Thanks. 516771 11 118231 webkit.review.bot 2011-12-07 11:22:02 -0800 Comment on attachment 118231 updated patch Clearing flags on attachment: 118231 Committed r102255: <http://trac.webkit.org/changeset/102255> 516772 12 webkit.review.bot 2011-12-07 11:22:07 -0800 All reviewed patches have been landed. Closing bug. 117908 2011-12-05 11:35:49 -0800 2011-12-07 10:51:40 -0800 patch 73849 text/plain 1840 wangxianzhu SW5kZXg6IFNvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2Vi Q29yZS9DaGFuZ2VMb2cJKHJldmlzaW9uIDEwMjAyMikKKysrIFNvdXJjZS9XZWJDb3JlL0NoYW5n ZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDEzIEBACisyMDExLTEyLTA1ICBYaWFuemh1 IFdhbmcgIDx3YW5neGlhbnpodUBjaHJvbWl1bS5vcmc+CisKKyAgICAgICAgSW4gRm9udENhY2hl QW5kcm9pZC5jcHAgc2hvdWxkIGtlZXAgdGhlIHBvaW50ZXIgdmFsaWQgcmV0dXJuZWQgZnJvbSBD U3RyaW5nOjpkYXRhKCkKKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcu Y2dpP2lkPTczODQ5CisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAg ICAgICAgKiBwbGF0Zm9ybS9ncmFwaGljcy9jaHJvbWl1bS9Gb250Q2FjaGVBbmRyb2lkLmNwcDoK KyAgICAgICAgKFdlYkNvcmU6OkZvbnRDYWNoZTo6Y3JlYXRlRm9udFBsYXRmb3JtRGF0YSk6CisK IDIwMTEtMTItMDUgIFBldGVyIEJldmVybG9vICA8cGV0ZXJAY2hyb21pdW0ub3JnPgogCiAgICAg ICAgIFtDaHJvbWl1bV0gQWRkIEFuZHJvaWQga2V5Y29kZXMgYW5kIGJ1aWxkIExpbnV4IGNsaXBi b2FyZC9maWxlc3lzdGVtIGZpbGVzLgpJbmRleDogU291cmNlL1dlYkNvcmUvcGxhdGZvcm0vZ3Jh cGhpY3MvY2hyb21pdW0vRm9udENhY2hlQW5kcm9pZC5jcHAKPT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gU291cmNl L1dlYkNvcmUvcGxhdGZvcm0vZ3JhcGhpY3MvY2hyb21pdW0vRm9udENhY2hlQW5kcm9pZC5jcHAJ KHJldmlzaW9uIDEwMjAyMSkKKysrIFNvdXJjZS9XZWJDb3JlL3BsYXRmb3JtL2dyYXBoaWNzL2No cm9taXVtL0ZvbnRDYWNoZUFuZHJvaWQuY3BwCSh3b3JraW5nIGNvcHkpCkBAIC0xNDcsMTMgKzE0 NywxNiBAQCB2b2lkIEZvbnRDYWNoZTo6Z2V0VHJhaXRzSW5GYW1pbHkoY29uc3QgCiBGb250UGxh dGZvcm1EYXRhKiBGb250Q2FjaGU6OmNyZWF0ZUZvbnRQbGF0Zm9ybURhdGEoY29uc3QgRm9udERl c2NyaXB0aW9uJiBmb250RGVzY3JpcHRpb24sIGNvbnN0IEF0b21pY1N0cmluZyYgZmFtaWx5KQog ewogICAgIGNvbnN0IGNoYXIqIG5hbWUgPSAwOworICAgIENTdHJpbmcgbmFtZVN0cmluZzsgLy8g S2VlcHMgbmFtZSB2YWxpZCB3aXRoaW4gc2NvcGUgb2YgdGhpcyBmdW5jdGlvbiBpbiBjYXNlIHRo YXQgbmFtZSBpcyBnb3QgZnJvbSBmYW1pbHkuCiAKICAgICAvLyBJZiBhIGZhbGxiYWNrIGZvbnQg aXMgYmVpbmcgY3JlYXRlZCAoZS5nLiAiLXdlYmtpdC1tb25vc3BhY2UiKSwgY29udmVydAogICAg IC8vIGl0IGluIHRvIHRoZSBmYWxsYmFjayBuYW1lIChlLmcuICJtb25vc3BhY2UiKS4KICAgICBp ZiAoIWZhbWlseS5sZW5ndGgoKSB8fCBmYW1pbHkuc3RhcnRzV2l0aCgiLXdlYmtpdC0iKSkKICAg ICAgICAgbmFtZSA9IGdldEZhbGxiYWNrRm9udE5hbWUoZm9udERlc2NyaXB0aW9uKTsKLSAgICBl bHNlCi0gICAgICAgIG5hbWUgPSBmYW1pbHkuc3RyaW5nKCkudXRmOCgpLmRhdGEoKTsKKyAgICBl bHNlIHsKKyAgICAgICAgbmFtZVN0cmluZyA9IGZhbWlseS5zdHJpbmcoKS51dGY4KCk7CisgICAg ICAgIG5hbWUgPSBuYW1lU3RyaW5nLmRhdGEoKTsKKyAgICB9CiAKICAgICBpbnQgc3R5bGUgPSBT a1R5cGVmYWNlOjprTm9ybWFsOwogICAgIGlmIChmb250RGVzY3JpcHRpb24ud2VpZ2h0KCkgPj0g Rm9udFdlaWdodEJvbGQpCg== 118231 2011-12-07 10:51:40 -0800 2011-12-07 11:22:02 -0800 updated patch 73849a text/plain 1910 wangxianzhu SW5kZXg6IFNvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2Vi Q29yZS9DaGFuZ2VMb2cJKHJldmlzaW9uIDEwMjAyMikKKysrIFNvdXJjZS9XZWJDb3JlL0NoYW5n ZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDE1IEBACisyMDExLTEyLTA1ICBYaWFuemh1 IFdhbmcgIDx3YW5neGlhbnpodUBjaHJvbWl1bS5vcmc+CisKKyAgICAgICAgSW4gRm9udENhY2hl QW5kcm9pZC5jcHAgc2hvdWxkIGtlZXAgdGhlIHBvaW50ZXIgdmFsaWQgcmV0dXJuZWQgZnJvbSBD U3RyaW5nOjpkYXRhKCkKKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcu Y2dpP2lkPTczODQ5CisKKyAgICAgICAgVGhlIGNoYW5nZWQgY29kZSBoYXMgYmVlbiBjb3ZlcmVk IGJ5IG1hbnkgZXhpc3RpbmcgbGF5b3V0IHRlc3RzLgorCisgICAgICAgIFJldmlld2VkIGJ5IEFk YW0gQmFydGguCisKKyAgICAgICAgKiBwbGF0Zm9ybS9ncmFwaGljcy9jaHJvbWl1bS9Gb250Q2Fj aGVBbmRyb2lkLmNwcDoKKyAgICAgICAgKFdlYkNvcmU6OkZvbnRDYWNoZTo6Y3JlYXRlRm9udFBs YXRmb3JtRGF0YSk6CisKIDIwMTEtMTItMDUgIFBldGVyIEJldmVybG9vICA8cGV0ZXJAY2hyb21p dW0ub3JnPgogCiAgICAgICAgIFtDaHJvbWl1bV0gQWRkIEFuZHJvaWQga2V5Y29kZXMgYW5kIGJ1 aWxkIExpbnV4IGNsaXBib2FyZC9maWxlc3lzdGVtIGZpbGVzLgpJbmRleDogU291cmNlL1dlYkNv cmUvcGxhdGZvcm0vZ3JhcGhpY3MvY2hyb21pdW0vRm9udENhY2hlQW5kcm9pZC5jcHAKPT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PQotLS0gU291cmNlL1dlYkNvcmUvcGxhdGZvcm0vZ3JhcGhpY3MvY2hyb21pdW0vRm9udENh Y2hlQW5kcm9pZC5jcHAJKHJldmlzaW9uIDEwMjAyMSkKKysrIFNvdXJjZS9XZWJDb3JlL3BsYXRm b3JtL2dyYXBoaWNzL2Nocm9taXVtL0ZvbnRDYWNoZUFuZHJvaWQuY3BwCSh3b3JraW5nIGNvcHkp CkBAIC0xNDcsMTMgKzE0NywxNiBAQCB2b2lkIEZvbnRDYWNoZTo6Z2V0VHJhaXRzSW5GYW1pbHko Y29uc3QgCiBGb250UGxhdGZvcm1EYXRhKiBGb250Q2FjaGU6OmNyZWF0ZUZvbnRQbGF0Zm9ybURh dGEoY29uc3QgRm9udERlc2NyaXB0aW9uJiBmb250RGVzY3JpcHRpb24sIGNvbnN0IEF0b21pY1N0 cmluZyYgZmFtaWx5KQogewogICAgIGNvbnN0IGNoYXIqIG5hbWUgPSAwOworICAgIENTdHJpbmcg bmFtZVN0cmluZzsgLy8gS2VlcHMgbmFtZSB2YWxpZCB3aXRoaW4gc2NvcGUgb2YgdGhpcyBmdW5j dGlvbiBpbiBjYXNlIHRoYXQgbmFtZSBpcyBmcm9tIGEgZmFtaWx5LgogCiAgICAgLy8gSWYgYSBm YWxsYmFjayBmb250IGlzIGJlaW5nIGNyZWF0ZWQgKGUuZy4gIi13ZWJraXQtbW9ub3NwYWNlIiks IGNvbnZlcnQKICAgICAvLyBpdCBpbiB0byB0aGUgZmFsbGJhY2sgbmFtZSAoZS5nLiAibW9ub3Nw YWNlIikuCiAgICAgaWYgKCFmYW1pbHkubGVuZ3RoKCkgfHwgZmFtaWx5LnN0YXJ0c1dpdGgoIi13 ZWJraXQtIikpCiAgICAgICAgIG5hbWUgPSBnZXRGYWxsYmFja0ZvbnROYW1lKGZvbnREZXNjcmlw dGlvbik7Ci0gICAgZWxzZQotICAgICAgICBuYW1lID0gZmFtaWx5LnN0cmluZygpLnV0ZjgoKS5k YXRhKCk7CisgICAgZWxzZSB7CisgICAgICAgIG5hbWVTdHJpbmcgPSBmYW1pbHkuc3RyaW5nKCku dXRmOCgpOworICAgICAgICBuYW1lID0gbmFtZVN0cmluZy5kYXRhKCk7CisgICAgfQogCiAgICAg aW50IHN0eWxlID0gU2tUeXBlZmFjZTo6a05vcm1hbDsKICAgICBpZiAoZm9udERlc2NyaXB0aW9u LndlaWdodCgpID49IEZvbnRXZWlnaHRCb2xkKQo=