Bug 28128 – [Haiku] Modifications on WebCore.

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Maxime Simon]

I intend to fill multiple patches in this bug.
I will make this because I want to post not too big patches, but they are all related to easy modifications for WebCore.

Regards,
Maxime

[Maxime Simon]

Created attachment 34416 [details]
Modifications on WebCore to allow Haiku port.

[Maxime Simon]

Created attachment 34417 [details]
Patch to add tiny modifications on WebCore/platform/graphics files to allow Haiku port.

[Maxime Simon]

Created attachment 34418 [details]
Patch to add modifications to the WebCore/platform/ files to allow the Haiku port.

[Eric Seidel (no email)]

Comment on attachment 34416 [details]
Modifications on WebCore to allow Haiku port.

I just got through asking the WINCE guys to use a #define for the FontCache changes.  So that we only need that long list of ||s in one place.  This change will conflict with theirs.  Make sure you coordinate.

One of you two needs to add the define.  I don't really care which. :)

[Eric Seidel (no email)]

Comment on attachment 34417 [details]
Patch to add tiny modifications on WebCore/platform/graphics files to allow Haiku port.

Why virtual?
#if PLATFORM(HAIKU)
 175     virtual BBitmap* getBitmap() const;
 176 #endif

Seems it should be createBBitmap()?  Or is it actually a getter?  in which case it can be just bitmap(), no?

We generally don't use "get" in getters:
 388         pattern	getHaikuStrokeStyle();

haikuStrokeStyle()

unless that's createing something in which case it should be create.  It looks like it's just returning a stack object though, so it doesn't need "create" in the name.

Why is this needed?
 #elif PLATFORM(HAIKU)
 82     Icon();
8183 #endif
please explain in the ChangeLog.

Seems like  a bad idea:
 #elif PLATFORM(HAIKU)
 127     IntPoint(const BPoint&);
 128     operator BPoint() const;
124129 #endif
You don't wan implicit conversion of floating point points to integer points.

Again, probably bad idea:
#elif PLATFORM(HAIKU)
 150     IntRect(const BRect&);
 151     operator BRect() const;
147152 #endif

Again:
 #if PLATFORM(HAIKU)
 119     IntSize(const BSize&);
 120     operator BSize() const;
 121 #endif

(this is all assuming that B* are floating point based).

[Eric Seidel (no email)]

Comment on attachment 34418 [details]
Patch to add modifications to the WebCore/platform/ files to allow the Haiku port.

What is:
 85         BPoint m_point;
here?  Is it the top-left of the menu?  Is it the click-point?  Seems it needs a better member name.

Otherwise this change looks fine.  r- for lack of useful name or explanation for m_point.  If you were a committer I would just ask you to rename it to something when you landed it instead of r-ing.

[Ryan Leavengood]

(In reply to comment #6)
> (From update of attachment 34418 [details])
> What is:
>  85         BPoint m_point;
> here?  Is it the top-left of the menu?  Is it the click-point?  Seems it needs
> a better member name.
> 
> Otherwise this change looks fine.  r- for lack of useful name or explanation
> for m_point.  If you were a committer I would just ask you to rename it to
> something when you landed it instead of r-ing.

As far as I can tell it isn't really used. It is assigned in the ContextMenu contructor but is otherwise not used. This is probably just more cruft from the old port that I should have helped Maxime purge :/

I'm not sure this context menu code even works at all at this point. I guess it is a start though.

[Ryan Leavengood]

(In reply to comment #5)
> (From update of attachment 34417 [details])
> Why virtual?
> #if PLATFORM(HAIKU)
>  175     virtual BBitmap* getBitmap() const;
>  176 #endif
> 
> Seems it should be createBBitmap()?  Or is it actually a getter?  in which case
> it can be just bitmap(), no?

Well all the other platforms seem to use the same pattern for their native bitmap getters:

#if PLATFORM(MAC)
    // Accessors for native image formats.
    virtual NSImage* getNSImage();
    virtual CFDataRef getTIFFRepresentation();
#endif
    
#if PLATFORM(CG)
    virtual CGImageRef getCGImageRef();
#endif

#if PLATFORM(WIN)
    virtual bool getHBITMAP(HBITMAP);
    virtual bool getHBITMAPOfSize(HBITMAP, LPSIZE);
#endif

#if PLATFORM(GTK)
    virtual GdkPixbuf* getGdkPixbuf();
#endif

Maybe we just need to move ours up with these, I'm not sure why it is down on that line.

> We generally don't use "get" in getters:
>  388         pattern    getHaikuStrokeStyle();
> 
> haikuStrokeStyle()
> 
> unless that's createing something in which case it should be create.  It looks
> like it's just returning a stack object though, so it doesn't need "create" in
> the name.

That is generally the philosophy in Haiku's API for getters too, but in this case this is more of a converter function (to convert WebKit stroke styles into Haiku stroke styles.) I guess haikuStrokeStyle() would still be OK in this case. Or to be explicit: convertToHaikuStrokeStyle()?
 
> Why is this needed?
>  #elif PLATFORM(HAIKU)
>  82     Icon();
> 8183 #endif
> please explain in the ChangeLog.

Declaring a default constructor for a currently unimplemented class?

> Seems like  a bad idea:
>  #elif PLATFORM(HAIKU)
>  127     IntPoint(const BPoint&);
>  128     operator BPoint() const;
> 124129 #endif
> You don't wan implicit conversion of floating point points to integer points.
> 
> Again, probably bad idea:
> #elif PLATFORM(HAIKU)
>  150     IntRect(const BRect&);
>  151     operator BRect() const;
> 147152 #endif
> 
> Again:
>  #if PLATFORM(HAIKU)
>  119     IntSize(const BSize&);
>  120     operator BSize() const;
>  121 #endif
> 
> (this is all assuming that B* are floating point based).

Yes all the B* classes above use floats. I guess it is better to make all these explicit?

[Maxime Simon]

(In reply to comment #8)
> (In reply to comment #5)
> > (From update of attachment 34417 [details] [details])
> > Why virtual?
> > #if PLATFORM(HAIKU)
> >  175     virtual BBitmap* getBitmap() const;
> >  176 #endif
> > 
> > Seems it should be createBBitmap()?  Or is it actually a getter?  in which case
> > it can be just bitmap(), no?
> 
> Well all the other platforms seem to use the same pattern for their native
> bitmap getters:
> […]
> 
> Maybe we just need to move ours up with these, I'm not sure why it is down on
> that line.

Even if we keep the "get" prefix, we should rename this function from getBitmap to getBBitmap (I can't see why I removed the B).
Concerning the virtual, all other ports use it… (Yes I like mimicry.)

> > Why is this needed?
> >  #elif PLATFORM(HAIKU)
> >  82     Icon();
> > 8183 #endif
> > please explain in the ChangeLog.
> 
> Declaring a default constructor for a currently unimplemented class?

Actually, I didn't check why we have this constructor. :-/

> > Seems like  a bad idea:
> >  #elif PLATFORM(HAIKU)
> >  127     IntPoint(const BPoint&);
> >  128     operator BPoint() const;
> > 124129 #endif
> > You don't wan implicit conversion of floating point points to integer points.
> > 
> > Again, probably bad idea:
> > #elif PLATFORM(HAIKU)
> >  150     IntRect(const BRect&);
> >  151     operator BRect() const;
> > 147152 #endif
> > 
> > Again:
> >  #if PLATFORM(HAIKU)
> >  119     IntSize(const BSize&);
> >  120     operator BSize() const;
> >  121 #endif
> > 
> > (this is all assuming that B* are floating point based).
> 
> Yes all the B* classes above use floats. I guess it is better to make all these
> explicit?

I changed this, it will be included in the next patches.

[Maxime Simon]

(In reply to comment #7)
> (In reply to comment #6)
> > (From update of attachment 34418 [details] [details])
> > What is:
> >  85         BPoint m_point;
> > here?  Is it the top-left of the menu?  Is it the click-point?  Seems it needs
> > a better member name.
> > 
> > Otherwise this change looks fine.  r- for lack of useful name or explanation
> > for m_point.  If you were a committer I would just ask you to rename it to
> > something when you landed it instead of r-ing.
> 
> As far as I can tell it isn't really used. It is assigned in the ContextMenu
> contructor but is otherwise not used. This is probably just more cruft from the
> old port that I should have helped Maxime purge :/
> 
> I'm not sure this context menu code even works at all at this point. I guess it
> is a start though.

Indeed, it's an unused variable. I removed it.
It calculated the origin of each BMenuItem (if I'm right).
But it seems not to be necessary now.

[Maxime Simon]

Created attachment 34554 [details]
Patch to modify some WebCore/platform/ files to allow the Haiku port.

[Eric Seidel (no email)]

Comment on attachment 34554 [details]
Patch to modify some WebCore/platform/ files to allow the Haiku port.

m_ for member variables:
 #if PLATFORM(MAC)
213218     WidgetPrivate* m_data;
 219 #elif PLATFORM(HAIKU)
 220     WidgetPrivate* data;
214221 #endif

Otherwise this looks fine.  If you were a committer I would r+ this and you could fix and land it yourself.

[Maxime Simon]

Created attachment 34557 [details]
Patch to modify some WebCore/platform/ files to allow the Haiku port.

[Eric Seidel (no email)]

Comment on attachment 34557 [details]
Patch to modify some WebCore/platform/ files to allow the Haiku port.

 220     WidgetPrivate* m_data;

WidgetPrivate is never declared... so that won't compile, or?

[Eric Seidel (no email)]

Comment on attachment 34557 [details]
Patch to modify some WebCore/platform/ files to allow the Haiku port.

Nevermind.  Looks fine.

[Maxime Simon]

(In reply to comment #14)
> (From update of attachment 34557 [details])
>  220     WidgetPrivate* m_data;
> 
> WidgetPrivate is never declared... so that won't compile, or?

Indeed… As I was changing data to m_data I saw that WidgetPrivate is never
declared, but what it compiles without problem anyway… :)

[Eric Seidel (no email)]

It's forward declared at the top of the file.

I hope you're not just adding these members because other ports have them?  Code should only be added when it's actually used....

[Maxime Simon]

(In reply to comment #17)
> It's forward declared at the top of the file.
> 
> I hope you're not just adding these members because other ports have them? 
> Code should only be added when it's actually used....

No, that's because we used to have it.
Since then I modify the port I forgot to remove it.
I'll submit a new patch without this WidgetPrivate.

[Eric Seidel (no email)]

It seems like you all are close to having all of your files upstreamed.  Have you started work on build-webkit integration and a DumpRenderTree?  Having working layout tests (aka having a DumpRenderTree implementation) is an essential part of being a port which is long-term successful.  w/o layout tests the port will be broken again in a matter of weeks. :(

[Maxime Simon]

(In reply to comment #19)
> It seems like you all are close to having all of your files upstreamed.  Have
> you started work on build-webkit integration and a DumpRenderTree?  Having
> working layout tests (aka having a DumpRenderTree implementation) is an
> essential part of being a port which is long-term successful.  w/o layout tests
> the port will be broken again in a matter of weeks. :(

I have a working version of DumpRenderTree but I didn't implement all the tests.

Concerning build-webkit I didn't make anything.
Currently we are using Jamfiles, but we want to change this and use GYP instead (we must write a backend to generate Jamfiles).

[Maxime Simon]

Created attachment 34559 [details]
Patch to modify some WebCore/platform/ files to allow the Haiku port.

[Eric Seidel (no email)]

Comment on attachment 34559 [details]
Patch to modify some WebCore/platform/ files to allow the Haiku port.

LGTM.

[Eric Seidel (no email)]

I've updated https://trac.webkit.org/wiki/SuccessfulPortHowTo for Haiku (and other ports) benefit.  You should feel encouraged to update it further!

[Maxime Simon]

Created attachment 34576 [details]
Patch to modify some WebCore/platform/graphics files to allow the Haiku port.

[Maxime Simon]

(In reply to comment #24)
> Created an attachment (id=34576) [details]
> Patch to modify some WebCore/platform/graphics files to allow the Haiku port.

I forgot some comments on this patch,
- I renamed getBitmap to getBBitmap.
- I didn't change the getHaikuStrokeStyle name.
- I put some explanations for the empty Icon constructor.
- I changed the Int* constructors so they are explicit.

[Eric Seidel (no email)]

Comment on attachment 34576 [details]
Patch to modify some WebCore/platform/graphics files to allow the Haiku port.

Looks OK.  I thought you decided Icon() was not used?

[Eric Seidel (no email)]

OK.  re-read the ChangeLog.

[Eric Seidel (no email)]

Comment on attachment 34416 [details]
Modifications on WebCore to allow Haiku port.

Clearing flags on attachment: 34416

Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	WebCore/ChangeLog
	M	WebCore/bindings/js/ScriptControllerHaiku.cpp
	M	WebCore/loader/CachedFont.cpp
	M	WebCore/page/EventHandler.cpp
	M	WebCore/page/haiku/DragControllerHaiku.cpp
	M	WebCore/platform/image-decoders/ImageDecoder.h
	M	WebCore/platform/text/PlatformString.h
	M	WebCore/platform/text/UnicodeRange.h
	M	WebCore/platform/text/haiku/TextBreakIteratorInternalICUHaiku.cpp
Committed r47144
	M	WebCore/ChangeLog
	M	WebCore/page/haiku/DragControllerHaiku.cpp
	M	WebCore/page/EventHandler.cpp
	M	WebCore/platform/text/UnicodeRange.h
	M	WebCore/platform/text/haiku/TextBreakIteratorInternalICUHaiku.cpp
	M	WebCore/platform/text/PlatformString.h
	M	WebCore/platform/image-decoders/ImageDecoder.h
	M	WebCore/bindings/js/ScriptControllerHaiku.cpp
	M	WebCore/loader/CachedFont.cpp
r47144 = dabd8ddbe25f0c6eb4e9e79ed11b7c7036c7ba22 (trunk)
No changes between current HEAD and refs/remotes/trunk
Resetting to the latest refs/remotes/trunk
http://trac.webkit.org/changeset/47144

[Eric Seidel (no email)]

Comment on attachment 34559 [details]
Patch to modify some WebCore/platform/ files to allow the Haiku port.

Clearing flags on attachment: 34559

Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	WebCore/ChangeLog
	M	WebCore/platform/ContextMenuItem.h
	M	WebCore/platform/Cursor.h
	M	WebCore/platform/DragData.h
	M	WebCore/platform/DragImage.h
	M	WebCore/platform/PlatformKeyboardEvent.h
	M	WebCore/platform/PlatformMenuDescription.h
	M	WebCore/platform/PlatformMouseEvent.h
	M	WebCore/platform/PlatformWheelEvent.h
	M	WebCore/platform/PopupMenu.h
	M	WebCore/platform/Widget.h
Committed r47145
	M	WebCore/ChangeLog
	M	WebCore/platform/Cursor.h
	M	WebCore/platform/DragData.h
	M	WebCore/platform/PlatformMenuDescription.h
	M	WebCore/platform/PlatformKeyboardEvent.h
	M	WebCore/platform/DragImage.h
	M	WebCore/platform/PopupMenu.h
	M	WebCore/platform/ContextMenuItem.h
	M	WebCore/platform/PlatformMouseEvent.h
	M	WebCore/platform/PlatformWheelEvent.h
	M	WebCore/platform/Widget.h
r47145 = a82dd2e4dac367994e9d09bc33a7aa1ceb3485ce (trunk)
No changes between current HEAD and refs/remotes/trunk
Resetting to the latest refs/remotes/trunk
http://trac.webkit.org/changeset/47145

[Eric Seidel (no email)]

Comment on attachment 34576 [details]
Patch to modify some WebCore/platform/graphics files to allow the Haiku port.

Clearing flags on attachment: 34576

Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	WebCore/ChangeLog
	M	WebCore/platform/graphics/BitmapImage.h
	M	WebCore/platform/graphics/Color.h
	M	WebCore/platform/graphics/FloatPoint.h
	M	WebCore/platform/graphics/FloatRect.h
	M	WebCore/platform/graphics/GraphicsContext.cpp
	M	WebCore/platform/graphics/GraphicsContext.h
	M	WebCore/platform/graphics/Icon.h
	M	WebCore/platform/graphics/ImageSource.h
	M	WebCore/platform/graphics/IntPoint.h
	M	WebCore/platform/graphics/IntRect.h
	M	WebCore/platform/graphics/IntSize.h
	M	WebCore/platform/graphics/Path.h
	M	WebCore/platform/graphics/Pattern.h
	M	WebCore/platform/graphics/SimpleFontData.h
Committed r47146
	M	WebCore/ChangeLog
	M	WebCore/platform/graphics/ImageSource.h
	M	WebCore/platform/graphics/GraphicsContext.h
	M	WebCore/platform/graphics/Color.h
	M	WebCore/platform/graphics/FloatPoint.h
	M	WebCore/platform/graphics/FloatRect.h
	M	WebCore/platform/graphics/Icon.h
	M	WebCore/platform/graphics/GraphicsContext.cpp
	M	WebCore/platform/graphics/IntRect.h
	M	WebCore/platform/graphics/IntSize.h
	M	WebCore/platform/graphics/IntPoint.h
	M	WebCore/platform/graphics/SimpleFontData.h
	M	WebCore/platform/graphics/Pattern.h
	M	WebCore/platform/graphics/BitmapImage.h
	M	WebCore/platform/graphics/Path.h
r47146 = 573bc04eec4313e26f6b78c3ccfe1a727e962d0b (trunk)
No changes between current HEAD and refs/remotes/trunk
Resetting to the latest refs/remotes/trunk
http://trac.webkit.org/changeset/47146

[Eric Seidel (no email)]

All reviewed patches have been landed.  Closing bug.

[Yong Li]

#if PLATFORM(CG) || PLATFORM(QT) || PLATFORM(GTK) || (PLATFORM(CHROMIUM) && (PLATFORM(WIN_OS) || PLATFORM(LINUX)))

Why was this allowed where we were required to replace with another macro?

See bug 27734

----  Comment #14 From  Eric Seidel   2009-08-06 19:48:50 PDT   (-) [reply] -------

(From update of attachment 34178 [details])
Please collapse this line into one nicely named define:

 35 #if PLATFORM(CG) || PLATFORM(QT) || PLATFORM(GTK) || (PLATFORM(CHROMIUM) &&
PLATFORM(WIN_OS)) || PLATFORM(WINCE)

like STORE_FONT_POINTER or whatever the heck the ifdefed code does.

[Eric Seidel (no email)]

(In reply to comment #32)
> #if PLATFORM(CG) || PLATFORM(QT) || PLATFORM(GTK) || (PLATFORM(CHROMIUM) &&
> (PLATFORM(WIN_OS) || PLATFORM(LINUX)))
> 
> Why was this allowed where we were required to replace with another macro?
> 
> See bug 27734
> 
> ----  Comment #14 From  Eric Seidel   2009-08-06 19:48:50 PDT   (-) [reply]
> -------
> 
> (From update of attachment 34178 [details] [details])
> Please collapse this line into one nicely named define:
> 
>  35 #if PLATFORM(CG) || PLATFORM(QT) || PLATFORM(GTK) || (PLATFORM(CHROMIUM) &&
> PLATFORM(WIN_OS)) || PLATFORM(WINCE)
> 
> like STORE_FONT_POINTER or whatever the heck the ifdefed code does.

See comment #4 on this bug.

[Ryan Leavengood]

(In reply to comment #32)
> #if PLATFORM(CG) || PLATFORM(QT) || PLATFORM(GTK) || (PLATFORM(CHROMIUM) &&
> (PLATFORM(WIN_OS) || PLATFORM(LINUX)))
> 
> Why was this allowed where we were required to replace with another macro?
> 
> See bug 27734
> 
> ----  Comment #14 From  Eric Seidel   2009-08-06 19:48:50 PDT   (-) [reply]
> -------
> 
> (From update of attachment 34178 [details] [details])
> Please collapse this line into one nicely named define:
> 
>  35 #if PLATFORM(CG) || PLATFORM(QT) || PLATFORM(GTK) || (PLATFORM(CHROMIUM) &&
> PLATFORM(WIN_OS)) || PLATFORM(WINCE)
> 
> like STORE_FONT_POINTER or whatever the heck the ifdefed code does.

If you WinCE guys want to add this we will modify our patch once yours has landed.

[Yong Li]

(In reply to comment #34)
> (In reply to comment #32)
> > #if PLATFORM(CG) || PLATFORM(QT) || PLATFORM(GTK) || (PLATFORM(CHROMIUM) &&
> > (PLATFORM(WIN_OS) || PLATFORM(LINUX)))
> > 
> > Why was this allowed where we were required to replace with another macro?
> > 
> > See bug 27734
> > 
> > ----  Comment #14 From  Eric Seidel   2009-08-06 19:48:50 PDT   (-) [reply]
> > -------
> > 
> > (From update of attachment 34178 [details] [details] [details])
> > Please collapse this line into one nicely named define:
> > 
> >  35 #if PLATFORM(CG) || PLATFORM(QT) || PLATFORM(GTK) || (PLATFORM(CHROMIUM) &&
> > PLATFORM(WIN_OS)) || PLATFORM(WINCE)
> > 
> > like STORE_FONT_POINTER or whatever the heck the ifdefed code does.
> 
> If you WinCE guys want to add this we will modify our patch once yours has
> landed.

Don't worry about this. Your patch has already been committed, and ours is still pending. So I had to solve conflicts when I updated our repo.

[Ryan Leavengood]

(In reply to comment #35)
> 
> Don't worry about this. Your patch has already been committed, and ours is
> still pending. So I had to solve conflicts when I updated our repo.

Sorry I did not realize our patch had already been committed.