Bug 157066

Created attachment 277464 [details] Patch

Attachment 277464 [details] did not pass style-queue: ERROR: Source/WebCore/platform/graphics/cairo/CairoUtilities.h:40: FT_Face is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] Total errors found: 1 in 11 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 277466 [details] Patch Just fixed some typos in the ChangeLog

Attachment 277466 [details] did not pass style-queue: ERROR: Source/WebCore/platform/graphics/cairo/CairoUtilities.h:40: FT_Face is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] Total errors found: 1 in 11 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 277467 [details] Updated patch I noticed that some tests (the ones using fallbacks) were crashing with this patch. The thing is that I wrongly assumed that the default copy constructor used the assign operator, but since that's not the case we need to keep our own implementation of the copy constructor. The good thing is that now we can use a unique_ptr (FcUniquePtr) for the fallbacks, which simplifies the code a bit more.

Attachment 277467 [details] did not pass style-queue: ERROR: Source/WebCore/platform/graphics/cairo/CairoUtilities.h:40: FT_Face is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] Total errors found: 1 in 13 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 277470 [details] Try to fix EFL build

Attachment 277470 [details] did not pass style-queue: ERROR: Source/WebCore/platform/graphics/cairo/CairoUtilities.h:40: FT_Face is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] Total errors found: 1 in 14 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 277470 [details] Try to fix EFL build View in context: https://bugs.webkit.org/attachment.cgi?id=277470&action=review > Source/WebCore/platform/graphics/cairo/CairoUtilities.h:64 > +#if USE(FREETYPE) > +class CairoFtFaceLocker { > +public: > + CairoFtFaceLocker(cairo_scaled_font_t*); > + ~CairoFtFaceLocker(); > + > + FT_Face ftFace() const { return m_ftFace; } > + > +private: > + cairo_scaled_font_t* m_scaledFont { nullptr }; > + FT_Face m_ftFace { nullptr }; > +}; One note is that by not putting the implementation of the constructor and destructor here, it is impossible for the compiler to inline them. It might be better to avoid changing that. > Source/WebCore/platform/graphics/freetype/FontPlatformDataFreeType.cpp:253 > + // We need to reinitialize the instance, because the difference in size > + // necessitates a new scaled font instance. > + ASSERT(copy.m_scaledFont.get()); > + copy.buildScaledFont(cairo_scaled_font_get_font_face(copy.m_scaledFont.get())); Why don't you also check if the size is different here similar to the methods above? > Source/WebCore/platform/graphics/freetype/FontPlatformDataFreeType.cpp:291 > - return m_scaledFont == other.m_scaledFont > - && m_size == other.m_size > - && m_syntheticOblique == other.m_syntheticOblique > - && m_orientation == other.m_orientation > - && m_syntheticBold == other.m_syntheticBold; > + return m_scaledFont == other.m_scaledFont; > } Yay! > Source/WebCore/platform/graphics/freetype/FontPlatformDataFreeType.cpp:349 > + ASSERT(m_scaledFont.get()); I'm not sure you need .get() here. > Source/WebCore/platform/graphics/freetype/FontPlatformDataFreeType.cpp:-412 > - cairo_ft_scaled_font_unlock_face(m_scaledFont); Wow. Really nice fix here.

Comment on attachment 277470 [details] Try to fix EFL build View in context: https://bugs.webkit.org/attachment.cgi?id=277470&action=review >> Source/WebCore/platform/graphics/cairo/CairoUtilities.h:64 >> +}; > > One note is that by not putting the implementation of the constructor and destructor here, it is impossible for the compiler to inline them. It might be better to avoid changing that. I don’t think that’s right. Why wouldn’t the compiler inline them?

(In reply to comment #10) > Comment on attachment 277470 [details] > Try to fix EFL build > > View in context: > https://bugs.webkit.org/attachment.cgi?id=277470&action=review > > >> Source/WebCore/platform/graphics/cairo/CairoUtilities.h:64 > >> +}; > > > > One note is that by not putting the implementation of the constructor and destructor here, it is impossible for the compiler to inline them. It might be better to avoid changing that. > > I don’t think that’s right. Why wouldn’t the compiler inline them? I thought that the compiler was unable to inline code across compilation units, but I could very well be wrong about that.

(In reply to comment #9) > Comment on attachment 277470 [details] > Try to fix EFL build > > View in context: > https://bugs.webkit.org/attachment.cgi?id=277470&action=review Thanks for the review! > > Source/WebCore/platform/graphics/cairo/CairoUtilities.h:64 > > +#if USE(FREETYPE) > > +class CairoFtFaceLocker { > > +public: > > + CairoFtFaceLocker(cairo_scaled_font_t*); > > + ~CairoFtFaceLocker(); > > + > > + FT_Face ftFace() const { return m_ftFace; } > > + > > +private: > > + cairo_scaled_font_t* m_scaledFont { nullptr }; > > + FT_Face m_ftFace { nullptr }; > > +}; > > One note is that by not putting the implementation of the constructor and > destructor here, it is impossible for the compiler to inline them. It might > be better to avoid changing that. It was my initial idea to do the implementation in the header, but I wanted to avoid including cairo-ft header in the header file. I didn't know this prevented the compiler from inline the implementation. > > Source/WebCore/platform/graphics/freetype/FontPlatformDataFreeType.cpp:253 > > + // We need to reinitialize the instance, because the difference in size > > + // necessitates a new scaled font instance. > > + ASSERT(copy.m_scaledFont.get()); > > + copy.buildScaledFont(cairo_scaled_font_get_font_face(copy.m_scaledFont.get())); > > Why don't you also check if the size is different here similar to the > methods above? I followed the previous code, tried to cleanup everything but keeping the same behavior to avoid introducing new regressions. > > Source/WebCore/platform/graphics/freetype/FontPlatformDataFreeType.cpp:291 > > - return m_scaledFont == other.m_scaledFont > > - && m_size == other.m_size > > - && m_syntheticOblique == other.m_syntheticOblique > > - && m_orientation == other.m_orientation > > - && m_syntheticBold == other.m_syntheticBold; > > + return m_scaledFont == other.m_scaledFont; > > } > > Yay! > > > Source/WebCore/platform/graphics/freetype/FontPlatformDataFreeType.cpp:349 > > + ASSERT(m_scaledFont.get()); > > I'm not sure you need .get() here. I don't, it doesn't hurt either, though. > > Source/WebCore/platform/graphics/freetype/FontPlatformDataFreeType.cpp:-412 > > - cairo_ft_scaled_font_unlock_face(m_scaledFont); > > Wow. Really nice fix here.

(In reply to comment #11) > (In reply to comment #10) > > Comment on attachment 277470 [details] > > Try to fix EFL build > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=277470&action=review > > > > >> Source/WebCore/platform/graphics/cairo/CairoUtilities.h:64 > > >> +}; > > > > > > One note is that by not putting the implementation of the constructor and destructor here, it is impossible for the compiler to inline them. It might be better to avoid changing that. > > > > I don’t think that’s right. Why wouldn’t the compiler inline them? > > I thought that the compiler was unable to inline code across compilation > units, but I could very well be wrong about that. I can move the implementation to the header in any case to be extra sure.

Committed r200129: <http://trac.webkit.org/changeset/200129>

It looks like this introduced a regression. See bug #157167.