Calculate paint invalidation rect for scrollbars

Calculate paint invalidation rect for scrollbars

Remove eager invalidation of scrollbars except when they are removed.
Invalidate scrollbars during paint invalidation so that we can calculate
paint invalidation rects for them. The rects are required by
PaintController to calculate display item bounds.

BUG=529938
Committed: https://crrev.com/77b23ecd9615ab026b9d1ffe0d8cf310f6efbc30
Cr-Commit-Position: refs/heads/master@{#360144}

[Xianzhu, 2015-11-10 22:23:55]

Patchset #11 (id:200001) has been deleted

[Xianzhu, 2015-11-10 22:24:04]

Patchset #10 (id:180001) has been deleted

[Xianzhu, 2015-11-10 22:24:15]

Patchset #9 (id:160001) has been deleted

[Xianzhu, 2015-11-11 00:45:38]

wangxianzhu@chromium.org changed reviewers:
+ chrishtr@chromium.org, pdr@chromium.org, wkorman@chromium.org

[chrishtr, 2015-11-11 01:40:29]

This CL is giant...is there any way to do it in smaller pieces? It will take me
a while to review. :)

https://codereview.chromium.org/1406133005/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/fetch/ImageResourceTest.cpp (right):

https://codereview.chromium.org/1406133005/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/ImageResourceTest.cpp:138:
ASSERT_GT(client.imageChangedCount(), 0);
This change is not a bug?

https://codereview.chromium.org/1406133005/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/PaintInvalidationState.h (right):

https://codereview.chromium.org/1406133005/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/PaintInvalidationState.h:56: // Disable
view clipping and scroll offset adjustment for paint invalidation of FrameView
scrollbars.
Why? What goes wrong and why is it wrong now and not before?

[wkorman, 2015-11-11 07:48:39]

It is a massive change! I am excited about it, though. Thanks for working on
this so quickly. I will take a look tomorrow.

[Xianzhu, 2015-11-11 17:35:26]

Please take your time to review :) This work took me more than 2 weeks (half
finished before sync painting), and I'm expecting 1 week or more to finish the
review. It's hard to split this CL into multiple ones because it works as a
whole by changing the scrollbar invalidation method.

Fwiw, some background information which may be helpful for review:
- Previous scrollbar paint invalidation method:
 * part-based invalidation:
    ScrollbarTheme::invalidateParts()
 -> ScrollbarTheme::invalidatePart()
 -> Scrollbar::invalidateRect()
 -> ScrollableArea::invalidateScrollbar()
 -> GraphicsLayer::setNeedsDisplay() (for scrollbar with composited layer)
    or FrameView::invalidateScrollbarRect() (for frame scrollbar --
non-root-layer-scrolls)
    or PaintLayerScrollableArea::invalidateScrollbarRect() (for normal
scrollbar)

 * full scrollbar invalidation:
    Scrollbar::invalidate()
 -> Scrollbar::invalidateRect()
 -> ScrollableArea::invalidateScrollbar()
 -> GraphicsLayer::setNeedsDisplay() (for scrollbar with composited layer)
    or FrameView::invalidateScrollbarRect() (for frame scrollbar --
non-root-layer-scrolls)
    or PaintLayerScrollableArea::invalidateScrollbarRect() (for normal
scrollbar)

 For a scrollbar without dedicated composited layer, at the last step, if we are
performing layout (most cases), the invalidated rect is accumulated into
ScrollableArea::xxxScrollbarDamage(), then during paint invalidation, we
invalidate the accumulated damage rect on the owner box.

The problems of the previous method are:
- Scrollbar damage rect != scrollbar paint invalidation rect (bounds rect needed
by PaintController);
- We may accumulate intermediate scrollbar layout positions into the damage rect
on which we never paint the scrollbar;
- Part-based invalidation is complex but is seldom used and benefits not much
over full scrollbar invalidation.

The new method:
- Removes part-based invalidation.
- Changes Scrollbar::invalidate() to Scrollbar::setNeedsPaintInvalidation().
- Expect when the scrollbar is being removed, always invalidate whole scrollbars
during paint invalidation if scrollbars marked needing paint invalidation.
The method is like what we do for normal LayoutObjects: during paint
invalidation, save previous paint invalidation rect, calculate new paint
invalidation rect, invalidate previous and new paint invalidation rect if the
scrollbar needs paint invalidation.

[Xianzhu, 2015-11-11 17:46:56]

On 2015/11/11 17:35:26, Xianzhu wrote:
> - Expect when the scrollbar is being removed, always invalidate whole
scrollbars
> during paint invalidation if scrollbars marked needing paint invalidation.
...

s/Expect/Except/

[Xianzhu, 2015-11-11 19:29:57]

https://codereview.chromium.org/1406133005/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/fetch/ImageResourceTest.cpp (right):

https://codereview.chromium.org/1406133005/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/ImageResourceTest.cpp:138:
ASSERT_GT(client.imageChangedCount(), 0);
On 2015/11/11 01:40:29, chrishtr wrote:
> This change is not a bug?

This was a really weird situation. Besides real image changes, the embedded SVG
image has a FrameView which also informs the SVG Image changes through
HostWindow::invalidateRect() interface. imageChangedCount() happened to be 2
before, because the invalidation caused by scroll corner was also counted as an
image change.

With this change, the extra invalidation of scroll corner is eliminated, so
imageChangeCount() is 1 (the real image change). Perhaps it's better to
ASSERT_EQ(..., 1).

https://codereview.chromium.org/1406133005/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/PaintInvalidationState.h (right):

https://codereview.chromium.org/1406133005/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/PaintInvalidationState.h:56: // Disable
view clipping and scroll offset adjustment for paint invalidation of FrameView
scrollbars.
On 2015/11/11 01:40:29, chrishtr wrote:
> Why? What goes wrong and why is it wrong now and not before?

Now we calculate paint invalidation rect for frame scrollbars using
PaintLayer::mapRectToPaintInvalidationBacking(layoutView,
paintInvalidationContainer, PaintInvalidationState-created-for-LayoutView). This
will call LayoutView::mapRectToPaintInvalidationBacking() which previously
treated the rect in coordinates of scrolling contents which is not suitable for
frame scrollbars. The following code makes
LayoutView::mapRectToPaintInvalidationBacking() work for frame scrollbars.

This is temporary before we switch to root-layer-scrolls.

[wkorman, 2015-11-13 17:43:13]

lgtm

Sounds reasonable to me. +skobes to Cc: as FYI.

Maybe the part-based invalidation was a more worthwhile optimization on older
hardware/platforms? I'd support at least attempting to remove it, as is done in
this change, to see whether we're fine without.

[skobes, 2015-11-13 18:11:55]

On 2015/11/13 17:43:13, wkorman wrote:
> lgtm
> 
> Sounds reasonable to me. +skobes to Cc: as FYI.
> 
> Maybe the part-based invalidation was a more worthwhile optimization on older
> hardware/platforms? I'd support at least attempting to remove it, as is done
in
> this change, to see whether we're fine without.

I doubt there are significant savings from invalidating only part of a
scrollbar.  Note that we are currently doing a lot of unnecessary (full)
invalidations, see http://crbug.com/549277.

[Xianzhu, 2015-11-16 19:55:46]

chrishtr@ do you have more comments?

[chrishtr, 2015-11-16 21:33:03]

Yes I have some more, going to look at the CL again today.

[chrishtr, 2015-11-17 00:35:47]

https://codereview.chromium.org/1406133005/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/fetch/ImageResourceTest.cpp (right):

https://codereview.chromium.org/1406133005/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/ImageResourceTest.cpp:138:
ASSERT_GT(client.imageChangedCount(), 0);
On 2015/11/11 at 19:29:57, Xianzhu wrote:
> On 2015/11/11 01:40:29, chrishtr wrote:
> > This change is not a bug?
> 
> This was a really weird situation. Besides real image changes, the embedded
SVG image has a FrameView which also informs the SVG Image changes through
HostWindow::invalidateRect() interface. imageChangedCount() happened to be 2
before, because the invalidation caused by scroll corner was also counted as an
image change.
> 
> With this change, the extra invalidation of scroll corner is eliminated, so
imageChangeCount() is 1 (the real image change). Perhaps it's better to
ASSERT_EQ(..., 1).

I'd prefer ASSERT_EQ(..., 1) just to lock things down a bit more. And add a
comment explaining.

https://codereview.chromium.org/1406133005/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/1406133005/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/FrameView.cpp:1075:
invalidatePaintOfScrollControlsIfNeeded(rootForPaintInvalidation,
paintInvalidationState, paintInvalidationState.paintInvalidationContainer(),
m_scrollCorner, nullptr);
How about just passing a null pointer for PaintInvalidationState here instead of
adding a parameter to it? I don't think keeping it has an efficiency gain if I'm
reading correctly, because it it is nulled out when recursing from LayoutView
into the parent frame.

[chrishtr, 2015-11-17 00:44:32]

https://codereview.chromium.org/1406133005/diff/280001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/core/paint/PaintInvalidationCapableScrollableArea.h
(right):

https://codereview.chromium.org/1406133005/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/PaintInvalidationCapableScrollableArea.h:26:
void willRemoveScrollbar(LayoutBox&, Scrollbar*, ScrollbarOrientation);
I don't think the first argument is necessary. Both subclasses can determine the
appropriate
LayoutObject, which is always the LayoutView in the case of FrameView scrolling,
or the associated
LayoutBox with the scrollable area.

https://codereview.chromium.org/1406133005/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/PaintInvalidationCapableScrollableArea.h:28:
void invalidatePaintOfScrollControlsIfNeeded(LayoutBox& ownerBox, const
PaintInvalidationState&, const LayoutBoxModelObject& paintInvalidationContainer,
LayoutScrollbarPart* scrollCorner, LayoutScrollbarPart* resizer);
Ditto.

[Xianzhu, 2015-11-17 00:56:05]

https://codereview.chromium.org/1406133005/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/fetch/ImageResourceTest.cpp (right):

https://codereview.chromium.org/1406133005/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/ImageResourceTest.cpp:138:
ASSERT_GT(client.imageChangedCount(), 0);
On 2015/11/17 00:35:47, chrishtr wrote:
> On 2015/11/11 at 19:29:57, Xianzhu wrote:
> > On 2015/11/11 01:40:29, chrishtr wrote:
> > > This change is not a bug?
> > 
> > This was a really weird situation. Besides real image changes, the embedded
> SVG image has a FrameView which also informs the SVG Image changes through
> HostWindow::invalidateRect() interface. imageChangedCount() happened to be 2
> before, because the invalidation caused by scroll corner was also counted as
an
> image change.
> > 
> > With this change, the extra invalidation of scroll corner is eliminated, so
> imageChangeCount() is 1 (the real image change). Perhaps it's better to
> ASSERT_EQ(..., 1).
> 
> I'd prefer ASSERT_EQ(..., 1) just to lock things down a bit more. And add a
> comment explaining.

Done.

https://codereview.chromium.org/1406133005/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/1406133005/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/FrameView.cpp:1075:
invalidatePaintOfScrollControlsIfNeeded(rootForPaintInvalidation,
paintInvalidationState, paintInvalidationState.paintInvalidationContainer(),
m_scrollCorner, nullptr);
On 2015/11/17 00:35:47, chrishtr wrote:
> How about just passing a null pointer for PaintInvalidationState here instead
of
> adding a parameter to it? I don't think keeping it has an efficiency gain if
I'm
> reading correctly, because it it is nulled out when recursing from LayoutView
> into the parent frame.

For now the only purpose of the paintInvalidationState here is to let
LayoutView::mapRectToPaintInvalidationBacking() not clip and scroll the rect.

https://codereview.chromium.org/1406133005/diff/280001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/core/paint/PaintInvalidationCapableScrollableArea.h
(right):

https://codereview.chromium.org/1406133005/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/PaintInvalidationCapableScrollableArea.h:26:
void willRemoveScrollbar(LayoutBox&, Scrollbar*, ScrollbarOrientation);
On 2015/11/17 00:44:32, chrishtr wrote:
> I don't think the first argument is necessary. Both subclasses can determine
the
> appropriate
> LayoutObject, which is always the LayoutView in the case of FrameView
scrolling,
> or the associated
> LayoutBox with the scrollable area.

The two cases share common code in this class which don't know how to get the
box. The common code needs subclasses to provide the box parameter.

Another method is to declare 'virtual LayoutBox& box() const = 0' here and let
subclasses implement it. I had such a version but decided to use this version
which has less changed lines.

[chrishtr, 2015-11-17 01:34:20]

https://codereview.chromium.org/1406133005/diff/300001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/FrameView.cpp (left):

https://codereview.chromium.org/1406133005/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/FrameView.cpp:1512: if
(!m_scrollbarsSuppressed)
So now we always invalidate, even with m_scrollbarsSuppressed, but merely avoid
painting in FramePainter?

https://codereview.chromium.org/1406133005/diff/300001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutObject.cpp (right):

https://codereview.chromium.org/1406133005/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutObject.cpp:3415:
DisableCompositingQueryAsserts disabler;
This is still necessary for some other use case?

https://codereview.chromium.org/1406133005/diff/300001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutScrollbarPart.cpp (right):

https://codereview.chromium.org/1406133005/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutScrollbarPart.cpp:209:
frameView->setScrollCornerNeedsPaintInvalidation();
Why is the scroll corner special?

https://codereview.chromium.org/1406133005/diff/300001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/PaintInvalidationState.h (right):

https://codereview.chromium.org/1406133005/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/PaintInvalidationState.h:57: //
TODO(wangxianzhu): Remove this for root-layer-scrolls.
"Remove this when root-layer-scrolls launches"

https://codereview.chromium.org/1406133005/diff/300001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/core/paint/PaintInvalidationCapableScrollableArea.cpp
(right):

https://codereview.chromium.org/1406133005/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/PaintInvalidationCapableScrollableArea.cpp:75:
if (scrollCorner)
Roll lines 75-78 into invalidatePaintOfScrollControlIfNeeded?

[chrishtr, 2015-11-17 01:35:05]

On 2015/11/17 at 00:56:05, wangxianzhu wrote:
>
https://codereview.chromium.org/1406133005/diff/260001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/fetch/ImageResourceTest.cpp (right):
> 
>
https://codereview.chromium.org/1406133005/diff/260001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/fetch/ImageResourceTest.cpp:138:
ASSERT_GT(client.imageChangedCount(), 0);
> On 2015/11/17 00:35:47, chrishtr wrote:
> > On 2015/11/11 at 19:29:57, Xianzhu wrote:
> > > On 2015/11/11 01:40:29, chrishtr wrote:
> > > > This change is not a bug?
> > > 
> > > This was a really weird situation. Besides real image changes, the
embedded
> > SVG image has a FrameView which also informs the SVG Image changes through
> > HostWindow::invalidateRect() interface. imageChangedCount() happened to be 2
> > before, because the invalidation caused by scroll corner was also counted as
an
> > image change.
> > > 
> > > With this change, the extra invalidation of scroll corner is eliminated,
so
> > imageChangeCount() is 1 (the real image change). Perhaps it's better to
> > ASSERT_EQ(..., 1).
> > 
> > I'd prefer ASSERT_EQ(..., 1) just to lock things down a bit more. And add a
> > comment explaining.
> 
> Done.
> 
>
https://codereview.chromium.org/1406133005/diff/260001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/frame/FrameView.cpp (right):
> 
>
https://codereview.chromium.org/1406133005/diff/260001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/frame/FrameView.cpp:1075:
invalidatePaintOfScrollControlsIfNeeded(rootForPaintInvalidation,
paintInvalidationState, paintInvalidationState.paintInvalidationContainer(),
m_scrollCorner, nullptr);
> On 2015/11/17 00:35:47, chrishtr wrote:
> > How about just passing a null pointer for PaintInvalidationState here
instead of
> > adding a parameter to it? I don't think keeping it has an efficiency gain if
I'm
> > reading correctly, because it it is nulled out when recursing from
LayoutView
> > into the parent frame.
> 
> For now the only purpose of the paintInvalidationState here is to let
LayoutView::mapRectToPaintInvalidationBacking() not clip and scroll the rect.
> 
>
https://codereview.chromium.org/1406133005/diff/280001/third_party/WebKit/Sou...
> File
third_party/WebKit/Source/core/paint/PaintInvalidationCapableScrollableArea.h
(right):
> 
>
https://codereview.chromium.org/1406133005/diff/280001/third_party/WebKit/Sou...
>
third_party/WebKit/Source/core/paint/PaintInvalidationCapableScrollableArea.h:26:
void willRemoveScrollbar(LayoutBox&, Scrollbar*, ScrollbarOrientation);
> On 2015/11/17 00:44:32, chrishtr wrote:
> > I don't think the first argument is necessary. Both subclasses can determine
the
> > appropriate
> > LayoutObject, which is always the LayoutView in the case of FrameView
scrolling,
> > or the associated
> > LayoutBox with the scrollable area.
> 
> The two cases share common code in this class which don't know how to get the
box. The common code needs subclasses to provide the box parameter.
> 
> Another method is to declare 'virtual LayoutBox& box() const = 0' here and let
subclasses implement it. I had such a version but decided to use this version
which has less changed lines.

I prefer the box() option, since it removes an argument from the other call
sites that always has to be the same value or you're doing it wrong, which will
result in easier to read code and fewer mistakes.

[Xianzhu, 2015-11-17 18:31:37]

https://codereview.chromium.org/1406133005/diff/280001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/core/paint/PaintInvalidationCapableScrollableArea.h
(right):

https://codereview.chromium.org/1406133005/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/PaintInvalidationCapableScrollableArea.h:26:
void willRemoveScrollbar(LayoutBox&, Scrollbar*, ScrollbarOrientation);
On 2015/11/17 00:56:05, Xianzhu wrote:
> On 2015/11/17 00:44:32, chrishtr wrote:
> > I don't think the first argument is necessary. Both subclasses can determine
> the
> > appropriate
> > LayoutObject, which is always the LayoutView in the case of FrameView
> scrolling,
> > or the associated
> > LayoutBox with the scrollable area.
> 
> The two cases share common code in this class which don't know how to get the
> box. The common code needs subclasses to provide the box parameter.
> 
> Another method is to declare 'virtual LayoutBox& box() const = 0' here and let
> subclasses implement it. I had such a version but decided to use this version
> which has less changed lines.

'box()' done.

https://codereview.chromium.org/1406133005/diff/300001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/FrameView.cpp (left):

https://codereview.chromium.org/1406133005/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/FrameView.cpp:1512: if
(!m_scrollbarsSuppressed)
On 2015/11/17 01:34:20, chrishtr wrote:
> So now we always invalidate, even with m_scrollbarsSuppressed, but merely
avoid
> painting in FramePainter?

Previously we invalidated or added scrollbar damage using the current scrollbar
geometry, so the suppression was to avoid invalidaiton of intermediate layout
status of the scrollbars. Now we set invalidation flag only, and finally we
invalidate the scrollbar using its previous geometry and the current geometry,
so we won't invalidate the intermediate status and no longer need the
suppression.

https://codereview.chromium.org/1406133005/diff/300001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutObject.cpp (right):

https://codereview.chromium.org/1406133005/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutObject.cpp:3415:
DisableCompositingQueryAsserts disabler;
On 2015/11/17 01:34:20, chrishtr wrote:
> This is still necessary for some other use case?

Yes. Most of the callers calls this before compositing-update.

https://codereview.chromium.org/1406133005/diff/300001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutScrollbarPart.cpp (right):

https://codereview.chromium.org/1406133005/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutScrollbarPart.cpp:209:
frameView->setScrollCornerNeedsPaintInvalidation();
On 2015/11/17 01:34:20, chrishtr wrote:
> Why is the scroll corner special?

Though scroll corner is a LayoutScrollbarPart, its m_part is NoPart and it
doesn't belong to any scrollbar, but belongs to a ScrollableArea. There is no
direct way to get its ScrollableArea, so we must find its ScrollableArea in this
way. (With root-layer-scrolls, there will be only one case for scroll corner and
we'll execute line 215 only.)

https://codereview.chromium.org/1406133005/diff/300001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/PaintInvalidationState.h (right):

https://codereview.chromium.org/1406133005/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/PaintInvalidationState.h:57: //
TODO(wangxianzhu): Remove this for root-layer-scrolls.
On 2015/11/17 01:34:20, chrishtr wrote:
> "Remove this when root-layer-scrolls launches"

Done.

https://codereview.chromium.org/1406133005/diff/300001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/core/paint/PaintInvalidationCapableScrollableArea.cpp
(right):

https://codereview.chromium.org/1406133005/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/PaintInvalidationCapableScrollableArea.cpp:75:
if (scrollCorner)
On 2015/11/17 01:34:20, chrishtr wrote:
> Roll lines 75-78 into invalidatePaintOfScrollControlIfNeeded?

It seems not feasible for invalidatePaintOfScrollControlIfNeeded to handle the
specialties of customized scrollbars and scroll corners/resizers.

[chrishtr, 2015-11-17 18:52:55]

https://codereview.chromium.org/1406133005/diff/300001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/FrameView.cpp (left):

https://codereview.chromium.org/1406133005/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/FrameView.cpp:1512: if
(!m_scrollbarsSuppressed)
On 2015/11/17 at 18:31:36, Xianzhu wrote:
> On 2015/11/17 01:34:20, chrishtr wrote:
> > So now we always invalidate, even with m_scrollbarsSuppressed, but merely
avoid
> > painting in FramePainter?
> 
> Previously we invalidated or added scrollbar damage using the current
scrollbar geometry, so the suppression was to avoid invalidaiton of intermediate
layout status of the scrollbars. Now we set invalidation flag only, and finally
we invalidate the scrollbar using its previous geometry and the current
geometry, so we won't invalidate the intermediate status and no longer need the
suppression.

Is there any benefit to the scrollbarsSuppressed() bit then now? With this path
it only suppresses painting.

[Xianzhu, 2015-11-17 19:04:24]

https://codereview.chromium.org/1406133005/diff/300001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/FrameView.cpp (left):

https://codereview.chromium.org/1406133005/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/FrameView.cpp:1512: if
(!m_scrollbarsSuppressed)
On 2015/11/17 18:52:54, chrishtr wrote:
> On 2015/11/17 at 18:31:36, Xianzhu wrote:
> > On 2015/11/17 01:34:20, chrishtr wrote:
> > > So now we always invalidate, even with m_scrollbarsSuppressed, but merely
> avoid
> > > painting in FramePainter?
> > 
> > Previously we invalidated or added scrollbar damage using the current
> scrollbar geometry, so the suppression was to avoid invalidaiton of
intermediate
> layout status of the scrollbars. Now we set invalidation flag only, and
finally
> we invalidate the scrollbar using its previous geometry and the current
> geometry, so we won't invalidate the intermediate status and no longer need
the
> suppression.
> 
> Is there any benefit to the scrollbarsSuppressed() bit then now? With this
path
> it only suppresses painting.

The comment for setScrollbarsSuppressed() seems inaccurate. It also disables
scrollbar/resizer creation/deletion in some cases to avoid excessive
intermediate back-and-forth operations. I'd not bother it in this CL because
these code will go away after we launch root-layer-scrolls.

[chrishtr, 2015-11-17 19:06:13]

On 2015/11/17 at 19:04:24, wangxianzhu wrote:
>
https://codereview.chromium.org/1406133005/diff/300001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/frame/FrameView.cpp (left):
> 
>
https://codereview.chromium.org/1406133005/diff/300001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/frame/FrameView.cpp:1512: if
(!m_scrollbarsSuppressed)
> On 2015/11/17 18:52:54, chrishtr wrote:
> > On 2015/11/17 at 18:31:36, Xianzhu wrote:
> > > On 2015/11/17 01:34:20, chrishtr wrote:
> > > > So now we always invalidate, even with m_scrollbarsSuppressed, but
merely
> > avoid
> > > > painting in FramePainter?
> > > 
> > > Previously we invalidated or added scrollbar damage using the current
> > scrollbar geometry, so the suppression was to avoid invalidaiton of
intermediate
> > layout status of the scrollbars. Now we set invalidation flag only, and
finally
> > we invalidate the scrollbar using its previous geometry and the current
> > geometry, so we won't invalidate the intermediate status and no longer need
the
> > suppression.
> > 
> > Is there any benefit to the scrollbarsSuppressed() bit then now? With this
path
> > it only suppresses painting.
> 
> The comment for setScrollbarsSuppressed() seems inaccurate. It also disables
scrollbar/resizer creation/deletion in some cases to avoid excessive
intermediate back-and-forth operations. I'd not bother it in this CL because
these code will go away after we launch root-layer-scrolls.

Or supporting it in root-layer-scrolls will block that project..

[chrishtr, 2015-11-17 19:06:16]

The CQ bit was checked by chrishtr@chromium.org

[chrishtr, 2015-11-17 19:06:17]

lgtm

[chrishtr, 2015-11-17 19:06:17]

The patchset sent to the CQ was uploaded after l-g-t-m from wkorman@chromium.org
Link to the patchset: https://codereview.chromium.org/1406133005/#ps320001
(title: " ")

[commit-bot: I haz the power, 2015-11-17 19:07:16]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1406133005/320001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1406133005/320001

[commit-bot: I haz the power, 2015-11-17 20:18:58]

Committed patchset #14 (id:320001)

[commit-bot: I haz the power, 2015-11-17 20:19:44]

Patchset 14 (id:??) landed as
https://crrev.com/77b23ecd9615ab026b9d1ffe0d8cf310f6efbc30
Cr-Commit-Position: refs/heads/master@{#360144}

[Nico, 2015-11-18 18:16:38]

A revert of this CL (patchset #14 id:320001) has been created in
https://codereview.chromium.org/1456953003/ by thakis@chromium.org.

The reason for reverting is: Speculative, very likely caused
https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Mac10.6?numbui...
to turn red.