[RLS] Update iframe scrollbar behaviour based on scrolling attribute

[RLS] Update iframe scrollbar behaviour based on scrolling attribute

Currently in RLS path, scrollbar existence is not updated when
"scrolling" attribute is "no". Updated PLSA::ComputeScrollbarExistence
and PLSA::UserInputScrollable to consider scrollbar modes to turn of
scrolling/scrollbars for root layers.

BUG=711474
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Review-Url: https://codereview.chromium.org/2831313002
Cr-Commit-Position: refs/heads/master@{#467207}
Committed: https://chromium.googlesource.com/chromium/src/+/31eb41c176f57dbdbf8d5ce4edd45d21a1c9a45c

[Srirama, 2017-04-21 11:00:18]

Description was changed from

==========
fix

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

to

==========
fix

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[Srirama, 2017-04-21 11:00:43]

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

[commit-bot: I haz the power, 2017-04-21 11:00:53]

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, 2017-04-21 11:55:21]

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

[commit-bot: I haz the power, 2017-04-21 11:55:23]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[Srirama, 2017-04-21 14:05:23]

Description was changed from

==========
fix

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
[RLS] Update iframe scrollbar behaviour based on scrolling attribute

Currently in RLS path, scrollbar existence is not updated when
"scrolling" attribute is "no". Updated PLSA::ComputeScrollbarExistence
and PLSA::UserInputScrollable to consider scrollbar modes to turn of
scrolling/scrollbars for root layers.

BUG=711474
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[Srirama, 2017-04-21 17:47:26]

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

[Srirama, 2017-04-21 17:47:27]

PTAL. Made changes according to the discussions. Verified locally by removing
the scroll related testcases from testexpectations but still they are failing.
Looks like some more investigation and changes are needed to pass those.

[skobes, 2017-04-21 20:00:58]

On 2017/04/21 17:47:27, Srirama wrote:
> PTAL. Made changes according to the discussions. Verified locally by removing
> the scroll related testcases from testexpectations but still they are failing.
> Looks like some more investigation and changes are needed to pass those.

Thanks, the change looks right to me.  The next step is to investigate why the
test still fails.

[Srirama, 2017-04-24 15:23:30]

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

[commit-bot: I haz the power, 2017-04-24 15:24:09]

Dry run: CQ is trying da patch.

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

[Srirama, 2017-04-24 15:36:55]

On 2017/04/21 20:00:58, skobes wrote:
> On 2017/04/21 17:47:27, Srirama wrote:
> > PTAL. Made changes according to the discussions. Verified locally by
removing
> > the scroll related testcases from testexpectations but still they are
failing.
> > Looks like some more investigation and changes are needed to pass those.
> 
> Thanks, the change looks right to me.  The next step is to investigate why the
> test still fails.

The scrollable areas which are queries in the test cases are being added in the
following way.
RLS OFF:
FrameView::UpdateParentScrollableAreaSet, this internally checks for
IsScrollable() before adding scrollable area. IsScrollable() is calculating the
scrollable reason using GetScrollingReasons().
In GetScrollingReasons() we consider a few conditions and one among them is
CalculateScrollbarModes().

RLS ON:
PaintLayerScrollableArea::UpdateScrollableAreaSet, this adds the scrollable area
based on hittest visibility and overflow flag which is passed as input.

So when i added the CalculateScrollbarModes() check here, the test cases are
working fine.
Is this change fine or probably we need to check why the input parameter
*has_overflow* is coming true instead of false in this case?

[commit-bot: I haz the power, 2017-04-24 16:41:30]

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

[commit-bot: I haz the power, 2017-04-24 16:41:31]

Dry run: This issue passed the CQ dry run.

[skobes, 2017-04-24 21:29:32]

On 2017/04/24 15:36:55, Srirama wrote:
> On 2017/04/21 20:00:58, skobes wrote:
> > On 2017/04/21 17:47:27, Srirama wrote:
> > > PTAL. Made changes according to the discussions. Verified locally by
> removing
> > > the scroll related testcases from testexpectations but still they are
> failing.
> > > Looks like some more investigation and changes are needed to pass those.
> > 
> > Thanks, the change looks right to me.  The next step is to investigate why
the
> > test still fails.
> 
> The scrollable areas which are queries in the test cases are being added in
the
> following way.
> RLS OFF:
> FrameView::UpdateParentScrollableAreaSet, this internally checks for
> IsScrollable() before adding scrollable area. IsScrollable() is calculating
the
> scrollable reason using GetScrollingReasons().
> In GetScrollingReasons() we consider a few conditions and one among them is
> CalculateScrollbarModes().
> 
> RLS ON:
> PaintLayerScrollableArea::UpdateScrollableAreaSet, this adds the scrollable
area
> based on hittest visibility and overflow flag which is passed as input.
> 
> So when i added the CalculateScrollbarModes() check here, the test cases are
> working fine.
> Is this change fine or probably we need to check why the input parameter
> *has_overflow* is coming true instead of false in this case?

I think this is fine for now.  It's a bit unfortunate to be calling
FrameView::CalculateScrollbarModes in so many places, I would like to move that
logic out of FrameView at some point but will need to think some more about what
that looks like.

Can you update the TestExpectations file to remove the tests that are now
passing?

[Srirama, 2017-04-25 05:41:44]

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

[commit-bot: I haz the power, 2017-04-25 05:41:51]

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, 2017-04-25 07:22:17]

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

[commit-bot: I haz the power, 2017-04-25 07:22:18]

Dry run: This issue passed the CQ dry run.

[Srirama, 2017-04-25 07:54:36]

On 2017/04/24 21:29:32, skobes wrote:
> On 2017/04/24 15:36:55, Srirama wrote:
> > On 2017/04/21 20:00:58, skobes wrote:
> > > On 2017/04/21 17:47:27, Srirama wrote:
> > > > PTAL. Made changes according to the discussions. Verified locally by
> > removing
> > > > the scroll related testcases from testexpectations but still they are
> > failing.
> > > > Looks like some more investigation and changes are needed to pass those.
> > > 
> > > Thanks, the change looks right to me.  The next step is to investigate why
> the
> > > test still fails.
> > 
> > The scrollable areas which are queries in the test cases are being added in
> the
> > following way.
> > RLS OFF:
> > FrameView::UpdateParentScrollableAreaSet, this internally checks for
> > IsScrollable() before adding scrollable area. IsScrollable() is calculating
> the
> > scrollable reason using GetScrollingReasons().
> > In GetScrollingReasons() we consider a few conditions and one among them is
> > CalculateScrollbarModes().
> > 
> > RLS ON:
> > PaintLayerScrollableArea::UpdateScrollableAreaSet, this adds the scrollable
> area
> > based on hittest visibility and overflow flag which is passed as input.
> > 
> > So when i added the CalculateScrollbarModes() check here, the test cases are
> > working fine.
> > Is this change fine or probably we need to check why the input parameter
> > *has_overflow* is coming true instead of false in this case?
> 
> I think this is fine for now.  It's a bit unfortunate to be calling
> FrameView::CalculateScrollbarModes in so many places, I would like to move
that
> logic out of FrameView at some point but will need to think some more about
what
> that looks like.
> 
> Can you update the TestExpectations file to remove the tests that are now
> passing?fractional-scroll-offset-fixed-position-non-composited.html

Updated TestExpectations, PTAL.
The test **** fractional-scroll-offset-fixed-position-non-composited.html ****
is passing without this patch as well, so i will verify again and raise it in a
seperate patch.

[skobes, 2017-04-25 19:58:31]

lgtm

[Srirama, 2017-04-26 02:20:34]

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

[commit-bot: I haz the power, 2017-04-26 02:21:08]

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-04-26 02:25:53]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1493173234495980,
"parent_rev": "c1f175f856da8298510aa3085b194342c00dfc7d", "commit_rev":
"31eb41c176f57dbdbf8d5ce4edd45d21a1c9a45c"}

[commit-bot: I haz the power, 2017-04-26 02:26:24]

Description was changed from

==========
[RLS] Update iframe scrollbar behaviour based on scrolling attribute

Currently in RLS path, scrollbar existence is not updated when
"scrolling" attribute is "no". Updated PLSA::ComputeScrollbarExistence
and PLSA::UserInputScrollable to consider scrollbar modes to turn of
scrolling/scrollbars for root layers.

BUG=711474
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
[RLS] Update iframe scrollbar behaviour based on scrolling attribute

Currently in RLS path, scrollbar existence is not updated when
"scrolling" attribute is "no". Updated PLSA::ComputeScrollbarExistence
and PLSA::UserInputScrollable to consider scrollbar modes to turn of
scrolling/scrollbars for root layers.

BUG=711474
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Review-Url: https://codereview.chromium.org/2831313002
Cr-Commit-Position: refs/heads/master@{#467207}
Committed:
https://chromium.googlesource.com/chromium/src/+/31eb41c176f57dbdbf8d5ce4edd4...
==========

[commit-bot: I haz the power, 2017-04-26 02:26:28]

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