[webkit-reviews] review denied: [Bug 38854] [chromium] Add WebKitScrollbar interface to allow Chromium code to reuse the scrollbar code. : [Attachment 55593] fix spacing

[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 38854: [chromium] Add WebKitScrollbar interface to allow Chromium code to
reuse the scrollbar code.
https://bugs.webkit.org/show_bug.cgi?id=38854

Attachment 55593: fix spacing
https://bugs.webkit.org/attachment.cgi?id=55593&action=review

------- Additional Comments from Darin Fisher (:fishd, Google)
<fishd at chromium.org>
WebKit/chromium/ChangeLog:16
 +	    (WebKit::WebScrollbarImpl::getThickness):
nit: please re-create this ChangeLog given the API changes

WebKit/chromium/public/WebScrollbar.h:56
 +  // These values must match WebCore::ScrollGranularity values
nit: indentation

WebKit/chromium/public/WebScrollbar.h:62
 +  };
nit: indentation

WebKit/chromium/public/WebScrollbar.h:45
 +	// These values must match WebCore::ScrollbarOrientation values
nit: this comment could become stale.  if the WebCore enum changes,
we might end up writing a translation layer for the enums instead of
changing our API.  in general, i just avoid mentioning WebCore
implementation details in the WebKit API headers.

WebKit/chromium/public/WebScrollbar.h:70
 +	static int defaultThickness();
nit: add WEBKIT_API

WebKit/chromium/public/WebScrollbar.h:82
 +	virtual void setTotalSize(int size) = 0;
it would be helpful to explain what this means.  it is not so obvious
from reading these comments how the range of values and the total size
fit together.  ascii art might help.

WebKit/chromium/public/WebScrollbarClient.h:42
 +	virtual void positionChanged(WebScrollbar*) = 0;
nit: valueChanged

WebKit/chromium/src/AssertMatchingEnums.cpp:293
 +  #endif
nit: add a set for orientation?

WebKit/chromium/src/WebScrollbarImpl.cpp:81
 +	  m_scrollbar->invalidate();
nit: indentation

WebKit/chromium/src/WebScrollbarImpl.cpp:153
 +	    WebMouseEvent mousedown = *static_cast<const
WebMouseEvent*>(&event);
indentation

WebKit/chromium/src/WebScrollbarImpl.cpp:169
 +	}
indentation

WebKit/chromium/src/WebScrollbarImpl.cpp:184
 +	}
indentation

WebKit/chromium/src/WebScrollbarImpl.cpp:191
 +		return m_scrollbar->mouseExited();
lots of indentation issues in this file