Bug 28129 – [Haiku] Adding three new files to WebCore/platform/haiku.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Maxime Simon]

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

[Maxime Simon]

Created attachment 34419 [details]
Patch to add three new files to WebCore/platform/haiku/

[Eric Seidel (no email)]

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".

[Maxime Simon]

Created attachment 34563 [details]
Patch to add three new files to WebCore/platform/haiku/

[Eric Seidel (no email)]

Comment on attachment 34563 [details]
Patch to add three new files to WebCore/platform/haiku/

None of those virtual methods should be inline.

[Maxime Simon]

Created attachment 34734 [details]
Adding two files: LocalizedString and Logging.

[Maxime Simon]

(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.

[Eric Seidel (no email)]

Comment on attachment 34734 [details]
Adding two files: LocalizedString and Logging.

LGTM.

[Eric Seidel (no email)]

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

[Maxime Simon]

(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?

[Adam Barth]

Could be a flaky test.

[Adam Barth]

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

[Adam Barth]

All reviewed patches have been landed.  Closing bug.