Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1272143003:
Remove code duplication in updateLifecycleToCompositingCleanPlusScrolling. (Closed)
|
Created:
5 years, 4 months ago by ojan Modified:
5 years, 4 months ago CC:
blink-reviews, blink-reviews-paint_chromium.org, dshwang, slimming-paint-reviews_chromium.org Base URL:
svn://svn.chromium.org/blink/trunk Target Ref:
refs/heads/master Project:
blink Visibility:
Public. |
DescriptionRemove code duplication in updateLifecycleToCompositingCleanPlusScrolling.
There should only be one code path the runs the blink pipeline.
Having multiple code paths means we mess up and end up with
multiple pipelines. At the moment, there's a security bug here
because only the updateLifecyclePhasesInternal code path has
a FrameView protector.
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=200595
Patch Set 1 #
Total comments: 2
Patch Set 2 : address review comment #Patch Set 3 : Fix asserts #Patch Set 4 : remove added assert #
Total comments: 1
Patch Set 5 : merge to ToT #Patch Set 6 : merge to ToT for real #Patch Set 7 : add back newline #Patch Set 8 : Try to manually recreate the patch #
Messages
Total messages: 57 (28 generated)
The CQ bit was checked by ojan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1272143003/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1272143003/1
ojan@chromium.org changed reviewers: + pdr@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_blink_rel on tryserver.blink (JOB_FAILED, http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/73083)
ping
The CQ bit was checked by ojan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1272143003/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1272143003/1
chrishtr@chromium.org changed reviewers: + chrishtr@chromium.org
https://codereview.chromium.org/1272143003/diff/1/Source/core/frame/FrameView.h File Source/core/frame/FrameView.h (right): https://codereview.chromium.org/1272143003/diff/1/Source/core/frame/FrameView... Source/core/frame/FrameView.h:615: OnlyUpToCompositingClean, plus scrolling
https://codereview.chromium.org/1272143003/diff/1/Source/core/frame/FrameView.h File Source/core/frame/FrameView.h (right): https://codereview.chromium.org/1272143003/diff/1/Source/core/frame/FrameView... Source/core/frame/FrameView.h:615: OnlyUpToCompositingClean, On 2015/08/11 at 16:56:12, chrishtr wrote: > plus scrolling It's weird to me to have a pipeline and list multiple phases in the pipeline. That said, scrolling isn't an official lifecycle step at the moment. We might want to make it a lifecycle step. Then this would only be OnlyUptoScrollingClean as that would imply compositing is clean. Changed to OnlyUpToCompositingCleanPlusScrolling.
On 2015/08/11 at 17:18:53, ojan wrote: > https://codereview.chromium.org/1272143003/diff/1/Source/core/frame/FrameView.h > File Source/core/frame/FrameView.h (right): > > https://codereview.chromium.org/1272143003/diff/1/Source/core/frame/FrameView... > Source/core/frame/FrameView.h:615: OnlyUpToCompositingClean, > On 2015/08/11 at 16:56:12, chrishtr wrote: > > plus scrolling > > It's weird to me to have a pipeline and list multiple phases in the pipeline. That said, scrolling isn't an official lifecycle step at the moment. We might want to make it a lifecycle step. Then this would only be OnlyUptoScrollingClean as that would imply compositing is clean. > > Changed to OnlyUpToCompositingCleanPlusScrolling. Agreed, I expect we'll probably make it a lifecycle stage. Paint will be one soon also.
The CQ bit was checked by ojan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1272143003/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1272143003/20001
The CQ bit was checked by chrishtr@chromium.org
lgtm
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1272143003/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1272143003/20001
On 2015/08/11 17:20:34, chrishtr wrote: > On 2015/08/11 at 17:18:53, ojan wrote: > > > https://codereview.chromium.org/1272143003/diff/1/Source/core/frame/FrameView.h > > File Source/core/frame/FrameView.h (right): > > > > > https://codereview.chromium.org/1272143003/diff/1/Source/core/frame/FrameView... > > Source/core/frame/FrameView.h:615: OnlyUpToCompositingClean, > > On 2015/08/11 at 16:56:12, chrishtr wrote: > > > plus scrolling > > > > It's weird to me to have a pipeline and list multiple phases in the pipeline. > That said, scrolling isn't an official lifecycle step at the moment. We might > want to make it a lifecycle step. Then this would only be OnlyUptoScrollingClean > as that would imply compositing is clean. > > > > Changed to OnlyUpToCompositingCleanPlusScrolling. > > Agreed, I expect we'll probably make it a lifecycle stage. Paint will be one > soon also. I think paint is different from other stages because painting the interest rect won't make the whole document 'paint clean'. We may leave the part outside of the interest rect 'paint dirty' (not painted and paint flags not cleared) across document cycles and frames. I think we can just make LayoutClean the last state of document lifecycle for slimming paint phase 2 and leave paint flags independent of document lifecycle.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_blink_rel on tryserver.blink (JOB_FAILED, http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/65806)
On 2015/08/11 17:35:05, Xianzhu wrote: > On 2015/08/11 17:20:34, chrishtr wrote: > > On 2015/08/11 at 17:18:53, ojan wrote: > > > > > > https://codereview.chromium.org/1272143003/diff/1/Source/core/frame/FrameView.h > > > File Source/core/frame/FrameView.h (right): > > > > > > > > > https://codereview.chromium.org/1272143003/diff/1/Source/core/frame/FrameView... > > > Source/core/frame/FrameView.h:615: OnlyUpToCompositingClean, > > > On 2015/08/11 at 16:56:12, chrishtr wrote: > > > > plus scrolling > > > > > > It's weird to me to have a pipeline and list multiple phases in the > pipeline. > > That said, scrolling isn't an official lifecycle step at the moment. We might > > want to make it a lifecycle step. Then this would only be > OnlyUptoScrollingClean > > as that would imply compositing is clean. > > > > > > Changed to OnlyUpToCompositingCleanPlusScrolling. > > > > Agreed, I expect we'll probably make it a lifecycle stage. Paint will be one > > soon also. > > I think paint is different from other stages because painting the interest rect > won't make the whole document 'paint clean'. We may leave the part outside of > the interest rect 'paint dirty' (not painted and paint flags not cleared) across > document cycles and frames. I think we can just make LayoutClean the last state > of document lifecycle for slimming paint phase 2 and leave paint flags > independent of document lifecycle. I think it is paint clean. The only thing that can make it unclean is a document invalidation or scrolling update from the compositor. The scrolling update will naturally invalidate paint because the interest rect may have changed.
The CQ bit was checked by ojan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1272143003/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1272143003/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by ojan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1272143003/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1272143003/60001
https://codereview.chromium.org/1272143003/diff/60001/Source/core/frame/Frame... File Source/core/frame/FrameView.cpp (right): https://codereview.chromium.org/1272143003/diff/60001/Source/core/frame/Frame... Source/core/frame/FrameView.cpp:2471: ASSERT(lifecycle().state() == DocumentLifecycle::PaintInvalidationClean); Please take another look. Moved this assert into the AllPhases branch. Of course we're not in PaintInvalidationClean if we didn't run all the phases. :) For the purposes of maintaining old behavior, I also moved the "ASSERT(!view->hasPendingSelection());" line inside the AllPhases if statement, but I'm not sure what that assert was for, so it maybe could be moved out in a future patch. A bit scary that only the unit tests caught this.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
esprehn@chromium.org changed reviewers: + esprehn@chromium.org
lgtm
The CQ bit was checked by ojan@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from chrishtr@chromium.org Link to the patchset: https://codereview.chromium.org/1272143003/#ps60001 (title: "remove added assert")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1272143003/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1272143003/60001
The CQ bit was unchecked by commit-bot@chromium.org
Failed to apply patch for Source/core/frame/FrameView.cpp:
While running patch -p1 --forward --force --no-backup-if-mismatch;
patching file Source/core/frame/FrameView.cpp
Hunk #1 succeeded at 2431 with fuzz 2 (offset -5 lines).
Hunk #2 FAILED at 2455.
1 out of 2 hunks FAILED -- saving rejects to file
Source/core/frame/FrameView.cpp.rej
Patch: Source/core/frame/FrameView.cpp
Index: Source/core/frame/FrameView.cpp
diff --git a/Source/core/frame/FrameView.cpp b/Source/core/frame/FrameView.cpp
index
f4436bcd726cc5ca68e9c20cc3cb5432733488b1..fc7f31cce80da18c4ff5e69ed6550db484cc4e3f
100644
--- a/Source/core/frame/FrameView.cpp
+++ b/Source/core/frame/FrameView.cpp
@@ -2436,22 +2436,16 @@ void FrameView::updateWidgetPositionsIfNeeded()
void FrameView::updateAllLifecyclePhases()
{
- frame().localFrameRoot()->view()->updateAllLifecyclePhasesInternal();
+ frame().localFrameRoot()->view()->updateLifecyclePhasesInternal(AllPhases);
}
// TODO(chrishtr): add a scrolling update lifecycle phase, after compositing
and before invalidation.
void FrameView::updateLifecycleToCompositingCleanPlusScrolling()
{
- frame().localFrameRoot()->view()->updateStyleAndLayoutIfNeededRecursive();
- LayoutView* view = layoutView();
- if (view)
- view->compositor()->updateIfNeededRecursive();
- scrollContentsIfNeededRecursive();
-
- ASSERT(lifecycle().state() >= DocumentLifecycle::CompositingClean);
+
frame().localFrameRoot()->view()->updateLifecyclePhasesInternal(OnlyUpToCompositingCleanPlusScrolling);
}
-void FrameView::updateAllLifecyclePhasesInternal()
+void FrameView::updateLifecyclePhasesInternal(LifeCycleUpdateOption phases)
{
// This must be called from the root frame, since it recurses down, not up.
Otherwise the lifecycles of the frames might be out of sync.
ASSERT(frame() == page()->mainFrame() ||
(!frame().tree().parent()->isLocalFrame()));
@@ -2461,19 +2455,22 @@ void FrameView::updateAllLifecyclePhasesInternal()
updateStyleAndLayoutIfNeededRecursive();
- LayoutView* view = layoutView();
- if (view) {
+ if (LayoutView* view = layoutView()) {
TRACE_EVENT1("devtools.timeline", "UpdateLayerTree", "data",
InspectorUpdateLayerTreeEvent::data(m_frame.get()));
view->compositor()->updateIfNeededRecursive();
scrollContentsIfNeededRecursive();
- invalidateTreeIfNeededRecursive();
- updatePostLifecycleData();
- ASSERT(!view->hasPendingSelection());
- }
+ ASSERT(lifecycle().state() >= DocumentLifecycle::CompositingClean);
+
+ if (phases == AllPhases) {
+ invalidateTreeIfNeededRecursive();
+ updatePostLifecycleData();
- ASSERT(lifecycle().state() == DocumentLifecycle::PaintInvalidationClean);
+ ASSERT(!view->hasPendingSelection());
+ ASSERT(lifecycle().state() ==
DocumentLifecycle::PaintInvalidationClean);
+ }
+ }
}
void FrameView::updatePostLifecycleData()
The CQ bit was unchecked by commit-bot@chromium.org
The CQ bit was checked by ojan@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from chrishtr@chromium.org, esprehn@chromium.org Link to the patchset: https://codereview.chromium.org/1272143003/#ps80001 (title: "merge to ToT")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1272143003/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1272143003/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_chromium_gn_compile_dbg on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...) android_chromium_gn_compile_rel on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
The CQ bit was checked by ojan@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from chrishtr@chromium.org, esprehn@chromium.org Link to the patchset: https://codereview.chromium.org/1272143003/#ps100001 (title: "merge to ToT for real")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1272143003/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1272143003/100001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_chromium_gn_compile_dbg on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...) android_chromium_gn_compile_rel on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
The CQ bit was checked by ojan@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from chrishtr@chromium.org, esprehn@chromium.org Link to the patchset: https://codereview.chromium.org/1272143003/#ps120001 (title: "add back newline")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1272143003/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1272143003/120001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
The CQ bit was checked by ojan@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from chrishtr@chromium.org, esprehn@chromium.org Link to the patchset: https://codereview.chromium.org/1272143003/#ps140001 (title: "Try to manually recreate the patch")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1272143003/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1272143003/140001
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as https://src.chromium.org/viewvc/blink?view=rev&revision=200595 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
