Allow objects without scrollbars to be scrollable

Allow objects without scrollbars to be scrollable

This CL is a refactoring of scrolling code to decouple the concept of 
'scrollable' from 'has scrollbars'. It adds a pure virtual userInputScrollable(orientation) method. Subclasses define
this to specify whether an object is allowed to scroll (e.g. 
A ScrollView can be scrolled if it has scroll mode Auto or Always). 

Also removed a few unused scrolling-related methods.

BUG=247055
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=155441

[aelias_OOO_until_Jul13, 2013-06-18 00:54:47]

https://codereview.chromium.org/16982005/diff/3001/Source/core/platform/Scrol...
File Source/core/platform/ScrollableArea.cpp (right):

https://codereview.chromium.org/16982005/diff/3001/Source/core/platform/Scrol...
Source/core/platform/ScrollableArea.cpp:119: // TODO: Where should we get the
step sizes here?
It looks like the step sizes should be moved out of the scrollbar object as
well.  How about making them part of the ScrollableArea?

https://codereview.chromium.org/16982005/diff/3001/Source/core/platform/Scrol...
File Source/core/platform/ScrollableArea.h (right):

https://codereview.chromium.org/16982005/diff/3001/Source/core/platform/Scrol...
Source/core/platform/ScrollableArea.h:178: virtual bool
isHorizontallyScrollable() const = 0;
Instead of making each subclass implement this, how about just checking if
visibleContentRect().size() is smaller than scrollSize()?

[bokan, 2013-06-18 23:03:26]

https://chromiumcodereview.appspot.com/16982005/diff/3001/Source/core/platfor...
File Source/core/platform/ScrollableArea.cpp (right):

https://chromiumcodereview.appspot.com/16982005/diff/3001/Source/core/platfor...
Source/core/platform/ScrollableArea.cpp:119: // TODO: Where should we get the
step sizes here?
On 2013/06/18 00:54:47, aelias wrote:
> It looks like the step sizes should be moved out of the scrollbar object as
> well.  How about making them part of the ScrollableArea?
Done, though ScrollableArea has a compile time assert about keeping its size
small. Is it ok that we're adding 24 bytes to the class size?

https://chromiumcodereview.appspot.com/16982005/diff/3001/Source/core/platfor...
File Source/core/platform/ScrollableArea.h (right):

https://chromiumcodereview.appspot.com/16982005/diff/3001/Source/core/platfor...
Source/core/platform/ScrollableArea.h:178: virtual bool
isHorizontallyScrollable() const = 0;
On 2013/06/18 00:54:47, aelias wrote:
> Instead of making each subclass implement this, how about just checking if
> visibleContentRect().size() is smaller than scrollSize()?
The meaning of this method is more "is the object allowed to scroll?", rather
than "is there overflow?". E.g. A RenderLayer isHorizontallyScrollable if it's
overflowX style is OAUTO or OSCROLL (and later if we want scrollable input text
boxes we'd add an "or isTextInputBox()"). I think it'll be different for each
subclass - though I think maybe renaming the method to something like
horizontalScrollEnabled() will reflect that better.

[aelias_OOO_until_Jul13, 2013-06-19 02:55:24]

https://chromiumcodereview.appspot.com/16982005/diff/3001/Source/core/platfor...
File Source/core/platform/ScrollableArea.cpp (right):

https://chromiumcodereview.appspot.com/16982005/diff/3001/Source/core/platfor...
Source/core/platform/ScrollableArea.cpp:119: // TODO: Where should we get the
step sizes here?
On 2013/06/18 23:03:27, bokan wrote:
> On 2013/06/18 00:54:47, aelias wrote:
> > It looks like the step sizes should be moved out of the scrollbar object as
> > well.  How about making them part of the ScrollableArea?
> Done, though ScrollableArea has a compile time assert about keeping its size
> small. Is it ok that we're adding 24 bytes to the class size?

Ah, no, that's not okay, sorry about that suggestion.

It looks like we could make the "step" information virtual getters in
ScrollableArea and have the subclasses implement them.  We could avoid separate
storage entirely and compute them on the fly in the getters.

https://chromiumcodereview.appspot.com/16982005/diff/3001/Source/core/platfor...
File Source/core/platform/ScrollableArea.h (right):

https://chromiumcodereview.appspot.com/16982005/diff/3001/Source/core/platfor...
Source/core/platform/ScrollableArea.h:178: virtual bool
isHorizontallyScrollable() const = 0;
On 2013/06/18 23:03:27, bokan wrote:
> On 2013/06/18 00:54:47, aelias wrote:
> > Instead of making each subclass implement this, how about just checking if
> > visibleContentRect().size() is smaller than scrollSize()?
> The meaning of this method is more "is the object allowed to scroll?", rather
> than "is there overflow?". E.g. A RenderLayer isHorizontallyScrollable if it's
> overflowX style is OAUTO or OSCROLL (and later if we want scrollable input
text
> boxes we'd add an "or isTextInputBox()"). I think it'll be different for each
> subclass - though I think maybe renaming the method to something like
> horizontalScrollEnabled() will reflect that better.

So it really means scrollable by user input events.  Maybe rename to "bool
userInputScrollable(Orientation orientation)"?

[bokan, 2013-06-19 18:56:59]

https://chromiumcodereview.appspot.com/16982005/diff/3001/Source/core/platfor...
File Source/core/platform/ScrollableArea.cpp (right):

https://chromiumcodereview.appspot.com/16982005/diff/3001/Source/core/platfor...
Source/core/platform/ScrollableArea.cpp:119: // TODO: Where should we get the
step sizes here?
On 2013/06/19 02:55:24, aelias wrote:
> On 2013/06/18 23:03:27, bokan wrote:
> > On 2013/06/18 00:54:47, aelias wrote:
> > > It looks like the step sizes should be moved out of the scrollbar object
as
> > > well.  How about making them part of the ScrollableArea?
> > Done, though ScrollableArea has a compile time assert about keeping its size
> > small. Is it ok that we're adding 24 bytes to the class size?
> 
> Ah, no, that's not okay, sorry about that suggestion.
> 
> It looks like we could make the "step" information virtual getters in
> ScrollableArea and have the subclasses implement them.  We could avoid
separate
> storage entirely and compute them on the fly in the getters.
Done.

https://chromiumcodereview.appspot.com/16982005/diff/3001/Source/core/platfor...
File Source/core/platform/ScrollableArea.h (right):

https://chromiumcodereview.appspot.com/16982005/diff/3001/Source/core/platfor...
Source/core/platform/ScrollableArea.h:178: virtual bool
isHorizontallyScrollable() const = 0;
On 2013/06/19 02:55:24, aelias wrote:
> On 2013/06/18 23:03:27, bokan wrote:
> > On 2013/06/18 00:54:47, aelias wrote:
> > > Instead of making each subclass implement this, how about just checking if
> > > visibleContentRect().size() is smaller than scrollSize()?
> > The meaning of this method is more "is the object allowed to scroll?",
rather
> > than "is there overflow?". E.g. A RenderLayer isHorizontallyScrollable if
it's
> > overflowX style is OAUTO or OSCROLL (and later if we want scrollable input
> text
> > boxes we'd add an "or isTextInputBox()"). I think it'll be different for
each
> > subclass - though I think maybe renaming the method to something like
> > horizontalScrollEnabled() will reflect that better.
> 
> So it really means scrollable by user input events.  Maybe rename to "bool
> userInputScrollable(Orientation orientation)"?
Done.

[aelias_OOO_until_Jul13, 2013-06-19 22:25:12]

lgtm

https://chromiumcodereview.appspot.com/16982005/diff/14001/Source/core/platfo...
File Source/core/platform/ScrollableArea.cpp (right):

https://chromiumcodereview.appspot.com/16982005/diff/14001/Source/core/platfo...
Source/core/platform/ScrollableArea.cpp:104: 
nit: accidental newline

https://chromiumcodereview.appspot.com/16982005/diff/14001/Source/core/platfo...
Source/core/platform/ScrollableArea.cpp:415: int
ScrollableArea::pageStep(ScrollbarOrientation orientation) const
This code appears specific to ScrollView so I suggest making it pure virtual and
moving the implementation to ScrollView.cpp.

https://chromiumcodereview.appspot.com/16982005/diff/14001/Source/core/platfo...
File Source/core/platform/ScrollableArea.h (right):

https://chromiumcodereview.appspot.com/16982005/diff/14001/Source/core/platfo...
Source/core/platform/ScrollableArea.h:109: virtual int
scrollPosition(Scrollbar*) const = 0;
It's no longer appropriate for this call to take a Scrollbar pointer, and this
is also redundant with the other scrollPosition().

Even worse, looking at the implementations, scrollPosition(ScrollBar*) is
relative to scrollOrigin whereas scrollPosition(void) is not.  FYI, scrollOrigin
expresses the top-left origin of the initial containing block.  Because RTL
pages initialize scrolled to the right, scrollOrigin().x() can be positive and
minimumScrollPosition().x() can be negative on RTL pages (but on non-RTL pages,
it will always be zero).

It looks like the only caller is in Scrollbar.cpp.  So please delete this method
and add a private method in Scrollbar.cpp:

int Scrollbar::currentPos() {
    if (!m_scrollableArea)
        return 0;
    if (m_orientation == HorizontalScrollbar) {
        m_scrollableArea->scrollPosition().x() +
m_scrollableArea.scrollOrigin().x();
    } else if (m_orientation == VerticalScrollbar) {
        m_scrollableArea->scrollPosition().y() +
m_scrollableArea.scrollOrigin().y();
    }
    return 0;
}

https://chromiumcodereview.appspot.com/16982005/diff/14001/Source/core/platfo...
Source/core/platform/ScrollableArea.h:191: 
nit: accidental newline

https://chromiumcodereview.appspot.com/16982005/diff/14001/Source/core/render...
File Source/core/rendering/RenderListBox.cpp (right):

https://chromiumcodereview.appspot.com/16982005/diff/14001/Source/core/render...
Source/core/rendering/RenderListBox.cpp:638: return IntPoint(x, y);
nit: return IntPoint(0, m_indexOffset);

[aelias_OOO_until_Jul13, 2013-06-19 22:26:44]

Sorry, not lgtm yet, I clicked on the "quick lgtm" button by accident :(.  But
anyway, this is a good incremental improvement.  Go ahead and add jamesr@ for
OWNERS review after you've applied my comments above.

[bokan, 2013-06-19 23:50:58]

https://chromiumcodereview.appspot.com/16982005/diff/14001/Source/core/platfo...
File Source/core/platform/ScrollableArea.cpp (right):

https://chromiumcodereview.appspot.com/16982005/diff/14001/Source/core/platfo...
Source/core/platform/ScrollableArea.cpp:104: 
On 2013/06/19 22:25:12, aelias wrote:
> nit: accidental newline

Done.

https://chromiumcodereview.appspot.com/16982005/diff/14001/Source/core/platfo...
Source/core/platform/ScrollableArea.cpp:415: int
ScrollableArea::pageStep(ScrollbarOrientation orientation) const
On 2013/06/19 22:25:12, aelias wrote:
> This code appears specific to ScrollView so I suggest making it pure virtual
and
> moving the implementation to ScrollView.cpp.
Done.

https://chromiumcodereview.appspot.com/16982005/diff/14001/Source/core/platfo...
File Source/core/platform/ScrollableArea.h (right):

https://chromiumcodereview.appspot.com/16982005/diff/14001/Source/core/platfo...
Source/core/platform/ScrollableArea.h:109: virtual int
scrollPosition(Scrollbar*) const = 0;
On 2013/06/19 22:25:12, aelias wrote:
> It's no longer appropriate for this call to take a Scrollbar pointer, and this
> is also redundant with the other scrollPosition().
> 
> Even worse, looking at the implementations, scrollPosition(ScrollBar*) is
> relative to scrollOrigin whereas scrollPosition(void) is not.  FYI,
scrollOrigin
> expresses the top-left origin of the initial containing block.  Because RTL
> pages initialize scrolled to the right, scrollOrigin().x() can be positive and
> minimumScrollPosition().x() can be negative on RTL pages (but on non-RTL
pages,
> it will always be zero).
> 
> It looks like the only caller is in Scrollbar.cpp.  So please delete this
method
> and add a private method in Scrollbar.cpp:
> 
> int Scrollbar::currentPos() {
>     if (!m_scrollableArea)
>         return 0;
>     if (m_orientation == HorizontalScrollbar) {
>         m_scrollableArea->scrollPosition().x() +
> m_scrollableArea.scrollOrigin().x();
>     } else if (m_orientation == VerticalScrollbar) {
>         m_scrollableArea->scrollPosition().y() +
> m_scrollableArea.scrollOrigin().y();
>     }
>     return 0;
> }
Done (there's an existing currentPos() accessor for m_currentPos, I called the
new method scrollableAreaCurrentPos()). 

Question about scroll origin: If the page is RTL, does that mean that
scrollOffsets are negative and maximumScrollPosition is 0?

https://chromiumcodereview.appspot.com/16982005/diff/14001/Source/core/platfo...
Source/core/platform/ScrollableArea.h:191: 
On 2013/06/19 22:25:12, aelias wrote:
> nit: accidental newline

Done.

https://chromiumcodereview.appspot.com/16982005/diff/14001/Source/core/render...
File Source/core/rendering/RenderListBox.cpp (right):

https://chromiumcodereview.appspot.com/16982005/diff/14001/Source/core/render...
Source/core/rendering/RenderListBox.cpp:638: return IntPoint(x, y);
On 2013/06/19 22:25:12, aelias wrote:
> nit: return IntPoint(0, m_indexOffset);

Done.

[aelias_OOO_until_Jul13, 2013-06-20 00:04:14]

lgtm, thanks.

https://chromiumcodereview.appspot.com/16982005/diff/14001/Source/core/platfo...
File Source/core/platform/ScrollableArea.h (right):

https://chromiumcodereview.appspot.com/16982005/diff/14001/Source/core/platfo...
Source/core/platform/ScrollableArea.h:109: virtual int
scrollPosition(Scrollbar*) const = 0;
On 2013/06/19 23:50:58, bokan wrote:
> Done (there's an existing currentPos() accessor for m_currentPos, I called the
> new method scrollableAreaCurrentPos()). 
> 
> Question about scroll origin: If the page is RTL, does that mean that
> scrollOffsets are negative and maximumScrollPosition is 0?

Not quite; it means maximumScrollPosition.x() == viewportWidth and
minimumScrollPosition.x() == -(contentsWidth - viewportWidth).

https://chromiumcodereview.appspot.com/16982005/diff/15023/Source/core/platfo...
File Source/core/platform/ScrollableArea.h (right):

https://chromiumcodereview.appspot.com/16982005/diff/15023/Source/core/platfo...
Source/core/platform/ScrollableArea.h:43: static int pixelsPerLineStep() {
return 40; }
nit: move the implementations into ScrollableArea.cpp

[aelias_OOO_until_Jul13, 2013-06-20 00:09:40]

https://chromiumcodereview.appspot.com/16982005/diff/14001/Source/core/platfo...
File Source/core/platform/ScrollableArea.h (right):

https://chromiumcodereview.appspot.com/16982005/diff/14001/Source/core/platfo...
Source/core/platform/ScrollableArea.h:109: virtual int
scrollPosition(Scrollbar*) const = 0;
On 2013/06/20 00:04:14, aelias wrote:
> On 2013/06/19 23:50:58, bokan wrote:
> > Done (there's an existing currentPos() accessor for m_currentPos, I called
the
> > new method scrollableAreaCurrentPos()). 
> > 
> > Question about scroll origin: If the page is RTL, does that mean that
> > scrollOffsets are negative and maximumScrollPosition is 0?
> 
> Not quite; it means maximumScrollPosition.x() == viewportWidth and
> minimumScrollPosition.x() == -(contentsWidth - viewportWidth).

Err, actually what you said was correct, since we are talking about maximum
scroll position; I meant the right side of the scrollable bounds is
viewportWidth.

[trchen, 2013-06-20 01:46:42]

https://chromiumcodereview.appspot.com/16982005/diff/15023/Source/core/platfo...
File Source/core/platform/Scrollbar.cpp (right):

https://chromiumcodereview.appspot.com/16982005/diff/15023/Source/core/platfo...
Source/core/platform/Scrollbar.cpp:593: return
m_scrollableArea->scrollPosition().x() + m_scrollableArea->scrollOrigin().x();
I recommend phasing out scrollOrigin(). Please use scrollPosition() -
minimumScrollPosition() instead.

https://chromiumcodereview.appspot.com/16982005/diff/15023/Source/core/render...
File Source/core/rendering/RenderLayer.cpp (right):

https://chromiumcodereview.appspot.com/16982005/diff/15023/Source/core/render...
Source/core/rendering/RenderLayer.cpp:2521: return m_scrollSize.height() -
box->pixelSnappedClientHeight();
Whoa. scrollSize() looks like an accessor to m_scrollSize, when it's not.

IMO scrollSize() should be equivalent to (maximumScrollPosition() -
minimumScrollPosition()). For m_scrollSize, I think we should renamed it to
m_overflowRect, and cache box->layoutOverflowRect() at
RenderLayer::computeScrollDimensions(). I'm even wondering if we really need to
cache scroll dimensions, but it is out of scope of this CL.

[bokan, 2013-06-20 19:00:15]

https://chromiumcodereview.appspot.com/16982005/diff/15023/Source/core/platfo...
File Source/core/platform/ScrollableArea.cpp (right):

https://chromiumcodereview.appspot.com/16982005/diff/15023/Source/core/platfo...
Source/core/platform/ScrollableArea.cpp:370: IntPoint
ScrollableArea::minimumScrollPosition() const
Shouldn't the default min/maxScrollPositions here take into account the
scrollOrigin?

https://chromiumcodereview.appspot.com/16982005/diff/15023/Source/core/platfo...
File Source/core/platform/ScrollableArea.h (right):

https://chromiumcodereview.appspot.com/16982005/diff/15023/Source/core/platfo...
Source/core/platform/ScrollableArea.h:43: static int pixelsPerLineStep() {
return 40; }
On 2013/06/20 00:04:14, aelias wrote:
> nit: move the implementations into ScrollableArea.cpp

Done.

https://chromiumcodereview.appspot.com/16982005/diff/15023/Source/core/platfo...
File Source/core/platform/Scrollbar.cpp (right):

https://chromiumcodereview.appspot.com/16982005/diff/15023/Source/core/platfo...
Source/core/platform/Scrollbar.cpp:593: return
m_scrollableArea->scrollPosition().x() + m_scrollableArea->scrollOrigin().x();
On 2013/06/20 01:46:43, trchen wrote:
> I recommend phasing out scrollOrigin(). Please use scrollPosition() -
> minimumScrollPosition() instead.

Done.

https://chromiumcodereview.appspot.com/16982005/diff/15023/Source/core/render...
File Source/core/rendering/RenderLayer.cpp (right):

https://chromiumcodereview.appspot.com/16982005/diff/15023/Source/core/render...
Source/core/rendering/RenderLayer.cpp:2521: return m_scrollSize.height() -
box->pixelSnappedClientHeight();
On 2013/06/20 01:46:43, trchen wrote:
> Whoa. scrollSize() looks like an accessor to m_scrollSize, when it's not.
> 
> IMO scrollSize() should be equivalent to (maximumScrollPosition() -
> minimumScrollPosition()). For m_scrollSize, I think we should renamed it to
> m_overflowRect, and cache box->layoutOverflowRect() at
> RenderLayer::computeScrollDimensions(). I'm even wondering if we really need
to
> cache scroll dimensions, but it is out of scope of this CL.

Done.

[jamesr, 2013-06-20 19:04:06]

I don't fully understand the motivation here.  Scrollability is a per-axis
concept, not boolean.  What's the advantage of having scrollability in an axis
be independent of scrollbars in that axis?

[aelias_OOO_until_Jul13, 2013-06-20 19:25:31]

On 2013/06/20 19:04:06, jamesr wrote:
> I don't fully understand the motivation here.  Scrollability is a per-axis
> concept, not boolean.  What's the advantage of having scrollability in an axis
> be independent of scrollbars in that axis?

I advocated this, so I'll explain.  As a matter of principle, scrollbars are a
superficial UI widget whose existence or not shouldn't affect core behaviors
(taking an MVC attitude).  Practically, several things came up:
1. The immediate impetus was the change https://codereview.chromium.org/15725009
to touch-scroll text input boxes which apparently don't necessarily have
scrollbars.  It applied the same separation of scrollbars/scrollability but just
to that specific element type, so I said we should do it more generally.
2. ScrollView::updateScrollbars() is a giant tangle that doesn't only update
scrollbars and this is a step towards being able to clean that up.
3. Android sometimes has special reasons for disabling scrollbars -- this will
come up in WebView due to styling problems for example.

[trchen, 2013-06-20 19:25:33]

On 2013/06/20 19:04:06, jamesr wrote:
> I don't fully understand the motivation here.  Scrollability is a per-axis
> concept, not boolean.  What's the advantage of having scrollability in an axis
> be independent of scrollbars in that axis?

The motivation is to allow things to scroll without scrollbars. There are a few
use cases for that. For example we want to be able to gesture scroll single line
text boxes, although it doesn't have a scrollbar. Another example is we want to
inhibit main frame (outer) scrollbars, while still allow main frame to scroll.

And most of all, it is weird to let Scrollbar class to handle so much scrolling
logic. IMO Scrollbar should be a slave of ScrollableArea that only handles the
rendering.

https://chromiumcodereview.appspot.com/16982005/diff/15023/Source/core/platfo...
File Source/core/platform/ScrollableArea.cpp (right):

https://chromiumcodereview.appspot.com/16982005/diff/15023/Source/core/platfo...
Source/core/platform/ScrollableArea.cpp:370: IntPoint
ScrollableArea::minimumScrollPosition() const
On 2013/06/20 19:00:16, bokan wrote:
> Shouldn't the default min/maxScrollPositions here take into account the
> scrollOrigin?

Yes. Better if we can make them pure virtual. ;)

[bokan, 2013-07-15 22:27:43]

Needs review by an OWNER

[tdanderson, 2013-07-15 23:17:43]

I found a few nits but otherwise lgtm.

Have you verified that the behavior for scrolling divs which have
overflow-{x,y}:hidden still works as intended?

https://codereview.chromium.org/16982005/diff/37001/Source/core/platform/Scro...
File Source/core/platform/ScrollView.cpp (right):

https://codereview.chromium.org/16982005/diff/37001/Source/core/platform/Scro...
Source/core/platform/ScrollView.cpp:288: // If no scrollbars are present, it
does not indicate content is not be scrollable.
I know you didn't write this comment in the first place, but can you please
reword (preferably with fewer negatives)?

https://codereview.chromium.org/16982005/diff/37001/Source/core/platform/Scro...
File Source/core/platform/ScrollableArea.cpp (right):

https://codereview.chromium.org/16982005/diff/37001/Source/core/platform/Scro...
Source/core/platform/ScrollableArea.cpp:62: return 0.875f;
I don't think the style guide explicitly mentions this, but I think it's better
to give these constants names (either at the top of this file or in a separate
file if there are enough scrolling-related constants that need to be kept track
of) instead of hard-coding return values. Maybe that's just personal preference,
though.

https://codereview.chromium.org/16982005/diff/37001/Source/core/platform/Scro...
Source/core/platform/ScrollableArea.cpp:421: return 1.0f;
The style guide says to omit the .0f since you're not doing floating point math.

https://codereview.chromium.org/16982005/diff/37001/Source/core/rendering/Ren...
File Source/core/rendering/RenderLayer.cpp (right):

https://codereview.chromium.org/16982005/diff/37001/Source/core/rendering/Ren...
Source/core/rendering/RenderLayer.cpp:2515: IntSize scrollDims =
maximumScrollPosition() - minimumScrollPosition();
scrollDims -> scrollDimensions

[bokan, 2013-07-22 15:08:20]

On 2013/07/15 23:17:43, tdanderson wrote:
> I found a few nits but otherwise lgtm.
> 
> Have you verified that the behavior for scrolling divs which have
> overflow-{x,y}:hidden still works as intended?
Yup, still works.

> 
>
https://codereview.chromium.org/16982005/diff/37001/Source/core/platform/Scro...
> File Source/core/platform/ScrollView.cpp (right):
> 
>
https://codereview.chromium.org/16982005/diff/37001/Source/core/platform/Scro...
> Source/core/platform/ScrollView.cpp:288: // If no scrollbars are present, it
> does not indicate content is not be scrollable.
> I know you didn't write this comment in the first place, but can you please
> reword (preferably with fewer negatives)?
> 
>
https://codereview.chromium.org/16982005/diff/37001/Source/core/platform/Scro...
> File Source/core/platform/ScrollableArea.cpp (right):
> 
>
https://codereview.chromium.org/16982005/diff/37001/Source/core/platform/Scro...
> Source/core/platform/ScrollableArea.cpp:62: return 0.875f;
> I don't think the style guide explicitly mentions this, but I think it's
better
> to give these constants names (either at the top of this file or in a separate
> file if there are enough scrolling-related constants that need to be kept
track
> of) instead of hard-coding return values. Maybe that's just personal
preference,
> though.
> 
>
https://codereview.chromium.org/16982005/diff/37001/Source/core/platform/Scro...
> Source/core/platform/ScrollableArea.cpp:421: return 1.0f;
> The style guide says to omit the .0f since you're not doing floating point
math.
> 
>
https://codereview.chromium.org/16982005/diff/37001/Source/core/rendering/Ren...
> File Source/core/rendering/RenderLayer.cpp (right):
> 
>
https://codereview.chromium.org/16982005/diff/37001/Source/core/rendering/Ren...
> Source/core/rendering/RenderLayer.cpp:2515: IntSize scrollDims =
> maximumScrollPosition() - minimumScrollPosition();
> scrollDims -> scrollDimensions

[bokan, 2013-07-22 15:08:46]

https://codereview.chromium.org/16982005/diff/37001/Source/core/platform/Scro...
File Source/core/platform/ScrollView.cpp (right):

https://codereview.chromium.org/16982005/diff/37001/Source/core/platform/Scro...
Source/core/platform/ScrollView.cpp:288: // If no scrollbars are present, it
does not indicate content is not be scrollable.
On 2013/07/15 23:17:44, tdanderson wrote:
> I know you didn't write this comment in the first place, but can you please
> reword (preferably with fewer negatives)?

Done.

https://codereview.chromium.org/16982005/diff/37001/Source/core/platform/Scro...
File Source/core/platform/ScrollableArea.cpp (right):

https://codereview.chromium.org/16982005/diff/37001/Source/core/platform/Scro...
Source/core/platform/ScrollableArea.cpp:62: return 0.875f;
On 2013/07/15 23:17:44, tdanderson wrote:
> I don't think the style guide explicitly mentions this, but I think it's
better
> to give these constants names (either at the top of this file or in a separate
> file if there are enough scrolling-related constants that need to be kept
track
> of) instead of hard-coding return values. Maybe that's just personal
preference,
> though.

Done.

https://codereview.chromium.org/16982005/diff/37001/Source/core/platform/Scro...
Source/core/platform/ScrollableArea.cpp:421: return 1.0f;
On 2013/07/15 23:17:44, tdanderson wrote:
> The style guide says to omit the .0f since you're not doing floating point
math.

Done.

https://codereview.chromium.org/16982005/diff/37001/Source/core/rendering/Ren...
File Source/core/rendering/RenderLayer.cpp (right):

https://codereview.chromium.org/16982005/diff/37001/Source/core/rendering/Ren...
Source/core/rendering/RenderLayer.cpp:2515: IntSize scrollDims =
maximumScrollPosition() - minimumScrollPosition();
On 2013/07/15 23:17:44, tdanderson wrote:
> scrollDims -> scrollDimensions

Done.

[leviw_travelin_and_unemployed, 2013-07-31 20:18:33]

lgtm

[commit-bot: I haz the power, 2013-07-31 20:21:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bokan@chromium.org/16982005/48001

[commit-bot: I haz the power, 2013-07-31 20:41:41]

Retried try job too often on blink_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=blink_pres...

[jamesr, 2013-07-31 22:44:47]

https://codereview.chromium.org/16982005/diff/48001/Source/web/ScrollbarGroup...
File Source/web/ScrollbarGroup.cpp (right):

https://codereview.chromium.org/16982005/diff/48001/Source/web/ScrollbarGroup...
Source/web/ScrollbarGroup.cpp:270: return 0;
this doesn't look right.  have you tested page up/down in a pdf document
rendered by chrome's pdf viewer?

[bokan, 2013-08-01 19:08:30]

https://codereview.chromium.org/16982005/diff/48001/Source/web/ScrollbarGroup...
File Source/web/ScrollbarGroup.cpp (right):

https://codereview.chromium.org/16982005/diff/48001/Source/web/ScrollbarGroup...
Source/web/ScrollbarGroup.cpp:270: return 0;
On 2013/07/31 22:44:48, jamesr wrote:
> this doesn't look right.  have you tested page up/down in a pdf document
> rendered by chrome's pdf viewer?
This was an oversight. Fixed and tested in pdf viewer.

[jamesr, 2013-08-01 19:25:19]

Thanks. Source/web/ lgtm

[commit-bot: I haz the power, 2013-08-02 14:30:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bokan@chromium.org/16982005/60001

[commit-bot: I haz the power, 2013-08-02 17:04:01]

Change committed as 155441