[webkit-reviews] review denied: [Bug 28129] [Haiku] Adding three new files to WebCore/platform/haiku. : [Attachment 34419] Patch to add three new files to WebCore/platform/haiku/

[bugzilla-daemon at webkit.org]

Eric Seidel <eric at webkit.org> has denied Maxime Simon
<simon.maxime at gmail.com>'s request for review:
Bug 28129: [Haiku] Adding three new files to WebCore/platform/haiku.
https://bugs.webkit.org/show_bug.cgi?id=28129

Attachment 34419: Patch to add three new files to WebCore/platform/haiku/
https://bugs.webkit.org/attachment.cgi?id=34419&action=review

------- Additional Comments from Eric Seidel <eric at webkit.org>
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".