Don't let RenderScrollbarPart have padding.

Don't let RenderScrollbarPart have padding.

Return 0 padding for all sides. While RenderScrollbarPart isn't laid
out normally and padding is ignored anyway, the padding getters may
still be used from outside layout, and if padding is percent-based, a
containing block is required, which not all RenderScrollbarPart
objects have, so it would crash.

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

[Julien - ping for review, 2014-02-12 02:41:40]

Morten, could you add a reduced layout test to ensure we don't regress this?
(while I think your approach is sound, it may help me for the review too)

[mstensho (USE GERRIT), 2014-02-12 10:32:53]

On 2014/02/12 02:41:40, Julien Chaffraix - PST wrote:
> Morten, could you add a reduced layout test to ensure we don't regress this?
> (while I think your approach is sound, it may help me for the review too)

Sure - done.

[Julien - ping for review, 2014-02-19 16:25:13]

lgtm with some suggestions.

https://codereview.chromium.org/138903031/diff/50001/LayoutTests/scrollbars/v...
File
LayoutTests/scrollbars/viewport-scrollbar-corner-with-percent-padding-crash.html
(right):

https://codereview.chromium.org/138903031/diff/50001/LayoutTests/scrollbars/v...
LayoutTests/scrollbars/viewport-scrollbar-corner-with-percent-padding-crash.html:4:
<title>Viewport scrollbar corner with percent padding and background
image</title>
I would prefer having this dumped into the output as:
- it isn't a burden on the test output
- it doesn't give us much to put the information into the <title> (you need to
open the test case in a browser / content_shell to see that)

https://codereview.chromium.org/138903031/diff/50001/LayoutTests/scrollbars/v...
LayoutTests/scrollbars/viewport-scrollbar-corner-with-percent-padding-crash.html:21:
</html>
The test doesn't crash for me on Chromium Mac (it seems like the scrollbars are
not allocated).

https://codereview.chromium.org/138903031/diff/50001/Source/core/rendering/Re...
File Source/core/rendering/RenderScrollbarPart.h (right):

https://codereview.chromium.org/138903031/diff/50001/Source/core/rendering/Re...
Source/core/rendering/RenderScrollbarPart.h:78: virtual LayoutUnit paddingEnd()
const OVERRIDE { return LayoutUnit(); }
Having a comment as to why we do that would be nice (it's because we can't
resolve percentage padding as we may not have a containing block).

Note that you could solve the problem by just returning
minimumValueForLength(rightLength, 0) which I would be more comfortable with as
it doesn't disable paddings. That would mean fixing the description.

[mstensho (USE GERRIT), 2014-02-22 20:54:50]

https://codereview.chromium.org/138903031/diff/50001/LayoutTests/scrollbars/v...
File
LayoutTests/scrollbars/viewport-scrollbar-corner-with-percent-padding-crash.html
(right):

https://codereview.chromium.org/138903031/diff/50001/LayoutTests/scrollbars/v...
LayoutTests/scrollbars/viewport-scrollbar-corner-with-percent-padding-crash.html:4:
<title>Viewport scrollbar corner with percent padding and background
image</title>
On 2014/02/19 16:25:14, Julien Chaffraix - PST wrote:
> I would prefer having this dumped into the output as:
> - it isn't a burden on the test output
> - it doesn't give us much to put the information into the <title> (you need to
> open the test case in a browser / content_shell to see that)

Done.

https://codereview.chromium.org/138903031/diff/50001/LayoutTests/scrollbars/v...
LayoutTests/scrollbars/viewport-scrollbar-corner-with-percent-padding-crash.html:21:
</html>
On 2014/02/19 16:25:14, Julien Chaffraix - PST wrote:
> The test doesn't crash for me on Chromium Mac (it seems like the scrollbars
are
> not allocated).

I don't know. Maybe Mac does things differently wrt. scrollbars. It crashes here
on Linux, at least.

https://codereview.chromium.org/138903031/diff/50001/Source/core/rendering/Re...
File Source/core/rendering/RenderScrollbarPart.h (right):

https://codereview.chromium.org/138903031/diff/50001/Source/core/rendering/Re...
Source/core/rendering/RenderScrollbarPart.h:78: virtual LayoutUnit paddingEnd()
const OVERRIDE { return LayoutUnit(); }
On 2014/02/19 16:25:14, Julien Chaffraix - PST wrote:
> Having a comment as to why we do that would be nice (it's because we can't
> resolve percentage padding as we may not have a containing block).
> 
> Note that you could solve the problem by just returning
> minimumValueForLength(rightLength, 0) which I would be more comfortable with
as
> it doesn't disable paddings. That would mean fixing the description.

Done.

Simply returning 0 is fine, since scroll bar corners aren't actually laid out,
so I prefer to keep it simple (return 0), since nobody cares.

[mstensho (USE GERRIT), 2014-02-22 20:55:01]

The CQ bit was checked by mstensho@opera.com

[commit-bot: I haz the power, 2014-02-22 20:55:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mstensho@opera.com/138903031/130001

[commit-bot: I haz the power, 2014-02-22 23:39:51]

Change committed as 167640