Issue 328023002: Layout overflow from transforms should update frame scrollbars

Issue 328023002: Layout overflow from transforms should update frame scrollbars (Closed)

Created:
6 years, 6 months ago by trchen

Modified:
6 years, 6 months ago

Reviewers:
Julien - ping for review, esprehn, ojan

CC:
blink-reviews

Base URL:
svn://svn.chromium.org/blink/trunk

Visibility:
Public.

More Reviews

Description

Layout overflow from transforms should update frame scrollbars This CL implements frame scrollbar update for the overflow recalc fast path without invoking layout. This fast path first determines whether scrollbars need to be created or removed (in that case, a layout will be needed), and update scrollbars without layout if possible. TEST=fast/transforms/transform-update-frame-overflow.html BUG=354167 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=176143

Patch Set 1 #

Patch Set 2 : need to more explicitly prevent scrollbar update #

Total comments: 13

Patch Set 3 : revised #

Patch Set 4 : revised 2 #

Total comments: 1

Patch Set 5 : rebase & revise #

Created: 6 years, 6 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+62 lines, -4 lines)			Patch
A	LayoutTests/fast/transforms/transform-update-frame-overflow.html	View	1 2	1 chunk	+18 lines, -0 lines	0 comments	Download
A	LayoutTests/fast/transforms/transform-update-frame-overflow-expected.html	View	1 2	1 chunk	+14 lines, -0 lines	0 comments	Download
M	Source/core/frame/FrameView.cpp	View	1 2 3 4	1 chunk	+19 lines, -2 lines	0 comments	Download
M	Source/platform/scroll/ScrollView.h	View	1 2 3 4	2 chunks	+10 lines, -0 lines	0 comments	Download
M	Source/platform/scroll/ScrollView.cpp	View	1 2	2 chunks	+1 line, -2 lines	0 comments	Download

Messages

Total messages: 22 (0 generated)

Expand Messages | Collapse Messages

trchen

Hello reviewers, This CL completes the transform update fast path, PTAL. Thanks!

6 years, 6 months ago (2014-06-11 00:05:03 UTC) #1

ojan

This looks reasonable to me. Just a bunch of nits. Hopefully some others can also ...

6 years, 6 months ago (2014-06-11 03:04:54 UTC) #3

ojan

https://codereview.chromium.org/328023002/diff/20001/Source/core/frame/FrameView.cpp File Source/core/frame/FrameView.cpp (right): https://codereview.chromium.org/328023002/diff/20001/Source/core/frame/FrameView.cpp#newcode653 Source/core/frame/FrameView.cpp:653: setNeedsLayout(); Should this not also call the following? setHasHorizontalScrollbar(shouldHaveHorizontalScrollbar); ...

6 years, 6 months ago (2014-06-11 03:07:24 UTC) #4

trchen

Thanks for reviewing! I also think the scrollbar logic has always been tricky and fragile. ...

6 years, 6 months ago (2014-06-11 05:04:13 UTC) #5

Thanks for reviewing! I also think the scrollbar logic has always been tricky
and fragile. I'm more than happy to implement it if you have some idea to
improve it. :)

https://codereview.chromium.org/328023002/diff/20001/LayoutTests/fast/transfo...
File LayoutTests/fast/transforms/transform-update-frame-overflow.html (right):

https://codereview.chromium.org/328023002/diff/20001/LayoutTests/fast/transfo...
LayoutTests/fast/transforms/transform-update-frame-overflow.html:1: <style>
On 2014/06/11 03:04:53, ojan wrote:
> Nit: Please used <!DOCTYPE html>. We try to default to standards mode unless
> we're specifically trying to test quirks mode.

Thanks for catching this!

https://codereview.chromium.org/328023002/diff/20001/Source/core/frame/FrameV...
File Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/328023002/diff/20001/Source/core/frame/FrameV...
Source/core/frame/FrameView.cpp:644: InUpdateScrollbarsScope
inUpdateScrollbarsScope(this);
On 2014/06/11 03:04:53, ojan wrote:
> Why do you need this here?

Without this, adjustViewSize() on line #657 will call setContentsSize() then
updateScrollbars() without protection, and FrameView::layout() will be called.
This will be caught by an assertion in Document::updateStyle() due to messed up
DocumentLifecycle.

I think it is okay to use InUpdateScrollbarsScope here, since we're essentially
doing a quick version of updateScrollbars().

https://codereview.chromium.org/328023002/diff/20001/Source/core/frame/FrameV...
Source/core/frame/FrameView.cpp:652: if (hasHorizontalScrollbar !=
shouldHaveHorizontalScrollbar || hasVerticalScrollbar !=
shouldHaveVerticalScrollbar) {
On 2014/06/11 03:04:53, ojan wrote:
> Nit: Can you do the following?
> if (horizontalScrollbar() != shouldHaveHorizontalScrollbar ||
> verticalScrollbar() != shouldHaveVerticalScrollbar) {
> 
> It would make the code a bit more concise.

Can't compare between pointer and bool. Will do this instead:

if (static_cast<bool>(horizontalScrollbar()) != shouldHaveHorizontalScrollbar ||
static_cast<bool>(verticalScrollbar()) != shouldHaveVerticalScrollbar) {

https://codereview.chromium.org/328023002/diff/20001/Source/core/frame/FrameV...
Source/core/frame/FrameView.cpp:653: setNeedsLayout();
On 2014/06/11 03:07:24, ojan wrote:
> Should this not also call the following?
>     setHasHorizontalScrollbar(shouldHaveHorizontalScrollbar);
>     setHasVerticalScrollbar(shouldHaveVerticalScrollbar);

adjustViewSize() will be called in post-layout phase, which will
setContentsSize() with the latest layout overflow and invoke full
updateScrollbars().

Adjusting scrollbar here may save us some time on the next updateScrollbars().
I'm worrying about unknown side effects, for example, newly revealed content
from deleting scrollbar might not get correctly invalidated.

https://codereview.chromium.org/328023002/diff/20001/Source/platform/scroll/S...
File Source/platform/scroll/ScrollView.cpp (right):

https://codereview.chromium.org/328023002/diff/20001/Source/platform/scroll/S...
Source/platform/scroll/ScrollView.cpp:39:
ScrollView::InUpdateScrollbarsScope::InUpdateScrollbarsScope(ScrollView* view)
On 2014/06/11 03:04:53, ojan wrote:
> Nit: This constructor doesn't really do any work. I'd just put it in the
header.

SGTM. I was hesitating.

ojan

lgtm Obviously, please address all the comments before committing. https://codereview.chromium.org/328023002/diff/20001/Source/core/frame/FrameView.cpp File Source/core/frame/FrameView.cpp (right): https://codereview.chromium.org/328023002/diff/20001/Source/core/frame/FrameView.cpp#newcode644 Source/core/frame/FrameView.cpp:644: ...

6 years, 6 months ago (2014-06-11 05:44:48 UTC) #6

trchen

Thank you! I'll run the bots first. Will commit tomorrow if no objections from other ...

6 years, 6 months ago (2014-06-11 06:01:23 UTC) #7

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/trchen@chromium.org/328023002/80001

6 years, 6 months ago (2014-06-11 06:02:18 UTC) #9

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/trchen@chromium.org/328023002/80001

6 years, 6 months ago (2014-06-11 20:41:57 UTC) #12

commit-bot: I haz the power

FYI, CQ is re-trying this CL (attempt #1). The failing builders are: blink_presubmit on tryserver.blink ...

6 years, 6 months ago (2014-06-11 21:05:02 UTC) #13

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

6 years, 6 months ago (2014-06-11 21:07:24 UTC) #14

commit-bot: I haz the power

Try jobs failed on following builders: blink_presubmit on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/blink_presubmit/builds/7474)

6 years, 6 months ago (2014-06-11 21:07:25 UTC) #15

trchen

Oops, just realize that it still needs owner review for ScrollView. Julien, could you help? ...

6 years, 6 months ago (2014-06-11 21:55:04 UTC) #18

esprehn

lgtm https://codereview.chromium.org/328023002/diff/80001/Source/platform/scroll/ScrollView.h File Source/platform/scroll/ScrollView.h (right): https://codereview.chromium.org/328023002/diff/80001/Source/platform/scroll/ScrollView.h#newcode300 Source/platform/scroll/ScrollView.h:300: explicit InUpdateScrollbarsScope(ScrollView* view) : m_scope(view->m_inUpdateScrollbars, true) { } ...

6 years, 6 months ago (2014-06-13 05:22:22 UTC) #19

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/trchen@chromium.org/328023002/100001

6 years, 6 months ago (2014-06-13 22:32:25 UTC) #21

Message was sent while issue was closed.

Change committed as 176143

Expand Messages | Collapse Messages