Bug 59048 – Need to track whether overlay scrollbar is currently visible and in lower-righthand corner

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Beth Dakin]

<rdar://problem/9211232>

We need to know in the UI process if an overlay scrollbar is visible AND in the lower righthand corner. Patch coming in a moment.

[Beth Dakin]

Created attachment 90459 [details]
Patch

[WebKit Review Bot]

Attachment 90459 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/platform/mac/ScrollAnimatorMac.h:86:  The parameter name "showing" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 20 files


If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 90459 [details]
Patch

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

I wonder if the naming here can be improved. I pointed out one specific name I found problematic, but I also find it odd that everything in the internal naming differs from the external method:
scrollbar <-> scroller thumb (which is more accurate, I think)
showing <-> visible
south-east corner <-> window-resize… something

Maybe our naming should be more along the lines of “the scroll thumb is touching the corner”.

I was going to r+ but it occurred to me that I’m not sure this does the right thing in the two-scrollbars-neither-of-which-can-cover-the-corner case. Does it?

> Source/WebCore/page/ChromeClient.h:310
> +        virtual void setScrollbarIsShowingInSouthEastCorner(bool) { }

This name may not be the best for a function in a client interface, which is essentially a delegate. This might sound like the client is asked to show or hide something, but in reality it is just being notified. I don’t have a super-great name, but maybe something like scrollbarVisibilityInSouthEastCornerDidChange(bool).

[Maciej Stachowiak]

Comment on attachment 90459 [details]
Patch

Mitz was too kind to r-, but it looks like this either needs some revision, or significant additional explanation.

[Beth Dakin]

(In reply to comment #3)
> (From update of attachment 90459 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=90459&action=review
> 
> I wonder if the naming here can be improved. I pointed out one specific name I found problematic, but I also find it odd that everything in the internal naming differs from the external method:
> scrollbar <-> scroller thumb (which is more accurate, I think)
> showing <-> visible
> south-east corner <-> window-resize… something
> 
> Maybe our naming should be more along the lines of “the scroll thumb is touching the corner”.
> 

I knew the naming was going to be a problem at review-time, though I only predicted that south-east corner would be a problem. I agree that "scroller thumb" and "visible" would be an improvement over  "scrollbar" and "showing" respectively. However, I really hate the window-resize phrase, and I think south-east corner is much more expressive. I adopted south-east in the first place because I saw it in the code of another component, and I really liked it. That being said, I can probably come up with something more consistent. 

> I was going to r+ but it occurred to me that I’m not sure this does the right thing in the two-scrollbars-neither-of-which-can-cover-the-corner case. Does it?
> 

Hummm, there is a bug in this case with my patch. With my patch, you can never resize the window when the vertical scrollbar is fully scrolled, and that should not always be the case in the two-scrollbar situation. I will have to think on this. A bool may be insufficient here. 

> > Source/WebCore/page/ChromeClient.h:310
> > +        virtual void setScrollbarIsShowingInSouthEastCorner(bool) { }
> 
> This name may not be the best for a function in a client interface, which is essentially a delegate. This might sound like the client is asked to show or hide something, but in reality it is just being notified. I don’t have a super-great name, but maybe something like scrollbarVisibilityInSouthEastCornerDidChange(bool).

Will think on this too.

[Beth Dakin]

Created attachment 90589 [details]
Patch that sends a rect through the client

Comment on attachment 90589 [details]
Patch that sends a rect through the client

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

> Source/WebCore/platform/mac/ScrollAnimatorMac.mm:426
> +            _animator->setVisibleScrollerThumbRect(WebCore::IntRect());

I don’t think the WebCore:: qualifier is needed here!

> Source/WebCore/platform/mac/ScrollAnimatorMac.mm:506
> +    , m_visibleScrollerThumbRect(IntRect())

No need to initialize an IntRect member variable

> Source/WebKit2/UIProcess/WebPageProxy.cpp:156
> +    , m_visibleScrollerThumbRect(IntRect())

Unnecessary initialization

> Source/WebKit2/UIProcess/WebPageProxy.h:494
> +    const WebCore::IntRect& visibleScrollerThumbRect() const { return m_visibleScrollerThumbRect; }

I think typically functions that return an IntRect just return it by value, not a by (const) reference.

> Source/WebKit2/UIProcess/API/mac/WKView.mm:1599
> +    const IntRect& visibleThumbRect = _data->_page->visibleScrollerThumbRect();
> +    NSRect visibleThumbNSRect = NSMakeRect(visibleThumbRect.x(), visibleThumbRect.y(), visibleThumbRect.width(), visibleThumbRect.height());

IntRect has an operator NSRect() so I think the conversion can just happen automatically or with a cast.

[Beth Dakin]

Thanks Dan! I fixed all of the things you found, and committed the change with revision 84553.