Bug 37508 – [Haiku] Rework FontPlatformData implementation

• Status: RESOLVED
• Resolution: WONTFIX
• Component: Platform

[Stephan Aßmus]

I've reimplemented FontPlatformData for Haiku. It appears the .cpp file was never landed by the previous porters. I have a faint memory that there was already a bug for this, but cannot find it. The new implementation follows closely the Qt version. A lot of tracing was done to make sure the FontCache works as expected on Haiku.

[Stephan Aßmus]

Created attachment 53260 [details]
[Haiku] Reimplementation of FontPlatformData.

Note that no build system files need to be adjusted despite the added .cpp, since Haiku's build system for WebKit is currently not commited into trunk at all.

[Stephan Aßmus]

Created attachment 53283 [details]
[Haiku] Reimplementation of FontPlatformData.

Sorry, I have forgotten to add the bug number to the ChangeLog yet again. No other changes to the patch.

[David Levin]

Comment on attachment 53283 [details]
[Haiku] Reimplementation of FontPlatformData.

Only two minor things to fix up.

> Index: WebCore/platform/graphics/haiku/FontPlatformDataHaiku.cpp
> +static inline bool isEmtpyValue(const float size, const bool bold, const bool oblique)

typo: isEmtpyValue

> +class FontPlatformData::FontPlatformDataPrivate {
> +    FontPlatformDataPrivate(const BFont& font);

No need to include the parameter name "font" since it adds no new information.

[Maxime Simon]

Created attachment 54037 [details]
[Haiku] Reimplementation of FontPlatformData.

Fix the two minor issues so that we can move forward with this tiny patch.

[Adam Barth]

Comment on attachment 54037 [details]
[Haiku] Reimplementation of FontPlatformData.

WebCore/ChangeLog:1
 +  2010-04-13  Stephan AÃmus  <[e-mail]>
Is this character in your name correct or do we have some sort of encoding problem?

WebCore/platform/graphics/haiku/FontPlatformDataHaiku.cpp:44
 +          if (familyName.contains("Sans", false) != B_ERROR)
This seems really hacky.  What if there's a non-san-serif font that happens to contain the string "sans"?

WebCore/platform/graphics/haiku/FontPlatformDataHaiku.cpp:81
 +  // #pragma mark -
What does this comment mean?

WebCore/platform/graphics/haiku/FontPlatformDataHaiku.cpp:91
 +      void ref();
Why doesn't this inherit from RefCounted?

WebCore/platform/graphics/haiku/FontPlatformDataHaiku.cpp:107
 +      : m_refCount(1)
Usually we have a static FontPlatformData::create method to help people not leak because of initializing the refcount to 1.

WebCore/platform/graphics/haiku/FontPlatformDataHaiku.cpp:152
 +  // #pragma mark -
This again.

WebCore/platform/graphics/haiku/FontPlatformDataHaiku.cpp:202
 +          m_data->ref();
Please don't use manual reference counting like this.  We have lots of tools to avoid these error-prone patterns.

WebCore/platform/graphics/haiku/FontPlatformDataHaiku.cpp:217
 +           m_data->deref();
Bad indent.

WebCore/platform/graphics/haiku/FontPlatformDataHaiku.cpp:231
 +          || isHashTableDeletedValue() || other.isHashTableDeletedValue()) {
Why is this clause split in a strange place?

WebCore/platform/graphics/haiku/FontPlatformDataHaiku.cpp:243
 +      return &(m_data->m_font);
No parens needed.

WebCore/platform/graphics/haiku/FontPlatformDataHaiku.cpp:290
 +      return m_data == reinterpret_cast<FontPlatformDataPrivate*>(-1);
Yuck

WebCore/platform/graphics/haiku/FontPlatformDataHaiku.cpp:309
 +      return String(fontFamily) + "/" + String(fontStyle) + String::format("/%.1f/%d&%d", size, isBold, isOblique);
Why do you use concatenation for part of this string but String::format for the rest?  Please pick one or the other.

[Stephan Aßmus]

Thanks a lot for the review.

* The patch should be UTF-8 encoded. Once my patches land, the name is displayed correctly (IIRC). I could write "Assmus" which is almost equivalent in German, but I thought I could use the correct spelling "Aßmus".

* The "// #pragma mark -" is just a directive for the editor which most Haiku devs are using, it causes a separator in the symbol popup menu improving clarity. No problem to remove it if this is inappropriate.

* Please advice what is appropriate for the hashTableDeletedValue, I just took this solution from the Qt port. The manual reference counting too, btw. ;-)

Will try to address the rest of your review points ASAP. Thanks again!

[Ahmad Saleem]

Haiku port doesn't exist and if there is any unofficial - these patch might not be as useful as they were in the past and might require total effort from scratch.

Marking this bug as 'RESOLVED WONTFIX' from usability perspective and if someone can salvage for their 'Haiku' port, they are happy to do so.