[webkit-reviews] review denied: [Bug 17172] Refactor platform checks in ScrollView.h : [Attachment 18904] updated patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Jan Alonzo <jmalonzo at gmail.com>'s
request for review:
Bug 17172: Refactor platform checks in ScrollView.h
http://bugs.webkit.org/show_bug.cgi?id=17172

Attachment 18904: updated patch
http://bugs.webkit.org/attachment.cgi?id=18904&action=edit

------- Additional Comments from Darin Adler <darin at apple.com>
This patch is OK, but there is a better way to do the
contentsToWindow/windowToContents thing. The inline implementation for non-Mac
platforms can be outside the ScrollView class. there's no reason we have to
muck up the declaration of those functions with #if just because the definition
needs to be different. In fact, I also think there's no good reason to make
them inline. It could just be in ScrollView.cpp.

 114	     // Other platforms can just implement these helper methods using
the corresponding point conversion methods.

And this comment really doesn't work when your rearrange the file. It's the
second half of a comment that was moved far away.

 118 #if PLATFORM(WIN) || PLATFORM(QT)
 119 
 120	     virtual void setParent(ScrollView*);	 
 121 
136122	       IntRect windowResizerRect();
137123	       bool resizerOverlapsContent() const;
138124	       void adjustOverlappingScrollbarCount(int overlapDelta);
 125 #endif // end PLATFORM(WIN) || PLATFORM(QT)

Seems ugly to have a space after the #if but not before the #endif.

 151	     PlatformScrollbar *horizontalScrollBar() const;
 152	     PlatformScrollbar *verticalScrollBar() const;

We put the * next to the type name, not the function name.

 177 #else // PLATFORM(WX)
228178	       HashSet<Widget*>* children();

Again, you have a space after the #if and before the #else, but no space after
the #else -- unnecessarily messy.

I'd like to see a better version of the patch. So I'm going to say review- for
now.

The complaints are just about formatting, but since the patch itself is largely
about formatting, I think it's OK for me to do that.