[webkit-reviews] review denied: [Bug 38077] [chromium] Prepare to making WebThemeEngine cross-platform : [Attachment 54211] Proposed patch

[bugzilla-daemon at webkit.org]

Darin Fisher (:fishd, Google) <fishd at chromium.org> has denied John Abd-El-Malek
<jam at chromium.org>'s request for review:
Bug 38077: [chromium] Prepare to making WebThemeEngine cross-platform
https://bugs.webkit.org/show_bug.cgi?id=38077

Attachment 54211: Proposed patch
https://bugs.webkit.org/attachment.cgi?id=54211&action=review

------- Additional Comments from Darin Fisher (:fishd, Google)
<fishd at chromium.org>
WebKit/chromium/public/WebThemeEngine.h:107
 +	// TODO(jam): make these pure virtual once this is rolled into Chromium
and
TODO(jam) should be FIXME in WebKit style.  also, it is actually preferred to
just provide default implementations for methods that are implemented by the
embedder.  that way it is easier to change the interface.  so, i would just
delete this TODO :)

WebKit/chromium/public/WebThemeEngine.h:79
 +	enum ThemePart {
can we do without the Theme prefix here?  these are already scoped by
WebThemeEngine which has the word Theme in it.	WebThemeEngine::Part* and
WebThemeEngine::State* seems sufficient to me.

WebKit/chromium/public/WebThemeEngine.h:109
 +	virtual void getSize(ThemePart, ThemeState, WebRect*) {}
what is the purpose of the getSize function?

WebKit/chromium/public/WebThemeEngine.h:72
 +	    const WebRect&, WebColor, bool fillContentArea, bool drawEdges) =
0;
how does the new API deal with this WebColor parameter and the bools?  will
there be more added to the ThemeExtraParams union for these parameters?