c-v: Distinguish the type of forced updates. [chromium/src : main]
vmpstr (Gerrit)
Attention is currently required from: Joey Arhar, Chris Harrelson.
1 comment:
Patchset:
Please take a loko.
To view, visit change 3168328. To unsubscribe, or for help writing mail filters, visit settings.
vmpstr (Gerrit)
Patchset:
Please take a loko.
look*
To view, visit change 3168328. To unsubscribe, or for help writing mail filters, visit settings.
Blink WPT Bot (Gerrit)
Attention is currently required from: Joey Arhar, Chris Harrelson.
Exportable changes to web-platform-tests were detected in this CL and a pull request in the upstream repo has been made: https://github.com/web-platform-tests/wpt/pull/30887.
When this CL lands, the bot will automatically merge the PR on GitHub if the required GitHub checks pass; otherwise, ecosystem-infra@ team will triage the failures and may contact you.
WPT Export docs:
https://chromium.googlesource.com/chromium/src/+/main/docs/testing/web_platform_tests.md#Automatic-export-process
Joey Arhar (Gerrit)
Attention is currently required from: vmpstr, Chris Harrelson.
6 comments:
Commit Message:
we can force an update for style without
forcing layoutso this is a performance optimization which shouldn't change any behavior?
File third_party/blink/renderer/core/display_lock/display_lock_context.h:
Patch Set #1, Line 79:
kStyleAndLayoutTree, kLayout, kPrePaintDo these correspond with UpdateStyleAndLayoutTree, UpdateStyleAndLayout, and a third thing I don't know?
Is calling UpdateStyleAndLayoutTree considered only updating style?
Patch Set #1, Line 345:
// If non-zero, then the update has been forced.should this comment still be here?
File third_party/blink/renderer/core/dom/range.cc:
Patch Set #1, Line 1595:
DisplayLockContext::ForcedPhase::kLayoutLet's say I'm debugging a content-visibility related DCHECK, and I suspect that one of these ForcedPhase flags is wrong *somewhere* in the codebase.
Is there an easy spot in DisplayLockContext for me to effectively change every usage of ForcedPhase to ForcedPhase::kPrePaint?
File third_party/blink/renderer/core/frame/local_frame_view.cc:
Patch Set #1, Line 1132:
LockedAncestorPreventingLayoutSo there are already many existing calls to LockedAncestorPreventingLayout/Style/Paint, right?
Before this patch, are all of the Prevening Layout vs Style vs Paint calls effectively the same because all DisplayLocks prevent... everything?
File third_party/blink/web_tests/external/wpt/html/semantics/embedded-content/the-embed-element/embed-document-under-content-visibility-gbcr.html:
// Ensure we process style and layout in the hidden object.
target.getBoundingClientRect();In similar tests I wrote, I checked to make sure that the resulting coordinates and sizes are greater than zero, which indicates whether we unlocked the DisplayLock: https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/web_tests/external/wpt/css/css-contain/content-visibility/content-visibility-svg.html
Do you think that would be worth doing here?
To view, visit change 3168328. To unsubscribe, or for help writing mail filters, visit settings.
vmpstr (Gerrit)
Attention is currently required from: Joey Arhar, Chris Harrelson.
7 comments:
Commit Message:
we can force an update for style without
forcing layoutso this is a performance optimization which shouldn't change any behavior?
It does have behavior implications (as the code I've changed below shows)
Patchset:
Just answers (will upload a new patch later)
File third_party/blink/renderer/core/display_lock/display_lock_context.h:
Patch Set #1, Line 79:
kStyleAndLayoutTree, kLayout, kPrePaint
Do these correspond with UpdateStyleAndLayoutTree, UpdateStyleAndLayout, and a third thing I don't k […]
Basically yeah. I don't think we have a force prepaint call, but for completeness it's here?
Patch Set #1, Line 345:
// If non-zero, then the update has been forced.should this comment still be here?
Oops.
File third_party/blink/renderer/core/dom/range.cc:
Patch Set #1, Line 1595:
DisplayLockContext::ForcedPhase::kLayout
Let's say I'm debugging a content-visibility related DCHECK, and I suspect that one of these ForcedP […]
Yeah, you can just add prepaint is forced count anytime you increase any other count. (in .h)
File third_party/blink/renderer/core/frame/local_frame_view.cc:
Patch Set #1, Line 1132:
LockedAncestorPreventingLayout
So there are already many existing calls to LockedAncestorPreventingLayout/Style/Paint, right? […]
Essentially yes. The one distinction is what happened when things were forced. Before this, *PreventingPaint is equivalent to "do we have a locked ancestor" and forcing locks didn't change anything. The rest *PreventingStyle/PrePaint/Layout would all be equivalent: true if locked, false if forced.
With this patch now, there is a distinction of what is forced. So if style is forced, then *PreventingLayout would still return a lock. This is important for this particular bug fix, since we have a style check for *PreventingLayout because it expects if that's "false" then layout will run on this object. However, because only style was forced, the layout would never run (style would run, then the forced scope would be destroyed, so nothing will run layout). The two tests confirm this.
File third_party/blink/web_tests/external/wpt/html/semantics/embedded-content/the-embed-element/embed-document-under-content-visibility-gbcr.html:
// Ensure we process style and layout in the hidden object.
target.getBoundingClientRect();
In similar tests I wrote, I checked to make sure that the resulting coordinates and sizes are greate […]
This is more about just forcing layout, so it's really any property would do. I don't think it's worth checking what the actual value is
To view, visit change 3168328. To unsubscribe, or for help writing mail filters, visit settings.
Joey Arhar (Gerrit)
Attention is currently required from: vmpstr, Chris Harrelson.
Patch set 1:Code-Review +1
vmpstr (Gerrit)
Attention is currently required from: Chris Harrelson.
6 comments:
Commit Message:
we can force an update for style without
forcing layout
It does have behavior implications (as the code I've changed below shows)
.
File third_party/blink/renderer/core/display_lock/display_lock_context.h:
Patch Set #1, Line 79:
kStyleAndLayoutTree, kLayout, kPrePaint
Basically yeah. […]
.
Patch Set #1, Line 345:
// If non-zero, then the update has been forced.
Oops.
Done
File third_party/blink/renderer/core/dom/range.cc:
Patch Set #1, Line 1595:
DisplayLockContext::ForcedPhase::kLayout
Yeah, you can just add prepaint is forced count anytime you increase any other count. (in . […]
.
File third_party/blink/renderer/core/frame/local_frame_view.cc:
Patch Set #1, Line 1132:
LockedAncestorPreventingLayout
Essentially yes. The one distinction is what happened when things were forced. […]
.
File third_party/blink/web_tests/external/wpt/html/semantics/embedded-content/the-embed-element/embed-document-under-content-visibility-gbcr.html:
// Ensure we process style and layout in the hidden object.
target.getBoundingClientRect();
This is more about just forcing layout, so it's really any property would do. […]
.
To view, visit change 3168328. To unsubscribe, or for help writing mail filters, visit settings.
vmpstr (Gerrit)
Attention is currently required from: Chris Harrelson.
Patch set 3:Commit-Queue +2
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted this change.
1 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: third_party/blink/renderer/core/display_lock/display_lock_context.cc
Insertions: 2, Deletions: 1.
@@ -647,7 +647,8 @@
STACK_ALLOCATED();
public:
- ScopedForceLayout(DisplayLockContext* context) : context_(context) {
+ explicit ScopedForceLayout(DisplayLockContext* context)
+ : context_(context) {
context_->forced_info_.start(ForcedPhase::kLayout);
}
~ScopedForceLayout() { context_->forced_info_.end(ForcedPhase::kLayout); }
```
```
The name of the file: third_party/blink/renderer/core/display_lock/display_lock_context.h
Insertions: 1, Deletions: 1.
@@ -342,7 +342,7 @@
WeakMember<Document> document_;
EContentVisibility state_ = EContentVisibility::kVisible;
- // If non-zero, then the update has been forced.
+ // A struct to keep track of forced unlocks, and reasons for it.
struct UpdateForcedInfo {
bool is_forced(ForcedPhase phase) const {
switch (phase) {
```
c-v: Distinguish the type of forced updates.
This patch ensures that we can force an update for style without
forcing layout, etc. This is useful since we need to determine whether
we have a lock that blocks, for e.g. layout, and if we force a lock
it's unclear whether layout would be updated without this patch.
With the patch, functions like UpdateStyleAndLayoutTree only force
style updates.
R=chri...@chromium.org, jar...@chromium.org
Fixed: 1250742
Change-Id: I9856acbcabedfb0b232518b438ca43a16a0cfd09
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3168328
Commit-Queue: vmpstr <vmp...@chromium.org>
Reviewed-by: Joey Arhar <jar...@chromium.org>
Cr-Commit-Position: refs/heads/main@{#923524}
---
M third_party/blink/renderer/core/display_lock/display_lock_context.cc
M third_party/blink/renderer/core/display_lock/display_lock_context.h
M third_party/blink/renderer/core/display_lock/display_lock_context_test.cc
M third_party/blink/renderer/core/display_lock/display_lock_utilities.cc
M third_party/blink/renderer/core/display_lock/display_lock_utilities.h
M third_party/blink/renderer/core/dom/document.cc
M third_party/blink/renderer/core/dom/range.cc
M third_party/blink/renderer/core/editing/frame_selection.cc
M third_party/blink/renderer/core/frame/local_frame_view.cc
M third_party/blink/renderer/core/html/html_plugin_element.cc
R third_party/blink/web_tests/external/wpt/html/semantics/embedded-content/the-embed-element/embed-document-under-content-visibility-focus.html
A third_party/blink/web_tests/external/wpt/html/semantics/embedded-content/the-embed-element/embed-document-under-content-visibility-gbcr.html
12 files changed, 169 insertions(+), 70 deletions(-)
Blink WPT Bot (Gerrit)
The WPT PR for this CL has been merged upstream! https://github.com/web-platform-tests/wpt/pull/30887