Bug 66094 - [chromium] Expose "min/max scroll size", "has horizontal/vertical scrollbar", "number of wheel handlers" to clients
Summary: [chromium] Expose "min/max scroll size", "has horizontal/vertical scrollbar",...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nico Weber
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-08-11 13:55 PDT by Nico Weber
Modified: 2011-08-11 17:19 PDT (History)
2 users (show)

See Also:


Attachments
Patch (6.67 KB, patch)
2011-08-11 13:59 PDT, Nico Weber
no flags Details | Formatted Diff | Diff
Patch (6.76 KB, patch)
2011-08-11 14:53 PDT, Nico Weber
no flags Details | Formatted Diff | Diff
Patch (7.17 KB, patch)
2011-08-11 15:31 PDT, Nico Weber
no flags Details | Formatted Diff | Diff
Patch (7.18 KB, patch)
2011-08-11 16:18 PDT, Nico Weber
no flags Details | Formatted Diff | Diff
Patch (7.06 KB, patch)
2011-08-11 16:31 PDT, Nico Weber
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Nico Weber 2011-08-11 13:55:29 PDT
[chromium] Expose "is pinned to left/right", "has horizontal/vertical scrollbar", "number of wheel handlers" to clients
Comment 1 Nico Weber 2011-08-11 13:59:22 PDT
Created attachment 103665 [details]
Patch
Comment 2 Darin Fisher (:fishd, Google) 2011-08-11 14:37:54 PDT
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.
Comment 3 Nico Weber 2011-08-11 14:52:16 PDT
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
Comment 4 Nico Weber 2011-08-11 14:53:31 PDT
Created attachment 103678 [details]
Patch
Comment 5 Darin Fisher (:fishd, Google) 2011-08-11 15:04:04 PDT
(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.
Comment 6 Darin Fisher (:fishd, Google) 2011-08-11 15:04:40 PDT
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.
Comment 7 Nico Weber 2011-08-11 15:29:56 PDT
> > 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.
Comment 8 Nico Weber 2011-08-11 15:31:23 PDT
Created attachment 103685 [details]
Patch
Comment 9 James Robinson 2011-08-11 16:16:15 PDT
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'?
Comment 10 Nico Weber 2011-08-11 16:18:28 PDT
(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.
Comment 11 Nico Weber 2011-08-11 16:18:51 PDT
Created attachment 103697 [details]
Patch
Comment 12 Nico Weber 2011-08-11 16:31:57 PDT
Created attachment 103701 [details]
Patch
Comment 13 WebKit Review Bot 2011-08-11 17:19:13 PDT
Comment on attachment 103701 [details]
Patch

Clearing flags on attachment: 103701

Committed r92893: <http://trac.webkit.org/changeset/92893>
Comment 14 WebKit Review Bot 2011-08-11 17:19:18 PDT
All reviewed patches have been landed.  Closing bug.