[webkit-reviews] review denied: [Bug 66094] [chromium] Expose "is pinned to left/right", "has horizontal/vertical scrollbar", "number of wheel handlers" to clients : [Attachment 103665] Patch

[bugzilla-daemon at webkit.org]

Darin Fisher (:fishd, Google) <fishd at chromium.org> has denied Nico Weber
<thakis at chromium.org>'s request for review:
Bug 66094: [chromium] Expose "is pinned to left/right", "has
horizontal/vertical scrollbar", "number of wheel handlers" to clients
https://bugs.webkit.org/show_bug.cgi?id=66094

Attachment 103665: Patch
https://bugs.webkit.org/attachment.cgi?id=103665&action=review

------- Additional Comments from Darin Fisher (:fishd, Google)
<fishd at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=103665&action=review


> Source/WebKit/chromium/public/WebFrame.h:169
> +    // Returns true if the frme is scrolled all the way to the right or
left.

nit: frme -> frame

> Source/WebKit/chromium/public/WebFrame.h:170
> +    virtual bool isPinnedToLeft() const = 0;

nit: these function names should be spell out more.  the names don't suggest
anything having to do with scrolling.  they should as WebFrame is a big
interface.

also, do we have to worry about how these are interpreted for RTL versus LTR?

> Source/WebKit/chromium/public/WebFrame.h:171
> +    virtual bool isPinnedToRight() const = 0;

nit: please preserve the two new lines between sections formatting

> Source/WebKit/chromium/public/WebViewClient.h:252
> +    virtual void numWheelEventHandlersChanged(unsigned) { }

nit: we usually do not use abbreviations like "num"

numberOfWheelEventHandlersChanged would be better, e.g.:

public> grep numberOf *.h
WebAnimationController.h:    virtual unsigned numberOfActiveAnimations() const
= 0;
WebAudioBus.h:	  WEBKIT_EXPORT void initialize(unsigned numberOfChannels,
size_t length, double sampleRate);
WebAudioBus.h:	  WEBKIT_EXPORT unsigned numberOfChannels() const;
WebAudioDevice.h:	 virtual void render(const WebVector<float*>&
audioData, size_t numberOfFrames) = 0;
WebGeolocationClientMock.h:    WEBKIT_EXPORT int
numberOfPendingPermissionRequests() const;
WebKitClient.h:    virtual WebAudioDevice* createAudioDevice(size_t bufferSize,
unsigned numberOfChannels, double sampleRate, WebAudioDevice::RenderCallback*)
{ return 0; }

Using "Count" as a suffix is also very common, so you might consider:

wheelEventHandlersCountChanged

My vote is for the numberOf variant.

> Source/WebKit/chromium/public/WebViewClient.h:253
>  

nit: please preserve the two new lines between sections formatting

> Source/WebKit/chromium/src/WebFrameImpl.cpp:598
> +    IntPoint minimumScrollPosition =
m_frame->view()->minimumScrollPosition();

perhaps it would be better to just reveal the minimum and maximum scroll
positions?
we already expose WebFrame::scrollOffset().  we often just expose the
primitives
that WebCore gives us since they allow for greater flexibility.