[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 bugzilla-daemon at webkit.org
Fri May 22 06:43:04 PDT 2009

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

Attachment 30002: new patch part 6 with ChangeLog

------- 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;

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

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

 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;

More information about the webkit-reviews mailing list