Bug 26620 – Haiku WebKit port

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

[Maxime Simon]

Hi,

Ryan Leavengood previously did a port of WebKit to Haiku. (it was in 2007.)
As part of my Google Summer of Code I have to update this port. And this is it.
I tried to observe a maximum the procedure described in the "Contributing code" page.

[Maxime Simon]

Created attachment 31665 [details]
Modifications on WebKit source code to allow Haiku port.

[Maxime Simon]

Created attachment 31667 [details]
Haiku-specific folders (WebCore/platform/haiku etc...)

[Maxime Simon]

Created attachment 31669 [details]
Modifications on WebKit source code to allow Haiku port. (including ChangeLogs)

[Eric Seidel (no email)]

Who knew?  http://www.haiku-os.org/

[Eric Seidel (no email)]

Comment on attachment 31669 [details]
Modifications on WebKit source code to allow Haiku port. (including ChangeLogs)

We should have a guidelines for starting a new port document.  And one of those guidelines should be about setting up a buildbot after getting JavaScriptCore compiling. ;)

Tab:
+		PlatformMenuDescription m_platformDescription;

Tab:
+	typedef BCursor* PlatformCursorHandle;

I believe the webkit style is FIXME:
+    // RJL TODO: May not need this, what is below may be fine

I'm surprised HAIKU needs its own notImplemented()... maybe all ports have their own...

Are you really intending to copy the BPoint here?
+        PlatformMouseEvent(BPoint where, BPoint screenPoint);

Tab:
+	BMenu* m_menu;

Tab:
+		pattern	getHaikuStrokeStyle();

I think this is OK.  It's sad to add all these #ifdefs... but that's the state of our porting infrastructure.

I would strongly encourage you to introduce yourself on webkit-dev along with your intent.

It's not very interesting to have someone come and get WebKit building on Haiku only to disappear 2 months later when GSoC is over.  Not saying you would, but that's a possibility obviously. :)

It's important for new ports to have buildbots as well as maintainers active in the community.  I think starting with some introductions on webkit-dev and appearing in #webkit (if you aren't already) will help alleviate fears of port-abandonment.

r- for the nits, and because not having ever met you or heard of Haiku before, I'm not sure I won't have to rip out this code 2 weeks from now when Haiku disappears. ;)  Lets talk on webkit-dev or #webkit.

[Eric Seidel (no email)]

Welcome to WebKit, btw.  The patch overall looks good.

[Ryan Leavengood]

(In reply to comment #5)
> 
> I would strongly encourage you to introduce yourself on webkit-dev along with
> your intent.

I agree that would be a good thing for Maxime to do, and I will pop in there myself too.
 
> It's not very interesting to have someone come and get WebKit building on Haiku
> only to disappear 2 months later when GSoC is over.  Not saying you would, but
> that's a possibility obviously. :)

Hopefully Maxime won't disappear, but I will be sure to stick around. As Maxime said most of this code is from my 2007 port of WebKit, and once it is in the WebKit tree myself and I am sure others from the Haiku community will keep it updated.

> It's important for new ports to have buildbots as well as maintainers active in
> the community.  I think starting with some introductions on webkit-dev and
> appearing in #webkit (if you aren't already) will help alleviate fears of
> port-abandonment.

I agree. I'm not sure if it helps but I had a few general patches accepted before when I did the first port. I will probably get more involved in the WebKit community as this project moves along.

> r- for the nits, and because not having ever met you or heard of Haiku before,
> I'm not sure I won't have to rip out this code 2 weeks from now when Haiku
> disappears. ;)  Lets talk on webkit-dev or #webkit.

I am not sure if you remember me, but I can vouch for Maxime. And don't worry, Haiku won't be disappearing. It has been around since 2001 (which I think is longer than WebKit) and is very close to an alpha release.

Also thanks a lot for making such a great web engine and for at least considering alternative ports like Haiku. Fixing those nits and introducing ourselves on email should hopefully get this committed :)

Regards,
Ryan Leavengood

[Maxime Simon]

Hi Eric,

Thanks for the review. ( Even if it is a r-, at least someone is interested. :)

I agree with you, a guideline for new ports should be good.

I easily corrected the “tabs issues”.
As for the `PlatformMouseEvent(BPoint where, BPoint screenPoint);' method,
in fact it isn't necessary. :)
Regarding to the `notImplemented()', I'm currently testing our port without using it.
( For now only Haiku specifies a personalized `notImplemented()' method. )

When the nits will be fixed, should I supply a new patch, or should I edit the existing one?
I tried but didn't succeed to edit the current one.

Regards,
Maxime

[Maxime Simon]

Created attachment 31804 [details]
Haiku-specific folders (WebCore/platform/haiku etc...)

I tried to make these files as compliant as possible with the WebKit coding guideline.

[Adam Treat]

Hint: in order to use the cross-platform NotImplemented.h header file you need to get a 'LoggingHaiku.cpp' file that works much like the other ports versions.  Also, you might need to look at Assertions.cpp and change the various 'vsprintf' methods.

Also, you should get used to 'r-' and not take it as discouraging.  Rather, be encouraged that people are honestly considering your patches!

Good Luck,
Adam

[Maxime Simon]

(In reply to comment #10)
> Hint: in order to use the cross-platform NotImplemented.h header file you need
> to get a 'LoggingHaiku.cpp' file that works much like the other ports versions.
>  Also, you might need to look at Assertions.cpp and change the various
> 'vsprintf' methods.

Indeed, I saw that when I examined how other ports handle this. Thanks.

> Also, you should get used to 'r-' and not take it as discouraging.  Rather, be
> encouraged that people are honestly considering your patches!

There is no problem, I'm not discouraged by the 'r-'. :)

I'm sorry I will mark as obsolete the attachments and add new one.
That's because I got an issue, there are some unwanted files in the archive I submitted:
Some files that I didn't archived but which are here when untaring.

Regards,
Maxime

[Maxime Simon]

Comment on attachment 31804 [details]
Haiku-specific folders (WebCore/platform/haiku etc...)

This archive contains some unwanted files. (tar issue?)

[Maxime Simon]

Created attachment 31812 [details]
Haiku-specific folders (WebCore/platform/haiku etc...)

As replacement to the previous archive which contained undesirable files.

[Maxime Simon]

Comment on attachment 31669 [details]
Modifications on WebKit source code to allow Haiku port. (including ChangeLogs)

For the corrections I made on this patch, should I provide a new one as attachment or should I edit this one as comment?
I would be prefer to attache a new one.

[Adam Treat]

For corrections to a patch you should generally obsolete old ones and add new ones.

Now, for your 'Haiku-specific folders' patch, can you kindly provide the mimetype as I am unable to view or see what this patch pertains to, much less review it.

Thanks!

[Adam Treat]

Also, you should have no need to attach a tar that just creates folders.  The folders will be created by new files in a regular patch file.

[Maxime Simon]

(In reply to comment #16)
> Also, you should have no need to attach a tar that just creates folders.  The
> folders will be created by new files in a regular patch file.
> 

Oh, I'm sorry. In fact I read this in the page about contributing:
“New files and layout tests must be added to Subversion or else they won't be included in your patch.”
That's why I attached a tar. But there is not problem to replace it by a patch.

[Maxime Simon]

Created attachment 31858 [details]
Modifications on WebKit source code to allow Haiku port. (including ChangeLogs)

[Maxime Simon]

Created attachment 31859 [details]
Haiku-specific folders (WebCore/platform/haiku etc...)

[Maxime Simon]

Created attachment 31941 [details]
Patch to add Haiku-specific files for JavaScriptCore.

[Maxime Simon]

Created attachment 31942 [details]
Patch to add Haiku-specific files for WebCore.

[Eric Seidel (no email)]

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

This patch is too large to review.

There are also numerous style issues:
+    if (pcData->font() == NULL)
+    {

[Ryan Leavengood]

Hi Eric,

Any suggestions for the optimal or "easiest to review" method for providing the patches needed to add this port?

As you probably know each port has quite a few files it needs, and each part of the code has a certain amount of dependencies on other related parts.

Would breaking the WebCore patch into one directory per patch help?

[Eric Seidel (no email)]

Comment on attachment 31858 [details]
Modifications on WebKit source code to allow Haiku port. (including ChangeLogs)

Looks fine to me.

 39 #include "stdint.h"

probably meant to be <stdint.h> no?

[Eric Seidel (no email)]

Comment on attachment 31941 [details]
Patch to add Haiku-specific files for JavaScriptCore.

I think you likely want these to have notImplemented().  Lack of implementation will cause you future grief here.  At least according to a brief conversation with Maciej in #webkit.

[Eric Seidel (no email)]

Comment on attachment 31941 [details]
Patch to add Haiku-specific files for JavaScriptCore.

Your ChangeLogs should also generally have a link to the bug.

[Maxime Simon]

(In reply to comment #24)
> (From update of attachment 31858 [details] [review])
> Looks fine to me.
> 
>  39 #include "stdint.h"
> 
> probably meant to be <stdint.h> no?
> 

Indeed, it should be <stdint.h>. Strangely it works as it.

[Maxime Simon]

Created attachment 32049 [details]
Patch to add Haiku-specific files for JavaScriptCore.

[Maxime Simon]

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

[Maxime Simon]

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

[Maxime Simon]

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

[Maxime Simon]

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

This patch would make obsolete some changes made in WebCore/platform/Widget.h.
In fact, we don't need some methods such as haikuWidget() (as the Widget class is much more flexible than when Ryan did the first port).

[Maxime Simon]

Created attachment 32060 [details]
Patch to add WebCore support.

[Maxime Simon]

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

[Maxime Simon]

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

[Maxime Simon]

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

[Maxime Simon]

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

[Maxime Simon]

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

Double submit

[Maxime Simon]

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

Missed some files.

[Maxime Simon]

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

[Eric Seidel (no email)]

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

Looks OK, but the ChangeLog doesn't need:

WARNING: NO TEST CASES ADDED OR CHANGED

[Eric Seidel (no email)]

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

Seems you might want a notImplemnted() at:
+const char* currentTextBreakLocaleID()

Again, don't need:
+        WARNING: NO TEST CASES ADDED OR CHANGED

Since someone would have to manually fix your ChangeLogs, gonna r- this.

[Eric Seidel (no email)]

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

Too large to review.

[Eric Seidel (no email)]

Comment on attachment 32060 [details]
Patch to add WebCore support.

Too large to review.

[Eric Seidel (no email)]

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

 8         WARNING: NO TEST CASES ADDED OR CHANGED

is not needed.  Otherwise this looks great.  r- because someone would have to manually edit your ChangeLogs when landing.

[Eric Seidel (no email)]

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

Looks fine.  Where is the implementation of ClipboardHaiku?

R- for someone having to edit your ChangeLogs when landing:
+        WARNING: NO TEST CASES ADDED OR CHANGED

(If you were a committer I would just r+ all these, but as is, it's best if you fix them first so they can be landed automatically by our scripts.)

[Eric Seidel (no email)]

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

Please don't include commented out code:
 //#include "Frame.h"
 32 //#include "FrameView.h"
 33 //#include "Page.h"
 
 108     //notImplemented();


Looks OK otherwise.  Please remove extra comments and update changeLog and re-post.  I would have r+'d this if you were a committer.

 8         WARNING: NO TEST CASES ADDED OR CHANGED

[Maxime Simon]

(In reply to comment #45)
> (From update of attachment 32069 [details] [review])
>  8         WARNING: NO TEST CASES ADDED OR CHANGED
> 
> is not needed.  Otherwise this looks great.  r- because someone would have to
> manually edit your ChangeLogs when landing.
> 

In fact, 'prepare-ChangeLog' puts this warning on the ChangeLogs generated for WebCore.
But there is no problems to change this.

[Eric Seidel (no email)]

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

What is a styleable port?
 45     // FIXME: subtract 1 from height and width? This was not done in Syllable port

I don't think you meant to leaven this in:
 52     printf("FontCache::getFontDataForCharacters\n");    

WebKit generally uses 0 for NULL in C++ code:
 72     pcDefaultFont->GetFamilyAndStyle(&family, NULL);

Please use c++ style casts:
 76     //return new FontPlatformData(fontDescription, (BFont*)pcDefaultFont);

We don't commit commented out code:
 76     //return new FontPlatformData(fontDescription, (BFont*)pcDefaultFont);

Style:
83     if (!pcData->font())
 84     {

You should consider using an OwnPtr here:
 82     FontPlatformData* pcData = new FontPlatformData(fontDescription, family);

Style:
 51 int char_unicode_to_utf8(unsigned short glyph, char* out) {

Tabs will cause the commit to fail:
54 	if (glyph < 0x0080)
 55 		out[i++] = (char)glyph;

Indent:
{
 74 	notImplemented();
 75 	return true;
 76 }

Why?
 43 int char_unicode_to_utf8(unsigned short glyph, char* out); // implemented in FontDataHaiku.cpp

Please use C++ style casts:
 57     BView* view = (BView*)graphicsContext->platformContext();

Double converting to float:
 float Font::floatWidthForComplexText(const TextRun& run, HashSet<const SimpleFontData*>* fallbackFonts) const
 83 {
 84     notImplemented();
 85     return 3.0;

Style:
59     if (f.m_font)
 60     {
 61         m_font = new BFont(f.m_font);
 62     }

Extra spaces and bad variable name:
 71     font_family	fam;

STyle:
 74     for(int i = 0; i < count_font_families(); i++) {


Please break this up into smaller patches.

Please do fewer patches per-bug.

[Eric Seidel (no email)]

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

Since you're not a committer this can't be landed by you. Please upload a new one with 
 8         WARNING: NO TEST CASES ADDED OR CHANGED

removed.  otherwise it looked fine.

[Maxime Simon]

(In reply to comment #46)
> (From update of attachment 32071 [details] [review])
> Looks fine.  Where is the implementation of ClipboardHaiku?
> 
> R- for someone having to edit your ChangeLogs when landing:
> +        WARNING: NO TEST CASES ADDED OR CHANGED
> 
> (If you were a committer I would just r+ all these, but as is, it's best if you
> fix them first so they can be landed automatically by our scripts.)
> 

The implementation of ClipboardHaiku is in WebCore/platform/haiku/.
Should I include it in this patch?

[Eric Seidel (no email)]

(In reply to comment #51)

> The implementation of ClipboardHaiku is in WebCore/platform/haiku/.
> Should I include it in this patch?

It's OK for it to be in a separate patch, that's fine.

[Adam Barth]

Comment on attachment 31858 [details]
Modifications on WebKit source code to allow Haiku port. (including ChangeLogs)

I'm sorry, but this patch no longer applies to top-of-tree.  DateMath.cpp has changed a bit and it's not obvious to me how to merge the patch.

[Adam Barth]

Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	JavaScriptCore/ChangeLog
	A	JavaScriptCore/wtf/haiku/MainThreadHaiku.cpp
Committed r46018
	A	JavaScriptCore/wtf/haiku/MainThreadHaiku.cpp
	M	JavaScriptCore/ChangeLog
r46018 = 9d379b1aa5f22afa8ef22b61116157d6c5e52541 (trunk)
No changes between current HEAD and refs/remotes/trunk
Resetting to the latest refs/remotes/trunk
http://trac.webkit.org/changeset/46018

[Adam Barth]

Should still be open.

[Adam Barth]

Comment on attachment 32049 [details]
Patch to add Haiku-specific files for JavaScriptCore.

I guess i need to clear this flag to remove this bug from the commit queue.

[Maxime Simon]

Created attachment 32917 [details]
Modifications on WebKit source code to allow Haiku port.

Concerning the issue with DateMath.cpp, as the issue had move to another place there is no more modifications in this file.

[Oliver Hunt]

Comment on attachment 32917 [details]
Modifications on WebKit source code to allow Haiku port.


> -#if !PLATFORM(WIN_OS) && !PLATFORM(SOLARIS) && !PLATFORM(SYMBIAN) && !COMPILER(RVCT)
> +#if !PLATFORM(WIN_OS) && !PLATFORM(SOLARIS) && !PLATFORM(SYMBIAN) && !COMPILER(RVCT) && !(HAIKU)
Should be !PLATFORM(HAIKU)

> --- a/WebCore/config.h
> +++ b/WebCore/config.h
> @@ -187,3 +187,7 @@ typedef float CGFloat;
>  #if PLATFORM(WIN) && PLATFORM(CG)
>  #define WTF_USE_SAFARI_THEME 1
>  #endif
> +
> +#if PLATFORM(HAIKU)
> +#include <stdio.h>
> +#endif
stdio.h should not be included in config.h

[Maxime Simon]

Created attachment 33041 [details]
Modifications on WebKit source code to allow Haiku port.

[Eric Seidel (no email)]

Comment on attachment 33041 [details]
Modifications on WebKit source code to allow Haiku port.

Looks OK.  In the future your ChangeLogs need *much* more detail as to what the Change is actually doing.  Especially one with as many pieces as this one.

[Adam Barth]

Comment on attachment 33041 [details]
Modifications on WebKit source code to allow Haiku port.

This patch does not apply cleanly:

patching file WebCore/platform/graphics/BitmapImage.h
Hunk #1 succeeded at 45 (offset 1 line).
Hunk #2 FAILED at 165.
1 out of 2 hunks FAILED -- saving rejects to file WebCore/platform/graphics/BitmapImage.h.rej
patch -p0 "WebCore/platform/graphics/BitmapImage.h" returned 1.  Pass --force to ignore patch failures.

I have not checked whether the merge is trivial.

[Maxime Simon]

(In reply to comment #61)
> (From update of attachment 33041 [details])
> This patch does not apply cleanly:
> 
> patching file WebCore/platform/graphics/BitmapImage.h
> Hunk #1 succeeded at 45 (offset 1 line).
> Hunk #2 FAILED at 165.
> 1 out of 2 hunks FAILED -- saving rejects to file
> WebCore/platform/graphics/BitmapImage.h.rej
> patch -p0 "WebCore/platform/graphics/BitmapImage.h" returned 1.  Pass --force
> to ignore patch failures.
> 
> I have not checked whether the merge is trivial.

If I am right, you are currently integrating WinCE in the repo.
So it may be why you get this issue. I made this patch around 3 weeks ago when the WinCE port wasn't integrated in the tree.
I will fill a new bug and post another patch.

[Eric Seidel (no email)]

I think it's time to retire this bug.  Please post new patches on new bugs.