| Summary: | Adding a bunch of Haiku-specific files for WebCore. | ||
|---|---|---|---|
| Product: | WebKit | Reporter: | Maxime Simon <simon.maxime> |
| Component: | WebCore Misc. | Assignee: | Nobody <webkit-unassigned> |
| Status: | RESOLVED FIXED | ||
| Severity: | Normal | CC: | leavengood |
| Priority: | P2 | ||
| Version: | 528+ (Nightly build) | ||
| Hardware: | PC | ||
| OS: | Other | ||
| Attachments: | |||
|
Description
Maxime Simon
2009-08-07 13:09:45 PDT
Created attachment 34316 [details]
Patch to add two Haiku-specfic files to WebCore: TemporaryLinkStubs.cpp and WidgetHaiku.cpp
Created attachment 34319 [details]
Patch to add four Haiku-specific files for WebCore: PopupMenuHaiku.cpp, ScreenHaiku.cpp, SearchPopupMenuHaiku.cpp and SoundHaiku.cpp
Created attachment 34322 [details]
Patch to add two Haiku-specific files for WebCore: SharedTimerHaiku.h and SharedTimerHaiku.cpp
Comment on attachment 34316 [details]
Patch to add two Haiku-specfic files to WebCore: TemporaryLinkStubs.cpp and WidgetHaiku.cpp
LGTM.
Comment on attachment 34319 [details]
Patch to add four Haiku-specific files for WebCore: PopupMenuHaiku.cpp, ScreenHaiku.cpp, SearchPopupMenuHaiku.cpp and SoundHaiku.cpp
In general "cs" is not a good variable name. Please refer to the webkit style guidelines for variable naming.
62 color_space cs = screen.ColorSpace();
This looks OK though, and not worth another round of review for just that.
Thank you for splitting these up, it makes reviewing *much* easier. :) (and much less waiting time for you).
Comment on attachment 34322 [details]
Patch to add two Haiku-specific files for WebCore: SharedTimerHaiku.h and SharedTimerHaiku.cpp
We prefer constants over defines:
38 #define FIRE_MESSAGE 'fire'
What does "sleepyTime" mean?
67 bigtime_t sleepyTime = static_cast<bigtime_t>((m_fireTime - WTF::currentTime()) * 1000000);
bigtime_t sleepTimeInSeconds would be a better name?
This needs a comment:
69 if (sleepyTime < 0)
70 sleepyTime = 0;
// If the fire time has already passed, fire immediately.
or similar.
Style:
91 BLooper *looper = BLooper::LooperForThread(find_thread(0));
Otherwise this looks sane enough. You might want to post the new patch on a new bug, since the other patches were approved.
Comment on attachment 34316 [details] Patch to add two Haiku-specfic files to WebCore: TemporaryLinkStubs.cpp and WidgetHaiku.cpp Clearing review flag on attachment: 34316 Committing to http://svn.webkit.org/repository/webkit/trunk ... M WebCore/ChangeLog A WebCore/platform/haiku/TemporaryLinkStubs.cpp A WebCore/platform/haiku/WidgetHaiku.cpp Committed r46923 M WebCore/ChangeLog A WebCore/platform/haiku/TemporaryLinkStubs.cpp A WebCore/platform/haiku/WidgetHaiku.cpp r46923 = fcf9bf19107405d715c42008c807645900f888e1 (trunk) No changes between current HEAD and refs/remotes/trunk Resetting to the latest refs/remotes/trunk http://trac.webkit.org/changeset/46923 Comment on attachment 34319 [details] Patch to add four Haiku-specific files for WebCore: PopupMenuHaiku.cpp, ScreenHaiku.cpp, SearchPopupMenuHaiku.cpp and SoundHaiku.cpp Clearing review flag on attachment: 34319 Committing to http://svn.webkit.org/repository/webkit/trunk ... M WebCore/ChangeLog A WebCore/platform/haiku/PopupMenuHaiku.cpp A WebCore/platform/haiku/ScreenHaiku.cpp A WebCore/platform/haiku/SearchPopupMenuHaiku.cpp A WebCore/platform/haiku/SoundHaiku.cpp Committed r46924 M WebCore/ChangeLog A WebCore/platform/haiku/SearchPopupMenuHaiku.cpp A WebCore/platform/haiku/ScreenHaiku.cpp A WebCore/platform/haiku/SoundHaiku.cpp A WebCore/platform/haiku/PopupMenuHaiku.cpp r46924 = 4c9b811d2965864e940504a2bca5bbc347f174a6 (trunk) No changes between current HEAD and refs/remotes/trunk Resetting to the latest refs/remotes/trunk http://trac.webkit.org/changeset/46924 |