Need to invalidate scrollbars in case they get repositioned.

Need to invalidate scrollbars in case they get repositioned.

If the owning renderer gets resized, the scrollbar may be repositioned. Don't
rely on other reasons for invalidation (background, borders, child content
getting relaid out, etc.) to accidentally get the area covered by the
scrollbars invalidated, since there may very well be no other reason.

BUG=460434
R=jchaffraix@chromium.org,leviw@chromium.org

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

[dsinclair, 2015-02-23 15:13:52]

dsinclair@chromium.org changed reviewers:
+ dsinclair@chromium.org

[dsinclair, 2015-02-23 15:13:53]

https://codereview.chromium.org/942963002/diff/1/Source/core/layout/LayoutObj...
File Source/core/layout/LayoutObject.cpp (right):

https://codereview.chromium.org/942963002/diff/1/Source/core/layout/LayoutObj...
Source/core/layout/LayoutObject.cpp:639: return rendererHasNoBoxEffect() &&
!hasNonCompositedScrollbars();
Can we pull this out to be (assuming the following logic is still valid):

// some comment as to why this is special ....
if (hasNonCompositedScrolbars())
  return false;

return rendererHasNoBoxEffect();

https://codereview.chromium.org/942963002/diff/1/Source/core/rendering/Render...
File Source/core/rendering/RenderBox.h (right):

https://codereview.chromium.org/942963002/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderBox.h:697: virtual bool hasNonCompositedScrollbars()
const final;
Should this be override instead of virtual?

[mstensho (USE GERRIT), 2015-02-23 17:22:38]

https://codereview.chromium.org/942963002/diff/1/Source/core/layout/LayoutObj...
File Source/core/layout/LayoutObject.cpp (right):

https://codereview.chromium.org/942963002/diff/1/Source/core/layout/LayoutObj...
Source/core/layout/LayoutObject.cpp:639: return rendererHasNoBoxEffect() &&
!hasNonCompositedScrollbars();
On 2015/02/23 15:13:53, dsinclair wrote:
> Can we pull this out to be (assuming the following logic is still valid):
> 
> // some comment as to why this is special ....
> if (hasNonCompositedScrolbars())
>   return false;
> 
> return rendererHasNoBoxEffect();

Done.

https://codereview.chromium.org/942963002/diff/1/Source/core/rendering/Render...
File Source/core/rendering/RenderBox.h (right):

https://codereview.chromium.org/942963002/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderBox.h:697: virtual bool hasNonCompositedScrollbars()
const final;
On 2015/02/23 15:13:53, dsinclair wrote:
> Should this be override instead of virtual?

It's already a final override (there's a call site from RenderBox, so making it
final should save a few CPU cycles). Should I drop the "virtual" keyword, or
what did you mean? Everyone else around here repeat "virtual" on overrides, so I
thought it would look weird and special to omit it.

[dsinclair, 2015-02-23 17:35:29]

On 2015/02/23 at 17:22:38, mstensho wrote:
>
https://codereview.chromium.org/942963002/diff/1/Source/core/layout/LayoutObj...
> File Source/core/layout/LayoutObject.cpp (right):
> 
>
https://codereview.chromium.org/942963002/diff/1/Source/core/layout/LayoutObj...
> Source/core/layout/LayoutObject.cpp:639: return rendererHasNoBoxEffect() &&
!hasNonCompositedScrollbars();
> On 2015/02/23 15:13:53, dsinclair wrote:
> > Can we pull this out to be (assuming the following logic is still valid):
> > 
> > // some comment as to why this is special ....
> > if (hasNonCompositedScrolbars())
> >   return false;
> > 
> > return rendererHasNoBoxEffect();
> 
> Done.
> 
>
https://codereview.chromium.org/942963002/diff/1/Source/core/rendering/Render...
> File Source/core/rendering/RenderBox.h (right):
> 
>
https://codereview.chromium.org/942963002/diff/1/Source/core/rendering/Render...
> Source/core/rendering/RenderBox.h:697: virtual bool
hasNonCompositedScrollbars() const final;
> On 2015/02/23 15:13:53, dsinclair wrote:
> > Should this be override instead of virtual?
> 
> It's already a final override (there's a call site from RenderBox, so making
it final should save a few CPU cycles). Should I drop the "virtual" keyword, or
what did you mean? Everyone else around here repeat "virtual" on overrides, so I
thought it would look weird and special to omit it.


Something like:
    virtual bool hasNonCompositedScrollbars() const final override;

As it's overriding the method in LayoutObject. I originally suggested dropping
the virtual since it isn't needed with override but let's keep it if the rest of
the file is using both.

[mstensho (USE GERRIT), 2015-02-23 17:52:13]

On 2015/02/23 17:35:29, dsinclair wrote:
> On 2015/02/23 at 17:22:38, mstensho wrote:
> >
>
https://codereview.chromium.org/942963002/diff/1/Source/core/layout/LayoutObj...
> > File Source/core/layout/LayoutObject.cpp (right):
> > 
> >
>
https://codereview.chromium.org/942963002/diff/1/Source/core/layout/LayoutObj...
> > Source/core/layout/LayoutObject.cpp:639: return rendererHasNoBoxEffect() &&
> !hasNonCompositedScrollbars();
> > On 2015/02/23 15:13:53, dsinclair wrote:
> > > Can we pull this out to be (assuming the following logic is still valid):
> > > 
> > > // some comment as to why this is special ....
> > > if (hasNonCompositedScrolbars())
> > >   return false;
> > > 
> > > return rendererHasNoBoxEffect();
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/942963002/diff/1/Source/core/rendering/Render...
> > File Source/core/rendering/RenderBox.h (right):
> > 
> >
>
https://codereview.chromium.org/942963002/diff/1/Source/core/rendering/Render...
> > Source/core/rendering/RenderBox.h:697: virtual bool
> hasNonCompositedScrollbars() const final;
> > On 2015/02/23 15:13:53, dsinclair wrote:
> > > Should this be override instead of virtual?
> > 
> > It's already a final override (there's a call site from RenderBox, so making
> it final should save a few CPU cycles). Should I drop the "virtual" keyword,
or
> what did you mean? Everyone else around here repeat "virtual" on overrides, so
I
> thought it would look weird and special to omit it.
> 
> 
> Something like:
>     virtual bool hasNonCompositedScrollbars() const final override;
> 
> As it's overriding the method in LayoutObject. I originally suggested dropping
> the virtual since it isn't needed with override but let's keep it if the rest
of
> the file is using both.

Hmm... I was so sure that someone said not to use both "override" and "final",
but all I could find on the topic was https://codereview.chromium.org/934163004
, and since check-webkit-style doesn't complain about using both, I'll just do
as you say (except that I say "override final" instead of "final override",
since that what others already use). :)

[dsinclair, 2015-02-23 17:55:54]

The CQ bit was checked by dsinclair@chromium.org

[dsinclair, 2015-02-23 17:55:54]

lgtm

[commit-bot: I haz the power, 2015-02-23 17:56:23]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/942963002/40001

[commit-bot: I haz the power, 2015-02-23 19:39:07]

Committed patchset #3 (id:40001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=190681