Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2531603003:
Only scroll on main if the targeted frames need to scroll on main (Closed)
DescriptionOnly scroll on main if the targeted frames need to scroll on main
BUG=568901
TEST=ScrollingCoordinatorTest.BackgroundAttachmentFixedShouldTriggerMainThreadScroll; FrameThrottlingTest.ScrollingCoordinatorShouldSkipThrottledFrame; ScrollingCoordinatorTest.RecalculateMainThreadScrollingReasonsUponResize
third_party/WebKit/LayoutTests/compositing/layer-creation/iframe-background-attachment-fixed.html
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
Committed: https://crrev.com/2e656635bd3f676e8385095b0e830af73d74d382
Cr-Commit-Position: refs/heads/master@{#439801}
Patch Set 1 #Patch Set 2 : Refine conditions for skipping frame check #Patch Set 3 : Performance update #Patch Set 4 : Update FrameTree recursively #
Total comments: 4
Patch Set 5 : No need to update the entire frame tree. Rather, search the subtree from the target frame to main f… #Patch Set 6 : Bug fix #
Total comments: 15
Patch Set 7 : Remove scroll layer creation for certain iframes & update the layout test files #Patch Set 8 : Recover two condition checks #
Total comments: 5
Patch Set 9 : Logic refactoring so that no modification is made in const function mainThreadScrollingReasons(Loca… #Patch Set 10 : Update sub-frame using loops instead of recursion & integrate a patch to fix spv2 problem #
Total comments: 9
Patch Set 11 : Add unit tests && remove mainthreadscrollingreasons when necessary #
Total comments: 5
Patch Set 12 : Refactoring && add unit tests #Patch Set 13 : Bug fixed #
Total comments: 14
Patch Set 14 : Moving mainThreadScrollingReason related function from ScrollingCoordinator to FrameView to indicat… #
Total comments: 12
Patch Set 15 : Remove unnecessary parameters & make some methods const #Patch Set 16 : Fix compiling error due to using backslash at the end of a commenting line" #
Total comments: 8
Patch Set 17 : Refactoring #Patch Set 18 : Add unit tests for the main thread scrolling reason kHasNonLayerViewportConstrainedObjects #Patch Set 19 : Resolve conflict #Messages
Total messages: 77 (33 generated)
The CQ bit was checked by yigu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Description was changed from ========== Only scroll on main if the targeted frames need to scroll on main BUG=568901 ========== to ========== Only scroll on main if the targeted frames need to scroll on main BUG=568901 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
yigu@chromium.org changed reviewers: + flackr@chromium.org
Hi Rob, Could you please take a look at the current solution? Thanks. Yi
Can you add tests of some of the cases we talked about? Like the following structure: fast - slow - fast - fast See for example https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/compositi... https://codereview.chromium.org/2531603003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.cpp (right): https://codereview.chromium.org/2531603003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.cpp:1219: if (toLocalFrame(frame)->localFrameRoot() != m_page->mainFrame()) This seems impossible given that frame is m_page->mainFrame() (from line 1215 above). https://codereview.chromium.org/2531603003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.cpp:1228: updateMainThreadScrollingReasonsOnFrameTree(m_page->mainFrame(), reasons); If we recalculate the reasons every time we ask for them, we should just follow the parent pointers from the targetFrame to the root and compute the reasons here (i.e. almost the same as the old for loop but walking up from targetFrame). This will save a lot of work calculating reasons for frames which we're not concerned with.
On 2016/11/30 16:02:11, flackr wrote: > Can you add tests of some of the cases we talked about? Like the following > structure: > fast > - slow > - fast > - fast > > See for example > https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/compositi... > > https://codereview.chromium.org/2531603003/diff/60001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.cpp > (right): > > https://codereview.chromium.org/2531603003/diff/60001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.cpp:1219: if > (toLocalFrame(frame)->localFrameRoot() != m_page->mainFrame()) > This seems impossible given that frame is m_page->mainFrame() (from line 1215 > above). > > https://codereview.chromium.org/2531603003/diff/60001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.cpp:1228: > updateMainThreadScrollingReasonsOnFrameTree(m_page->mainFrame(), reasons); > If we recalculate the reasons every time we ask for them, we should just follow > the parent pointers from the targetFrame to the root and compute the reasons > here (i.e. almost the same as the old for loop but walking up from targetFrame). > This will save a lot of work calculating reasons for frames which we're not > concerned with. Hi Rob. The latest patch has been updated. PTAL. Thanks!
Hi Rob, Made few changes to fix a bug. PTAL. Thanks. https://codereview.chromium.org/2531603003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.cpp (right): https://codereview.chromium.org/2531603003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.cpp:1219: if (toLocalFrame(frame)->localFrameRoot() != m_page->mainFrame()) On 2016/11/30 16:02:11, flackr wrote: > This seems impossible given that frame is m_page->mainFrame() (from line 1215 > above). Done. https://codereview.chromium.org/2531603003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.cpp:1228: updateMainThreadScrollingReasonsOnFrameTree(m_page->mainFrame(), reasons); On 2016/11/30 16:02:11, flackr wrote: > If we recalculate the reasons every time we ask for them, we should just follow > the parent pointers from the targetFrame to the root and compute the reasons > here (i.e. almost the same as the old for loop but walking up from targetFrame). > This will save a lot of work calculating reasons for frames which we're not > concerned with. Done.
https://codereview.chromium.org/2531603003/diff/100001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/compositing/layer-creation/iframe-background-attachment-fixed.html (right): https://codereview.chromium.org/2531603003/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/compositing/layer-creation/iframe-background-attachment-fixed.html:19: <div class="composited scroller"> Does this scroller affect the test? https://codereview.chromium.org/2531603003/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/compositing/layer-creation/iframe-background-attachment-fixed.html:32: text = !reasons ? "Main frame scrolls on impl." : "Main frame scrolls on main thread, reasons: " + reasons; Turn this ternary into a helper function and reuse it, i.e.: function scrollingLocationAndReasons(reasons) { return reasons ? "scrolls on main: " + reasons : "scrolls on impl"; } https://codereview.chromium.org/2531603003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.cpp (left): https://codereview.chromium.org/2531603003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.cpp:1157: if (!frame->isLocalFrame()) We should still continue if we hit a non local frame right? https://codereview.chromium.org/2531603003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.cpp:1164: if (toLocalFrame(frame)->localFrameRoot() != m_page->mainFrame()) Is this not still a concern with the new approach? https://codereview.chromium.org/2531603003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.cpp (right): https://codereview.chromium.org/2531603003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.cpp:1192: ->setCompositingModeEnabled(true); I don't think we should force the creation of a scroll layer (or at least not in this patch), we should just add the main thread scrolling reasons to the outer scroll layer if the iframe doesn't have its own layer. https://codereview.chromium.org/2531603003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.cpp:1210: mainThreadScrollingReasons(toLocalFrame(frame)); Shouldn't it have already been called / the reason added? If you have to call it here it suggests that we may not have added the reason in a real world use case. https://codereview.chromium.org/2531603003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.h (right): https://codereview.chromium.org/2531603003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.h:102: #endif Is this from a bad merge?
Hi Rob, Updated the layout test and removed the scroll layer creation. PTAL. Thanks! https://codereview.chromium.org/2531603003/diff/100001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/compositing/layer-creation/iframe-background-attachment-fixed.html (right): https://codereview.chromium.org/2531603003/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/compositing/layer-creation/iframe-background-attachment-fixed.html:19: <div class="composited scroller"> On 2016/12/02 19:24:02, flackr wrote: > Does this scroller affect the test? Not really. Just want to create a page with the following structure: fast slow fast https://codereview.chromium.org/2531603003/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/compositing/layer-creation/iframe-background-attachment-fixed.html:32: text = !reasons ? "Main frame scrolls on impl." : "Main frame scrolls on main thread, reasons: " + reasons; On 2016/12/02 19:24:01, flackr wrote: > Turn this ternary into a helper function and reuse it, i.e.: > function scrollingLocationAndReasons(reasons) { > return reasons ? "scrolls on main: " + reasons : "scrolls on impl"; > } Done. https://codereview.chromium.org/2531603003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.cpp (left): https://codereview.chromium.org/2531603003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.cpp:1157: if (!frame->isLocalFrame()) On 2016/12/02 19:24:02, flackr wrote: > We should still continue if we hit a non local frame right? Done. https://codereview.chromium.org/2531603003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.cpp:1164: if (toLocalFrame(frame)->localFrameRoot() != m_page->mainFrame()) On 2016/12/02 19:24:02, flackr wrote: > Is this not still a concern with the new approach? Done. https://codereview.chromium.org/2531603003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.cpp (right): https://codereview.chromium.org/2531603003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.cpp:1192: ->setCompositingModeEnabled(true); On 2016/12/02 19:24:02, flackr wrote: > I don't think we should force the creation of a scroll layer (or at least not in > this patch), we should just add the main thread scrolling reasons to the outer > scroll layer if the iframe doesn't have its own layer. Deleted the scroll layer creation. Instead, I forced it by changing the layouttest html file. https://codereview.chromium.org/2531603003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.cpp:1210: mainThreadScrollingReasons(toLocalFrame(frame)); On 2016/12/02 19:24:02, flackr wrote: > Shouldn't it have already been called / the reason added? If you have to call it > here it suggests that we may not have added the reason in a real world use case. In some layout tests regarding iframe that has background-attachment:fixed, mainThreadScrollingReasonsAsText may be called without mainThreadScrollingReasons therefore the scrollLayer of the iframe is not created. In those cases trying to access the reasons for an iframe will fail. https://codereview.chromium.org/2531603003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.h (right): https://codereview.chromium.org/2531603003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.h:102: #endif On 2016/12/02 19:24:02, flackr wrote: > Is this from a bad merge? Yes.
The CQ bit was checked by yigu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_layout_tests_slimming_paint_v2 on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_layout_te...)
yigu@chromium.org changed reviewers: + pdr@chromium.org
yigu@chromium.org changed required reviewers: + pdr@chromium.org
Hi Philip, This patch is meant to guarantee that the main frame will not scroll on main thread with an iframe scrolling on main. It failed a layout test in SPV2 but passed the linux_chromium_rel_ng test. I've noticed that you made some relevant changes recently in https://codereview.chromium.org/2506353002. Could you please take a look at my CL? Thanks! Yi
https://codereview.chromium.org/2531603003/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.cpp (right): https://codereview.chromium.org/2531603003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.cpp:1141: MainThreadScrollingReasons ScrollingCoordinator::mainThreadScrollingReasons( Changing this function to call addMainThreadScrollingReasons seems wrong since this function is const. WDYT of refactoring this logic so "scrollLayer->addMainThreadScrollingReasons" is only called when the reasons change (update functions, etc)?
https://codereview.chromium.org/2531603003/diff/100001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/compositing/layer-creation/iframe-background-attachment-fixed.html (right): https://codereview.chromium.org/2531603003/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/compositing/layer-creation/iframe-background-attachment-fixed.html:19: <div class="composited scroller"> On 2016/12/02 at 20:32:49, yigu wrote: > On 2016/12/02 19:24:02, flackr wrote: > > Does this scroller affect the test? > > Not really. Just want to create a page with the following structure: > fast > slow > fast Ah, this is meant to be frames though (i.e. iframes) e.g.: <iframe src="resources/background-attachment-fixed.html"></iframe> <iframe src="resources/fast-scroll-containing-slow-scroll.html"></iframe> (and within fast-scroll-containing-slow-scroll.html include <iframe src="resources/background-attachment-fixed.html") https://codereview.chromium.org/2531603003/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.cpp (right): https://codereview.chromium.org/2531603003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.cpp:1141: MainThreadScrollingReasons ScrollingCoordinator::mainThreadScrollingReasons( On 2016/12/06 at 06:39:14, pdr. wrote: > Changing this function to call addMainThreadScrollingReasons seems wrong since this function is const. WDYT of refactoring this logic so "scrollLayer->addMainThreadScrollingReasons" is only called when the reasons change (update functions, etc)? Agreed, it looks like ScrollingCoordinator::updateAfterCompositingChangeIfNeeded is the appropriate place to do an update of the reasons. https://codereview.chromium.org/2531603003/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.h (right): https://codereview.chromium.org/2531603003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.h:102: #endif It looks like you still have this merge artifact, please remove.
Thank you for pointing out this issue! The addMainThreadScrollingReasons function is now called in "setShouldUpdateScrollLayerPositionOnMainThread" after the reasons for main frame being set. https://codereview.chromium.org/2531603003/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.cpp (right): https://codereview.chromium.org/2531603003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.cpp:1141: MainThreadScrollingReasons ScrollingCoordinator::mainThreadScrollingReasons( On 2016/12/06 14:50:53, flackr wrote: > On 2016/12/06 at 06:39:14, pdr. wrote: > > Changing this function to call addMainThreadScrollingReasons seems wrong since > this function is const. WDYT of refactoring this logic so > "scrollLayer->addMainThreadScrollingReasons" is only called when the reasons > change (update functions, etc)? > > Agreed, it looks like ScrollingCoordinator::updateAfterCompositingChangeIfNeeded > is the appropriate place to do an update of the reasons. Done. https://codereview.chromium.org/2531603003/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.h (right): https://codereview.chromium.org/2531603003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.h:102: #endif On 2016/12/06 14:50:53, flackr wrote: > It looks like you still have this merge artifact, please remove. Done.
I wrote a patch that should fix the spv2 codepath: https://codereview.chromium.org/2550273004 Would you like to integrate that into this patch?
On 2016/12/07 01:55:55, pdr. wrote: > I wrote a patch that should fix the spv2 codepath: > https://codereview.chromium.org/2550273004 > > Would you like to integrate that into this patch? Thanks! Have it integrated into the current patch. PTAL.
https://codereview.chromium.org/2531603003/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.cpp (right): https://codereview.chromium.org/2531603003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.cpp:856: } Do the reasons need to be cleared if we no longer have main thread scrolling reasons? Can you add a test which verifies that the main thread scrolling reasons go away (on the same frame, and on a child frame) if the cause goes away? https://codereview.chromium.org/2531603003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.cpp:1180: for (Frame* checkFrame = targetFrame; checkFrame; nit: This can continue to just be called frame. https://codereview.chromium.org/2531603003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.cpp:1230: mainThreadScrollingReasons(toLocalFrame(frame)); This shouldn't be necessary anymore right?
https://codereview.chromium.org/2531603003/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/page/Page.cpp (right): https://codereview.chromium.org/2531603003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/Page.cpp:170: // TODO(pdr): This walk should be unified with ScrollingCoordinator's Can you do this TODO in this patch? Basically, pull the frame iteration loop (including that comment about OOPIF) into this function, and have this function call the ScrollingCoordinator::mainThreadScrollingReasons(frame) for !SPV2, and do the LayoutObject loop for SPV2. https://codereview.chromium.org/2531603003/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/page/Page.h (right): https://codereview.chromium.org/2531603003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/Page.h:174: String mainThreadScrollingReasonsAsText(Frame*); nit: String mainThreadScrollingReasonsAsText(const Frame&) const;
pdr@chromium.org changed required reviewers: - pdr@chromium.org
Hi Rob. Please take a look the current patch which adds the unit test and remove reasons for iframes when necessary. https://codereview.chromium.org/2531603003/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/page/Page.h (right): https://codereview.chromium.org/2531603003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/Page.h:174: String mainThreadScrollingReasonsAsText(Frame*); On 2016/12/07 22:25:29, pdr. wrote: > nit: String mainThreadScrollingReasonsAsText(const Frame&) const; This function cannot be const as it calls Page::scrollingCoordinator() which is not const. https://codereview.chromium.org/2531603003/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.cpp (right): https://codereview.chromium.org/2531603003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.cpp:856: } On 2016/12/07 21:53:11, flackr wrote: > Do the reasons need to be cleared if we no longer have main thread scrolling > reasons? Can you add a test which verifies that the main thread scrolling > reasons go away (on the same frame, and on a child frame) if the cause goes > away? Done. https://codereview.chromium.org/2531603003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.cpp:1180: for (Frame* checkFrame = targetFrame; checkFrame; On 2016/12/07 21:53:11, flackr wrote: > nit: This can continue to just be called frame. Done. https://codereview.chromium.org/2531603003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.cpp:1230: mainThreadScrollingReasons(toLocalFrame(frame)); On 2016/12/07 21:53:11, flackr wrote: > This shouldn't be necessary anymore right? Done.
Sorry I didn't get to this today. Fixing the spv2 issue in this patch may not be the best idea after all since it requires some re-plumbing. I'll go ahead and fix that as a followup to make this review simpler. Can you remove the slimmingPaintV2 code? If any spv2 tests fail, please add them with crbug.com/671838 and [ Failure ] lines to TestExpectations/FlagExpectations/enable-slimming-paint-v2 (similar to the existing iframe-background-attachment-fixed.html failure).
https://codereview.chromium.org/2531603003/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.cpp (right): https://codereview.chromium.org/2531603003/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.cpp:843: for (Frame* frame = m_page->mainFrame(); frame; This walk may be O(n^2) because all frames are walked here, and each frame walks all ancestors. https://codereview.chromium.org/2531603003/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.cpp:847: if (toLocalFrame(frame)->localFrameRoot() != m_page->mainFrame()) Is this check needed here and in ScrollingCoordinator::mainThreadScrollingReasons? I think just one is needed.
Thanks! I will remove the spv2 fix in my patch as you suggested and change Page::mainThreadScrollingReasonsAsText to const accordingly. Please see my reply for other question. Thank you! https://codereview.chromium.org/2531603003/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.cpp (right): https://codereview.chromium.org/2531603003/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.cpp:843: for (Frame* frame = m_page->mainFrame(); frame; On 2016/12/09 05:33:50, pdr. wrote: > This walk may be O(n^2) because all frames are walked here, and each frame walks > all ancestors. Yes that is not good. I was thinking of using the code regarding reasons update in ScrollingCoordinator::mainThreadScrollingReasons here in this function to update the SubFrame recursively. That way, the time complexity would drop as we only need to walk through the tree once (no need to walk all ancestors). However, the code will be duplicated including the OOPIF TODO. Given that there wouldn't be too many frames in general, I chose the current implementation which is not a perfect solution of course. Any suggestions? https://codereview.chromium.org/2531603003/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.cpp:847: if (toLocalFrame(frame)->localFrameRoot() != m_page->mainFrame()) On 2016/12/09 05:33:50, pdr. wrote: > Is this check needed here and in > ScrollingCoordinator::mainThreadScrollingReasons? I think just one is needed. Done.
https://codereview.chromium.org/2531603003/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.cpp (right): https://codereview.chromium.org/2531603003/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.cpp:843: for (Frame* frame = m_page->mainFrame(); frame; On 2016/12/09 at 14:33:52, yigu wrote: > On 2016/12/09 05:33:50, pdr. wrote: > > This walk may be O(n^2) because all frames are walked here, and each frame walks > > all ancestors. > > Yes that is not good. I was thinking of using the code regarding reasons update in ScrollingCoordinator::mainThreadScrollingReasons here in this function to update the SubFrame recursively. That way, the time complexity would drop as we only need to walk through the tree once (no need to walk all ancestors). However, the code will be duplicated including the OOPIF TODO. Given that there wouldn't be too many frames in general, I chose the current implementation which is not a perfect solution of course. Any suggestions? Create a common function which collects the reason for a single frame, and then use it from both ScrollingCoordinator::mainThreadScrollingReasons (on all ancestors) and here to accumulate the reasons as you walk to children frames.
On 2016/12/09 15:03:40, flackr wrote: > https://codereview.chromium.org/2531603003/diff/200001/third_party/WebKit/Sou... > File third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.cpp > (right): > > https://codereview.chromium.org/2531603003/diff/200001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.cpp:843: for > (Frame* frame = m_page->mainFrame(); frame; > On 2016/12/09 at 14:33:52, yigu wrote: > > On 2016/12/09 05:33:50, pdr. wrote: > > > This walk may be O(n^2) because all frames are walked here, and each frame > walks > > > all ancestors. > > > > Yes that is not good. I was thinking of using the code regarding reasons > update in ScrollingCoordinator::mainThreadScrollingReasons here in this function > to update the SubFrame recursively. That way, the time complexity would drop as > we only need to walk through the tree once (no need to walk all ancestors). > However, the code will be duplicated including the OOPIF TODO. Given that there > wouldn't be too many frames in general, I chose the current implementation which > is not a perfect solution of course. Any suggestions? > > Create a common function which collects the reason for a single frame, and then > use it from both ScrollingCoordinator::mainThreadScrollingReasons (on all > ancestors) and here to accumulate the reasons as you walk to children frames. Thanks Rob! New patch is on. PTAL.
On 2016/12/09 at 16:32:37, yigu wrote: > On 2016/12/09 15:03:40, flackr wrote: > > https://codereview.chromium.org/2531603003/diff/200001/third_party/WebKit/Sou... > > File third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.cpp > > (right): > > > > https://codereview.chromium.org/2531603003/diff/200001/third_party/WebKit/Sou... > > third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.cpp:843: for > > (Frame* frame = m_page->mainFrame(); frame; > > On 2016/12/09 at 14:33:52, yigu wrote: > > > On 2016/12/09 05:33:50, pdr. wrote: > > > > This walk may be O(n^2) because all frames are walked here, and each frame > > walks > > > > all ancestors. > > > > > > Yes that is not good. I was thinking of using the code regarding reasons > > update in ScrollingCoordinator::mainThreadScrollingReasons here in this function > > to update the SubFrame recursively. That way, the time complexity would drop as > > we only need to walk through the tree once (no need to walk all ancestors). > > However, the code will be duplicated including the OOPIF TODO. Given that there > > wouldn't be too many frames in general, I chose the current implementation which > > is not a perfect solution of course. Any suggestions? > > > > Create a common function which collects the reason for a single frame, and then > > use it from both ScrollingCoordinator::mainThreadScrollingReasons (on all > > ancestors) and here to accumulate the reasons as you walk to children frames. > > Thanks Rob! New patch is on. PTAL. I might misunderstand Rob's point here, but I think he was describing a way to avoid the O(n^2) behavior by only doing a walk of size O(|# frames|). The basic idea is to pass the current main thread scrolling reasons down as you walk the tree, so you never have to crawl back up the tree. This can be implemented as a recursive function call on FrameView that looks something like: "MainThreadScrollingReasons FrameView::mainThreadScrollingReasons(MainThreadScrollingReasons parentReasons)", or "MainThreadScrollingReasons ScrollingCoordinator::mainThreadScrollingReasons(MainThreadScrollingReasons parentReasons, Frame& subframe)".
Hi Philip, Thanks for the suggestion! I used to update the frame tree recursively using ScrollingCoordinator::mainThreadScrollingReasons(subframe, parentReason) in patch 9 ( https://codereview.chromium.org/2531603003/diff/160001/third_party/WebKit/Sou...) but not the current patch. I thought the current implementation also did a walk of size O(|#frames|), right? For querying reasons of a specific frame using mainThreadScrollingReasons(frame), I walk up the subtree from the target frame to mainFrame. For updating the frame tree, I walk through the tree only once using frame->tree().traverseNext(). Am I right? Or did I miss something? Yi On Mon, Dec 12, 2016 at 4:07 PM, <pdr@chromium.org> wrote: > On 2016/12/09 at 16:32:37, yigu wrote: > > On 2016/12/09 15:03:40, flackr wrote: > > > > https://codereview.chromium.org/2531603003/diff/200001/ > third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.cpp > > > File third_party/WebKit/Source/core/page/scrolling/ > ScrollingCoordinator.cpp > > > (right): > > > > > > > https://codereview.chromium.org/2531603003/diff/200001/ > third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.cpp# > newcode843 > > > third_party/WebKit/Source/core/page/scrolling/ > ScrollingCoordinator.cpp:843: > for > > > (Frame* frame = m_page->mainFrame(); frame; > > > On 2016/12/09 at 14:33:52, yigu wrote: > > > > On 2016/12/09 05:33:50, pdr. wrote: > > > > > This walk may be O(n^2) because all frames are walked here, and > each > frame > > > walks > > > > > all ancestors. > > > > > > > > Yes that is not good. I was thinking of using the code regarding > reasons > > > update in ScrollingCoordinator::mainThreadScrollingReasons here in > this > function > > > to update the SubFrame recursively. That way, the time complexity > would drop > as > > > we only need to walk through the tree once (no need to walk all > ancestors). > > > However, the code will be duplicated including the OOPIF TODO. Given > that > there > > > wouldn't be too many frames in general, I chose the current > implementation > which > > > is not a perfect solution of course. Any suggestions? > > > > > > Create a common function which collects the reason for a single frame, > and > then > > > use it from both ScrollingCoordinator::mainThreadScrollingReasons (on > all > > > ancestors) and here to accumulate the reasons as you walk to children > frames. > > > > Thanks Rob! New patch is on. PTAL. > > I might misunderstand Rob's point here, but I think he was describing a > way to > avoid the O(n^2) behavior by only doing a walk of size O(|# frames|). > > The basic idea is to pass the current main thread scrolling reasons down > as you > walk the tree, so you never have to crawl back up the tree. This can be > implemented as a recursive function call on FrameView that looks something > like: > "MainThreadScrollingReasons > FrameView::mainThreadScrollingReasons(MainThreadScrollingReasons > parentReasons)", or "MainThreadScrollingReasons > ScrollingCoordinator::mainThreadScrollingReasons( > MainThreadScrollingReasons > parentReasons, Frame& subframe)". > > https://codereview.chromium.org/2531603003/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Hi Philip, Thanks for the suggestion! I used to update the frame tree recursively using ScrollingCoordinator::mainThreadScrollingReasons(subframe, parentReason) in patch 9 ( https://codereview.chromium.org/2531603003/diff/160001/third_party/WebKit/Sou...) but not the current patch. I thought the current implementation also did a walk of size O(|#frames|), right? For querying reasons of a specific frame using mainThreadScrollingReasons(frame), I walk up the subtree from the target frame to mainFrame. For updating the frame tree, I walk through the tree only once using frame->tree().traverseNext(). Am I right? Or did I miss something? Yi On Mon, Dec 12, 2016 at 4:07 PM, <pdr@chromium.org> wrote: > On 2016/12/09 at 16:32:37, yigu wrote: > > On 2016/12/09 15:03:40, flackr wrote: > > > > https://codereview.chromium.org/2531603003/diff/200001/ > third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.cpp > > > File third_party/WebKit/Source/core/page/scrolling/ > ScrollingCoordinator.cpp > > > (right): > > > > > > > https://codereview.chromium.org/2531603003/diff/200001/ > third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.cpp# > newcode843 > > > third_party/WebKit/Source/core/page/scrolling/ > ScrollingCoordinator.cpp:843: > for > > > (Frame* frame = m_page->mainFrame(); frame; > > > On 2016/12/09 at 14:33:52, yigu wrote: > > > > On 2016/12/09 05:33:50, pdr. wrote: > > > > > This walk may be O(n^2) because all frames are walked here, and > each > frame > > > walks > > > > > all ancestors. > > > > > > > > Yes that is not good. I was thinking of using the code regarding > reasons > > > update in ScrollingCoordinator::mainThreadScrollingReasons here in > this > function > > > to update the SubFrame recursively. That way, the time complexity > would drop > as > > > we only need to walk through the tree once (no need to walk all > ancestors). > > > However, the code will be duplicated including the OOPIF TODO. Given > that > there > > > wouldn't be too many frames in general, I chose the current > implementation > which > > > is not a perfect solution of course. Any suggestions? > > > > > > Create a common function which collects the reason for a single frame, > and > then > > > use it from both ScrollingCoordinator::mainThreadScrollingReasons (on > all > > > ancestors) and here to accumulate the reasons as you walk to children > frames. > > > > Thanks Rob! New patch is on. PTAL. > > I might misunderstand Rob's point here, but I think he was describing a > way to > avoid the O(n^2) behavior by only doing a walk of size O(|# frames|). > > The basic idea is to pass the current main thread scrolling reasons down > as you > walk the tree, so you never have to crawl back up the tree. This can be > implemented as a recursive function call on FrameView that looks something > like: > "MainThreadScrollingReasons > FrameView::mainThreadScrollingReasons(MainThreadScrollingReasons > parentReasons)", or "MainThreadScrollingReasons > ScrollingCoordinator::mainThreadScrollingReasons( > MainThreadScrollingReasons > parentReasons, Frame& subframe)". > > https://codereview.chromium.org/2531603003/ > -- You received this message because you are subscribed to the Google Groups "Blink Reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to blink-reviews+unsubscribe@chromium.org.
https://codereview.chromium.org/2531603003/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/page/Page.cpp (right): https://codereview.chromium.org/2531603003/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/Page.cpp:45: #include "core/layout/LayoutView.h" Nit: Is this needed anymore? https://codereview.chromium.org/2531603003/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.cpp (right): https://codereview.chromium.org/2531603003/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.cpp:193: mainThreadScrollingReasons(m_page->deprecatedLocalMainFrame())); Nit: lets refactor this to not use deprecated functions (here, and on line 188). The first line of this function checks "if (!m_page->mainFrame()->isLocalFrame())" so I think we can just use "toLocalFrame(frame)" here. https://codereview.chromium.org/2531603003/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.cpp:843: void ScrollingCoordinator::updateSubFrameScrollOnMainReason() { Ah, I misread this code. You are correct that this is no longer O(n^2). I think the general design of how you split up the calls into per-frame reasons and global reasons is a good approach. https://codereview.chromium.org/2531603003/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.cpp:852: for (Frame* frame = m_page->mainFrame(); frame; This loop doesn't quite look right, or I may misunderstand. Imagine we have the following tree: A | B | C If B has background attachment fixed objects, it will have local main thread scrolling reasons. In that case, I think A's scroll layer should also have main thread scrolling reasons, but C's scroll layer should not--is that correct? Won't this loop set main thread scrolling reasons incorrectly on C? A slightly more complex example: ..A ./.\ B...D | C If B has background attachment fixed objects, what main thread scrolling reasons should the scroll layers of A, C, and D have? I thought they would be: A(has main thread reasons), B(has main thread reasons), C(no main thread reasons), and D(no main thread reasons). https://codereview.chromium.org/2531603003/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.cpp:989: // If there's a handler on the window, document, html or body element Nit: please re-flow this multi-line comment so it's 80 cols. https://codereview.chromium.org/2531603003/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.cpp:1182: ScrollingCoordinator::mainThreadScrollingReasonsPerFrame( Does this need to be on ScrollingCoordinator? I wonder if we could just move all of this code into FrameView::getScrollingReasons()? Then it's a little easier to understand that the reasons are local to a single FrameView. https://codereview.chromium.org/2531603003/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.cpp:1223: if (targetFrame->localFrameRoot() != m_page->mainFrame()) Please add the comment back about OOPIF: // TODO(alexmos,kenrb): For OOPIF, local roots that are different from // the main frame can't be used in the calculation, since they use // different compositors with unrelated state, which breaks some of the // calculations below. https://codereview.chromium.org/2531603003/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.h (right): https://codereview.chromium.org/2531603003/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.h:98: MainThreadScrollingReasons mainThreadScrollingReasons(LocalFrame*) const; Nit: LocalFrame& (const LocalFrame& if possible) Similarly for the rest of this patch, switch from passing pointers to references (const if possible).
https://codereview.chromium.org/2531603003/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.cpp (right): https://codereview.chromium.org/2531603003/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.cpp:852: for (Frame* frame = m_page->mainFrame(); frame; On 2016/12/13 at 05:51:16, pdr. wrote: > This loop doesn't quite look right, or I may misunderstand. I was wrong in how this works. For A-B-C, if B has main thread reasons, only A needs it too (not C). I am still insure this loop is correct though, because of how frame->tree().traverseNext() works. Consider the following: ..A ./.\ B...D | C If B has main thread scrolling reasons, won't this loop also set them unnecessarily on D?
On 2016/12/13 23:07:10, pdr. wrote: > https://codereview.chromium.org/2531603003/diff/240001/third_party/WebKit/Sou... > File third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.cpp > (right): > > https://codereview.chromium.org/2531603003/diff/240001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.cpp:852: for > (Frame* frame = m_page->mainFrame(); frame; > On 2016/12/13 at 05:51:16, pdr. wrote: > > This loop doesn't quite look right, or I may misunderstand. > > I was wrong in how this works. For A-B-C, if B has main thread reasons, only A > needs it too (not C). > > I am still insure this loop is correct though, because of how > frame->tree().traverseNext() works. > > Consider the following: > ..A > ./.\ > B...D > | > C > > If B has main thread scrolling reasons, won't this loop also set them > unnecessarily on D? The reasons are separate. I'm doing "reasons =" rather than "reasons |=". Right?
On 2016/12/13 at 23:09:46, yigu wrote: > The reasons are separate. I'm doing "reasons =" rather than "reasons |=". Right? In that case, how does C get main thread reasons set?
On 2016/12/13 23:15:23, pdr. wrote: > On 2016/12/13 at 23:09:46, yigu wrote: > > The reasons are separate. I'm doing "reasons =" rather than "reasons |=". > Right? > > In that case, how does C get main thread reasons set? I didn't set the reason of B for C because technically speaking C may not have that reason. Instead, in the function mainThreadScrollingReason(const LocalFrame& frame), when we pass frame C in, we check the subtree from C to its parent and all the way to the main frame. If any of C's ancestor has some main thread scrolling reasons, the function would return them as expected.
On 2016/12/14 at 00:05:24, yigu wrote: > On 2016/12/13 23:15:23, pdr. wrote: > > On 2016/12/13 at 23:09:46, yigu wrote: > > > The reasons are separate. I'm doing "reasons =" rather than "reasons |=". > > Right? > > > > In that case, how does C get main thread reasons set? > > I didn't set the reason of B for C because technically speaking C may not have that reason. Instead, in the function mainThreadScrollingReason(const LocalFrame& frame), when we pass frame C in, we check the subtree from C to its parent and all the way to the main frame. If any of C's ancestor has some main thread scrolling reasons, the function would return them as expected. Don't we need to set scroll layer reasons for B and C in that case? scrollLayer->addMainThreadScrollingReasons(reasons);
On 2016/12/14 00:18:57, pdr. wrote: > On 2016/12/14 at 00:05:24, yigu wrote: > > On 2016/12/13 23:15:23, pdr. wrote: > > > On 2016/12/13 at 23:09:46, yigu wrote: > > > > The reasons are separate. I'm doing "reasons =" rather than "reasons |=". > > > Right? > > > > > > In that case, how does C get main thread reasons set? > > > > I didn't set the reason of B for C because technically speaking C may not have > that reason. Instead, in the function mainThreadScrollingReason(const > LocalFrame& frame), when we pass frame C in, we check the subtree from C to its > parent and all the way to the main frame. If any of C's ancestor has some main > thread scrolling reasons, the function would return them as expected. > > Don't we need to set scroll layer reasons for B and C in that case? > scrollLayer->addMainThreadScrollingReasons(reasons); Is that the same as the one I used in updateSubFrameScrollOnMainReason?
On 2016/12/14 at 00:24:35, yigu wrote: > On 2016/12/14 00:18:57, pdr. wrote: > > On 2016/12/14 at 00:05:24, yigu wrote: > > > On 2016/12/13 23:15:23, pdr. wrote: > > > > On 2016/12/13 at 23:09:46, yigu wrote: > > > > > The reasons are separate. I'm doing "reasons =" rather than "reasons |=". > > > > Right? > > > > > > > > In that case, how does C get main thread reasons set? > > > > > > I didn't set the reason of B for C because technically speaking C may not have > > that reason. Instead, in the function mainThreadScrollingReason(const > > LocalFrame& frame), when we pass frame C in, we check the subtree from C to its > > parent and all the way to the main frame. If any of C's ancestor has some main > > thread scrolling reasons, the function would return them as expected. > > > > Don't we need to set scroll layer reasons for B and C in that case? > > scrollLayer->addMainThreadScrollingReasons(reasons); > > Is that the same as the one I used in updateSubFrameScrollOnMainReason? Yeah.
On 2016/12/14 00:32:07, pdr. wrote: > On 2016/12/14 at 00:24:35, yigu wrote: > > On 2016/12/14 00:18:57, pdr. wrote: > > > On 2016/12/14 at 00:05:24, yigu wrote: > > > > On 2016/12/13 23:15:23, pdr. wrote: > > > > > On 2016/12/13 at 23:09:46, yigu wrote: > > > > > > The reasons are separate. I'm doing "reasons =" rather than "reasons > |=". > > > > > Right? > > > > > > > > > > In that case, how does C get main thread reasons set? > > > > > > > > I didn't set the reason of B for C because technically speaking C may not > have > > > that reason. Instead, in the function mainThreadScrollingReason(const > > > LocalFrame& frame), when we pass frame C in, we check the subtree from C to > its > > > parent and all the way to the main frame. If any of C's ancestor has some > main > > > thread scrolling reasons, the function would return them as expected. > > > > > > Don't we need to set scroll layer reasons for B and C in that case? > > > scrollLayer->addMainThreadScrollingReasons(reasons); > > > > Is that the same as the one I used in updateSubFrameScrollOnMainReason? > > Yeah. Sorry for lacking of enough comments in the code. I should have brought you in from the beginning of this CL. That way we could understand the logic better:)
On 2016/12/14 at 00:35:25, yigu wrote: > On 2016/12/14 00:32:07, pdr. wrote: > > On 2016/12/14 at 00:24:35, yigu wrote: > > > On 2016/12/14 00:18:57, pdr. wrote: > > > > On 2016/12/14 at 00:05:24, yigu wrote: > > > > > On 2016/12/13 23:15:23, pdr. wrote: > > > > > > On 2016/12/13 at 23:09:46, yigu wrote: > > > > > > > The reasons are separate. I'm doing "reasons =" rather than "reasons > > |=". > > > > > > Right? > > > > > > > > > > > > In that case, how does C get main thread reasons set? > > > > > > > > > > I didn't set the reason of B for C because technically speaking C may not > > have > > > > that reason. Instead, in the function mainThreadScrollingReason(const > > > > LocalFrame& frame), when we pass frame C in, we check the subtree from C to > > its > > > > parent and all the way to the main frame. If any of C's ancestor has some > > main > > > > thread scrolling reasons, the function would return them as expected. > > > > > > > > Don't we need to set scroll layer reasons for B and C in that case? > > > > scrollLayer->addMainThreadScrollingReasons(reasons); > > > > > > Is that the same as the one I used in updateSubFrameScrollOnMainReason? > > > > Yeah. > > Sorry for lacking of enough comments in the code. I should have brought you in from the beginning of this CL. That way we could understand the logic better:) I still don't see how we propagate the main thread scrolling reason down. Do we have a test of this? Specifically: A | B where A has background attachment fixed but B does not. B's scrolling layer should still have main thread scrolling reasons.
On 2016/12/14 00:40:52, pdr. wrote: > On 2016/12/14 at 00:35:25, yigu wrote: > > On 2016/12/14 00:32:07, pdr. wrote: > > > On 2016/12/14 at 00:24:35, yigu wrote: > > > > On 2016/12/14 00:18:57, pdr. wrote: > > > > > On 2016/12/14 at 00:05:24, yigu wrote: > > > > > > On 2016/12/13 23:15:23, pdr. wrote: > > > > > > > On 2016/12/13 at 23:09:46, yigu wrote: > > > > > > > > The reasons are separate. I'm doing "reasons =" rather than > "reasons > > > |=". > > > > > > > Right? > > > > > > > > > > > > > > In that case, how does C get main thread reasons set? > > > > > > > > > > > > I didn't set the reason of B for C because technically speaking C may > not > > > have > > > > > that reason. Instead, in the function mainThreadScrollingReason(const > > > > > LocalFrame& frame), when we pass frame C in, we check the subtree from C > to > > > its > > > > > parent and all the way to the main frame. If any of C's ancestor has > some > > > main > > > > > thread scrolling reasons, the function would return them as expected. > > > > > > > > > > Don't we need to set scroll layer reasons for B and C in that case? > > > > > scrollLayer->addMainThreadScrollingReasons(reasons); > > > > > > > > Is that the same as the one I used in updateSubFrameScrollOnMainReason? > > > > > > Yeah. > > > > Sorry for lacking of enough comments in the code. I should have brought you in > from the beginning of this CL. That way we could understand the logic better:) > > I still don't see how we propagate the main thread scrolling reason down. Do we > have a test of this? > > Specifically: > A > | > B > > where A has background attachment fixed but B does not. B's scrolling layer > should still have main thread scrolling reasons. I think you are right. In this case, B's scrolling layer doesn't have main thread scrolling reasons because B has no fixed background attachment. However, if we call mainThreadScrollingReason(B) it will return kHasBackgroundAttachmentFixed because in that function we have something like: reason |= reasonOfB; -- reason: 0 reason |= reasonOfA; -- reason: kHasBackgroundAttachmentFixed return reason; Although we can "get" the reason of B scrolling on main thread, the reason itself is not stored in its layer and therefore we actually scroll it on cc. I should use the recursive method to update all subFrames.
Cool, I think we're on the same page. Please add a unit test for this case too.
Hi Philip, In this patch I recursively update the frame tree to make sure all reasons from ancestors could be propagated to the descendants. Related unit test has been added as well. In addition, the logic regarding mainThreadScrollingReasons has been moved to FrameView. PTAL. Thanks. Yi https://codereview.chromium.org/2531603003/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/page/Page.cpp (right): https://codereview.chromium.org/2531603003/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/Page.cpp:45: #include "core/layout/LayoutView.h" On 2016/12/13 05:51:16, pdr. wrote: > Nit: Is this needed anymore? Done. https://codereview.chromium.org/2531603003/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.cpp (right): https://codereview.chromium.org/2531603003/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.cpp:193: mainThreadScrollingReasons(m_page->deprecatedLocalMainFrame())); On 2016/12/13 05:51:17, pdr. wrote: > Nit: lets refactor this to not use deprecated functions (here, and on line 188). > The first line of this function checks "if > (!m_page->mainFrame()->isLocalFrame())" so I think we can just use > "toLocalFrame(frame)" here. Done. https://codereview.chromium.org/2531603003/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.cpp:843: void ScrollingCoordinator::updateSubFrameScrollOnMainReason() { On 2016/12/13 05:51:16, pdr. wrote: > Ah, I misread this code. You are correct that this is no longer O(n^2). I think > the general design of how you split up the calls into per-frame reasons and > global reasons is a good approach. Done. https://codereview.chromium.org/2531603003/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.cpp:989: // If there's a handler on the window, document, html or body element On 2016/12/13 05:51:16, pdr. wrote: > Nit: please re-flow this multi-line comment so it's 80 cols. Done. https://codereview.chromium.org/2531603003/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.h (right): https://codereview.chromium.org/2531603003/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.h:98: MainThreadScrollingReasons mainThreadScrollingReasons(LocalFrame*) const; On 2016/12/13 05:51:17, pdr. wrote: > Nit: LocalFrame& > (const LocalFrame& if possible) > > Similarly for the rest of this patch, switch from passing pointers to references > (const if possible). Done.
Sorry about taking so long to review this. I really like this latest patch, just a bunch of nits and small comments left. https://codereview.chromium.org/2531603003/diff/260001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/frame/FrameView.cpp (right): https://codereview.chromium.org/2531603003/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/FrameView.cpp:4682: bool FrameView::hasVisibleSlowRepaintViewportConstrainedObjects( Nit: I don't think you need to pass in frameView here, just make this a member function on FrameView. E.g.: bool FrameView::hasVisibleSlowRepaintViewportConstrainedObjects() const { if (!viewportConstrainedObjects()) return false; for (const auto* layoutObject : *viewportConstrainedObjects()) { ...etc... https://codereview.chromium.org/2531603003/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/FrameView.cpp:4740: ~0 & ~MainThreadScrollingReason::kHandlingScrollFromMainThread); What is ~0 for? https://codereview.chromium.org/2531603003/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/FrameView.cpp:4754: MainThreadScrollingReasons FrameView::mainThreadScrollingReasonsPerFrame( Is "const LocalFrame& frame" needed? Similar to hasVisibleSlowRepaintViewportConstrainedObjects, I think this can be refactored as a member function: MainThreadScrollingReasons FrameView::localMainThreadScrollingReasons() const { MainThreadScrollingReasons reasons = static_cast<MainThreadScrollingReasons>(0); if (shouldThrottleRendering()) return reasons; ...etc... In FrameView.h, can you add a very short comment above mainThreadScrollingReasons and localMainThreadScrollingReasons describing the difference? Maybe something like: // Main thread scrolling reasons including reasons from ancestors. MainThreadScrollingReasons mainThreadScrollingReasons(); private: // Main thread scrolling reasons for this object only. For all reasons, // see: mainThreadScrollingReasons(). MainThreadScrollingReasons localMainThreadScrollingReasons() const; https://codereview.chromium.org/2531603003/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/FrameView.cpp:4800: // Walk through the subtree from the target frame to root. Use the gathered Nit: Update this to reflect that the function is now on FrameView. e.g., Walk the tree to the root. Use the gathered reasons...etc https://codereview.chromium.org/2531603003/diff/260001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/frame/FrameView.h (right): https://codereview.chromium.org/2531603003/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/FrameView.h:771: // Recursively update frame tree. Each frame has its only +1, nice comment. https://codereview.chromium.org/2531603003/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/FrameView.h:1155: MainThreadScrollingReasons m_lastMainThreadScrollingReasons; Is "last" important? m_mainThreadScrollingReasons may be simpler.
Description was changed from ========== Only scroll on main if the targeted frames need to scroll on main BUG=568901 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== Only scroll on main if the targeted frames need to scroll on main BUG=568901 TEST=ScrollingCoordinatorTest.BackgroundAttachmentFixedShouldTriggerMainThreadScroll; FrameThrottlingTest.ScrollingCoordinatorShouldSkipThrottledFrame third_party/WebKit/LayoutTests/compositing/layer-creation/iframe-background-attachment-fixed.html CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
yigu@chromium.org changed required reviewers: + pdr@chromium.org
The CQ bit was checked by yigu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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_...) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
The CQ bit was checked by yigu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Hi Philip, Thanks for your thorough feedback! Made the changes as you suggested. PTAL. Yi https://codereview.chromium.org/2531603003/diff/260001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/frame/FrameView.cpp (right): https://codereview.chromium.org/2531603003/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/FrameView.cpp:4682: bool FrameView::hasVisibleSlowRepaintViewportConstrainedObjects( On 2016/12/17 05:50:16, pdr. wrote: > Nit: I don't think you need to pass in frameView here, just make this a member > function on FrameView. > > E.g.: > bool FrameView::hasVisibleSlowRepaintViewportConstrainedObjects() const { > if (!viewportConstrainedObjects()) > return false; > for (const auto* layoutObject : *viewportConstrainedObjects()) { > ...etc... Done. https://codereview.chromium.org/2531603003/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/FrameView.cpp:4740: ~0 & ~MainThreadScrollingReason::kHandlingScrollFromMainThread); On 2016/12/17 05:50:16, pdr. wrote: > What is ~0 for? That was silly.. https://codereview.chromium.org/2531603003/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/FrameView.cpp:4754: MainThreadScrollingReasons FrameView::mainThreadScrollingReasonsPerFrame( On 2016/12/17 05:50:16, pdr. wrote: > Is "const LocalFrame& frame" needed? Similar to > hasVisibleSlowRepaintViewportConstrainedObjects, I think this can be refactored > as a member function: > MainThreadScrollingReasons FrameView::localMainThreadScrollingReasons() const { > MainThreadScrollingReasons reasons = > static_cast<MainThreadScrollingReasons>(0); > if (shouldThrottleRendering()) > return reasons; > ...etc... > > In FrameView.h, can you add a very short comment above > mainThreadScrollingReasons and localMainThreadScrollingReasons describing the > difference? > Maybe something like: > // Main thread scrolling reasons including reasons from ancestors. > MainThreadScrollingReasons mainThreadScrollingReasons(); > > private: > // Main thread scrolling reasons for this object only. For all reasons, > // see: mainThreadScrollingReasons(). > MainThreadScrollingReasons localMainThreadScrollingReasons() const; Done. https://codereview.chromium.org/2531603003/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/FrameView.cpp:4800: // Walk through the subtree from the target frame to root. Use the gathered On 2016/12/17 05:50:16, pdr. wrote: > Nit: Update this to reflect that the function is now on FrameView. > > e.g., Walk the tree to the root. Use the gathered reasons...etc Done. https://codereview.chromium.org/2531603003/diff/260001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/frame/FrameView.h (right): https://codereview.chromium.org/2531603003/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/FrameView.h:771: // Recursively update frame tree. Each frame has its only On 2016/12/17 05:50:16, pdr. wrote: > +1, nice comment. Thanks for your previous suggestion on this :) https://codereview.chromium.org/2531603003/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/FrameView.h:1155: MainThreadScrollingReasons m_lastMainThreadScrollingReasons; On 2016/12/17 05:50:16, pdr. wrote: > Is "last" important? m_mainThreadScrollingReasons may be simpler. Done.
https://codereview.chromium.org/2531603003/diff/300001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/compositing/layer-creation/resources/background-attachment-fixed.html (right): https://codereview.chromium.org/2531603003/diff/300001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/compositing/layer-creation/resources/background-attachment-fixed.html:15: if (window.testRunner) Nit: is this test only loaded as an iframe in other tests? If so, this dumpAsText line should be unnecessary. https://codereview.chromium.org/2531603003/diff/300001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/frame/FrameView.cpp (right): https://codereview.chromium.org/2531603003/diff/300001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/FrameView.cpp:4756: scrollLayer->clearMainThreadScrollingReasons( Please add a comment here: // Clear all main thread scrolling reasons except the one that's set // if there is a running scroll animation. I think there is a bug with how kHandlingScrollFromMainThread works. It looks like we set kHandlingScrollFromMainThread when a scroll animation is active, but we don't update the rest of the tree (i.e., force main thread scrolling for subframes). Do you agree? Can you file a bug for this and address it in a followup patch? https://codereview.chromium.org/2531603003/diff/300001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/FrameView.cpp:4781: ScrollingReasons scrollingReasons = getScrollingReasons(); I think there's a bug similar to https://crbug.com/675296 here. If this value changes (e.g., the contents size changes which causes getScrollingReasons() to go from NotScrollableNoOverflow to Scrollable), do we re-calculate the main thread scrolling reasons for all frames? Can you make a unittest for this? I'm thinking 3 nested frames (A->B->C) with no scrolling reasons, then B's contents size changes which causes it to have NotScrollableNoOverflow which leads to B and C both having kHasNonLayerViewportConstrainedObjects set. Changing the size of B again should reset all reasons, etc. If this test fails, I think it's okay to land the test failing (with a DISABLED_ prefix) and fix it in a followup. https://codereview.chromium.org/2531603003/diff/300001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/tests/ScrollingCoordinatorTest.cpp (right): https://codereview.chromium.org/2531603003/diff/300001/third_party/WebKit/Sou... third_party/WebKit/Source/web/tests/ScrollingCoordinatorTest.cpp:914: "background-image: url('white-1x1.png'); background-attachment: fixed;;", Nit: extra semicolon
Hi Philip, New patch regarding you feedback is on. PTAL. Thanks! Yi https://codereview.chromium.org/2531603003/diff/300001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/compositing/layer-creation/resources/background-attachment-fixed.html (right): https://codereview.chromium.org/2531603003/diff/300001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/compositing/layer-creation/resources/background-attachment-fixed.html:15: if (window.testRunner) On 2016/12/19 04:04:28, pdr. wrote: > Nit: is this test only loaded as an iframe in other tests? If so, this > dumpAsText line should be unnecessary. Done. https://codereview.chromium.org/2531603003/diff/300001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/frame/FrameView.cpp (right): https://codereview.chromium.org/2531603003/diff/300001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/FrameView.cpp:4756: scrollLayer->clearMainThreadScrollingReasons( On 2016/12/19 04:04:28, pdr. wrote: > Please add a comment here: > // Clear all main thread scrolling reasons except the one that's set > // if there is a running scroll animation. > > I think there is a bug with how kHandlingScrollFromMainThread works. It looks > like we set kHandlingScrollFromMainThread when a scroll animation is active, but > we don't update the rest of the tree (i.e., force main thread scrolling for > subframes). Do you agree? Can you file a bug for this and address it in a > followup patch? Done. Filed the bug as crbug.com/675677. https://codereview.chromium.org/2531603003/diff/300001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/FrameView.cpp:4781: ScrollingReasons scrollingReasons = getScrollingReasons(); On 2016/12/19 04:04:28, pdr (OOO Dec 23 to Jan 2) wrote: > I think there's a bug similar to https://crbug.com/675296 here. If this value > changes (e.g., the contents size changes which causes getScrollingReasons() to > go from NotScrollableNoOverflow to Scrollable), do we re-calculate the main > thread scrolling reasons for all frames? > > Can you make a unittest for this? I'm thinking 3 nested frames (A->B->C) with no > scrolling reasons, then B's contents size changes which causes it to have > NotScrollableNoOverflow which leads to B and C both having > kHasNonLayerViewportConstrainedObjects set. Changing the size of B again should > reset all reasons, etc. > > If this test fails, I think it's okay to land the test failing (with a DISABLED_ > prefix) and fix it in a followup. It seems we can handle it now. Unit test added. https://codereview.chromium.org/2531603003/diff/300001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/tests/ScrollingCoordinatorTest.cpp (right): https://codereview.chromium.org/2531603003/diff/300001/third_party/WebKit/Sou... third_party/WebKit/Source/web/tests/ScrollingCoordinatorTest.cpp:914: "background-image: url('white-1x1.png'); background-attachment: fixed;;", On 2016/12/19 04:04:28, pdr (OOO Dec 23 to Jan 2) wrote: > Nit: extra semicolon Done.
Description was changed from ========== Only scroll on main if the targeted frames need to scroll on main BUG=568901 TEST=ScrollingCoordinatorTest.BackgroundAttachmentFixedShouldTriggerMainThreadScroll; FrameThrottlingTest.ScrollingCoordinatorShouldSkipThrottledFrame third_party/WebKit/LayoutTests/compositing/layer-creation/iframe-background-attachment-fixed.html CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== Only scroll on main if the targeted frames need to scroll on main BUG=568901 TEST=ScrollingCoordinatorTest.BackgroundAttachmentFixedShouldTriggerMainThreadScroll; FrameThrottlingTest.ScrollingCoordinatorShouldSkipThrottledFrame; ScrollingCoordinatorTest.RecalculateMainThreadScrollingReasonsUponResize third_party/WebKit/LayoutTests/compositing/layer-creation/iframe-background-attachment-fixed.html CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
LGTM
The CQ bit was checked by yigu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by yigu@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pdr@chromium.org Link to the patchset: https://codereview.chromium.org/2531603003/#ps360001 (title: "Resolve conflict")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 360001, "attempt_start_ts": 1482244602515110,
"parent_rev": "a230553653d6a18f4c9151a3456ba105a9a050a7", "commit_rev":
"19f3da24348ce018a906e6ccd859d9a7eb9065fa"}
Message was sent while issue was closed.
Description was changed from ========== Only scroll on main if the targeted frames need to scroll on main BUG=568901 TEST=ScrollingCoordinatorTest.BackgroundAttachmentFixedShouldTriggerMainThreadScroll; FrameThrottlingTest.ScrollingCoordinatorShouldSkipThrottledFrame; ScrollingCoordinatorTest.RecalculateMainThreadScrollingReasonsUponResize third_party/WebKit/LayoutTests/compositing/layer-creation/iframe-background-attachment-fixed.html CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== Only scroll on main if the targeted frames need to scroll on main BUG=568901 TEST=ScrollingCoordinatorTest.BackgroundAttachmentFixedShouldTriggerMainThreadScroll; FrameThrottlingTest.ScrollingCoordinatorShouldSkipThrottledFrame; ScrollingCoordinatorTest.RecalculateMainThreadScrollingReasonsUponResize third_party/WebKit/LayoutTests/compositing/layer-creation/iframe-background-attachment-fixed.html CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 Review-Url: https://codereview.chromium.org/2531603003 ==========
Message was sent while issue was closed.
Committed patchset #19 (id:360001)
Message was sent while issue was closed.
Description was changed from ========== Only scroll on main if the targeted frames need to scroll on main BUG=568901 TEST=ScrollingCoordinatorTest.BackgroundAttachmentFixedShouldTriggerMainThreadScroll; FrameThrottlingTest.ScrollingCoordinatorShouldSkipThrottledFrame; ScrollingCoordinatorTest.RecalculateMainThreadScrollingReasonsUponResize third_party/WebKit/LayoutTests/compositing/layer-creation/iframe-background-attachment-fixed.html CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 Review-Url: https://codereview.chromium.org/2531603003 ========== to ========== Only scroll on main if the targeted frames need to scroll on main BUG=568901 TEST=ScrollingCoordinatorTest.BackgroundAttachmentFixedShouldTriggerMainThreadScroll; FrameThrottlingTest.ScrollingCoordinatorShouldSkipThrottledFrame; ScrollingCoordinatorTest.RecalculateMainThreadScrollingReasonsUponResize third_party/WebKit/LayoutTests/compositing/layer-creation/iframe-background-attachment-fixed.html CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 Committed: https://crrev.com/2e656635bd3f676e8385095b0e830af73d74d382 Cr-Commit-Position: refs/heads/master@{#439801} ==========
Message was sent while issue was closed.
Patchset 19 (id:??) landed as https://crrev.com/2e656635bd3f676e8385095b0e830af73d74d382 Cr-Commit-Position: refs/heads/master@{#439801} |
