Commit d9b6e52a authored by eseckler's avatar eseckler Committed by Commit bot

Fix performance regression and bug with hide_scrollbars setting.

FrameView and PaintLayerScrollableArea may perform more work than
necessary if hide_scrollbars is true (e.g. trigger unnecessary
layouts), because they think that scrollbar existence has changed
even if it didn't.

For FrameView, we can avoid this by enforcing hide_scrollbars
in computeScrollbarExistence instead. For PLSA, we check for
hide_scrollbars in updateAfterLayout to determine if the
scrollbar existence will change.

Also fixes another potential bug in PLSA, which may incorrectly
assume existence of scrollbars if hide_scrollbars is true.

BUG=652317,639806
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Review-Url: https://codereview.chromium.org/2394163002
Cr-Commit-Position: refs/heads/master@{#423671}
parent a729edf9
...@@ -3451,9 +3451,6 @@ void FrameView::addChild(Widget* child) { ...@@ -3451,9 +3451,6 @@ void FrameView::addChild(Widget* child) {
} }
void FrameView::setHasHorizontalScrollbar(bool hasBar) { void FrameView::setHasHorizontalScrollbar(bool hasBar) {
if (m_frame->settings() && m_frame->settings()->hideScrollbars())
hasBar = false;
if (hasBar == !!m_horizontalScrollbar) if (hasBar == !!m_horizontalScrollbar)
return; return;
...@@ -3477,9 +3474,6 @@ void FrameView::setHasHorizontalScrollbar(bool hasBar) { ...@@ -3477,9 +3474,6 @@ void FrameView::setHasHorizontalScrollbar(bool hasBar) {
} }
void FrameView::setHasVerticalScrollbar(bool hasBar) { void FrameView::setHasVerticalScrollbar(bool hasBar) {
if (m_frame->settings() && m_frame->settings()->hideScrollbars())
hasBar = false;
if (hasBar == !!m_verticalScrollbar) if (hasBar == !!m_verticalScrollbar)
return; return;
...@@ -3691,6 +3685,12 @@ void FrameView::computeScrollbarExistence( ...@@ -3691,6 +3685,12 @@ void FrameView::computeScrollbarExistence(
bool& newHasVerticalScrollbar, bool& newHasVerticalScrollbar,
const IntSize& docSize, const IntSize& docSize,
ComputeScrollbarExistenceOption option) const { ComputeScrollbarExistenceOption option) const {
if (m_frame->settings() && m_frame->settings()->hideScrollbars()) {
newHasHorizontalScrollbar = false;
newHasVerticalScrollbar = false;
return;
}
bool hasHorizontalScrollbar = m_horizontalScrollbar; bool hasHorizontalScrollbar = m_horizontalScrollbar;
bool hasVerticalScrollbar = m_verticalScrollbar; bool hasVerticalScrollbar = m_verticalScrollbar;
......
...@@ -691,14 +691,20 @@ void PaintLayerScrollableArea::updateAfterLayout() { ...@@ -691,14 +691,20 @@ void PaintLayerScrollableArea::updateAfterLayout() {
// We need to layout again if scrollbars are added or removed by // We need to layout again if scrollbars are added or removed by
// overflow:auto, or by changing between native and custom. // overflow:auto, or by changing between native and custom.
DCHECK(box().frame()->settings());
bool horizontalScrollbarShouldChange = bool horizontalScrollbarShouldChange =
(box().hasAutoHorizontalScrollbar() && ((box().hasAutoHorizontalScrollbar() &&
(hasHorizontalScrollbar() != shouldHaveAutoHorizontalScrollbar)) || (hasHorizontalScrollbar() != shouldHaveAutoHorizontalScrollbar)) ||
(box().style()->overflowX() == OverflowScroll && !horizontalScrollbar()); (box().style()->overflowX() == OverflowScroll &&
!horizontalScrollbar())) &&
(!box().frame()->settings()->hideScrollbars() ||
hasHorizontalScrollbar());
bool verticalScrollbarShouldChange = bool verticalScrollbarShouldChange =
(box().hasAutoVerticalScrollbar() && ((box().hasAutoVerticalScrollbar() &&
(hasVerticalScrollbar() != shouldHaveAutoVerticalScrollbar)) || (hasVerticalScrollbar() != shouldHaveAutoVerticalScrollbar)) ||
(box().style()->overflowY() == OverflowScroll && !verticalScrollbar()); (box().style()->overflowY() == OverflowScroll &&
!verticalScrollbar())) &&
(!box().frame()->settings()->hideScrollbars() || hasVerticalScrollbar());
bool scrollbarsWillChange = bool scrollbarsWillChange =
!scrollbarsAreFrozen && !visualViewportSuppliesScrollbars() && !scrollbarsAreFrozen && !visualViewportSuppliesScrollbars() &&
(horizontalScrollbarShouldChange || verticalScrollbarShouldChange); (horizontalScrollbarShouldChange || verticalScrollbarShouldChange);
...@@ -938,15 +944,13 @@ void PaintLayerScrollableArea::updateAfterStyleChange( ...@@ -938,15 +944,13 @@ void PaintLayerScrollableArea::updateAfterStyleChange(
// With overflow: scroll, scrollbars are always visible but may be disabled. // With overflow: scroll, scrollbars are always visible but may be disabled.
// When switching to another value, we need to re-enable them (see bug 11985). // When switching to another value, we need to re-enable them (see bug 11985).
if (needsHorizontalScrollbar && oldStyle && if (hasHorizontalScrollbar() && oldStyle &&
oldStyle->overflowX() == OverflowScroll && overflowX != OverflowScroll) { oldStyle->overflowX() == OverflowScroll && overflowX != OverflowScroll) {
ASSERT(hasHorizontalScrollbar());
horizontalScrollbar()->setEnabled(true); horizontalScrollbar()->setEnabled(true);
} }
if (needsVerticalScrollbar && oldStyle && if (hasVerticalScrollbar() && oldStyle &&
oldStyle->overflowY() == OverflowScroll && overflowY != OverflowScroll) { oldStyle->overflowY() == OverflowScroll && overflowY != OverflowScroll) {
ASSERT(hasVerticalScrollbar());
verticalScrollbar()->setEnabled(true); verticalScrollbar()->setEnabled(true);
} }
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment