[webkit-reviews] review denied: [Bug 23296] add Android platform-specific files to WebCore/platform : [Attachment 30002] new patch part 6 with ChangeLog

[bugzilla-daemon at webkit.org]

Eric Seidel <eric at webkit.org> has denied Feng Qian <feng at chromium.org>'s
request for review:
Bug 23296: add Android platform-specific files to WebCore/platform
https://bugs.webkit.org/show_bug.cgi?id=23296

Attachment 30002: new patch part 6 with ChangeLog
https://bugs.webkit.org/attachment.cgi?id=30002&action=review

------- Additional Comments from Eric Seidel <eric at webkit.org>
WK style is to prefer constants over preprocessor #defines:
 39 #define MAX_COMBO_HEIGHT 20

Sad this can't share more with Chromium.  I guess eventually maybe it can. 
Especially if a RenderThemeSkia is made (talk to Albert Wong)

It's nice to use longer variable names:
 62 static bool paintBrush(RenderSkinAndroid* rSkin, RenderObject* o, const
RenderObject::PaintInfo& i,  const IntRect& ir)

Please use C++ style constructor initialization and OwnPtrs so you don't ahve
to do manual memory management:
 83	m_radio = new RenderSkinRadio(false);
 90	delete m_radio;

Style:
     if (!element->isEnabled()) {
 203	     RenderSkinButton::Draw(getCanvasFromInfo(i), ir,
RenderSkinAndroid::kDisabled);
 204	 } else {

Style:
46     if (o->isMenuList()) {
 247	     return paintCombo(o, i, ir);
 248	 }

Style:
 38	ThemeData() :m_part(0), m_state(0) {}

We don't name arguments in headers unless the names add clarity:
 51	virtual bool supportsFocusRing(const RenderStyle* style) const;

As noted above, these should be OwnPtrs:
 101	 RenderSkinRadio*    m_radio;