Bug 26949 – WebCore part of the Haiku WebKit port

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

[Maxime Simon]

As the previous bug contains too much patches,
( https://bugs.webkit.org/show_bug.cgi?id=26620 )
I fill a new bug with only patches related to WebCore.
Patches about JavaScriptCore and other modifications have already been r+.
I will fill another bug for the WebCoreSupport ( in WebKit ),
and maybe for WebCore/platform/haiku/

[Maxime Simon]

Created attachment 32219 [details]
Patch to add Haiku-specific files for WebCore/platform/image-decoders/.

[Maxime Simon]

Created attachment 32220 [details]
Patch to add Haiku-specific files for WebCore/platform/text/.

[Maxime Simon]

Created attachment 32221 [details]
Patch to add Haiku-specific files for WebCore/bindings/js/.

[Maxime Simon]

Created attachment 32222 [details]
Patch to add Haiku-specific files for WebCore/editing/.

[Maxime Simon]

Created attachment 32223 [details]
Patch to add Haiku-specific files for WebCore/page/.

[Maxime Simon]

Created attachment 32227 [details]
Patch to add Haiku-specific font files for WebCore/platform/graphics/.

[Maxime Simon]

Created attachment 32228 [details]
Patch to add Haiku-specific drawing files for WebCore/platform/graphics/.

[Eric Seidel (no email)]

Comment on attachment 32222 [details]
Patch to add Haiku-specific files for WebCore/editing/.

Looks OK.

[Eric Seidel (no email)]

Comment on attachment 32221 [details]
Patch to add Haiku-specific files for WebCore/bindings/js/.

OK.  Seems kinda silly that this code can't be shared between more ports?

[Eric Seidel (no email)]

Comment on attachment 32223 [details]
Patch to add Haiku-specific files for WebCore/page/.

Sad that more of this code can't be shared between ports.

[Eric Seidel (no email)]

Comment on attachment 32220 [details]
Patch to add Haiku-specific files for WebCore/platform/text/.

Does this copy the data?
string.SetTo(utf8().data());

If it doesn't then your WebKit will crash all the time as StringImpls get released before the corresponding BStrings do...

r+ assuming this won't crash. :)

[Eric Seidel (no email)]

Comment on attachment 32219 [details]
Patch to add Haiku-specific files for WebCore/platform/image-decoders/.

This cast seems unecessary:
+    const void* bytes = (const void*)m_bytes.data();

STyle:
+int RGBA32Buffer::width() const {
+    return m_size.width();
+}
+
+int RGBA32Buffer::height() const {
+    return m_size.height();
+}

[Eric Seidel (no email)]

Comment on attachment 32227 [details]
Patch to add Haiku-specific font files for WebCore/platform/graphics/.

Why the prefix "pc"?
65     const BFont* pcDefaultFont = be_plain_font;

Are you sure you meant to levae this in?
 72     printf("FontCache::getLastResortFallbackFont -> %s\n", family);

Spacing:
 74     BView* view = static_cast<BView*> (graphicsContext->platformContext());

Style:
 50         FontPlatformData(const FontPlatformData& f);
 48         FontPlatformData(const FontDescription&, BFont* font);

count_font_families() sounds expensive. :)
 71     for(int i = 0; i < count_font_families(); i++) {

style:
 73         if(!BString(familyName.string()).Compare(family)){

MOre style:
 90             if ( get_font_style(family, i, &style, &flags) == B_OK) {
 91                 if((bItalic && bBold) &&  (!strcmp( style, "BoldItalic")
 92                         || !strcmp(style, "Bold Oblique"))) {


You could also convert "style" to be a String and then style == "Bold Oblique" would read a bit better.

Style:
53         } else {
 54             character = characterBuffer[i];
 55         }

Doesn't ICU, and other parts of WebCore already have code for dealign with this:
         if(isUtf16) {
 50             UChar lead = characterBuffer[i * 2];
 51             UChar trail = characterBuffer[i * 2 + 1];
 52             character = U16_GET_SUPPLEMENTARY(lead, trail);
 53         } else {
 54             character = characterBuffer[i];
 55         } 

Style:
 46     BFont *font = m_platformData.font();

[Eric Seidel (no email)]

Comment on attachment 32228 [details]
Patch to add Haiku-specific drawing files for WebCore/platform/graphics/.

Too large for me to review tonight.

[Maxime Simon]

(In reply to comment #11)
> (From update of attachment 32220 [details])
> Does this copy the data?
> string.SetTo(utf8().data());
> 
> If it doesn't then your WebKit will crash all the time as StringImpls get
> released before the corresponding BStrings do...
> 
> r+ assuming this won't crash. :)

SetTo() copy the data, so it may not crash.

[Maxime Simon]

Created attachment 32364 [details]
Patch to add Haiku-specific files for WebCore/platform/image-decoders/.

[Maxime Simon]

(In reply to comment #13)
> (From update of attachment 32227 [details])
> Why the prefix "pc"?
> 65     const BFont* pcDefaultFont = be_plain_font;

Good question. :)

> count_font_families() sounds expensive. :)
>  71     for(int i = 0; i < count_font_families(); i++) {

count_font_families() sounds indeed a bit expensive,
but it's a C function from the Haiku API.

> Doesn't ICU, and other parts of WebCore already have code for dealign with
> this:
>          if(isUtf16) {
>  50             UChar lead = characterBuffer[i * 2];
>  51             UChar trail = characterBuffer[i * 2 + 1];
>  52             character = U16_GET_SUPPLEMENTARY(lead, trail);
>  53         } else {
>  54             character = characterBuffer[i];
>  55         } 

I know that the wx port handles this as it,
and the gtk port doesn't support Unicode supplementary characters...

[Maxime Simon]

Created attachment 32371 [details]
Patch to add Haiku-specific font files for WebCore/platform/graphics/.

[Eric Seidel (no email)]

Comment on attachment 32228 [details]
Patch to add Haiku-specific drawing files for WebCore/platform/graphics/.

Who owns the BView in the GraphicsContextPlatformPrivate?  I'm surprised that GCPlatformPrivate doesnt' need to retain it?

style:
npoints should be numberOfPoints or pointsLength or similar.

Style:
8     if (rects.size() > 1)
 229     {

spacing:
 235         m_data->view->StrokeRect(region.Frame(),B_MIXED_COLORS);

Naming:
 257 FloatRect GraphicsContext::roundToDevicePixels(const FloatRect& frect)

Naming:
 287 void GraphicsContext::strokeRect(const FloatRect& r, float width)
 298 void GraphicsContext::setLineCap(LineCap lc)

No need to wrap lines like this:
void GraphicsContext::addInnerRoundedRectClip(const IntRect& rect,
 451                                               int thickness)
 452 

I'm surprised your compiler can't convert from "return 0":
    return PassRefPtr<Icon>(0);

Why are you adding the empy ImageBufferData class?

I think you mean something like "if the image hasn't fully loaded":
 130     if (!image) // If it's too early we won't have an image yet.

But I'm still confused as to why we'd be calling pattern drawing routines with an image w/o a first frame.

I think there are functions to do this for you:
 140     ctxt->clip(IntRect(dstRect.x(), dstRect.y(), dstRect.width(), dstRect.height()));

IntRect(dstRect) probably does what you want.  Although you might want enclosingIntRect() there.

c++ style casts please:
 57     const unsigned char* uContents = (const unsigned char*) data.data();


I'm surprised:
 50 ImageDecoder* createDecoder(const Vector<char>& data)

isn't shared between teh ports?

Spacing:
 45     return BRect(BPoint(x(), y()), BSize(width(),height()));

It's better to break patches down by size instead of directory.

r- for all the nits above.

[Oliver Hunt]

Comment on attachment 32371 [details]
Patch to add Haiku-specific font files for WebCore/platform/graphics/.


> +// Fix temp routine to check the conversion works, please move it ASAP	
> +int charUnicodeToUtf8(unsigned short glyph, char* out)
> +{
> +    int i = 0;
> +
> +    if (glyph < 0x0080)
> +        out[i++] = (char)glyph;
C++ style casts please :D

> +    else if (glyph < 0x0800) { //two bytes
> +        out[i++] = 0xc0 | (( glyph ) >> 6 );
> +        out[i++] = 0x80 | (( glyph ) & 0x3F );
> +    } else if (glyph > 0x0800) { //3 bytes
> +        out[i++] = 0xe0 | (( glyph ) >> 12 );
> +        out[i++] = 0x80 | ( (( glyph ) >> 6 ) & 0x3F );
> +        out[i++] = 0x80 | (( glyph ) & 0x3F);
> +    }
Should have excess braces, eg.
out[i++] = 0x80 | ( (( glyph ) >> 6 ) & 0x3F ); => out[i++] = 0x80 | ((glyph >> 6 ) & 0x3F );
Shouldn't have spaces around the content of parenthesis, eg.
out[i++] = 0x80 | ( (( glyph ) >> 6 ) & 0x3F ); => out[i++] = 0x80 | ((glyph >> 6) & 0x3F);


> +FontPlatformData::FontPlatformData(const FontDescription& fontDescription, const AtomicString& familyName)
> +    : m_font(0)
> +{
> +    bool bFound = false;
> +    bool bItalic = fontDescription.italic();
> +    bool bBold = fontDescription.weight() == FontWeightBold;
> +    font_family family;
> +
> +    for (int i = 0; i < count_font_families(); i++) {
> +        get_font_family(i, &family, 0);
> +        if (!BString(familyName.string()).Compare(family)) {
> +            m_font = new BFont();
> +            m_font->SetFamilyAndStyle(family, 0);
> +            m_font->SetSize(fontDescription.computedSize());
> +
> +            bFound = true;
> +            break;
> +        }
> +    }
> +
> +    if (bFound) {
> +        int32 numStyles = count_font_styles(family);
> +        font_style fontStyle;
> +        uint32 flags;
> +        bFound = false;
> +
> +        for (int i = 0; i < numStyles; i++) {
> +            if (get_font_style(family, i, &fontStyle, &flags) == B_OK) {
> +                String style(fontStyle);
> +                if ((bItalic && bBold)
> +                    && (style == "BoldItalic" || style == "Bold Oblique")) {
There's no reason for this line break, we don't have an 80-cloumn limit in our style guidelines :D

> +                     bFound = true;
> +                     break;
> +                 }
> +                if ((bItalic && !bBold)
> +                    && (style == "Italic" || style == "Oblique")) {
ditto

...
> +                 }
> +                if ((!bItalic && !bBold)
> +                    && (style == "Roma"
> +                        || style == "Book" || style == "Condensed"
> +                        || style == "Regular" || style == "Medium")) {
You may want multiple lines here for clarity, but 4 lines seems excessive

[Maxime Simon]

> Why are you adding the empy ImageBufferData class?

It seems that all ports add it. And it's required because ImageBuffer.h
include it. :)

> I'm surprised:
>  50 ImageDecoder* createDecoder(const Vector<char>& data)
> 
> isn't shared between teh ports?

No it isn't. Every port defines its own.

[Maxime Simon]

Created attachment 32846 [details]
Patch to add Haiku-specific drawing files for WebCore/platform/graphics/.

[Maxime Simon]

Created attachment 32856 [details]
Patch to add Haiku-specific font files for WebCore/platform/graphics/.

[Maxime Simon]

Created attachment 32867 [details]
Patch to add Haiku-specific drawing files for WebCore/platform/graphics/.

[David Levin]

Comment on attachment 32867 [details]
Patch to add Haiku-specific drawing files for WebCore/platform/graphics/.

You may want to consider using the (alpha version) lint tool for WebKit.

I downloaded this patch and ran on your files in this patch like this:
   python WebKitTools/Scripts/modules/cpplint.py WebCore/platform/graphics/haiku/*.cpp

It found these errors:
WebCore/platform/graphics/haiku/FloatRectHaiku.cpp:44:  Missing space after ,  [whitespace/comma] [3]
WebCore/platform/graphics/haiku/GraphicsContextHaiku.cpp:51:  This { should be at the end of the previous line  [whitespace/braces] [4]
WebCore/platform/graphics/haiku/GraphicsContextHaiku.cpp:229:  Tab found; better to use spaces  [whitespace/tab] [1]
WebCore/platform/graphics/haiku/GraphicsContextHaiku.cpp:495:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
WebCore/platform/graphics/haiku/GraphicsContextHaiku.cpp:495:  Missing space before {  [whitespace/braces] [5]
WebCore/platform/graphics/haiku/GraphicsContextHaiku.cpp:511:  Tab found; better to use spaces  [whitespace/tab] [1]
WebCore/platform/graphics/haiku/GraphicsContextHaiku.cpp:516:  Tab found; better to use spaces  [whitespace/tab] [1]
WebCore/platform/graphics/haiku/GraphicsContextHaiku.cpp:541:  Missing space after ,  [whitespace/comma] [3]
WebCore/platform/graphics/haiku/ImageHaiku.cpp:109:  Tab found; better to use spaces  [whitespace/tab] [1]
WebCore/platform/graphics/haiku/ImageHaiku.cpp:127:  Tab found; better to use spaces  [whitespace/tab] [1]
WebCore/platform/graphics/haiku/ImageSourceHaiku.cpp:78:  Tests for true/false, zero/non-zero, etc. should be done without equality comparisons.  [readability/comparison_to_zero] [5]
WebCore/platform/graphics/haiku/ImageSourceHaiku.cpp:91:  Extra space before )  [whitespace/parens] [2]
WebCore/platform/graphics/haiku/ImageSourceHaiku.cpp:124:  Missing space before ( in if(  [whitespace/parens] [5]
WebCore/platform/graphics/haiku/IntRectHaiku.cpp:45:  Missing space after ,  [whitespace/comma] [3]

When glancing through the code, I also noticed the following issues.

In WebCore/platform/graphics/haiku/GraphicsContextHaiku.cpp

> 56 BView* view;
"Prefix C++ data members with "m_"."So it should be m_view 

> 59 GraphicsContextPlatformPrivate::GraphicsContextPlatformPrivate(BView* p_view)
p_view is not the correct format for a variable.

> 210     // creatin the region.
typo: creatin

> 318 void GraphicsContext::setLineJoin(LineJoin lj)
Use full words, except in the rare case where an abbreviation would be more canonical and easier to understand.
s/lj/lineJoin/


In WebCore/platform/graphics/haiku/ImageBufferData.h
#include "OwnPtr.h" should be #include <wtf/OwnPtr.h>


In WebCore/platform/graphics/haiku/ImageSourceHaiku.cpp
> 65     if (uContents[0] == 0x89 &&
> 66         uContents[1] == 0x50 &&
Boolean expressions at the same nesting level that span multiple lines should have their operators on the left side of the line instead of the right side.

So it should look like this:
     if (uContents[0] == 0x89
         && uContents[1] == 0x50

> 72     // if (uContents[0] == 0xFF &&
Commented out code isn't checked in.  If you want to note the need to address JPEG, you could put a 
  // FIXME: Implement JPEG support.
comment here.

[Adam Barth]

Comment on attachment 32220 [details]
Patch to add Haiku-specific files for WebCore/platform/text/.

Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	WebCore/ChangeLog
	A	WebCore/platform/text/haiku/StringHaiku.cpp
	A	WebCore/platform/text/haiku/TextBreakIteratorInternalICUHaiku.cpp
Committed r46008
	M	WebCore/ChangeLog
	A	WebCore/platform/text/haiku/TextBreakIteratorInternalICUHaiku.cpp
	A	WebCore/platform/text/haiku/StringHaiku.cpp
r46008 = 8630eaa003c63558969f14b36e91f5e32d8a506e (trunk)
No changes between current HEAD and refs/remotes/trunk
Resetting to the latest refs/remotes/trunk
http://trac.webkit.org/changeset/46008

[Adam Barth]

Comment on attachment 32221 [details]
Patch to add Haiku-specific files for WebCore/bindings/js/.

Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	WebCore/ChangeLog
	A	WebCore/bindings/js/ScriptControllerHaiku.cpp
Committed r46009
	M	WebCore/ChangeLog
	A	WebCore/bindings/js/ScriptControllerHaiku.cpp
r46009 = a884a839568b38c02917163be46d2b7046d7d82e (trunk)
No changes between current HEAD and refs/remotes/trunk
Resetting to the latest refs/remotes/trunk
http://trac.webkit.org/changeset/46009

[Adam Barth]

Comment on attachment 32222 [details]
Patch to add Haiku-specific files for WebCore/editing/.

Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	WebCore/ChangeLog
	A	WebCore/editing/haiku/EditorHaiku.cpp
Committed r46010
	A	WebCore/editing/haiku/EditorHaiku.cpp
	M	WebCore/ChangeLog
r46010 = c2dae2a1c3654e83314d76f79f67fb856f927061 (trunk)
No changes between current HEAD and refs/remotes/trunk
Resetting to the latest refs/remotes/trunk
http://trac.webkit.org/changeset/46010

[Adam Barth]

Comment on attachment 32223 [details]
Patch to add Haiku-specific files for WebCore/page/.

Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	WebCore/ChangeLog
	A	WebCore/page/haiku/DragControllerHaiku.cpp
	A	WebCore/page/haiku/EventHandlerHaiku.cpp
	A	WebCore/page/haiku/FrameHaiku.cpp
Committed r46011
	M	WebCore/ChangeLog
	A	WebCore/page/haiku/EventHandlerHaiku.cpp
	A	WebCore/page/haiku/DragControllerHaiku.cpp
	A	WebCore/page/haiku/FrameHaiku.cpp
r46011 = c459ecc2d08dd92a3d1d9c259067fad664cda3b1 (trunk)
No changes between current HEAD and refs/remotes/trunk
Resetting to the latest refs/remotes/trunk
http://trac.webkit.org/changeset/46011

[Adam Barth]

Comment on attachment 32364 [details]
Patch to add Haiku-specific files for WebCore/platform/image-decoders/.

Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	WebCore/ChangeLog
	A	WebCore/platform/image-decoders/haiku/ImageDecoderHaiku.cpp
Committed r46012
	M	WebCore/ChangeLog
	A	WebCore/platform/image-decoders/haiku/ImageDecoderHaiku.cpp
r46012 = 0d02984b5f636d37396785e694037c7a96e6f977 (trunk)
No changes between current HEAD and refs/remotes/trunk
Resetting to the latest refs/remotes/trunk
http://trac.webkit.org/changeset/46012

[Adam Barth]

All reviewed patches landed, closing.

[Adam Barth]

Bugzilla-tool closed this bug, but it should stay open.

[Maxime Simon]

(In reply to comment #25)
> (From update of attachment 32867 [details])
> You may want to consider using the (alpha version) lint tool for WebKit.
> 
> I downloaded this patch and ran on your files in this patch like this:
>    python WebKitTools/Scripts/modules/cpplint.py
> WebCore/platform/graphics/haiku/*.cpp

Thanks a lot for the tip.
This tool should really ease the work for reviewers.
And in the same time it will avoid people like me to fill too much patches. :)

[Maxime Simon]

Created attachment 32923 [details]
Patch to add Haiku-specific drawing files for WebCore/platform/graphics/.

[Maxime Simon]

Created attachment 32924 [details]
Patch to add Haiku-specific font files for WebCore/platform/graphics/.

I made some style cleanup (thanks to the cpplint.py tool).

[Maxime Simon]

Created attachment 32931 [details]
Patch to add Haiku-specific drawing files for WebCore/platform/graphics/.

Sorry, in the previous patch for the purpose of style cleanup I renamed a variable, but it makes the compilation to fail.

[Maxime Simon]

Created attachment 33042 [details]
Patch to add Haiku-specific drawing files for WebCore/platform/graphics/.

The latest modifications on the patch "Modifications on WebKit source code to allow Haiku port." ( https://bugs.webkit.org/show_bug.cgi?id=26620#c59 ) require that I add "#include <stdio.h>" in 2 files of WebCore/platform/graphics/haiku/.

[Eric Seidel (no email)]

Comment on attachment 32924 [details]
Patch to add Haiku-specific font files for WebCore/platform/graphics/.

What is this and why?
 // Fix temp routine to check the conversion works, please move it ASAP    
 44 int charUnicodeToUtf8(unsigned short glyph, char* out)

It's not mentioned in the ChangeLog.  I needs a more decriptive name if it's just a hack.

convertUTF16ToUTF8RenderingHACK or something.

We don't use argument names in headers when they don't add value:
47         FontPlatformData(const FontDescription& fontDescription, const AtomicString& family);
 48         FontPlatformData(const FontDescription& fontDescription, BFont* font);
 49         FontPlatformData(float size, bool bold, bool italic);
 50         FontPlatformData(const FontPlatformData& fontPlatformData);
"family" can stay.  but the fontDescription at least shoudl go.

No hungarian notation here ;)
 66     bool bFound = false;
 67     bool bItalic = fontDescription.italic();
 68     bool bBold = fontDescription.weight() == FontWeightBold;
We ain't no microsoft...

Why isn't this an OwnPtr<BFont>:
 63         BFont* m_font;

Your leaking BFonts.

Why is this correct?
        if (isUtf16) {
 50             UChar lead = characterBuffer[i * 2];
 51             UChar trail = characterBuffer[i * 2 + 1];
 52             character = U16_GET_SUPPLEMENTARY(lead, trail);
 53         } else

Not all characters will be composed characters.

Use the height of the X, can't you ask the font for the height of a glyph at a certain size?
 52         m_xHeight = font->Size(); // Not sure about this

This information should be on the BFont:
 53         m_unitsPerEm = 1; // FIXME!

You'd have more luck posting this in smaller pices.  Some of this was just fine, but some of the font code is wrong here too.  If you broke it out, I coudl more easily approve the OK parts.

[Eric Seidel (no email)]

Comment on attachment 33042 [details]
Patch to add Haiku-specific drawing files for WebCore/platform/graphics/.

Better if m_data could be an OwnPtr for haiku:
 79     delete m_data;

Why?
setFocusRingColorChangeFunction

Spacing:
 230         BRegion    region;

It's better to have conversion funtions outside of these setters:
302     cap_mode mode = B_BUTT_CAP;
 303     switch (lineCap) {
 304     case RoundCap:

I think CG follows that model.

So remove it:
61         // FIXME: Not sure why be_app is checked
 62         if (be_app)
 63             delete m_frame;

Please break this into smaller patches.  AGain, parts of hsi woudl be super-easy to approve.  Other parts no.  If you posted a patch with just the empty implementatins that would be an easy r+, the harder stuff can e done in separate patches.

[Maxime Simon]

> What is this and why?
>  // Fix temp routine to check the conversion works, please move it ASAP    
>  44 int charUnicodeToUtf8(unsigned short glyph, char* out)
> 
> It's not mentioned in the ChangeLog.  I needs a more decriptive name if it's
> just a hack.

I'm not sure about this function. I would like Ryan to explain it. (If he wrote it.)

> convertUTF16ToUTF8RenderingHACK or something.

This may be a possibility.

> No hungarian notation here ;)
>  66     bool bFound = false;
>  67     bool bItalic = fontDescription.italic();
>  68     bool bBold = fontDescription.weight() == FontWeightBold;
> We ain't no microsoft...

Erk, it comes from the Syllable port. Ryan tried to avoid them as much as possible but there may have some remains.

> Use the height of the X, can't you ask the font for the height of a glyph at a
> certain size?
>  52         m_xHeight = font->Size(); // Not sure about this

This was an issue I avoided when improving the port.
But I should update this patch.

> This information should be on the BFont:
>  53         m_unitsPerEm = 1; // FIXME!

The BFont (from Haiku) don't handle this.

> You'd have more luck posting this in smaller pices.  Some of this was just
> fine, but some of the font code is wrong here too.  If you broke it out, I
> coudl more easily approve the OK parts.

I think I will fill a new bug with multiple patches from this one.

[Maxime Simon]

No need to keep this bug opened.