Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(422)

Issue 99663004: Avoid layout/full-repaint on view height change if possible (Closed)

Created:
7 years ago by Xianzhu
Modified:
6 years, 11 months ago
CC:
blink-reviews, bemjb+rendering_chromium.org, zoltan1, eae+blinkwatch, leviw+renderwatch, blink-layers+watch_chromium.org, jchaffraix+rendering
Visibility:
Public.

Description

Avoid layout/full-repaint on view height change if possible Previously we always layout/full-repaint of the whole view when the height of the RenderView changes. In many cases this is unnecessary. Especially, this causes unnecessary cost on Android when the top control shows or hides. Avoid repaint if the RenderView doesn't needs full layout. BUG=258219 BUG=327815 (about RenderBlock::percentHeightDescendants) TEST=WebFrameTest.heightChangeRepaint TEST=all layout tests still pass Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=164645

Patch Set 1 : #

Patch Set 2 : works #

Patch Set 3 : Always layout fixed positin #

Patch Set 4 : Protect the feature with a runtime flag #

Total comments: 14

Patch Set 5 : quicks mode; dialogs #

Total comments: 10

Patch Set 6 : We have Document::hasViewportUnits() now #

Total comments: 2

Patch Set 7 : Avoid hang in seamless frames #

Unified diffs Side-by-side diffs Delta from patch set Stats (+208 lines, -12 lines) Patch
Source/core/frame/FrameView.cpp View 1 2 3 4 5 6 1 chunk +14 lines, -1 line 0 comments Download
M Source/core/rendering/RenderBlock.h View 1 2 3 4 5 1 chunk +7 lines, -1 line 0 comments Download
M Source/core/rendering/RenderBox.cpp View 1 2 3 4 5 2 chunks +3 lines, -3 lines 0 comments Download
M Source/core/rendering/RenderView.h View 1 2 3 4 5 2 chunks +4 lines, -0 lines 0 comments Download
M Source/core/rendering/RenderView.cpp View 1 2 3 4 5 1 chunk +59 lines, -0 lines 0 comments Download
M Source/core/rendering/svg/RenderSVGRoot.cpp View 1 2 3 4 5 1 chunk +2 lines, -3 lines 0 comments Download
M Source/web/tests/WebFrameTest.cpp View 1 2 3 4 5 2 chunks +59 lines, -4 lines 0 comments Download
A Source/web/tests/data/repaint/height-change-no-full-repaint1.html View 1 1 chunk +3 lines, -0 lines 0 comments Download
A Source/web/tests/data/repaint/height-change-no-full-repaint2.html View 1 1 chunk +5 lines, -0 lines 0 comments Download
A Source/web/tests/data/repaint/height-change-repaint1.html View 1 1 chunk +4 lines, -0 lines 0 comments Download
A Source/web/tests/data/repaint/height-change-repaint2.html View 1 1 chunk +5 lines, -0 lines 0 comments Download
A Source/web/tests/data/repaint/height-change-repaint3.html View 1 1 chunk +7 lines, -0 lines 0 comments Download
A Source/web/tests/data/repaint/height-change-repaint4.html View 1 1 chunk +3 lines, -0 lines 0 comments Download
A Source/web/tests/data/repaint/height-change-repaint5.html View 1 1 chunk +3 lines, -0 lines 0 comments Download
A Source/web/tests/data/repaint/height-change-repaint6.html View 1 1 chunk +3 lines, -0 lines 0 comments Download
A Source/web/tests/data/repaint/height-change-repaint7.html View 1 1 chunk +17 lines, -0 lines 0 comments Download
A Source/web/tests/data/repaint/height-change-repaint8.html View 1 2 3 4 5 1 chunk +10 lines, -0 lines 0 comments Download

Messages

Total messages: 15 (0 generated)
Xianzhu
This CL depends on https://codereview.chromium.org/82083002/ about the correctness of viewport-percentage-lengths.
7 years ago (2013-12-14 00:21:43 UTC) #1
Xianzhu
Now added RuntimeEnabledFeatures::optimizedLayoutOnViewHeightChangeEnabled to disable the changed feature by default.
7 years ago (2013-12-16 23:34:53 UTC) #2
ojan
FYI, tony doesn't work on Chromium anymore. I'm worried that needsLayoutOnHeightChange is fragile and is ...
7 years ago (2013-12-18 00:48:19 UTC) #3
Xianzhu
On 2013/12/18 00:48:19, ojan wrote: > I'm worried that needsLayoutOnHeightChange is fragile and is missing ...
7 years ago (2013-12-18 20:22:45 UTC) #4
Xianzhu
Tried the following: - flex box: the height of <body> or <html> won't be automatically ...
7 years ago (2013-12-19 20:51:16 UTC) #5
ojan
This is close... https://codereview.chromium.org/99663004/diff/100001/Source/core/frame/FrameView.cpp File Source/core/frame/FrameView.cpp (right): https://codereview.chromium.org/99663004/diff/100001/Source/core/frame/FrameView.cpp#newcode3471 Source/core/frame/FrameView.cpp:3471: ScrollView::contentsResized(); This deserves a comment as ...
6 years, 11 months ago (2014-01-03 02:04:25 UTC) #6
Xianzhu
https://codereview.chromium.org/99663004/diff/100001/Source/core/frame/FrameView.cpp File Source/core/frame/FrameView.cpp (right): https://codereview.chromium.org/99663004/diff/100001/Source/core/frame/FrameView.cpp#newcode3471 Source/core/frame/FrameView.cpp:3471: ScrollView::contentsResized(); On 2014/01/03 02:04:26, ojan wrote: > This deserves ...
6 years, 11 months ago (2014-01-03 20:46:24 UTC) #7
ojan
lgtm https://codereview.chromium.org/99663004/diff/180001/Source/web/tests/WebFrameTest.cpp File Source/web/tests/WebFrameTest.cpp (right): https://codereview.chromium.org/99663004/diff/180001/Source/web/tests/WebFrameTest.cpp#newcode106 Source/web/tests/WebFrameTest.cpp:106: #define EXPECT_EQ_RECT(a, b) \ Does this really need ...
6 years, 11 months ago (2014-01-03 21:19:00 UTC) #8
Xianzhu
Thanks for review. Will make sure the positioned/non-positioned percent height code-paths before submitting. https://codereview.chromium.org/99663004/diff/180001/Source/web/tests/WebFrameTest.cpp File ...
6 years, 11 months ago (2014-01-03 22:11:52 UTC) #9
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/wangxianzhu@chromium.org/99663004/180001
6 years, 11 months ago (2014-01-07 01:06:25 UTC) #10
commit-bot: I haz the power
Retried try job too often on linux_blink_rel for step(s) webkit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_blink_rel&number=16542
6 years, 11 months ago (2014-01-07 11:12:15 UTC) #11
Xianzhu
PTAL. In the previous patch set the added synchronous sendResizeEventIfNeeded() call failed fast/frames/seamless/seamless-resize-event-hang.html. Changed the ...
6 years, 11 months ago (2014-01-07 18:33:03 UTC) #12
ojan
lgtm
6 years, 11 months ago (2014-01-07 21:35:24 UTC) #13
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/wangxianzhu@chromium.org/99663004/460001
6 years, 11 months ago (2014-01-07 21:35:33 UTC) #14
commit-bot: I haz the power
6 years, 11 months ago (2014-01-08 01:16:30 UTC) #15
Message was sent while issue was closed.
Change committed as 164645

Powered by Google App Engine
This is Rietveld 408576698