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

Issue 2413723002: Re-Compute slow scroll region on frameview visibility change. (Closed)

Created:
4 years, 2 months ago by MuVen
Modified:
4 years, 1 month ago
Reviewers:
majidvp, skobes
CC:
chromium-reviews, blink-reviews, kenneth.christiansen, blink-layers+watch_chromium.org, skobes
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Re-Compute slow scroll region on frameview visibility change. Re-Compute slow scroll region on frameview visibility change, also if the Frame is hidden skip computing slow scroll region for hidden frames. BUG=544133 Committed: https://crrev.com/e300a10cc746c646d5f1a7b0ddd48776ad090224 Cr-Commit-Position: refs/heads/master@{#429452}

Patch Set 1 #

Patch Set 2 : addressed review comments #

Patch Set 3 : testharness based test case #

Patch Set 4 : modified test case #

Total comments: 2

Patch Set 5 : addressed reviiew comments #

Total comments: 6

Patch Set 6 : addressed review comments #

Messages

Total messages: 58 (36 generated)
MuVen
@Majid, PTAL
4 years, 2 months ago (2016-10-13 06:22:42 UTC) #9
majidvp
On 2016/10/13 06:22:42, MuVen wrote: > @Majid, PTAL Do we need to add a layout ...
4 years, 2 months ago (2016-10-13 14:34:42 UTC) #10
MuVen
On 2016/10/13 14:34:42, majidvp wrote: > On 2016/10/13 06:22:42, MuVen wrote: > > @Majid, PTAL ...
4 years, 2 months ago (2016-10-13 16:02:56 UTC) #12
majidvp
On 2016/10/13 16:02:56, MuVen wrote: > On 2016/10/13 14:34:42, majidvp wrote: > > On 2016/10/13 ...
4 years, 2 months ago (2016-10-13 16:07:17 UTC) #13
MuVen
Hi Majid, PTAL at the test case. While debugging this test case i saw that, ...
4 years, 2 months ago (2016-10-14 09:44:56 UTC) #22
majidvp
On 2016/10/14 09:44:56, MuVen wrote: > The reason for this > is in FrameView::removeScrollableArea m_scrollableAreas ...
4 years, 2 months ago (2016-10-17 13:59:47 UTC) #23
MuVen
Hi Majid, PTAL at this patch. Currently this patch will recompute slow scrolls, if the ...
4 years, 2 months ago (2016-10-21 12:51:04 UTC) #29
MuVen
@majid, Friendly Ping.
4 years, 1 month ago (2016-10-24 15:29:21 UTC) #31
majidvp
On 2016/10/24 15:29:21, MuVen wrote: > @majid, Friendly Ping. > @majid, Friendly Ping. > PTAL ...
4 years, 1 month ago (2016-11-01 20:59:33 UTC) #32
majidvp
https://codereview.chromium.org/2413723002/diff/60001/third_party/WebKit/LayoutTests/scrollingcoordinator/donot-compute-non-fast-scrollable-region-for-hidden-frames.html File third_party/WebKit/LayoutTests/scrollingcoordinator/donot-compute-non-fast-scrollable-region-for-hidden-frames.html (right): https://codereview.chromium.org/2413723002/diff/60001/third_party/WebKit/LayoutTests/scrollingcoordinator/donot-compute-non-fast-scrollable-region-for-hidden-frames.html#newcode31 third_party/WebKit/LayoutTests/scrollingcoordinator/donot-compute-non-fast-scrollable-region-for-hidden-frames.html:31: iframeelement.style.visibility = 'hidden'; I don't understand why you need ...
4 years, 1 month ago (2016-11-01 20:59:50 UTC) #33
MuVen
PTAL.
4 years, 1 month ago (2016-11-02 13:15:51 UTC) #40
majidvp
lgtm with nit. https://codereview.chromium.org/2413723002/diff/80001/third_party/WebKit/Source/core/frame/FrameView.cpp File third_party/WebKit/Source/core/frame/FrameView.cpp (right): https://codereview.chromium.org/2413723002/diff/80001/third_party/WebKit/Source/core/frame/FrameView.cpp#newcode4223 third_party/WebKit/Source/core/frame/FrameView.cpp:4223: void FrameView::frameViewVisibilityChanged() { This function seems ...
4 years, 1 month ago (2016-11-02 14:15:17 UTC) #41
majidvp
https://codereview.chromium.org/2413723002/diff/80001/third_party/WebKit/LayoutTests/scrollingcoordinator/donot-compute-non-fast-scrollable-region-for-hidden-frames.html File third_party/WebKit/LayoutTests/scrollingcoordinator/donot-compute-non-fast-scrollable-region-for-hidden-frames.html (right): https://codereview.chromium.org/2413723002/diff/80001/third_party/WebKit/LayoutTests/scrollingcoordinator/donot-compute-non-fast-scrollable-region-for-hidden-frames.html#newcode28 third_party/WebKit/LayoutTests/scrollingcoordinator/donot-compute-non-fast-scrollable-region-for-hidden-frames.html:28: var iframeelement = document.querySelector("iframe"); nit: s/iframeelement/iframeElement https://codereview.chromium.org/2413723002/diff/80001/third_party/WebKit/LayoutTests/scrollingcoordinator/donot-compute-non-fast-scrollable-region-for-hidden-frames.html#newcode37 third_party/WebKit/LayoutTests/scrollingcoordinator/donot-compute-non-fast-scrollable-region-for-hidden-frames.html:37: }, ...
4 years, 1 month ago (2016-11-02 14:17:57 UTC) #42
MuVen
addressed review comments. Committing the changes. https://codereview.chromium.org/2413723002/diff/80001/third_party/WebKit/LayoutTests/scrollingcoordinator/donot-compute-non-fast-scrollable-region-for-hidden-frames.html File third_party/WebKit/LayoutTests/scrollingcoordinator/donot-compute-non-fast-scrollable-region-for-hidden-frames.html (right): https://codereview.chromium.org/2413723002/diff/80001/third_party/WebKit/LayoutTests/scrollingcoordinator/donot-compute-non-fast-scrollable-region-for-hidden-frames.html#newcode28 third_party/WebKit/LayoutTests/scrollingcoordinator/donot-compute-non-fast-scrollable-region-for-hidden-frames.html:28: var iframeelement = ...
4 years, 1 month ago (2016-11-02 17:07:48 UTC) #43
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2413723002/100001
4 years, 1 month ago (2016-11-02 17:09:09 UTC) #46
majidvp
> addressed review comments. Committing the changes. You need an OWNER's approval before being able ...
4 years, 1 month ago (2016-11-02 17:11:13 UTC) #47
commit-bot: I haz the power
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presubmit/builds/295022)
4 years, 1 month ago (2016-11-02 17:17:12 UTC) #49
MuVen
@majid, Thanks for your suggestion. I thought u r the owner. @skobes, PTAL.
4 years, 1 month ago (2016-11-02 17:43:47 UTC) #50
skobes
lgtm
4 years, 1 month ago (2016-11-02 18:17:42 UTC) #52
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2413723002/100001
4 years, 1 month ago (2016-11-02 23:29:23 UTC) #54
commit-bot: I haz the power
Committed patchset #6 (id:100001)
4 years, 1 month ago (2016-11-02 23:36:38 UTC) #56
commit-bot: I haz the power
4 years, 1 month ago (2016-11-02 23:41:19 UTC) #58
Message was sent while issue was closed.
Patchset 6 (id:??) landed as
https://crrev.com/e300a10cc746c646d5f1a7b0ddd48776ad090224
Cr-Commit-Position: refs/heads/master@{#429452}

Powered by Google App Engine
This is Rietveld 408576698