Nested OOPIFs don't always get throttled when their parents are out of view |
|||||||||
Issue descriptionboliu@ noticed in an unrelated change (https://chromium-review.googlesource.com/c/chromium/src/+/1132468/) that when there are nested OOPIFs -- e.g. A(B(C)) -- and B is scrolled out of the main viewport, C does not get an IPC message update to throttle it and/or set its own viewport intersection to empty. I think this is a regression from r576343, and an oversight that we don't already have a test for that. It seems to fix the problem if we have LocalFrame::SetViewportIntersectionFromParent() set its LocalFrameView as requiring intersection observations, as this forces the B OOPIF to generate new observations on its next lifecycle update, triggering the right updates to be sent to C. If that is reasonable then it is easy to fix, I just need to write a test.
,
Jul 28
,
Sep 5
Issue 880673 has been merged into this issue.
,
Sep 5
kenrb@ - Adding RBS label for M-69 and would it be safe enough to merge the change in C#1 to M-69 ? Thanks...!!
,
Sep 5
This should be safe to merge, it's a small change.
,
Sep 5
This bug requires manual review: Request affecting a post-stable build Please contact the milestone owner if you have questions. Owners: amineer@(Android), kariahda@(iOS), cindyb@(ChromeOS), govind@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Sep 5
Is issue 880673 is duplicate of this? Also M69 is already out, how critical and safe is this change to merge to M69 this late in release cycle?
,
Sep 5
,
Sep 5
The request is based on issue 880669 and issue 880673 , both of which are fixed by the CL in comment #1. I assume it is critical because those had been marked as M69 stable blockers. The risk is low. This is a one-line change that landed over a month ago, and I haven't seen any problems resulting from it.
,
Sep 5
Comments 7 and 9: Agreed that this looks like a safe and necessary merge for M69. The fix looks simple and low risk, and it's a pretty noticeable regression in M69 for users. Not sure if it warrants a respin of its own or not, but if there are other pending fixes as well, this should probably be included. It was introduced in r576343 (69.0.3497.0) and fixed in r578922 (70.0.3506.0), which is consistent with what I'm seeing in practice-- the repro steps for issue 880669 don't show a problem in 68.0.3440.118, do show a problem on 69.0.3497.81 after the regression, and don't show a problem on 70.0.3534.4 after the fix.
,
Sep 5
Approving merge to M69 branch 3497 based on comments #9 and #10. Pls merge. Per offline gorup chat with Charlie & Ken, we won't block M69 further roll out for this but will take the merge in for next M69 respin.
,
Sep 5
I've merged this into 3497.
,
Sep 5
Since bugdroid isn't working, manually updating flags. Merge at https://chromium-review.googlesource.com/c/chromium/src/+/1207096
,
Sep 5
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/a188cb0ab5fd9b4aefc0a5a2eabba615e58764ca commit a188cb0ab5fd9b4aefc0a5a2eabba615e58764ca Author: Ken Buchanan <kenrb@chromium.org> Date: Wed Sep 05 17:38:14 2018 Force OOPIF intersection observations after viewport update Currently when an OOPIF receives an updated viewport intersection rect from its parent, it schedules a lifecycle update. However if the OOPIF is now throttled it will not calculate new intersection observations based on the new viewport rect, and as a consequence any contained OOPIFs will not be throttled. This forces intersection observations whenever an OOPIF has its viewport intersection changed. Bug: 868558 Change-Id: I18c955b8cc40ec2ebe31d1f0fb35aa5a26f9f447 Reviewed-on: https://chromium-review.googlesource.com/1153838 Commit-Queue: Ken Buchanan <kenrb@chromium.org> Reviewed-by: Chris Harrelson <chrishtr@chromium.org> Reviewed-by: Alex Moshchuk <alexmos@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#578922}(cherry picked from commit eb3c53e64b63a74417d11b566c24081e5f386eb9) Reviewed-on: https://chromium-review.googlesource.com/1207096 Reviewed-by: Ben Mason <benmason@chromium.org> Cr-Commit-Position: refs/branch-heads/3497@{#878} Cr-Branched-From: 271eaf50594eb818c9295dc78d364aea18c82ea8-refs/heads/master@{#576753} [modify] https://crrev.com/a188cb0ab5fd9b4aefc0a5a2eabba615e58764ca/content/browser/site_per_process_browsertest.cc [add] https://crrev.com/a188cb0ab5fd9b4aefc0a5a2eabba615e58764ca/content/test/data/frame_tree/scrollable_page_with_positioned_frame.html [modify] https://crrev.com/a188cb0ab5fd9b4aefc0a5a2eabba615e58764ca/third_party/blink/renderer/core/frame/local_frame.cc
,
Sep 6
Issue 880777 has been merged into this issue.
,
Sep 6
Issue 880874 has been merged into this issue.
,
Sep 7
Tested on 69.0.4397.85 (Linux), and none of the duped bugs currently reproduce. I tried issue 880669 , issue 880673 , issue 880777 and issue 880874.
,
Sep 10
Issue 881709 has been merged into this issue.
,
Sep 24
|
|||||||||
►
Sign in to add a comment |
|||||||||
Comment 1 by bugdroid1@chromium.org
, Jul 28