Bug 28131 – [Haiku] Adding font-specific files to WebCore.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Maxime Simon]

I will post two patches in this bug for these font-specific files:
WebCore/platform/graphics/haiku/FontCustomPlatformData.cpp
WebCore/platform/graphics/haiku/FontCustomPlatformData.h
WebCore/platform/graphics/haiku/FontPlatformData.h
WebCore/platform/graphics/haiku/FontPlatformDataHaiku.cpp

WebCore/platform/graphics/haiku/FontCacheHaiku.cpp
WebCore/platform/graphics/haiku/FontDataHaiku.cpp
WebCore/platform/graphics/haiku/FontHaiku.cpp
WebCore/platform/graphics/haiku/SimpleFontHaiku.cpp

Regards,
Maxime

[Maxime Simon]

Created attachment 34421 [details]
Patch to add four font-specific files to WebCore/platform/graphics/haiku/.

[Maxime Simon]

Created attachment 34422 [details]
Patch to add four other font-specific files to WebCore/platform/graphics/haiku/.

[Eric Seidel (no email)]

Comment on attachment 34421 [details]
Patch to add four font-specific files to WebCore/platform/graphics/haiku/.

 39 static void releaseData(void* data)
looks to never be used, please remove it.

strange indenting here:
     struct FontCustomPlatformData : Noncopyable {
 33         public:
 34             FontCustomPlatformData() { }
 35

No need for the "buffer" argument name here:
 40     FontCustomPlatformData* createFontCustomPlatformData(SharedBuffer* buffer);

Wrong:
  * This file is part of the internal font implementation.  It should not be included by anyone other than
 3  * FontMac.cpp, FontWin.cpp and Font.cpp.

So strange:
120     if (m_font && m_font != hashTableDeletedFontValue())
 121         delete m_font;
That's gonna die a horrible death with this as a copy-constructor:
FontPlatformData::FontPlatformData(const FontPlatformData& other)
 114 {
 115     *this = other;
 116 }
your' going to be double-deleting pointers left and right.

Why does FontPlatformData use FontDescription at all?  the Mac version doesn't at least.  Shouldn't this just wrap a BFont?

[Eric Seidel (no email)]

Comment on attachment 34422 [details]
Patch to add four other font-specific files to WebCore/platform/graphics/haiku/.

naming style:
 71     static AtomicString s_DefaultStr(family);

This should indicate that it's a hack somehow:
1 // Fix temp routine to check the conversion works, please move it ASAP
 42 int charUnicodeToUtf8(unsigned short glyph, char* out)
it's not clear to me from reading that method name that this method isn't a good thing. :)  Although since I started at it enought o see what it does, you're basically just flattening all passed in chars to utf8 so that you can pass them off to B drawing routines.  This is a hack, and needs to be designated as such.

Shouldn't char* out be char[4]& out?

Don't we already have typedefs making these casts uneeded?
2     BView* view = static_cast<BView*>(graphicsContext->platformContext());
 73     BFont* m_font = static_cast<BFont*>(font->platformData().font());

Why is the const_cast needed?
 79     GlyphBufferGlyph* glyphs = const_cast<GlyphBufferGlyph*>(glyphBuffer.glyphs(from));

Early return is preferred:
45     BFont* font = m_platformData.font();
 46     if (font) {
if (!font)
    return;

It seems the handling of m_smallCapsFontData is wrong on other ports.  But I'm not convinced your handling is correct either.  It certainly is strange that SimpleFontData.cpp would try to delete it too.

     if (m_platformData.font())
 90         m_treatAsFixedPitch = m_platformData.font()->IsFixed();
 91     else
 92         m_treatAsFixedPitch = false;

m_treadAsFixedPitch = m_platformData.font() && m_platformData.font()->IsFixed();

What is an "escapement"?  Wow a B-only term it seems:
http://www.acm.uiuc.edu/bug/Be%20Book/The%20Interface%20Kit/Font.html

[Maxime Simon]

(In reply to comment #3)
> Why does FontPlatformData use FontDescription at all?  the Mac version doesn't
> at least.  Shouldn't this just wrap a BFont?

At least the gtk port does it.
But with some simple tests it seems that the type of FontDescription is always NoFamily.

[Maxime Simon]

Created attachment 34556 [details]
Patch to add four font-specific files to WebCore/platform/graphics/haiku/.

[Maxime Simon]

(In reply to comment #4)
> It seems the handling of m_smallCapsFontData is wrong on other ports.  But I'm
> not convinced your handling is correct either.  It certainly is strange that
> SimpleFontData.cpp would try to delete it too.

I took a look on other ports. It seems that gtk, win, wince, wx and mac are handling m_smallCapsFontData the same way I do (if I'm right). But in fact it could be wrong.

> What is an "escapement"?  Wow a B-only term it seems:
> http://www.acm.uiuc.edu/bug/Be%20Book/The%20Interface%20Kit/Font.html

Eh Eh, an escapement is the B-term to design the width of a glyph. ;)

[Maxime Simon]

(In reply to comment #4)
> Shouldn't char* out be char[4]& out?

I tried with char[4]& but it didn't compile. I got this output "error: 'out' was not declared in this scope."

> Why is the const_cast needed?
>  79     GlyphBufferGlyph* glyphs =
> const_cast<GlyphBufferGlyph*>(glyphBuffer.glyphs(from));

This const_cast is needed because the glyphBuffer argument of the function is const, and the glyphs we are creating will be modified.

[Maxime Simon]

Created attachment 34561 [details]
Patch to add four other font-specific files to WebCore/platform/graphics/haiku/.

[Eric Seidel (no email)]

Comment on attachment 34556 [details]
Patch to add four font-specific files to WebCore/platform/graphics/haiku/.

I would have put this in a static inline:

+    int32 numStyles = count_font_styles(family);
+    font_style fontStyle;
+    uint32 flags;
+
+    for (int i = 0; i < numStyles; i++) {
+        if (get_font_style(family, i, &fontStyle, &flags) == B_OK) {
+            String style(fontStyle);
+            if ((m_oblique && m_bold) && (style == "BoldItalic" || style == "Bold Oblique"))
+                 break;
+            if ((m_oblique && !m_bold) && (style == "Italic" || style == "Oblique"))
+                 break;
+            if ((!m_oblique && m_bold) && style == "Bold")
+                 break;
+            if ((!m_oblique && !m_bold) && (style == "Roma" || style == "Book"
+                || style == "Condensed" || style == "Regular" || style == "Medium"))
+                 break;
+            }
+        }

Something like findMatchingFontStyle(family, m_bold, m_oblique);


found is not used here:
+    if (BFont().SetFamilyAndStyle(BString(familyName.string()).String(), 0) == B_OK) {
+        strcpy(family, BString(familyName.string()).String());
+        found = true;
+    }
+
+    // If no font family is found for the given name, we use a generic font.
+    if (!found) {
+        int type = fontDescription.genericFamily();
+
+        if (BString(familyName.string()).FindFirst("Sans") != B_ERROR)
+            strcpy(family, "DejaVu Sans");
+        else if (BString(familyName.string()).FindFirst("Serif") != B_ERROR)
+            strcpy(family, "DejaVu Serif");
+        else if (BString(familyName.string()).FindFirst("Mono") != B_ERROR)
+            strcpy(family, "DejaVu Mono");
+        else // This is the fallback font.
+            strcpy(family, "DejaVu Sans");
+    }

Seems that should just be an else.

Again, I would have put the family lookup in a separate static inline.

font_family family = findMatchingFontFamily(familyName);

I think this could use on more round of cleanup.  In general, smaller functions are better. :)

[Eric Seidel (no email)]

Comment on attachment 34561 [details]
Patch to add four other font-specific files to WebCore/platform/graphics/haiku/.

Why execute the family code ever time when you're only creating the AtomicString once?
 69     font_family family;
 70     defaultFont->GetFamilyAndStyle(&family, 0);
 71     static AtomicString defaultString(family);

Seems like a bad idea...
 bool FontData::containsCharacters(const UChar* characters, int length) const
 43 {
 44     notImplemented();
 45     return true;
 46 }

Certainly will make international sites look awful. :)

This is not an english sentence:
 41 // FIXME: Temp routine to check the conversion works.
I'm not quite sure what you mean by that.

Is it possible to pass these through to the simple versions?
 97 float Font::floatWidthForComplexText(const TextRun& run, HashSet<const SimpleFontData*>* fallbackFonts) const

I'm not sure that it would help you any, but it might.

I take it Haiku does not have easy options for drawing complex text?

Not a good variable name:
 76         const FontPlatformData* pdata = new FontPlatformData(desc, desc.family().family());
 74         FontDescription desc = FontDescription(fontDescription);

Otherwise looks fine.

[Maxime Simon]

Created attachment 34894 [details]
Adding four font-specific files to WebCore.

[Maxime Simon]

Created attachment 34895 [details]
Adding three font-specific files to WebCore.

[Oliver Hunt]

Comment on attachment 34895 [details]
Adding three font-specific files to WebCore.

r=me (again)

[Eric Seidel (no email)]

Comment on attachment 34895 [details]
Adding three font-specific files to WebCore.

I can't remember if this was landed or not.  But the commit-queue is smart enough to reject it if it's already been landed.

[Oliver Hunt]

Comment on attachment 34894 [details]
Adding four font-specific files to WebCore.

> +static font_family* findMatchingFontFamily(const AtomicString& familyName)
> +{
> +    font_family family;
> +
> +    if (BFont().SetFamilyAndStyle(BString(familyName.string()).String(), 0)
== B_OK)
> +	   strcpy(family, BString(familyName.string()).String());
Not sure where familyName comes from, but i suspect it's from CSS, and that
means you have a buffer overflow vulnerability here.  Never use strcpy --
always strncpy.  That said, wth is a font_family?  From reading this it looks
like it's just a string?

> +    else {
> +	   // If no font family is found for the given name, we use a generic
font.
> +	   if (BString(familyName.string()).FindFirst("Sans") != B_ERROR)
> +	       strcpy(family, "DejaVu Sans");
> +	   else if (BString(familyName.string()).FindFirst("Serif") != B_ERROR)

> +	       strcpy(family, "DejaVu Serif");
> +	   else if (BString(familyName.string()).FindFirst("Mono") != B_ERROR)
> +	       strcpy(family, "DejaVu Mono");
> +	   else // This is the fallback font.
> +	       strcpy(family, "DejaVu Serif");
This code seems to repeatedly create the same bstring, which seems
unnecessarily expensive.  More over i'm fairly sure AtomicString has all the
logic you need to do this without ever creating a bstring anyway.

> +    }
> +
> +    font_family* fontFamily = &family;
> +    return fontFamily;
You can't return a pointer to a stack allocated variable, it's just not okay --
the code structure implies that the compiler even warned you.

> +}
> +
> +static font_style* findMatchingFontStyle(font_family family, bool bold, bool
oblique)
> +{
> +    font_style style;
...
> +    font_style* fontStyle = &style;
> +    return fontStyle;
Once again, don't return pointers to locals -- you're far better off just using
out parameters

[Eric Seidel (no email)]

Comment on attachment 34895 [details]
Adding three font-specific files to WebCore.

Rejecting patch 34895 from commit-queue.  This patch will require manual commit.

Failed to run "['git', 'svn', 'rebase']"  exit_code: 1  cwd: None

[Eric Seidel (no email)]

Comment on attachment 34895 [details]
Adding three font-specific files to WebCore.

commit-queue error.

[Eric Seidel (no email)]

Comment on attachment 34895 [details]
Adding three font-specific files to WebCore.

Rejecting patch 34895 from commit-queue.  This patch will require manual commit.

['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--quiet', '--exit-after-n-failures=1'] failed with exit code 1

[Maxime Simon]

(In reply to comment #16)
> (From update of attachment 34894 [details])
> Not sure where familyName comes from, but i suspect it's from CSS, and that
> means you have a buffer overflow vulnerability here.  Never use strcpy --
> always strncpy.  That said, wth is a font_family?  From reading this it looks
> like it's just a string?

font_family is in fact a simple array of chars.
definition from the BeBook:
typedef char font_family[B_FONT_FAMILY_LENGTH + 1]
But you're right using strncpy would be more secure than a simple strcpy.

> > +    else {
> > +	   // If no font family is found for the given name, we use a generic
> font.
> > +	   if (BString(familyName.string()).FindFirst("Sans") != B_ERROR)
> > +	       strcpy(family, "DejaVu Sans");
> > +	   else if (BString(familyName.string()).FindFirst("Serif") != B_ERROR)
> 
> > +	       strcpy(family, "DejaVu Serif");
> > +	   else if (BString(familyName.string()).FindFirst("Mono") != B_ERROR)
> > +	       strcpy(family, "DejaVu Mono");
> > +	   else // This is the fallback font.
> > +	       strcpy(family, "DejaVu Serif");
> This code seems to repeatedly create the same bstring, which seems
> unnecessarily expensive.  More over i'm fairly sure AtomicString has all the
> logic you need to do this without ever creating a bstring anyway.

Indeed, I should have investigate to see if AtomicString has some functions to find a motif in a string.

> > +    }
> > +
> > +    font_family* fontFamily = &family;
> > +    return fontFamily;
> You can't return a pointer to a stack allocated variable, it's just not okay --
> the code structure implies that the compiler even warned you.

If I wrote "return &family" then the compiler warned me. But if I made it like this, it didn't complain (maybe an issue in our gcc version). Anyway, I made some tests and it works as it. It may be a stroke of luck and it will not work another time.

> Once again, don't return pointers to locals -- you're far better off just using
> out parameters

In fact, it should be better. :)
Thanks for the advices.

[Oliver Hunt]

> If I wrote "return &family" then the compiler warned me. But if I made it like
> this, it didn't complain (maybe an issue in our gcc version). Anyway, I made
> some tests and it works as it. It may be a stroke of luck and it will not work
> another time.

GCC was warning you precisely because return the address of a local is unsafe -- it may "work" but it is fundamentally unsound.  What your patch does is deliberately circumvent gcc attempting to tell you that your code is broken.

take (for example):

int* foo() {
    int bar = 3;
    return &bar;
}

int main() {
   int* f = foo();
   printf("%d\n", *f);
   printf("%d\n", *f);
   return 0;
}

You will (probably) get two different numbers output, because you have returned the address of a local, eg. a pointer to some arbitrary location on the stack.  The next function you call uses the same section of the stack for whatever it needs to do and the value you thought you were pointing to is now gone.

In general when gcc warns you against do something it's better to not do it than to just try and make the warning go away :D

[Eric Seidel (no email)]

Comment on attachment 34895 [details]
Adding three font-specific files to WebCore.

storage/database-lock-after-reload.html -> failed

I think that test may be flakey.  adding back to cq.

[Eric Seidel (no email)]

Comment on attachment 34895 [details]
Adding three font-specific files to WebCore.

Clearing flags on attachment: 34895

Committed r47637: <http://trac.webkit.org/changeset/47637>

[Eric Seidel (no email)]

All reviewed patches have been landed.  Closing bug.

[Oliver Hunt]

Except the patches have not all been landed -- one was r-'d, and it was not obsoleted  because 'lo the patch you landed, and the r-'d patch were different parts of this bug.

Please do not just automatically close bugs if there are unlanded non-obsolete attachments.

[Eric Seidel (no email)]

(In reply to comment #25)
> Except the patches have not all been landed -- one was r-'d, and it was not
> obsoleted  because 'lo the patch you landed, and the r-'d patch were different
> parts of this bug.
> 
> Please do not just automatically close bugs if there are unlanded non-obsolete
> attachments.

bug 28230 covers some of this.  Historically I made bugzilla-tool more aggressive because people don't always obsolete patches when they post a new one, so it was never closing bugs, even though the bug was "done".

I'm happy to make it less-aggressive about closing.

[Maxime Simon]

Created attachment 38538 [details]
Adding four font-specific files to WebCore.

[Oliver Hunt]

Comment on attachment 38538 [details]
Adding four font-specific files to WebCore.

For the sake of those of us who aren't familiar with haiku, make the font_family argument to findMatchingFontFamily a pointer, and dereference appropriately.  Also replace all uses of strcpy with strncpy (never use strcpy -- most modern analysis tools will flag any use of strcpy as being bad).

I don't know haiku idioms but
m_font->SetFamilyAndStyle(fontFamily, 0);
seems superfluous as it's immediately followed by
m_font->SetFamilyAndStyle(fontFamily, *fontStyle);

Fix those up and it will be good.

[Maxime Simon]

(In reply to comment #28)
> (From update of attachment 38538 [details])
> For the sake of those of us who aren't familiar with haiku, make the
> font_family argument to findMatchingFontFamily a pointer, and dereference
> appropriately. 

I tried, but it "segfault" when making str(n)py(*family…);

> Also replace all uses of strcpy with strncpy (never use strcpy
> -- most modern analysis tools will flag any use of strcpy as being bad).

I used strncpy for the copy which may be "dangerous". Should I do the same even if the source string is known (and will not exceed 64)?

> I don't know haiku idioms but
> m_font->SetFamilyAndStyle(fontFamily, 0);
> seems superfluous as it's immediately followed by
> m_font->SetFamilyAndStyle(fontFamily, *fontStyle);

Strange, I thought I passed m_font to findMatchingFontStyle(), but in fact not. So indeed this should be removed. :)

[Maxime Simon]

(In reply to comment #29)
> (In reply to comment #28)
> > (From update of attachment 38538 [details] [details])
> > For the sake of those of us who aren't familiar with haiku, make the
> > font_family argument to findMatchingFontFamily a pointer, and dereference
> > appropriately. 
> 
> I tried, but it "segfault" when making str(n)py(*family…);

It seems normal… I must initialize the family before using it (Silly me). So I put a "static_cast<font_family*>(malloc(sizeof(font_family)));" when creating the font_family pointer.

[Maxime Simon]

Created attachment 38545 [details]
Adding four font-specific files to WebCore.

[Oliver Hunt]

Comment on attachment 38545 [details]
Adding four font-specific files to WebCore.


> +    font_family* fontFamily = static_cast<font_family*>(malloc(sizeof(font_family)));
> +    findMatchingFontFamily(familyName, fontFamily);

You are leaking the fontFamily allocation, this should be done as
> +    font_family fontFamily;
> +    findMatchingFontFamily(familyName, &fontFamily);

[Maxime Simon]

Created attachment 38638 [details]
Adding four font-specific files to WebCore.

[Eric Seidel (no email)]

Comment on attachment 38638 [details]
Adding four font-specific files to WebCore.

Not needed:
+#include "NotImplemented.h"

I probably wouldn't have bothered to maintain these copyrights after such extensive modification:
2  * Copyright (C) 2006 Apple Computer, Inc.
 3  * Copyright (C) 2006 Zack Rusin <[e-mail]>
 4  * Copyright (C) 2006 Dirk Mueller <[e-mail]>
 5  * Copyright (C) 2006 Nikolas Zimmermann <[e-mail]>
 6  * Copyright (C) 2007 Ryan Leavengood <[e-mail]>
 7  * Copyright (C) 2009 Maxime Simon <[e-mail]>

This:
115     if (m_font)
 116         return m_font->Spacing() == B_FIXED_SPACING;
 117 
 118     return false;
Should just be:

return m_font && m_font->Spacing() == B_FIXED_SPACING;

Why is this needed?
09     if (m_font && m_font != hashTableDeletedFontValue())
 110         m_font = 0;
Seems useless in the destructor.  setting the raw pointer to 0 does nothing. :)

So you're leaking m_font.  I think you meant to make it an OwnPtr<BFont>

This is going to need at least one more round of review since there were still obvious leaks in this one. :(  Do you all have any way to check for leaks on Haiku?

[Ryan Leavengood]

(In reply to comment #34)
> 
> I probably wouldn't have bothered to maintain these copyrights after such
> extensive modification:
> 2  * Copyright (C) 2006 Apple Computer, Inc.
>  3  * Copyright (C) 2006 Zack Rusin <[e-mail]>
>  4  * Copyright (C) 2006 Dirk Mueller <[e-mail]>
>  5  * Copyright (C) 2006 Nikolas Zimmermann <[e-mail]>
>  6  * Copyright (C) 2007 Ryan Leavengood <[e-mail]>
>  7  * Copyright (C) 2009 Maxime Simon <[e-mail]>

I was always pretty worried about this in my original port as copyright stuff can be pretty tricky, especially in the US. But if you think it is OK, I say Maxime can probably remove most of those (maybe even me.)
 
> This is going to need at least one more round of review since there were still
> obvious leaks in this one. :(  Do you all have any way to check for leaks on
> Haiku?

I at least tried rigging up a LeakChecker.h that overrode new and delete for our work on the new browser, but it didn't work that well in my testing. We could definitely work to improve this area though, as memory leaking in browsers has always really, really annoyed me. It wouldn't do to have my browser so leaky!

[Eric Seidel (no email)]

(In reply to comment #35)
> I was always pretty worried about this in my original port as copyright stuff
> can be pretty tricky, especially in the US. But if you think it is OK, I say
> Maxime can probably remove most of those (maybe even me.)

I am not a lawyer, so certainly my comments should not be construed as legal advice.  But looking at this case, there is nothing interesting in the header that you actually copied.  It would be impossible for a reviewer to tell if you copied that or wrote it yourself.

My general rule of thumb is that if I am copying any code of substance I bring along copyrights.  Otherwise new files like this are mostly all typed by hand anyway.

[Maxime Simon]

> I at least tried rigging up a LeakChecker.h that overrode new and delete for
> our work on the new browser, but it didn't work that well in my testing. We
> could definitely work to improve this area though, as memory leaking in
> browsers has always really, really annoyed me. It wouldn't do to have my
> browser so leaky!

I tried using LeakTracker.h with HaikuLauncher, and it doesn't work at all. It didn't even compile. And in fact, memory leaking is really annoying. :-/
( In OSX it's easier as they have a tool named leaks which detect them. )

Actually, I can't delete the m_font as it. (because of the copy-constructor).
Eric, you suggested using OwPtr, why not RefPtr?

[Maxime Simon]

> I tried using LeakTracker.h with HaikuLauncher, and it doesn't work at all. It
> didn't even compile.

Sorry, the fault was mine. I used the wrong #ifdef … #endif.
And even if I didn't delete the font, it didn't detect any leak… I supposed this tool isn't perfect. :)
We would really need a tool like valgrind or leaks in Haiku. Especially for our WebKit port and the browser.

[Maxime Simon]

Created attachment 38793 [details]
Adding three font-specific files to WebCore.

[Maxime Simon]

(In reply to comment #39)
> Created an attachment (id=38793) [details]
> Adding three font-specific files to WebCore.

As the three other files than FontPlatformDataHaiku.cpp are mostly correct I fill a patch for them.

[Eric Seidel (no email)]

Comment on attachment 38793 [details]
Adding three font-specific files to WebCore.

Yay for small patches!

[Eric Seidel (no email)]

Comment on attachment 38793 [details]
Adding three font-specific files to WebCore.

Clearing flags on attachment: 38793

Committed r48006: <http://trac.webkit.org/changeset/48006>

[Eric Seidel (no email)]

All reviewed patches have been landed.  Closing bug.