Bug 66094 – [chromium] Expose "min/max scroll size", "has horizontal/vertical scrollbar", "number of wheel handlers" to clients

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Nico Weber]

[chromium] Expose "is pinned to left/right", "has horizontal/vertical scrollbar", "number of wheel handlers" to clients

[Nico Weber]

Created attachment 103665 [details]
Patch

[Darin Fisher (:fishd, Google)]

Comment on attachment 103665 [details]
Patch

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.

[Nico Weber]

Comment on attachment 103665 [details]
Patch

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

Done

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

RTL should be fine (chrome/mac UI doesn't do RTL yet since the OS didn't support that pre 10.7, and WK2 has some reassuring comment about RTL wrt these functions somewhere)

>> Source/WebKit/chromium/public/WebFrame.h:171
>> +    virtual bool isPinnedToRight() const = 0;
> 
> nit: please preserve the two new lines between sections formatting

Done

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

It's called numWheelEventHandlersChanged() in ChromeClient. Since this is just the forwarded version of that, I figured it should have the same name.

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

a) That produces more IPC traffic than necessary if a page's js makes a page wider and scrolls to the right. That doesn't change the pinned state, but both scroll pos and max scroll pos
b) This matches WK2, which is nice in a way

[Nico Weber]

Created attachment 103678 [details]
Patch

[Darin Fisher (:fishd, Google)]

(In reply to comment #3)
> > also, do we have to worry about how these are interpreted for RTL versus LTR?
> 
> RTL should be fine (chrome/mac UI doesn't do RTL yet since the OS didn't support that pre 10.7, and WK2 has some reassuring comment about RTL wrt these functions somewhere)

OK

> >> 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.:
...
> It's called numWheelEventHandlersChanged() in ChromeClient. Since this is just the forwarded version of that, I figured it should have the same name.

That's not a good reason.  The ChromeClient name is probably wrong too.  We can at least try to make the WebKit API consistent.


> >> 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.
> 
> a) That produces more IPC traffic than necessary if a page's js makes a page wider and scrolls to the right. That doesn't change the pinned state, but both scroll pos and max scroll pos

I don't understand.  I was suggesting that you move the code you are adding here to the Chrome renderer process.  You can compute whether you are pinned to the left or the right if you had access to the minimum and maximum scroll offsets.  Obviously, this is true since you have shown how to do it in WebFrameImpl.cpp.

[Darin Fisher (:fishd, Google)]

Comment on attachment 103678 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=103678&action=review

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

nit: we use the term "scroll offset" not "scroll position" in the WebKit API.  See WebFrame::scrollOffset.  Please do not add additional names for the same thing.

[Nico Weber]

> > It's called numWheelEventHandlersChanged() in ChromeClient. Since this is just the forwarded version of that, I figured it should have the same name.
> 
> That's not a good reason.  The ChromeClient name is probably wrong too.  We can at least try to make the WebKit API consistent.

Done.

> > a) That produces more IPC traffic than necessary if a page's js makes a page wider and scrolls to the right. That doesn't change the pinned state, but both scroll pos and max scroll pos
> 
> I don't understand.  I was suggesting that you move the code you are adding here to the Chrome renderer process.  You can compute whether you are pinned to the left or the right if you had access to the minimum and maximum scroll offsets.  Obviously, this is true since you have shown how to do it in WebFrameImpl.cpp.

Ah, that makes a lot of sense! Done.

[Nico Weber]

Created attachment 103685 [details]
Patch

[James Robinson]

Comment on attachment 103685 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=103685&action=review

> Source/WebKit/chromium/src/ChromeClientImpl.cpp:971
> +void ChromeClientImpl::numWheelEventHandlersChanged(unsigned numWheelHandlers)

we don't abbreviate - this should be 'numberOfWheelHandlers'

> Source/WebKit/chromium/src/ChromeClientImpl.h:202
> +    virtual void numWheelEventHandlersChanged(unsigned);

this function seems badly named, shouldn't it be 'numberOfWheelHandlersChanged'?

[Nico Weber]

(In reply to comment #9)
> (From update of attachment 103685 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=103685&action=review
> 
> > Source/WebKit/chromium/src/ChromeClientImpl.cpp:971
> > +void ChromeClientImpl::numWheelEventHandlersChanged(unsigned numWheelHandlers)
> 
> we don't abbreviate - this should be 'numberOfWheelHandlers'
> 
> > Source/WebKit/chromium/src/ChromeClientImpl.h:202
> > +    virtual void numWheelEventHandlersChanged(unsigned);
> 
> this function seems badly named, shouldn't it be 'numberOfWheelHandlersChanged'?

This overrides a function. If I rename it here, it won't override the parent method any longer, and my code will be buggy. (And renaming the parent function too means I'll break 2 ports where I forget to rename their override). I did rename the variable name though.

[Nico Weber]

Created attachment 103697 [details]
Patch

[Nico Weber]

Created attachment 103701 [details]
Patch

[WebKit Review Bot]

Comment on attachment 103701 [details]
Patch

Clearing flags on attachment: 103701

Committed r92893: <http://trac.webkit.org/changeset/92893>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.