Bug 28129 - [Haiku] Adding three new files to WebCore/platform/haiku.
Summary: [Haiku] Adding three new files to WebCore/platform/haiku.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Other
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-08-09 07:04 PDT by Maxime Simon
Modified: 2009-08-15 00:31 PDT (History)
2 users (show)

See Also:


Attachments
Patch to add three new files to WebCore/platform/haiku/ (19.98 KB, patch)
2009-08-09 07:09 PDT, Maxime Simon
eric: review-
Details | Formatted Diff | Diff
Patch to add three new files to WebCore/platform/haiku/ (19.40 KB, patch)
2009-08-11 09:04 PDT, Maxime Simon
no flags Details | Formatted Diff | Diff
Adding two files: LocalizedString and Logging. (12.00 KB, patch)
2009-08-13 05:29 PDT, Maxime Simon
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Maxime Simon 2009-08-09 07:04:47 PDT
Here is a patch to add these three files to WebCore:
WebCore/platform/haiku/LocalizedStringsHaiku.cpp
WebCore/platform/haiku/LoggingHaiku.cpp
WebCore/platform/haiku/RenderThemeHaiku.cpp

Regards,
Maxime
Comment 1 Maxime Simon 2009-08-09 07:09:38 PDT
Created attachment 34419 [details]
Patch to add three new files to WebCore/platform/haiku/
Comment 2 Eric Seidel (no email) 2009-08-09 08:26:29 PDT
Comment on attachment 34419 [details]
Patch to add three new files to WebCore/platform/haiku/

No need to do explicit string construction here:
 37     return String("Submit");

return "Submit";
works just as well and is less to type/read.

 32 void InitializeLoggingChannelsIfNecessary()

probably needs a FIXME About how it really should read the logging config from a file.

I'm surprised this method name violates modern webkit style:
 32 void InitializeLoggingChannelsIfNecessary()
maybe it's just so old...?

Are you sure this is right?
 151     fontDescription.setIsAbsoluteSize(true);
 152     fontDescription.setGenericFamily(FontDescription::NoFamily);
 153     fontDescription.setSpecifiedSize(static_cast<int>(be_plain_font->Size()));
That seems wrong.  last-resort fallback fonts are determined by font family.  I also would have expect you to set a font face name there.
Also, that's completely ignoring the various CSS values passed in...

Why create a new theme for each page?
PassRefPtr<RenderTheme> RenderTheme::themeForPage(Page* page)
 180 {
 181     if (page)
 182         return RenderThemeHaiku::create();
 183 
 184     static RenderTheme* fallback = RenderThemeHaiku::create().releaseRef();
 185     return fallback;
 186 }
You don't seem to store any state on the themes.  SEems all pages should just use the "fallback".
Comment 3 Maxime Simon 2009-08-11 09:04:14 PDT
Created attachment 34563 [details]
Patch to add three new files to WebCore/platform/haiku/
Comment 4 Eric Seidel (no email) 2009-08-12 10:29:45 PDT
Comment on attachment 34563 [details]
Patch to add three new files to WebCore/platform/haiku/

None of those virtual methods should be inline.
Comment 5 Maxime Simon 2009-08-13 05:29:47 PDT
Created attachment 34734 [details]
Adding two files: LocalizedString and Logging.
Comment 6 Maxime Simon 2009-08-13 05:31:27 PDT
(In reply to comment #5)
> Created an attachment (id=34734) [details]
> Adding two files: LocalizedString and Logging.

As RenderTheme will require more code (at least a header), I will open another bug for it.
Comment 7 Eric Seidel (no email) 2009-08-14 17:31:26 PDT
Comment on attachment 34734 [details]
Adding two files: LocalizedString and Logging.

LGTM.
Comment 8 Eric Seidel (no email) 2009-08-14 17:58:32 PDT
Comment on attachment 34734 [details]
Adding two files: LocalizedString and Logging.

Rejecting patch 34734 from commit-queue.  This patch will require manual commit.

['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--quiet', '--exit-after-n-failures=1'] failed with exit code 1
Comment 9 Maxime Simon 2009-08-14 22:49:06 PDT
(In reply to comment #8)
> (From update of attachment 34734 [details])
> Rejecting patch 34734 from commit-queue.  This patch will require manual
> commit.
> 
> ['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--quiet',
> '--exit-after-n-failures=1'] failed with exit code 1

Uh? How can "run-webkit-tests" fail while we haven't changed it to test the Haiku port? And even more when the patch didn't touch other parts of WebKit. May it be another "commit" which makes the tests to fail?
Comment 10 Adam Barth 2009-08-15 00:27:26 PDT
Could be a flaky test.
Comment 11 Adam Barth 2009-08-15 00:31:19 PDT
Comment on attachment 34734 [details]
Adding two files: LocalizedString and Logging.

Clearing flags on attachment: 34734

Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	WebCore/ChangeLog
	A	WebCore/platform/haiku/LocalizedStringsHaiku.cpp
	A	WebCore/platform/haiku/LoggingHaiku.cpp
Committed r47317
	M	WebCore/ChangeLog
	A	WebCore/platform/haiku/LocalizedStringsHaiku.cpp
	A	WebCore/platform/haiku/LoggingHaiku.cpp
r47317 = c207727851d56028b4bb671ac9b358fc2ff4f1cb (trunk)
No changes between current HEAD and refs/remotes/trunk
Resetting to the latest refs/remotes/trunk
http://trac.webkit.org/changeset/47317
Comment 12 Adam Barth 2009-08-15 00:31:24 PDT
All reviewed patches have been landed.  Closing bug.