Re-Compute slow scroll region on frameview visibility change.

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}

[MuVen, 2016-10-12 15:08:00]

The CQ bit was checked by sataya.m@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2016-10-12 15:08:14]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[MuVen, 2016-10-12 15:09:00]

Description was changed from

==========
done


done


done

BUG=
==========

to

==========
RLS scrolling coordinator tests

BUG=544133
==========

[commit-bot: I haz the power, 2016-10-12 16:40:51]

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

[commit-bot: I haz the power, 2016-10-12 16:40:51]

Dry run: This issue passed the CQ dry run.

[MuVen, 2016-10-13 06:16:08]

Description was changed from

==========
RLS scrolling coordinator tests

BUG=544133
==========

to

==========
[RLS] Don't compute slow scroll region if its hidden.

[RLS] Don't compute slow scroll region if the frameview visibility is 
hidden.

BUG=544133
==========

[MuVen, 2016-10-13 06:16:29]

sataya.m@samsung.com changed reviewers:
+ majidvp@chromium.org, skobes@chromium.org

[MuVen, 2016-10-13 06:17:27]

Description was changed from

==========
[RLS] Don't compute slow scroll region if its hidden.

[RLS] Don't compute slow scroll region if the frameview visibility is 
hidden.

BUG=544133
==========

to

==========
[RLS] Don't compute slow scroll region if its hidden.

[RLS] Don't compute slow scroll region if the frameview visibility is 
hidden. This patch fixes the 
scrollingcoordinator/non-fast-scrollable-visibility-change.html test
in case of RLS.

BUG=544133
==========

[MuVen, 2016-10-13 06:22:42]

@Majid, PTAL

[majidvp, 2016-10-13 14:34:42]

On 2016/10/13 06:22:42, MuVen wrote:
> @Majid, PTAL


Do we need to add a layout test that verifies we don't compute scroll regions
for 
hidden frames? This should hold true whether root layer scrolls or not.

nit:
RLS is not necessary in CL title because the change is applied regardless.
Prefer something like: Don't compute slow scroll regions for hidden frames.

Also, please don't use RLS acronym in CL description rather use "root layer
scrolls"
which is easier to understand.

[MuVen, 2016-10-13 16:02:18]

Description was changed from

==========
[RLS] Don't compute slow scroll region if its hidden.

[RLS] Don't compute slow scroll region if the frameview visibility is 
hidden. This patch fixes the 
scrollingcoordinator/non-fast-scrollable-visibility-change.html test
in case of RLS.

BUG=544133
==========

to

==========
Don't compute slow scroll region for hidden frames.

Don't compute slow scroll region for hidden frames irrespective of root layer
scrolls.

BUG=544133
==========

[MuVen, 2016-10-13 16:02:56]

On 2016/10/13 14:34:42, majidvp wrote:
> On 2016/10/13 06:22:42, MuVen wrote:
> > @Majid, PTAL
> 
> 
> Do we need to add a layout test that verifies we don't compute scroll regions
> for 
> hidden frames? This should hold true whether root layer scrolls or not.
> 
> nit:
> RLS is not necessary in CL title because the change is applied regardless.
> Prefer something like: Don't compute slow scroll regions for hidden frames.
> 
> Also, please don't use RLS acronym in CL description rather use "root layer
> scrolls"
> which is easier to understand.

Non-fast-scrollable-visibility-change.html
Already does the test for compute scroll regions for hidden frames irrespective
of root layer scrolls. Wdyt?

[majidvp, 2016-10-13 16:07:17]

On 2016/10/13 16:02:56, MuVen wrote:
> On 2016/10/13 14:34:42, majidvp wrote:
> > On 2016/10/13 06:22:42, MuVen wrote:
> > > @Majid, PTAL
> > 
> > 
> > Do we need to add a layout test that verifies we don't compute scroll
regions
> > for 
> > hidden frames? This should hold true whether root layer scrolls or not.
> > 
> > nit:
> > RLS is not necessary in CL title because the change is applied regardless.
> > Prefer something like: Don't compute slow scroll regions for hidden frames.
> > 
> > Also, please don't use RLS acronym in CL description rather use "root layer
> > scrolls"
> > which is easier to understand.
> 
> Non-fast-scrollable-visibility-change.html
> Already does the test for compute scroll regions for hidden frames
irrespective
> of root layer scrolls. Wdyt?

I don't think it does otherwise it would have failed without this change but it
is 
passing.

I think what you want is a slow scroll region within the iframe which gets
hidden.
If I am correct right now, we include that slow scroll region but we shouldn't.
There is no test that covers that case.

[MuVen, 2016-10-14 06:43:32]

The CQ bit was checked by sataya.m@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2016-10-14 06:43:42]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-14 07:51:26]

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

[commit-bot: I haz the power, 2016-10-14 07:51:27]

Dry run: This issue passed the CQ dry run.

[MuVen, 2016-10-14 08:01:50]

The CQ bit was checked by sataya.m@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2016-10-14 08:02:06]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-14 09:33:35]

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

[commit-bot: I haz the power, 2016-10-14 09:33:36]

Dry run: This issue passed the CQ dry run.

[MuVen, 2016-10-14 09:44:56]

Hi Majid,

PTAL at the test case. While debugging this test case i saw that, this testcase
was passing when i run for RLS(virtual/rootlayerscrolls) and was failing for non
RLS.
js-test.js(patch#2) based test was passing for both RLS and Non RLS.

After debugging a bit i saw that, when the iframe was hidden
scrollableAreasDidChange
wasnt triggered which sets m_scrollGestureRegionIsDirty to true. The reason for
this 
is in FrameView::removeScrollableArea  m_scrollableAreas is null for the
iframe's parent
frameview due to which it is returning without dirtying the
scrollingcoordinator.

do you think the above changes in frameview looks good ? i see irrespective of
adding/removing scrollablearea we are gonna update after compositing.

Please share your thoughts.

Thanks,

[majidvp, 2016-10-17 13:59:47]

On 2016/10/14 09:44:56, MuVen wrote:
>  The reason for this 
> is in FrameView::removeScrollableArea  m_scrollableAreas is null for the
> iframe's parent
> frameview due to which it is returning without dirtying the
> scrollingcoordinator.

So if m_scrollableAreas is null, that means we never added an scrollable
area to this frame which is odd! How come we are removing something that 
was never added? Also if it was never added why do we need to dirty 
m_scrollGestureRegionIsDirty given that there was not change to the set.


> do you think the above changes in frameview looks good ? i see irrespective of
> adding/removing scrollablearea we are gonna update after compositing.
> 

I don't think this change make sense. In particular,
FrameView::addScrollableArea
are not just called by child frames but also all other inner scrollable. So you 
change breaks those.  {add/remove}ScrollableArea seems the logical place to 
update the dirty flag as opposed to all the calling functions.

[MuVen, 2016-10-21 10:11:49]

The CQ bit was checked by sataya.m@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2016-10-21 10:11:54]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[MuVen, 2016-10-21 10:13:45]

Description was changed from

==========
Don't compute slow scroll region for hidden frames.

Don't compute slow scroll region for hidden frames irrespective of root layer
scrolls.

BUG=544133
==========

to

==========
Don't compute slow scroll region for hidden frames.

Don't compute slow scroll region for hidden frames. If the Frame is hidden skip
computing slow scroll region for hidden frames.

BUG=544133
==========

[commit-bot: I haz the power, 2016-10-21 12:37:52]

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

[commit-bot: I haz the power, 2016-10-21 12:37:53]

Dry run: This issue passed the CQ dry run.

[MuVen, 2016-10-21 12:51:04]

Hi Majid,

PTAL at this patch. Currently this patch will recompute slow scrolls, if the
we hide div/iframe with in the iframe. If there is a iframe for the main
Frameview,
and iframe is hidden, in that case recomputation is not happening, as 
we didnt add iframe scrollable area to main Frameview's scrollableArea set.
Ideally we should recompute slow scrolls on hiding iframe. 
I shall raise the bug for this issue and work. wdyt ? let me know your thoughts.

Thanks,

[MuVen, 2016-10-24 15:29:20]

sataya.m@samsung.com changed reviewers:
- skobes@chromium.org

[MuVen, 2016-10-24 15:29:21]

@majid, Friendly Ping.

[majidvp, 2016-11-01 20:59:33]

On 2016/10/24 15:29:21, MuVen wrote:
> @majid, Friendly Ping.

> @majid, Friendly Ping.

> PTAL at this patch. Currently this patch will recompute slow scrolls, if the
we hide div/iframe with in the iframe. If there is a iframe for the main
Frameview,

I am very confused about why you need to have three nested levels of iframes.


> Ideally we should recompute slow scrolls on hiding iframe. 
I agree! Let's do it in this patch. I think it will fix the problems.

Here is what I understand and suggest.

The Bug 
--------
we used to include scrollable regions of nested iframes into our
final calculations even if that frame was not visible.
This showed up when using "--root-layer-scrolls" but it applies to any inner
slow scrollable within a hidden frame.

Proposed fix
------------
Do not recurse over hidden frame.

This seemed to work fine except we don't always trigger recomputation when a
frame shows/hides but only when its scrollability changes which may not 
always be the case (I think this was the cause of the odd behaviour you were
seeing before).


Here is what I think may work

1) Only recurse visible frames when computing slow scroll regions in
ScrollingCoordinator (you 
  already have that in your patch)
2) Always trigger a re-computation when FrameView visibility changes. (You had
some version of 
  that in your second patch but it was at the wrong location). I suggest adding
a new method to 
  ScrollingCoordinator which dirties the relevant flag and calling that from
FrameView::{show,hide}.
  (e.g., ScrollingCoordinator::frameViewVisibilitDidChange)

3) leave FrameView::updateScrollableAreaSet as is. (suggested nit: perhaps
rename to 
   updateParentScrollableAreaSet to make its function clear)

Let me know if this makes sense or if I missed something.

[majidvp, 2016-11-01 20:59:50]

https://codereview.chromium.org/2413723002/diff/60001/third_party/WebKit/Layo...
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/Layo...
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 to have 3 levels of nested iframes. 
Cannot the test just have an a single scrollable iframe within the top level
document 
and hide/show that frame and veify the effect?

https://codereview.chromium.org/2413723002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/scrollingcoordinator/donot-compute-non-fast-scrollable-region-for-hidden-frames.html:34:
divelement.style.visibility = 'hidden';
Why do you need to hide the div here?

[MuVen, 2016-11-02 11:33:24]

The CQ bit was checked by sataya.m@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2016-11-02 11:33:32]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[MuVen, 2016-11-02 12:04:49]

Description was changed from

==========
Don't compute slow scroll region for hidden frames.

Don't compute slow scroll region for hidden frames. If the Frame is hidden skip
computing slow scroll region for hidden frames.

BUG=544133
==========

to

==========
Re-Compute slow scroll region on frameview visibility change.

Re-Compute slow scroll region on frameview visibility change, and 
also if the Frame is hidden skip computing slow scroll region for 
hidden frames.

BUG=544133
==========

[MuVen, 2016-11-02 12:05:54]

Description was changed from

==========
Re-Compute slow scroll region on frameview visibility change.

Re-Compute slow scroll region on frameview visibility change, and 
also if the Frame is hidden skip computing slow scroll region for 
hidden frames.

BUG=544133
==========

to

==========
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
==========

[commit-bot: I haz the power, 2016-11-02 13:03:55]

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

[commit-bot: I haz the power, 2016-11-02 13:03:56]

Dry run: This issue passed the CQ dry run.

[MuVen, 2016-11-02 13:15:51]

PTAL.

[majidvp, 2016-11-02 14:15:17]

lgtm with nit.

https://codereview.chromium.org/2413723002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/2413723002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/FrameView.cpp:4223: void
FrameView::frameViewVisibilityChanged() {
This function seems superfulous. It is reducing one very
simple line. I would get rid of it.

[majidvp, 2016-11-02 14:17:57]

https://codereview.chromium.org/2413723002/diff/80001/third_party/WebKit/Layo...
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/Layo...
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/Layo...
third_party/WebKit/LayoutTests/scrollingcoordinator/donot-compute-non-fast-scrollable-region-for-hidden-frames.html:37:
}, "This test ensures that non-fast scrollable area is re-computed on FrameView
visibility changed.");
nit: s/changed/changes/

[MuVen, 2016-11-02 17:07:48]

addressed review comments. Committing the changes.

https://codereview.chromium.org/2413723002/diff/80001/third_party/WebKit/Layo...
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/Layo...
third_party/WebKit/LayoutTests/scrollingcoordinator/donot-compute-non-fast-scrollable-region-for-hidden-frames.html:28:
var iframeelement = document.querySelector("iframe");
On 2016/11/02 14:17:57, majidvp wrote:
> nit: s/iframeelement/iframeElement

Done.

https://codereview.chromium.org/2413723002/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/scrollingcoordinator/donot-compute-non-fast-scrollable-region-for-hidden-frames.html:37:
}, "This test ensures that non-fast scrollable area is re-computed on FrameView
visibility changed.");
On 2016/11/02 14:17:57, majidvp wrote:
> nit: s/changed/changes/

Done.

https://codereview.chromium.org/2413723002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/2413723002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/FrameView.cpp:4223: void
FrameView::frameViewVisibilityChanged() {
On 2016/11/02 14:15:17, majidvp wrote:
> This function seems superfulous. It is reducing one very
> simple line. I would get rid of it.

Done.

[MuVen, 2016-11-02 17:08:36]

The CQ bit was checked by sataya.m@samsung.com

[MuVen, 2016-11-02 17:08:36]

The patchset sent to the CQ was uploaded after l-g-t-m from majidvp@chromium.org
Link to the patchset: https://codereview.chromium.org/2413723002/#ps100001
(title: "addressed review comments")

[commit-bot: I haz the power, 2016-11-02 17:09:09]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[majidvp, 2016-11-02 17:11:13]

> addressed review comments. Committing the changes.

You need an OWNER's approval before being able to commit.

[commit-bot: I haz the power, 2016-11-02 17:17:10]

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

[commit-bot: I haz the power, 2016-11-02 17:17:12]

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_presub...)

[MuVen, 2016-11-02 17:43:47]

@majid, Thanks for your suggestion. I thought u r the owner.

@skobes, PTAL.

[skobes, 2016-11-02 18:17:41]

skobes@chromium.org changed reviewers:
+ skobes@chromium.org

[skobes, 2016-11-02 18:17:42]

lgtm

[MuVen, 2016-11-02 23:28:48]

The CQ bit was checked by sataya.m@samsung.com

[commit-bot: I haz the power, 2016-11-02 23:29:23]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-02 23:36:36]

Description was changed from

==========
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
==========

to

==========
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
==========

[commit-bot: I haz the power, 2016-11-02 23:36:38]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-11-02 23:41:18]

Description was changed from

==========
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
==========

to

==========
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}
==========

[commit-bot: I haz the power, 2016-11-02 23:41:19]

Patchset 6 (id:??) landed as
https://crrev.com/e300a10cc746c646d5f1a7b0ddd48776ad090224
Cr-Commit-Position: refs/heads/master@{#429452}