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=460434R=jchaffraix@chromium.org,leviw@chromium.org
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=190681
https://codereview.chromium.org/942963002/diff/1/Source/core/layout/LayoutObject.cpp File Source/core/layout/LayoutObject.cpp (right): https://codereview.chromium.org/942963002/diff/1/Source/core/layout/LayoutObject.cpp#newcode639 Source/core/layout/LayoutObject.cpp:639: return rendererHasNoBoxEffect() && !hasNonCompositedScrollbars(); Can we pull this out ...
5 years, 10 months ago
(2015-02-23 15:13:53 UTC)
#3
On 2015/02/23 at 17:22:38, mstensho wrote: > https://codereview.chromium.org/942963002/diff/1/Source/core/layout/LayoutObject.cpp > File Source/core/layout/LayoutObject.cpp (right): > > https://codereview.chromium.org/942963002/diff/1/Source/core/layout/LayoutObject.cpp#newcode639 ...
5 years, 10 months ago
(2015-02-23 17:35:29 UTC)
#5
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)
On 2015/02/23 17:35:29, dsinclair wrote: > On 2015/02/23 at 17:22:38, mstensho wrote: > > > ...
5 years, 10 months ago
(2015-02-23 17:52:13 UTC)
#6
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
The CQ bit was checked by dsinclair@chromium.org
5 years, 10 months ago
(2015-02-23 17:55:54 UTC)
#7
Issue 942963002: Need to invalidate scrollbars in case they get repositioned.
(Closed)
Created 5 years, 10 months ago by mstensho (USE GERRIT)
Modified 5 years, 10 months ago
Reviewers: dsinclair, Julien - ping for review, leviw_travelin_and_unemployed
Base URL: svn://svn.chromium.org/blink/trunk
Comments: 4
Chromium Code Reviews
Issue
942963002:
Need to invalidate scrollbars in case they get repositioned. (Closed)
