Valgrind found this: http://code.google.com/p/chromium/issues/detail?id=9475
Created attachment 30285 [details] fix some stuff WebCore/ChangeLog | 11 +++++++++++ .../platform/graphics/chromium/FontCacheLinux.cpp | 5 ++--- 2 files changed, 13 insertions(+), 3 deletions(-)
Please note that I don't really know what I'm doing, so review carefully.
I apologize for not quite getting the review process right, but from some offline discussion it appears Brett is the right reviewer. Brett, agl wrote the code but he'd never seen these getCachedFont* bits. As our font man, what do you think?
Comment on attachment 30285 [details] fix some stuff I believe this is the way it's supposed to work, it's what we do on Windows. > - FontPlatformData* fpd = > - createFontPlatformData(font.fontDescription(), AtomicString((char*) family)); > - ret = new SimpleFontData(*fpd); > + FontPlatformData* fpd = getCachedFontPlatformData(font.fontDescription(), AtomicString((char*) family), false); > + ret = getCachedFontData(fpd); Is it WebKit style to require static_cast? Brett
Comment on attachment 30285 [details] fix some stuff > +2009-05-13 Evan Martin <evan@chromium.org> > + > + Reviewed by Darin Fisher <darin@chromium.org>. > + > + Fix a font-related leak in Chromium's Skia backend found by Valgrind. > + http://code.google.com/p/chromium/issues/detail?id=9475 Please add a reference to the bugs.webkit.org bug instead. In that bug you should/may refer to the chromium bug. yes, static_cast for the win!
static_cast where?
Created attachment 30294 [details] fix WebCore/ChangeLog | 11 +++++++++++ .../platform/graphics/chromium/FontCacheLinux.cpp | 5 ++--- 2 files changed, 13 insertions(+), 3 deletions(-)
Comment on attachment 30294 [details] fix WebKit style guidelines: "Use full words, except in the rare case where an abbreviation would be more canonical and easier to understand." I would re-write this block: + FontPlatformData* fpd = getCachedFontPlatformData(font.fontDescription(), AtomicString(reinterpret_cast<char*>(family)), false); + ret = getCachedFontData(fpd); as: AtomicString fontFamily(reinterpret_cast<char*>(family)); ret = getCachedFontData(getCachedFontPlatformData(font.fontDescription(), fontFamily), false) or as platformData = Otherwise looks fine. I can fix it an land for you.
review collision it would appear. Stupid bugzilla.
Please feel free to munge at will. (In my defense, note that both the C-style cast and the "ret" were there before my change...)
Committing to http://svn.webkit.org/repository/webkit/trunk ... M WebCore/ChangeLog M WebCore/platform/graphics/chromium/FontCacheLinux.cpp Committed r43756