New issue
Advanced search Search tips

Issue 851513 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Jun 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Task

Blocking:
issue 836905



Sign in to add a comment

Move ScrollingCoordinator:UpdateAfterCompositingChangeIfNeeded to after PrePaint

Project Member Reported by pdr@chromium.org, Jun 11 2018

Issue description

PrePaint doesn't seem to depend on ScrollingCoordinator:UpdateAfterCompositingChangeIfNeeded and it will be much easier to compare PaintTouchActionRects to !PaintTouchActionRects if the scrolling coordinator work is refactored to be next to the paint touch action work.
 
Project Member

Comment 1 by bugdroid1@chromium.org, Jun 12 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/377243e018ca292d8bbc298da214577557656fdc

commit 377243e018ca292d8bbc298da214577557656fdc
Author: Philip Rogers <pdr@chromium.org>
Date: Tue Jun 12 20:08:48 2018

Move main scrolling coordinator update after PrePaint

The main scrolling coordinator update, ScrollingCoordinator::
UpdateAfterCompositingChangeIfNeeded, does not depend on the PrePaint
phase and can be moved after PrePaint. This change will make it easier
to compare the new PaintTouchActionRects update
(ScrollingCoordinator::UpdateTouchActionRects) to the current update
(ScrollingCoordinator::UpdateAfterCompositingChangeIfNeeded).

This patch also renames UpdateAfterCompositingChangeIfNeeded to
UpdateAfterPrePaint and adds a comment describing what it does.

Bug:  851513 
Change-Id: Idbcd515708a33d6955deb513808874b38d731c4f
Reviewed-on: https://chromium-review.googlesource.com/1096297
Reviewed-by: Robert Flack <flackr@chromium.org>
Commit-Queue: Philip Rogers <pdr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#566542}
[modify] https://crrev.com/377243e018ca292d8bbc298da214577557656fdc/third_party/blink/renderer/core/frame/local_frame_view.cc
[modify] https://crrev.com/377243e018ca292d8bbc298da214577557656fdc/third_party/blink/renderer/core/page/page.cc
[modify] https://crrev.com/377243e018ca292d8bbc298da214577557656fdc/third_party/blink/renderer/core/page/scrolling/scrolling_coordinator.cc
[modify] https://crrev.com/377243e018ca292d8bbc298da214577557656fdc/third_party/blink/renderer/core/page/scrolling/scrolling_coordinator.h
[modify] https://crrev.com/377243e018ca292d8bbc298da214577557656fdc/third_party/blink/renderer/core/page/scrolling/scrolling_coordinator_test.cc
[modify] https://crrev.com/377243e018ca292d8bbc298da214577557656fdc/third_party/blink/renderer/core/scheduler/frame_throttling_test.cc
[modify] https://crrev.com/377243e018ca292d8bbc298da214577557656fdc/third_party/blink/renderer/core/testing/internals.cc

Project Member

Comment 2 by bugdroid1@chromium.org, Jun 13 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/a1f6d82332c89ceb1eff76cf6430c3e37ab0c24f

commit a1f6d82332c89ceb1eff76cf6430c3e37ab0c24f
Author: Yuki Shiino <yukishiino@chromium.org>
Date: Wed Jun 13 07:42:42 2018

Revert "Move main scrolling coordinator update after PrePaint"

This reverts commit 377243e018ca292d8bbc298da214577557656fdc.

Reason for revert:

SitePerProcessTextInputManagerTest.TrackTextSelectionForAllFrames is failing on Mac-10.12.
https://ci.chromium.org/p/chromium/builders/luci.chromium.ci/Mac10.12%20Tests/13719

Findit has 93% confident with this CL, and nothing else looks suspiciou so far (although this one doesn't look suspicious to me, neither).
https://findit-for-me.appspot.com/waterfall/build-failure?url=https://build.chromium.org/p/chromium.mac/builders/Mac10.12%20Tests/builds/13719

Let me give it a try to revert this one to see whether the revert fixes the issue or not.  Please land this again, if this is irrelevant at all.


Original change's description:
> Move main scrolling coordinator update after PrePaint
> 
> The main scrolling coordinator update, ScrollingCoordinator::
> UpdateAfterCompositingChangeIfNeeded, does not depend on the PrePaint
> phase and can be moved after PrePaint. This change will make it easier
> to compare the new PaintTouchActionRects update
> (ScrollingCoordinator::UpdateTouchActionRects) to the current update
> (ScrollingCoordinator::UpdateAfterCompositingChangeIfNeeded).
> 
> This patch also renames UpdateAfterCompositingChangeIfNeeded to
> UpdateAfterPrePaint and adds a comment describing what it does.
> 
> Bug:  851513 
> Change-Id: Idbcd515708a33d6955deb513808874b38d731c4f
> Reviewed-on: https://chromium-review.googlesource.com/1096297
> Reviewed-by: Robert Flack <flackr@chromium.org>
> Commit-Queue: Philip Rogers <pdr@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#566542}

TBR=flackr@chromium.org,pdr@chromium.org,xidachen@chromium.org

Change-Id: Ic1839d7e51752fd2b5f974b958dadd2b98f2569c
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug:  851513 
Reviewed-on: https://chromium-review.googlesource.com/1098815
Reviewed-by: Yuki Shiino <yukishiino@chromium.org>
Commit-Queue: Yuki Shiino <yukishiino@chromium.org>
Cr-Commit-Position: refs/heads/master@{#566749}
[modify] https://crrev.com/a1f6d82332c89ceb1eff76cf6430c3e37ab0c24f/third_party/blink/renderer/core/frame/local_frame_view.cc
[modify] https://crrev.com/a1f6d82332c89ceb1eff76cf6430c3e37ab0c24f/third_party/blink/renderer/core/page/page.cc
[modify] https://crrev.com/a1f6d82332c89ceb1eff76cf6430c3e37ab0c24f/third_party/blink/renderer/core/page/scrolling/scrolling_coordinator.cc
[modify] https://crrev.com/a1f6d82332c89ceb1eff76cf6430c3e37ab0c24f/third_party/blink/renderer/core/page/scrolling/scrolling_coordinator.h
[modify] https://crrev.com/a1f6d82332c89ceb1eff76cf6430c3e37ab0c24f/third_party/blink/renderer/core/page/scrolling/scrolling_coordinator_test.cc
[modify] https://crrev.com/a1f6d82332c89ceb1eff76cf6430c3e37ab0c24f/third_party/blink/renderer/core/scheduler/frame_throttling_test.cc
[modify] https://crrev.com/a1f6d82332c89ceb1eff76cf6430c3e37ab0c24f/third_party/blink/renderer/core/testing/internals.cc

Project Member

Comment 3 by bugdroid1@chromium.org, Jun 13 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/196724e50a63dcd24bbb3cc79d4d606b5f725e5c

commit 196724e50a63dcd24bbb3cc79d4d606b5f725e5c
Author: Yuki Shiino <yukishiino@chromium.org>
Date: Wed Jun 13 12:54:27 2018

Reland "Move main scrolling coordinator update after PrePaint"

This reverts commit a1f6d82332c89ceb1eff76cf6430c3e37ab0c24f.

Reason for revert:

This revert didn't fix SitePerProcessTextInputManagerTest in interactive_ui_tests on Mac-10.12.
https://ci.chromium.org/p/chromium/builders/luci.chromium.ci/Mac10.12%20Tests/13732

Original change's description:
> Revert "Move main scrolling coordinator update after PrePaint"
> 
> This reverts commit 377243e018ca292d8bbc298da214577557656fdc.
> 
> Reason for revert:
> 
> SitePerProcessTextInputManagerTest.TrackTextSelectionForAllFrames is failing on Mac-10.12.
> https://ci.chromium.org/p/chromium/builders/luci.chromium.ci/Mac10.12%20Tests/13719
> 
> Findit has 93% confident with this CL, and nothing else looks suspiciou so far (although this one doesn't look suspicious to me, neither).
> https://findit-for-me.appspot.com/waterfall/build-failure?url=https://build.chromium.org/p/chromium.mac/builders/Mac10.12%20Tests/builds/13719
> 
> Let me give it a try to revert this one to see whether the revert fixes the issue or not.  Please land this again, if this is irrelevant at all.
> 
> 
> Original change's description:
> > Move main scrolling coordinator update after PrePaint
> > 
> > The main scrolling coordinator update, ScrollingCoordinator::
> > UpdateAfterCompositingChangeIfNeeded, does not depend on the PrePaint
> > phase and can be moved after PrePaint. This change will make it easier
> > to compare the new PaintTouchActionRects update
> > (ScrollingCoordinator::UpdateTouchActionRects) to the current update
> > (ScrollingCoordinator::UpdateAfterCompositingChangeIfNeeded).
> > 
> > This patch also renames UpdateAfterCompositingChangeIfNeeded to
> > UpdateAfterPrePaint and adds a comment describing what it does.
> > 
> > Bug:  851513 
> > Change-Id: Idbcd515708a33d6955deb513808874b38d731c4f
> > Reviewed-on: https://chromium-review.googlesource.com/1096297
> > Reviewed-by: Robert Flack <flackr@chromium.org>
> > Commit-Queue: Philip Rogers <pdr@chromium.org>
> > Cr-Commit-Position: refs/heads/master@{#566542}
> 
> TBR=flackr@chromium.org,pdr@chromium.org,xidachen@chromium.org
> 
> Change-Id: Ic1839d7e51752fd2b5f974b958dadd2b98f2569c
> No-Presubmit: true
> No-Tree-Checks: true
> No-Try: true
> Bug:  851513 
> Reviewed-on: https://chromium-review.googlesource.com/1098815
> Reviewed-by: Yuki Shiino <yukishiino@chromium.org>
> Commit-Queue: Yuki Shiino <yukishiino@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#566749}

TBR=flackr@chromium.org,yukishiino@chromium.org,pdr@chromium.org,xidachen@chromium.org

Change-Id: I3745bcf4c8c381c97c6373edc2b26553ffb6aaeb
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug:  851513 
Reviewed-on: https://chromium-review.googlesource.com/1098995
Reviewed-by: Yuki Shiino <yukishiino@chromium.org>
Commit-Queue: Yuki Shiino <yukishiino@chromium.org>
Cr-Commit-Position: refs/heads/master@{#566810}
[modify] https://crrev.com/196724e50a63dcd24bbb3cc79d4d606b5f725e5c/third_party/blink/renderer/core/frame/local_frame_view.cc
[modify] https://crrev.com/196724e50a63dcd24bbb3cc79d4d606b5f725e5c/third_party/blink/renderer/core/page/page.cc
[modify] https://crrev.com/196724e50a63dcd24bbb3cc79d4d606b5f725e5c/third_party/blink/renderer/core/page/scrolling/scrolling_coordinator.cc
[modify] https://crrev.com/196724e50a63dcd24bbb3cc79d4d606b5f725e5c/third_party/blink/renderer/core/page/scrolling/scrolling_coordinator.h
[modify] https://crrev.com/196724e50a63dcd24bbb3cc79d4d606b5f725e5c/third_party/blink/renderer/core/page/scrolling/scrolling_coordinator_test.cc
[modify] https://crrev.com/196724e50a63dcd24bbb3cc79d4d606b5f725e5c/third_party/blink/renderer/core/scheduler/frame_throttling_test.cc
[modify] https://crrev.com/196724e50a63dcd24bbb3cc79d4d606b5f725e5c/third_party/blink/renderer/core/testing/internals.cc

Project Member

Comment 4 by bugdroid1@chromium.org, Jun 19 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/d757661d2df9265008565ad1877f0eb807a1e9eb

commit d757661d2df9265008565ad1877f0eb807a1e9eb
Author: Philip Rogers <pdr@chromium.org>
Date: Tue Jun 19 14:07:39 2018

Move main scrolling coordinator update after Paint

The main scrolling coordinator update, ScrollingCoordinator::
UpdateAfterPrePaint, does not affect paint and can be moved after paint.
This change will make it easier to compare the new PaintTouchActionRects
update to the current scrolling coordinator update.

This patch also moves the PaintTouchActionRects update into
ScrollingCoordinator::UpdateTouchEventTargetRectsIfNeeded so the two
codepaths can be directly compared. With this change, the
PaintTouchActionRects codepath now uses the existing dirty bits and only
runs when touch event rects might have changed.

This patch has a bugfix for the following layout test where
PaintTouchActionRects would not invalidate the touch action dirty bit:
fast/events/touch/compositor-touch-hit-rects-transform-changed-nolayout.html

This is a followup to https://crrev.com/566542 which moved the scrolling
coordinator update to after PrePaint.

Bug:  851513 
Change-Id: I8ca1373832b49edd5518b43c524b6dcd57e3b36b
Reviewed-on: https://chromium-review.googlesource.com/1103777
Commit-Queue: Philip Rogers <pdr@chromium.org>
Reviewed-by: Robert Flack <flackr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#568437}
[modify] https://crrev.com/d757661d2df9265008565ad1877f0eb807a1e9eb/third_party/blink/renderer/core/frame/local_frame_view.cc
[modify] https://crrev.com/d757661d2df9265008565ad1877f0eb807a1e9eb/third_party/blink/renderer/core/page/page.cc
[modify] https://crrev.com/d757661d2df9265008565ad1877f0eb807a1e9eb/third_party/blink/renderer/core/page/page.h
[modify] https://crrev.com/d757661d2df9265008565ad1877f0eb807a1e9eb/third_party/blink/renderer/core/page/scrolling/scrolling_coordinator.cc
[modify] https://crrev.com/d757661d2df9265008565ad1877f0eb807a1e9eb/third_party/blink/renderer/core/page/scrolling/scrolling_coordinator.h
[modify] https://crrev.com/d757661d2df9265008565ad1877f0eb807a1e9eb/third_party/blink/renderer/core/page/scrolling/scrolling_coordinator_test.cc
[modify] https://crrev.com/d757661d2df9265008565ad1877f0eb807a1e9eb/third_party/blink/renderer/core/scheduler/frame_throttling_test.cc
[modify] https://crrev.com/d757661d2df9265008565ad1877f0eb807a1e9eb/third_party/blink/renderer/core/testing/internals.cc

Comment 5 by pdr@chromium.org, Jun 19 2018

Status: Fixed (was: Assigned)

Sign in to add a comment