Don't omit ScrollableAreas in non-clickable FrameViews.

Don't omit ScrollableAreas in non-clickable FrameViews.

The FrameView-is-clickable check is an optimization to avoid
over-reporting non-fast-scrollable regions.  However, there is code
in ScrollingCoordinator that serves the same purpose:

  if (!frameView || frameView->shouldThrottleRendering() ||
      !frameView->isVisible())
    return shouldHandleScrollGestureOnMainThreadRegion;

This only starts causing problems when root layer scrolling is
enabled; in particular, this scenario:

- A div has visibility:hidden
- An iframe containing a non-fast-scrollable region is appended
to the div
- After the iframe has loaded and run layout, the div is set to
visibility:visible

When the iframe first loads and runs layout, it will eventually get to
PLSA::updateScrollableAreaSet; but that method won't do anything
because the owner iframe element isn't visible to hit testing.  So the
PLSA isn't added to its FrameView's scrollable area set.

When the final style change is applied to the LayoutIFrame, it calls
FrameView::show().  Pre-RLS, that would cause the subframe to add
itself to the parent's scrollable area set.  With RLS enabled, that's
not expected to happen.  Because the iframe's document is layout
clean, PLSA::updateScrollableAreaSet() doesn't get called again, so
the PLSA never gets into its FrameView's scrollable area set.

This patch deletes a couple of tests that check for the number of
ScrollableArea's included in the FrameView's scrollableAreaSet.
That's no longer a useful number to check; there are other existing
tests that check the non-scrollable rects that are produced, which is
really the important thing.

BUG=417782
R=skobes@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Review-Url: https://codereview.chromium.org/2658333002
Cr-Commit-Position: refs/heads/master@{#447285}
Committed: https://chromium.googlesource.com/chromium/src/+/4139980b8a49249ba5389f942d808caa7c464896

[szager1, 2017-01-27 23:52:41]

Description was changed from

==========
Don't omit ScrollableAreas in non-clickable FrameViews.

The FrameView-is-clickable check is an optimization to avoid
over-reporting non-fast-scrollable regions.  However, there is code
in ScrollingCoordinator that serves the same purpose:

  if (!frameView || frameView->shouldThrottleRendering() ||
      !frameView->isVisible())
    return shouldHandleScrollGestureOnMainThreadRegion;

This only starts causing problems when root layer scrolling is
enabled; in particular, this scenario:

- A div has visibility:hidden
- An iframe containing a non-fast-scrollable region is appended
to the div
- After the iframe has loaded and run layout, the div is set to
visibility:visible

When the iframe first loads and runs layout, it will eventually get to
PLSA::updateScrollableAreaSet; but that method won't do anything
because the owner iframe element isn't visible to hit testing.  So the
PLSA isn't added to its FrameView's scrollable area set.

When the final style change is applied to the LayoutIFrame, it calls
FrameView::show().  Pre-RLS, that would cause the subframe to add
itself to the parent's scrollable area set.  With RLS enabled, that's
not expected to happen.  Because the iframe's document is layout
clean, PLSA::updateScrollableAreaSet() doesn't get called again, so
the PLSA never gets into its FrameView's scrollable area set.

This patch deletes a couple of tests that check for the number of
ScrollableArea's included in the FrameView's scrollableAreaSet.
That's no longer a useful number to check; there are other existing
tests that check the non-scrollable rects that are produced, which is
really the important thing.

BUG=417782
R=skobes@chromium.org
==========

to

==========
Don't omit ScrollableAreas in non-clickable FrameViews.

The FrameView-is-clickable check is an optimization to avoid
over-reporting non-fast-scrollable regions.  However, there is code
in ScrollingCoordinator that serves the same purpose:

  if (!frameView || frameView->shouldThrottleRendering() ||
      !frameView->isVisible())
    return shouldHandleScrollGestureOnMainThreadRegion;

This only starts causing problems when root layer scrolling is
enabled; in particular, this scenario:

- A div has visibility:hidden
- An iframe containing a non-fast-scrollable region is appended
to the div
- After the iframe has loaded and run layout, the div is set to
visibility:visible

When the iframe first loads and runs layout, it will eventually get to
PLSA::updateScrollableAreaSet; but that method won't do anything
because the owner iframe element isn't visible to hit testing.  So the
PLSA isn't added to its FrameView's scrollable area set.

When the final style change is applied to the LayoutIFrame, it calls
FrameView::show().  Pre-RLS, that would cause the subframe to add
itself to the parent's scrollable area set.  With RLS enabled, that's
not expected to happen.  Because the iframe's document is layout
clean, PLSA::updateScrollableAreaSet() doesn't get called again, so
the PLSA never gets into its FrameView's scrollable area set.

This patch deletes a couple of tests that check for the number of
ScrollableArea's included in the FrameView's scrollableAreaSet.
That's no longer a useful number to check; there are other existing
tests that check the non-scrollable rects that are produced, which is
really the important thing.

BUG=417782
R=skobes@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[szager1, 2017-01-27 23:52:58]

The CQ bit was checked by szager@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-27 23:53:19]

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

[skobes, 2017-01-28 00:26:33]

lgtm w/ nit

https://codereview.chromium.org/2658333002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp (right):

https://codereview.chromium.org/2658333002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp:1675: //
FIXME: Does this need to be fixed later for OOPI?
Remove this FIXME.

[commit-bot: I haz the power, 2017-01-28 02:01:34]

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

[commit-bot: I haz the power, 2017-01-28 02:01:35]

Dry run: This issue passed the CQ dry run.

[szager1, 2017-01-31 00:02:02]

On 2017/01/28 00:26:33, skobes wrote:
> lgtm w/ nit
> 
>
https://codereview.chromium.org/2658333002/diff/1/third_party/WebKit/Source/c...
> File third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp
(right):
> 
>
https://codereview.chromium.org/2658333002/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp:1675: //
> FIXME: Does this need to be fixed later for OOPI?
> Remove this FIXME.

Done.

[szager1, 2017-01-31 17:03:15]

The CQ bit was checked by szager@chromium.org

[szager1, 2017-01-31 17:03:16]

The patchset sent to the CQ was uploaded after l-g-t-m from skobes@chromium.org
Link to the patchset: https://codereview.chromium.org/2658333002/#ps40001
(title: "Remove obsolete FIXME")

[commit-bot: I haz the power, 2017-01-31 17:03:42]

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, 2017-01-31 19:05:34]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1485882195671530,
"parent_rev": "f0728f745fd97872e37e3b0fedb47264a7c6df13", "commit_rev":
"4139980b8a49249ba5389f942d808caa7c464896"}

[commit-bot: I haz the power, 2017-01-31 19:06:05]

Description was changed from

==========
Don't omit ScrollableAreas in non-clickable FrameViews.

The FrameView-is-clickable check is an optimization to avoid
over-reporting non-fast-scrollable regions.  However, there is code
in ScrollingCoordinator that serves the same purpose:

  if (!frameView || frameView->shouldThrottleRendering() ||
      !frameView->isVisible())
    return shouldHandleScrollGestureOnMainThreadRegion;

This only starts causing problems when root layer scrolling is
enabled; in particular, this scenario:

- A div has visibility:hidden
- An iframe containing a non-fast-scrollable region is appended
to the div
- After the iframe has loaded and run layout, the div is set to
visibility:visible

When the iframe first loads and runs layout, it will eventually get to
PLSA::updateScrollableAreaSet; but that method won't do anything
because the owner iframe element isn't visible to hit testing.  So the
PLSA isn't added to its FrameView's scrollable area set.

When the final style change is applied to the LayoutIFrame, it calls
FrameView::show().  Pre-RLS, that would cause the subframe to add
itself to the parent's scrollable area set.  With RLS enabled, that's
not expected to happen.  Because the iframe's document is layout
clean, PLSA::updateScrollableAreaSet() doesn't get called again, so
the PLSA never gets into its FrameView's scrollable area set.

This patch deletes a couple of tests that check for the number of
ScrollableArea's included in the FrameView's scrollableAreaSet.
That's no longer a useful number to check; there are other existing
tests that check the non-scrollable rects that are produced, which is
really the important thing.

BUG=417782
R=skobes@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Don't omit ScrollableAreas in non-clickable FrameViews.

The FrameView-is-clickable check is an optimization to avoid
over-reporting non-fast-scrollable regions.  However, there is code
in ScrollingCoordinator that serves the same purpose:

  if (!frameView || frameView->shouldThrottleRendering() ||
      !frameView->isVisible())
    return shouldHandleScrollGestureOnMainThreadRegion;

This only starts causing problems when root layer scrolling is
enabled; in particular, this scenario:

- A div has visibility:hidden
- An iframe containing a non-fast-scrollable region is appended
to the div
- After the iframe has loaded and run layout, the div is set to
visibility:visible

When the iframe first loads and runs layout, it will eventually get to
PLSA::updateScrollableAreaSet; but that method won't do anything
because the owner iframe element isn't visible to hit testing.  So the
PLSA isn't added to its FrameView's scrollable area set.

When the final style change is applied to the LayoutIFrame, it calls
FrameView::show().  Pre-RLS, that would cause the subframe to add
itself to the parent's scrollable area set.  With RLS enabled, that's
not expected to happen.  Because the iframe's document is layout
clean, PLSA::updateScrollableAreaSet() doesn't get called again, so
the PLSA never gets into its FrameView's scrollable area set.

This patch deletes a couple of tests that check for the number of
ScrollableArea's included in the FrameView's scrollableAreaSet.
That's no longer a useful number to check; there are other existing
tests that check the non-scrollable rects that are produced, which is
really the important thing.

BUG=417782
R=skobes@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Review-Url: https://codereview.chromium.org/2658333002
Cr-Commit-Position: refs/heads/master@{#447285}
Committed:
https://chromium.googlesource.com/chromium/src/+/4139980b8a49249ba5389f942d80...
==========

[commit-bot: I haz the power, 2017-01-31 19:06:06]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/4139980b8a49249ba5389f942d80...