Explicitly invalidate LayoutView when overflow is recalculated.

Explicitly invalidate LayoutView when overflow is recalculated.

This bug occurs because during an animation we may change the amount of layout
overflow (via FrameView::recalcOverflowAfterStyleChange). Previously, this
would cause a change in scrollbar existance until r432965 made it so that the
FrameView would prevent scrollbar creation earlier in cases where they're known
to be unneeded. Changing scrollbar existence causes a layout so we'd implicitly
invalidate and repaint. Since that change, we (correctly) don't bother doing a
layout but because of that we no longer invalidate so the additionally exposed
background region doesn't get painted.

BUG=683814
Review-Url: https://codereview.chromium.org/2685883004
Cr-Commit-Position: refs/heads/master@{#452170}
Committed: https://chromium.googlesource.com/chromium/src/+/717789e00f9ff34efe3a2ca23ebd69e802d8b6ec

[bokan, 2017-02-08 23:12:49]

Description was changed from

==========
Explicitly invalidate LayoutView when overflow is recalculated.

This bug occurs because during an animation, we may change the amount of
overflow layout. Previously, this would cause a change in scrollbar appearance
until r432965 made it so that the FrameView would prevent scrollbar creation
earlier. That would implicitly cause a layout so that we'd invalidate. Since
that change, the layout is gone and we no longer invalidate, causing us not
to paint additionally scrollable background.

BUG=683814
==========

to

==========
Explicitly invalidate LayoutView when overflow is recalculated.

This bug occurs because during an animation we may change the amount of layout
overflow (via FrameView::recalcOverflowAfterStyleChange). Previously, this
would cause a change in scrollbar existance until r432965 made it so that the
FrameView would prevent scrollbar creation earlier in cases where they're known
to be unneeded. Changing scrollbar existence causes a layout so we'd implicitly
invalidate and repaint. Since that change, we (correctly) don't bother doing a
layout but because of that we no longer invalidate so the additionally exposed
background region doesn't get painted.

BUG=683814
==========

[bokan, 2017-02-08 23:12:49]

bokan@chromium.org changed reviewers:
+ pdr@chromium.org

[bokan, 2017-02-08 23:13:19]

Hey pdr@,

Does this look like the correct fix? If so, I'll add a test.

Thanks

[bokan, 2017-02-09 00:27:45]

bokan@chromium.org changed reviewers:
- pdr@chromium.org

[bokan, 2017-02-09 00:28:26]

bokan@chromium.org changed reviewers:
+ pdr@chromium.org

[pdr., 2017-02-10 22:25:33]

https://codereview.chromium.org/2685883004/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/2685883004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/FrameView.cpp:818:
layoutViewItem.setMayNeedPaintInvalidation();
Yeah I think this patch looks like the bug. We should only need to do this when
the visual viewport supplies the scrollbars though, so WDYT of something like:
if (visualViewportSuppliesScrollbars()) {
  layoutViewItem.setMayNeedPaintInvalidation();
} else {
  // do the regular computeScrollbarExistence code here.
}

If you can launch a debug build with "--enable-viewport
--enable-blink-features=PaintUnderInvalidationChecking" and run the demo, you
can confirm this is under-invalidation (missing call to
setMayNeedPaintInvalidation). This is significantly less useful _after_ you've
spent a day locating the bug but maybe it can be of use in the future :)

Idea for testing: you can add a test under paint/invalidation and ensure the
right invalidation rect is output.

[bokan, 2017-02-21 22:14:14]

https://codereview.chromium.org/2685883004/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/2685883004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/FrameView.cpp:818:
layoutViewItem.setMayNeedPaintInvalidation();
On 2017/02/10 22:25:33, pdr. wrote:
> Yeah I think this patch looks like the bug. We should only need to do this
when
> the visual viewport supplies the scrollbars though, so WDYT of something like:
> if (visualViewportSuppliesScrollbars()) {
>   layoutViewItem.setMayNeedPaintInvalidation();
> } else {
>   // do the regular computeScrollbarExistence code here.
> }

Yah, I agree we can avoid the scrollbar updates below if
visualViewportSuppliesScrollbars. However, given that this needs to get merged
to M57, I'm not keen to change any more behavior than necessary.  How about I
land a version like this:

if (visualViewportSuppliesScrollbars())
  layoutViewItem.setMayNeedPaintInvalidation()

And put up a followup patch that conditionally does the scrollbar stuff below?
 
> If you can launch a debug build with "--enable-viewport
> --enable-blink-features=PaintUnderInvalidationChecking" and run the demo, you
> can confirm this is under-invalidation (missing call to
> setMayNeedPaintInvalidation). This is significantly less useful _after_ you've
> spent a day locating the bug but maybe it can be of use in the future :)

Neat, thanks for the tip!
 
> Idea for testing: you can add a test under paint/invalidation and ensure the
> right invalidation rect is output.

Thanks, will do.

[bokan, 2017-02-22 00:00:51]

The test is proving more finicky to write than I expected since it relies on
animations and timing. Would you mind if I land the current patch now for the
merge and I'll add the test and make the change you suggested in a follow-up
(I'm working on that now but this issue is RB-stable)

[pdr., 2017-02-22 18:05:31]

On 2017/02/22 at 00:00:51, bokan wrote:
> The test is proving more finicky to write than I expected since it relies on
animations and timing. Would you mind if I land the current patch now for the
merge and I'll add the test and make the change you suggested in a follow-up
(I'm working on that now but this issue is RB-stable)

OK SG LGTM

[bokan, 2017-02-22 18:13:34]

The CQ bit was checked by bokan@chromium.org

[commit-bot: I haz the power, 2017-02-22 18:14:15]

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-02-22 18:34:13]

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

[commit-bot: I haz the power, 2017-02-22 18:34:14]

Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)

[bokan, 2017-02-22 18:38:24]

The CQ bit was checked by bokan@chromium.org

[commit-bot: I haz the power, 2017-02-22 18:38:53]

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-02-22 19:56:25]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1487788704833110,
"parent_rev": "6e609d4a2968e734d87d2f1675f9cd4785a416dc", "commit_rev":
"717789e00f9ff34efe3a2ca23ebd69e802d8b6ec"}

[commit-bot: I haz the power, 2017-02-22 19:56:58]

Description was changed from

==========
Explicitly invalidate LayoutView when overflow is recalculated.

This bug occurs because during an animation we may change the amount of layout
overflow (via FrameView::recalcOverflowAfterStyleChange). Previously, this
would cause a change in scrollbar existance until r432965 made it so that the
FrameView would prevent scrollbar creation earlier in cases where they're known
to be unneeded. Changing scrollbar existence causes a layout so we'd implicitly
invalidate and repaint. Since that change, we (correctly) don't bother doing a
layout but because of that we no longer invalidate so the additionally exposed
background region doesn't get painted.

BUG=683814
==========

to

==========
Explicitly invalidate LayoutView when overflow is recalculated.

This bug occurs because during an animation we may change the amount of layout
overflow (via FrameView::recalcOverflowAfterStyleChange). Previously, this
would cause a change in scrollbar existance until r432965 made it so that the
FrameView would prevent scrollbar creation earlier in cases where they're known
to be unneeded. Changing scrollbar existence causes a layout so we'd implicitly
invalidate and repaint. Since that change, we (correctly) don't bother doing a
layout but because of that we no longer invalidate so the additionally exposed
background region doesn't get painted.

BUG=683814

Review-Url: https://codereview.chromium.org/2685883004
Cr-Commit-Position: refs/heads/master@{#452170}
Committed:
https://chromium.googlesource.com/chromium/src/+/717789e00f9ff34efe3a2ca23ebd...
==========

[commit-bot: I haz the power, 2017-02-22 19:56:59]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/717789e00f9ff34efe3a2ca23ebd...