Bug 28128 - [Haiku] Modifications on WebCore.
: [Haiku] Modifications on WebCore.
Status: RESOLVED FIXED
: WebKit
WebCore Misc.
: 528+ (Nightly build)
: PC Other
: P2 Normal
Assigned To:
:
:
:
:
  Show dependency treegraph
 
Reported: 2009-08-09 06:00 PST by
Modified: 2009-08-12 16:12 PST (History)


Attachments
Modifications on WebCore to allow Haiku port. (8.64 KB, patch)
2009-08-09 06:25 PST, Maxime Simon
no flags Review Patch | Details | Formatted Diff | Diff
Patch to add tiny modifications on WebCore/platform/graphics files to allow Haiku port. (11.32 KB, patch)
2009-08-09 06:41 PST, Maxime Simon
eric: review-
Review Patch | Details | Formatted Diff | Diff
Patch to add modifications to the WebCore/platform/ files to allow the Haiku port. (8.86 KB, patch)
2009-08-09 06:56 PST, Maxime Simon
eric: review-
Review Patch | Details | Formatted Diff | Diff
Patch to modify some WebCore/platform/ files to allow the Haiku port. (8.24 KB, patch)
2009-08-11 07:22 PST, Maxime Simon
eric: review-
Review Patch | Details | Formatted Diff | Diff
Patch to modify some WebCore/platform/ files to allow the Haiku port. (8.24 KB, patch)
2009-08-11 07:58 PST, Maxime Simon
eric: review+
eric: commit‑queue+
Review Patch | Details | Formatted Diff | Diff
Patch to modify some WebCore/platform/ files to allow the Haiku port. (8.10 KB, patch)
2009-08-11 08:18 PST, Maxime Simon
no flags Review Patch | Details | Formatted Diff | Diff
Patch to modify some WebCore/platform/graphics files to allow the Haiku port. (11.36 KB, patch)
2009-08-11 10:26 PST, Maxime Simon
no flags Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2009-08-09 06:00:04 PST
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
------- Comment #1 From 2009-08-09 06:25:09 PST -------
Created an attachment (id=34416) [details]
Modifications on WebCore to allow Haiku port.
------- Comment #2 From 2009-08-09 06:41:09 PST -------
Created an attachment (id=34417) [details]
Patch to add tiny modifications on WebCore/platform/graphics files to allow Haiku port.
------- Comment #3 From 2009-08-09 06:56:07 PST -------
Created an attachment (id=34418) [details]
Patch to add modifications to the WebCore/platform/ files to allow the Haiku port.
------- Comment #4 From 2009-08-09 08:12:01 PST -------
(From update of attachment 34416 [details])
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. :)
------- Comment #5 From 2009-08-09 08:16:21 PST -------
(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?

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).
------- Comment #6 From 2009-08-09 08:19:53 PST -------
(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.
------- Comment #7 From 2009-08-10 20:01:11 PST -------
(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.
------- Comment #8 From 2009-08-10 20:22:50 PST -------
(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:

#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?
------- Comment #9 From 2009-08-11 05:55:25 PST -------
(In reply to comment #8)
> (In reply to comment #5)
> > (From update of attachment 34417 [details] [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.
------- Comment #10 From 2009-08-11 07:08:42 PST -------
(In reply to comment #7)
> (In reply to comment #6)
> > (From update of attachment 34418 [details] [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.
------- Comment #11 From 2009-08-11 07:22:36 PST -------
Created an attachment (id=34554) [details]
Patch to modify some WebCore/platform/ files to allow the Haiku port.
------- Comment #12 From 2009-08-11 07:55:52 PST -------
(From update of attachment 34554 [details])
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.
------- Comment #13 From 2009-08-11 07:58:17 PST -------
Created an attachment (id=34557) [details]
Patch to modify some WebCore/platform/ files to allow the Haiku port.
------- Comment #14 From 2009-08-11 08:00:59 PST -------
(From update of attachment 34557 [details])
 220     WidgetPrivate* m_data;

WidgetPrivate is never declared... so that won't compile, or?
------- Comment #15 From 2009-08-11 08:02:16 PST -------
(From update of attachment 34557 [details])
Nevermind.  Looks fine.
------- Comment #16 From 2009-08-11 08:04:14 PST -------
(In reply to comment #14)
> (From update of attachment 34557 [details] [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… :)
------- Comment #17 From 2009-08-11 08:05:57 PST -------
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....
------- Comment #18 From 2009-08-11 08:07:41 PST -------
(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.
------- Comment #19 From 2009-08-11 08:10:49 PST -------
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. :(
------- Comment #20 From 2009-08-11 08:14:11 PST -------
(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).
------- Comment #21 From 2009-08-11 08:18:14 PST -------
Created an attachment (id=34559) [details]
Patch to modify some WebCore/platform/ files to allow the Haiku port.
------- Comment #22 From 2009-08-11 09:16:26 PST -------
(From update of attachment 34559 [details])
LGTM.
------- Comment #23 From 2009-08-11 09:49:18 PST -------
I've updated https://trac.webkit.org/wiki/SuccessfulPortHowTo for Haiku (and other ports) benefit.  You should feel encouraged to update it further!
------- Comment #24 From 2009-08-11 10:26:05 PST -------
Created an attachment (id=34576) [details]
Patch to modify some WebCore/platform/graphics files to allow the Haiku port.
------- Comment #25 From 2009-08-11 10:28:56 PST -------
(In reply to comment #24)
> Created an attachment (id=34576) [details] [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.
------- Comment #26 From 2009-08-11 10:29:45 PST -------
(From update of attachment 34576 [details])
Looks OK.  I thought you decided Icon() was not used?
------- Comment #27 From 2009-08-11 10:30:19 PST -------
OK.  re-read the ChangeLog.
------- Comment #28 From 2009-08-12 13:35:38 PST -------
(From update of attachment 34416 [details])
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
------- Comment #29 From 2009-08-12 13:44:21 PST -------
(From update of attachment 34559 [details])
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
------- Comment #30 From 2009-08-12 13:55:56 PST -------
(From update of attachment 34576 [details])
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
------- Comment #31 From 2009-08-12 13:56:01 PST -------
All reviewed patches have been landed.  Closing bug.
------- Comment #32 From 2009-08-12 14:00:56 PST -------
#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.
------- Comment #33 From 2009-08-12 14:21:57 PST -------
(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.

See comment #4 on this bug.
------- Comment #34 From 2009-08-12 15:11:36 PST -------
(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.
------- Comment #35 From 2009-08-12 15:21:00 PST -------
(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] [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.
------- Comment #36 From 2009-08-12 16:12:33 PST -------
(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.