Bug 62530 – [Chromium] Remove dependencies on harfbuzz from FontPlatformDataLinux and FontLinux

• Component: Text
• Status: RESOLVED FIXED

[Kenichi Ishibashi]

The current FontPlatformDataLinux and FontLinux classes depends on the harfbuzz APIs.  It would be better to remove these dependencies since we are going to switch harfbuzz to harfbuzz-ng.

[Kenichi Ishibashi]

Created attachment 96925 [details]
Patch V0

[Tony Chang]

Evan would be an appropriate reviewer for this, but he is on vacation this week.

[Kenichi Ishibashi]

Ping?

[Hajime Morrita]

Comment on attachment 96925 [details]
Patch V0

View in context: https://bugs.webkit.org/attachment.cgi?id=96925&action=review

I'm not an expert in this area. So let me try a review just for style nits.

> Source/WebCore/platform/graphics/chromium/ComplexTextControllerLinux.h:-105
> -    const unsigned width() const { return m_pixelWidth; }

How about just to move this to private?
It makes change smaller - and having an accessor is not bad in any time.

> Source/WebCore/platform/graphics/chromium/ComplexTextControllerLinux.h:113
> +    int offsetForPosition(int);

const?

> Source/WebCore/platform/graphics/chromium/ComplexTextControllerLinux.h:114
> +    FloatRect selectionRect(const FloatPoint&, int, int, int);

const?

> Source/WebCore/platform/graphics/chromium/ComplexTextControllerLinux.h:130
> +    int glyphIndexForXPositionInScriptRun(int targetX);

const?

> Source/WebCore/platform/graphics/chromium/FontLinux.cpp:247
>                                            bool includePartialGlyphs) const

Ditto to selectionRectForComplexText.

> Source/WebCore/platform/graphics/chromium/FontLinux.cpp:273
>      controller.setWordSpacingAdjustment(wordSpacing());

If you can move these setter invocations into another consructor, you can call ComplexTextcontroller(....).selectionRect(...); 
What do you think?

> Source/WebCore/platform/graphics/chromium/FontLinux.cpp:278
>          controller.reset(controller.widthOfFullRun());

How about to extract this if (run.rtl()) block into some function like setRTL() ?

> Source/WebCore/platform/graphics/chromium/FontPlatformDataLinux.cpp:-34
> -#include "HarfbuzzSkia.h"

You can move this to .cpp if you move constructors and the destructor.

> Source/WebCore/platform/graphics/chromium/FontPlatformDataLinux.h:36
> +#include "HarfbuzzSkia.h"

Isn't a forward declaration enough?

> Source/WebCore/platform/graphics/chromium/HarfbuzzSkia.h:61
> +HB_Font_* allocHarfbuzzFont();

I know you don't write this.
But it looks horrible idea to have asymmetric alloc/free function.
A possible idea is to subclass HB_ShaperItem as a, namely, HarfBuzzShaperItem. Then you can define its constructor and destructor.

[Kenichi Ishibashi]

Created attachment 98696 [details]
Patch V1

[Kenichi Ishibashi]

View in context: https://bugs.webkit.org/attachment.cgi?id=96925&action=review

Hi Morita-san,

Thank you for review! I've addressed your comments. As mentioned below, I didn't address removing asymmetric alloc/free problem because I'll remove the function in the future.

Regards,

>> Source/WebCore/platform/graphics/chromium/ComplexTextControllerLinux.h:-105
>> -    const unsigned width() const { return m_pixelWidth; }
> 
> How about just to move this to private?
> It makes change smaller - and having an accessor is not bad in any time.

Done.

>> Source/WebCore/platform/graphics/chromium/ComplexTextControllerLinux.h:113
>> +    int offsetForPosition(int);
> 
> const?

Done.

>> Source/WebCore/platform/graphics/chromium/ComplexTextControllerLinux.h:114
>> +    FloatRect selectionRect(const FloatPoint&, int, int, int);
> 
> const?

Done.

>> Source/WebCore/platform/graphics/chromium/ComplexTextControllerLinux.h:130
>> +    int glyphIndexForXPositionInScriptRun(int targetX);
> 
> const?

Done.

>> Source/WebCore/platform/graphics/chromium/FontLinux.cpp:247
>>                                            bool includePartialGlyphs) const
> 
> Ditto to selectionRectForComplexText.

Ditto to selectionRectForComplexText.

>> Source/WebCore/platform/graphics/chromium/FontLinux.cpp:273
>>      controller.setWordSpacingAdjustment(wordSpacing());
> 
> If you can move these setter invocations into another consructor, you can call ComplexTextcontroller(....).selectionRect(...); 
> What do you think?

These setters are always called after construction, so I've moved these invocations into the constructor.  However, I didn't change the code like ComplexTextController(...).selectionRect(...) because we need to call setupRTL() if the run is rtl and I don't want to call it if it's not necessary.

>> Source/WebCore/platform/graphics/chromium/FontLinux.cpp:278
>>          controller.reset(controller.widthOfFullRun());
> 
> How about to extract this if (run.rtl()) block into some function like setRTL() ?

Added it as setupRTL().

>> Source/WebCore/platform/graphics/chromium/FontPlatformDataLinux.cpp:-34
>> -#include "HarfbuzzSkia.h"
> 
> You can move this to .cpp if you move constructors and the destructor.

I couldn't do this because it is needed in .h.

>> Source/WebCore/platform/graphics/chromium/FontPlatformDataLinux.h:36
>> +#include "HarfbuzzSkia.h"
> 
> Isn't a forward declaration enough?

I've tried it, but RefPtr requires the definition of the type.

>> Source/WebCore/platform/graphics/chromium/HarfbuzzSkia.h:61
>> +HB_Font_* allocHarfbuzzFont();
> 
> I know you don't write this.
> But it looks horrible idea to have asymmetric alloc/free function.
> A possible idea is to subclass HB_ShaperItem as a, namely, HarfBuzzShaperItem. Then you can define its constructor and destructor.

I definitely agree with you, but once we move to use new harfbuzz APIs, this won't be needed anymore and I'll remove this function so I'd like to keep it for now.

[Tony Chang]

Comment on attachment 98696 [details]
Patch V1

View in context: https://bugs.webkit.org/attachment.cgi?id=98696&action=review

In general, looks fine.  Just some style nits.

> Source/WebCore/platform/graphics/chromium/ComplexTextControllerLinux.h:114
> +    const int offsetForPosition(int);
> +    const FloatRect selectionRect(const FloatPoint&, int, int, int);

I think morrita meant to put const at the end of the function (e.g., int offsetForPosition(int) const).

Can you name the parameters in selectionRect()?  It's confusing when there are multiple ints passed in.

> Source/WebCore/platform/graphics/chromium/ComplexTextControllerLinux.h:118
> +    const unsigned width() const { return m_pixelWidth; }

Remove the first const.

> Source/WebCore/platform/graphics/chromium/ComplexTextControllerLinux.h:133
> +    const int glyphIndexForXPositionInScriptRun(int targetX);

const at the end.

> Source/WebCore/platform/graphics/chromium/HarfbuzzSkia.h:56
> +    HarfbuzzFace(FontPlatformData*);

explicit

[Kenichi Ishibashi]

Created attachment 98817 [details]
Patch V2

[Kenichi Ishibashi]

Created attachment 98819 [details]
Patch V3

[Kenichi Ishibashi]

Comment on attachment 98696 [details]
Patch V1

View in context: https://bugs.webkit.org/attachment.cgi?id=98696&action=review

Hi Tony,

Thank you for review!

>> Source/WebCore/platform/graphics/chromium/ComplexTextControllerLinux.h:114
>> +    const FloatRect selectionRect(const FloatPoint&, int, int, int);
> 
> I think morrita meant to put const at the end of the function (e.g., int offsetForPosition(int) const).
> 
> Can you name the parameters in selectionRect()?  It's confusing when there are multiple ints passed in.

Named the parameters in selectionRect().  I wanted to put const these functions, but they call nextScriptRun(), which certainly changes the state of the ComplexTextController so I couldn't make them const.

>> Source/WebCore/platform/graphics/chromium/ComplexTextControllerLinux.h:118
>> +    const unsigned width() const { return m_pixelWidth; }
> 
> Remove the first const.

Done.

>> Source/WebCore/platform/graphics/chromium/ComplexTextControllerLinux.h:133
>> +    const int glyphIndexForXPositionInScriptRun(int targetX);
> 
> const at the end.

Done. (Patch V2 didn't address this)

>> Source/WebCore/platform/graphics/chromium/HarfbuzzSkia.h:56
>> +    HarfbuzzFace(FontPlatformData*);
> 
> explicit

Done.

[Tony Chang]

Comment on attachment 98819 [details]
Patch V3

View in context: https://bugs.webkit.org/attachment.cgi?id=98819&action=review

> Source/WebCore/platform/graphics/chromium/ComplexTextControllerLinux.h:92
> +    // setupForRTL sets up the controller to handle RTL text.

Nit: I would remove this comment because it doesn't tell us anything useful.

> Source/WebCore/platform/graphics/chromium/ComplexTextControllerLinux.h:114
> +    const int offsetForPosition(int);
> +    const FloatRect selectionRect(const FloatPoint&, int height, int from , int to);

I would remove the const in front.  They don't really do anything.

> Source/WebCore/platform/graphics/chromium/ComplexTextControllerLinux.h:133
> +    const int glyphIndexForXPositionInScriptRun(int targetX) const;

This one too.

[Kenichi Ishibashi]

Committed r89988: <http://trac.webkit.org/changeset/89988>

[Kenichi Ishibashi]

Hi Tony,

Thank you for r+. I've addressed your comments and added initialization of m_padError field (I forgot to move it from constructor to setPadding()).

Regards,

(In reply to comment #11)
> (From update of attachment 98819 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=98819&action=review
> 
> > Source/WebCore/platform/graphics/chromium/ComplexTextControllerLinux.h:92
> > +    // setupForRTL sets up the controller to handle RTL text.
> 
> Nit: I would remove this comment because it doesn't tell us anything useful.
> 
> > Source/WebCore/platform/graphics/chromium/ComplexTextControllerLinux.h:114
> > +    const int offsetForPosition(int);
> > +    const FloatRect selectionRect(const FloatPoint&, int height, int from , int to);
> 
> I would remove the const in front.  They don't really do anything.
> 
> > Source/WebCore/platform/graphics/chromium/ComplexTextControllerLinux.h:133
> > +    const int glyphIndexForXPositionInScriptRun(int targetX) const;
> 
> This one too.