Bug 157066

Summary: REGRESSION(r200094): [FreeType] Vertical text is broken after r200094
Product: WebKit Reporter: Carlos Garcia Campos <cgarcia>
Component: PlatformAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: bugs-noreply, commit-queue, darin, jeremyhu, mmaxfield, mrobinson
Priority: P2 Keywords: Gtk, LayoutTestFailure, Regression
Version: WebKit Local Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Updated patch
none
Try to fix EFL build mrobinson: review+

Description Carlos Garcia Campos 2016-04-27 02:20:46 PDT
And possibly other attributes. This is causing a lot of failures in GTK+ layout tests. The problem is that Freetype implementation needs to call buildScaledFont() when orientation, SyntheticOblique or size change, but the new static clone methods don't do that.
Comment 1 Carlos Garcia Campos 2016-04-27 02:34:13 PDT
Created attachment 277464 [details]
Patch
Comment 2 WebKit Commit Bot 2016-04-27 02:35:56 PDT
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.
Comment 3 Carlos Garcia Campos 2016-04-27 02:53:01 PDT
Created attachment 277466 [details]
Patch

Just fixed some typos in the ChangeLog
Comment 4 WebKit Commit Bot 2016-04-27 02:55:22 PDT
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.
Comment 5 Carlos Garcia Campos 2016-04-27 05:37:55 PDT
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.
Comment 6 WebKit Commit Bot 2016-04-27 05:40:15 PDT
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.
Comment 7 Carlos Garcia Campos 2016-04-27 05:55:32 PDT
Created attachment 277470 [details]
Try to fix EFL build
Comment 8 WebKit Commit Bot 2016-04-27 05:57:33 PDT
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 9 Martin Robinson 2016-04-27 07:50:14 PDT
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 10 Darin Adler 2016-04-27 08:35:30 PDT
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?
Comment 11 Martin Robinson 2016-04-27 08:38:22 PDT
(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.
Comment 12 Carlos Garcia Campos 2016-04-27 09:19:08 PDT
(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.
Comment 13 Carlos Garcia Campos 2016-04-27 09:20:05 PDT
(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.
Comment 14 Carlos Garcia Campos 2016-04-27 09:47:00 PDT
Committed r200129: <http://trac.webkit.org/changeset/200129>
Comment 15 Jeremy Huddleston Sequoia 2016-04-30 20:45:49 PDT
It looks like this introduced a regression.  See bug #157167.