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
Created attachment 34416 [details] Modifications on WebCore to allow Haiku port.
Created attachment 34417 [details] Patch to add tiny modifications on WebCore/platform/graphics files to allow Haiku port.
Created attachment 34418 [details] Patch to add modifications to the WebCore/platform/ files to allow the Haiku port.
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. :)
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).
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.
(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.
(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?
(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.
(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.
Created attachment 34554 [details] Patch to modify some WebCore/platform/ files to allow the Haiku port.
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.
Created attachment 34557 [details] Patch to modify some WebCore/platform/ files to allow the Haiku port.
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?
Comment on attachment 34557 [details] Patch to modify some WebCore/platform/ files to allow the Haiku port. Nevermind. Looks fine.
(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… :)
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....
(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.
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. :(
(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).
Created attachment 34559 [details] Patch to modify some WebCore/platform/ files to allow the Haiku port.
Comment on attachment 34559 [details] Patch to modify some WebCore/platform/ files to allow the Haiku port. LGTM.
I've updated https://trac.webkit.org/wiki/SuccessfulPortHowTo for Haiku (and other ports) benefit. You should feel encouraged to update it further!
Created attachment 34576 [details] Patch to modify some WebCore/platform/graphics files to allow the Haiku port.
(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.
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?
OK. re-read the ChangeLog.
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
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
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
All reviewed patches have been landed. Closing bug.
#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.
(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.
(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.
(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.
(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.