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();
Created attachment 117908 [details] patch
Comment on attachment 117908 [details] patch Is this code testable in any way?
Comment on attachment 117908 [details] 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"?
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.
Comment on attachment 117908 [details] 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.
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?
Ok. Looks like we just need to fix the typo in the comment and we're ready to land.
Created attachment 118231 [details] 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.
Verified that the fallback behavior is also covered by at least fast/css/font-family-pictograph.html.
Thanks.
Comment on attachment 118231 [details] updated patch Clearing flags on attachment: 118231 Committed r102255: <http://trac.webkit.org/changeset/102255>
All reviewed patches have been landed. Closing bug.