Summary: | [Haiku] Modifications on WebCore. | ||
---|---|---|---|
Product: | WebKit | Reporter: | Maxime Simon <simon.maxime> |
Component: | WebCore Misc. | Assignee: | Nobody <webkit-unassigned> |
Status: | RESOLVED FIXED | ||
Severity: | Normal | CC: | eric, leavengood, yong.li.webkit |
Priority: | P2 | ||
Version: | 528+ (Nightly build) | ||
Hardware: | PC | ||
OS: | Other | ||
Attachments: |
Description
Maxime Simon
2009-08-09 06:00:04 PDT
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. |