New issue
Advanced search Search tips

Issue 914784 link

Starred by 1 user

Issue metadata

Status: Started
Owner:
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Task

Blocked on:
issue 915669
issue 920599
issue 920600



Sign in to add a comment

Squad: unified computed style storage

Project Member Reported by futhark@chromium.org, Dec 13

Issue description

We should store ComputedStyle on Element for rendered elements. Currently we store ComputedStyle for elements only for display:contents, a couple of other elements which do not get a LayoutObject, and ComputedStyle constructed for getComputedStyle inside display:none subtrees. Use the ComputedStyle member in NodeRenderingData for all such ComputedStyles and remove the ComputedStyle member from ElementRareData.

 
Project Member

Comment 1 by bugdroid1@chromium.org, Dec 14

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

commit 7d4424206c7dbd6b1aea1da44aab92ba30e955d4
Author: Rune Lillesveen <futhark@chromium.org>
Date: Fri Dec 14 09:23:04 2018

Use custom computed style instead of LayoutObjectIsNeeded.

Using display:none in the computed style in the UA shadow for native
appearance makes layout tree changes easier to detect than having to
know the LayoutObjectIsNeeded is going to return a different result.
This caused an issue in the WIP CL for unified computed style storage.

Bug: 914784
Change-Id: Icad9726c91972e9aca7566403cbe5650ea821c93
Reviewed-on: https://chromium-review.googlesource.com/c/1375736
Reviewed-by: Kent Tamura <tkent@chromium.org>
Commit-Queue: Rune Lillesveen <futhark@chromium.org>
Cr-Commit-Position: refs/heads/master@{#616619}
[modify] https://crrev.com/7d4424206c7dbd6b1aea1da44aab92ba30e955d4/third_party/blink/renderer/core/html/shadow/progress_shadow_element.cc
[modify] https://crrev.com/7d4424206c7dbd6b1aea1da44aab92ba30e955d4/third_party/blink/renderer/core/html/shadow/progress_shadow_element.h

Blockedon: 915669
Project Member

Comment 3 by bugdroid1@chromium.org, Dec 17

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

commit 157d9c65bb9a5b9c5fb0fb37543830679e1846d4
Author: Rune Lillesveen <futhark@chromium.org>
Date: Mon Dec 17 19:50:19 2018

Do not use a local WhitespaceAttacher for <content>.

<content> elements are not in the flat tree and should be treated like
display:contents in order to attach whitespace LayoutText objects
correctly.

This bug caused regressions for the unification of ComputedStyle
storage.

Bug:  915669 , 914784
Change-Id: I8afce3bd7fd67244a8784e17acbefc6dd068a8e3
Reviewed-on: https://chromium-review.googlesource.com/c/1379767
Reviewed-by: Morten Stenshorne <mstensho@chromium.org>
Commit-Queue: Rune Lillesveen <futhark@chromium.org>
Cr-Commit-Position: refs/heads/master@{#617193}
[modify] https://crrev.com/157d9c65bb9a5b9c5fb0fb37543830679e1846d4/third_party/blink/renderer/core/dom/element.cc
[add] https://crrev.com/157d9c65bb9a5b9c5fb0fb37543830679e1846d4/third_party/blink/web_tests/fast/dom/shadow/content-whitespace-attach-expected.html
[add] https://crrev.com/157d9c65bb9a5b9c5fb0fb37543830679e1846d4/third_party/blink/web_tests/fast/dom/shadow/content-whitespace-attach-expected.txt
[add] https://crrev.com/157d9c65bb9a5b9c5fb0fb37543830679e1846d4/third_party/blink/web_tests/fast/dom/shadow/content-whitespace-attach.html

Project Member

Comment 4 by bugdroid1@chromium.org, Jan 8

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

commit bba43c0b375b05007b19acc3730616c35fa5038f
Author: Rune Lillesveen <futhark@chromium.org>
Date: Tue Jan 08 18:16:44 2019

Check LayoutView parent for top layer siblings.

The check for the need for re-attachment would no longer work with the
current patch in [1]. Instead check if the sibling LayoutObject
candidate is in the top layer (LayoutView child) already, in which case
we can use it as a sibling. If the candidate needs re-attaching, it will
be removed and re-inserted in the correct place.

[1] https://chromium-review.googlesource.com/c/chromium/src/+/1323555

TEST=html/dialog

Bug: 914784
Change-Id: I5df489382fb95ca6de8c5ea25d0270206f2fbe4a
Reviewed-on: https://chromium-review.googlesource.com/c/1400698
Reviewed-by: Morten Stenshorne <mstensho@chromium.org>
Commit-Queue: Rune Lillesveen <futhark@chromium.org>
Cr-Commit-Position: refs/heads/master@{#620795}
[modify] https://crrev.com/bba43c0b375b05007b19acc3730616c35fa5038f/third_party/blink/renderer/core/dom/layout_tree_builder_traversal.cc

Project Member

Comment 5 by bugdroid1@chromium.org, Jan 8

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

commit 66bc943f8f0557fc1d522546df16f4752db8014a
Author: Rune Lillesveen <futhark@chromium.org>
Date: Tue Jan 08 21:50:31 2019

We never ask for text style without parent style.

Just DCHECK that the layout tree parent node is non-null.

This is split out of:

https://chromium-review.googlesource.com/c/chromium/src/+/1323555.

Bug: 914784

Change-Id: Ib1e17f2cb9279d1f3c87fb0d3d42af937d3ca3ca
Reviewed-on: https://chromium-review.googlesource.com/c/1400689
Commit-Queue: Rune Lillesveen <futhark@chromium.org>
Reviewed-by: Morten Stenshorne <mstensho@chromium.org>
Cr-Commit-Position: refs/heads/master@{#620891}
[modify] https://crrev.com/66bc943f8f0557fc1d522546df16f4752db8014a/third_party/blink/renderer/core/css/resolver/style_resolver.cc

Blockedon: 920599
Blockedon: 920600
Project Member

Comment 8 by bugdroid1@chromium.org, Jan 14

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

commit 55321ed58a44b095732e4c59ad889923ab3f676e
Author: Rune Lillesveen <futhark@chromium.org>
Date: Mon Jan 14 11:01:08 2019

Avoid style recalc in display:none subtrees.

ParentComputedStyle() should be null in display:none subtrees to avoid
computing style unnecessarily when we start at a recalc root inside a
display:none subtree where the parent of the recalc root has a
ComputedStyle because it's forced by Window.getComputedStyle().

First, we introduce a flag to mark ComputedStyle as being forced inside
a display:none subtree. This is strictly not necessary for the current
code since we can check where it is stored, but for the
unify-computed-style storage issue (914784), we will start storing
ComputedStyle in the same place for both rendered and display:none
elements, so we'll need it soon.

Let ParentComputedStyle() return null when this flag is set to make sure
display:none subtrees stay free of ComputedStyles unless enforced.

Bug:  920600 , 914784
Change-Id: Iea07af009c0237d4ac6ba155af774d2e3dece354
Reviewed-on: https://chromium-review.googlesource.com/c/1404171
Reviewed-by: Anders Hartvoll Ruud <andruud@chromium.org>
Commit-Queue: Rune Lillesveen <futhark@chromium.org>
Cr-Commit-Position: refs/heads/master@{#622414}
[modify] https://crrev.com/55321ed58a44b095732e4c59ad889923ab3f676e/third_party/blink/renderer/core/css/style_engine_test.cc
[modify] https://crrev.com/55321ed58a44b095732e4c59ad889923ab3f676e/third_party/blink/renderer/core/dom/element.cc
[modify] https://crrev.com/55321ed58a44b095732e4c59ad889923ab3f676e/third_party/blink/renderer/core/dom/node_computed_style.h
[modify] https://crrev.com/55321ed58a44b095732e4c59ad889923ab3f676e/third_party/blink/renderer/core/style/computed_style_extra_fields.json5

Project Member

Comment 9 by bugdroid1@chromium.org, Jan 14

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

commit f930f101b81d07cd66b0642d10c88edd1a93d2d7
Author: Rune Lillesveen <futhark@chromium.org>
Date: Mon Jan 14 12:31:01 2019

Don't compute style in display:none iframes.

display:none iframes do not allow the layout tree to be generated, but
when Squad introduced style recalc for reattachment and
SetNonAttachedStyle(), style computation was no longer blocked by
AttachLayoutTree and the check in LayoutView::CanHaveChildren(). This
lead to style being computed as normal in display:none iframes.

Return null from ParentComputedStyle() when ChildrenCanHaveStyle()
returns false and override ChildrenCanHaveStyle() to return false for
Document when LayoutView::CanHaveChildren() returns false.

Bug:  920599 , 914784
Change-Id: Ibb8f4033cb1347c535e0b53d3e2ab9b46aacbd4e
Reviewed-on: https://chromium-review.googlesource.com/c/1405010
Commit-Queue: Rune Lillesveen <futhark@chromium.org>
Reviewed-by: Anders Hartvoll Ruud <andruud@chromium.org>
Cr-Commit-Position: refs/heads/master@{#622431}
[modify] https://crrev.com/f930f101b81d07cd66b0642d10c88edd1a93d2d7/third_party/blink/renderer/core/dom/container_node.h
[modify] https://crrev.com/f930f101b81d07cd66b0642d10c88edd1a93d2d7/third_party/blink/renderer/core/dom/document.cc
[modify] https://crrev.com/f930f101b81d07cd66b0642d10c88edd1a93d2d7/third_party/blink/renderer/core/dom/document.h
[modify] https://crrev.com/f930f101b81d07cd66b0642d10c88edd1a93d2d7/third_party/blink/renderer/core/dom/element.h
[modify] https://crrev.com/f930f101b81d07cd66b0642d10c88edd1a93d2d7/third_party/blink/renderer/core/dom/node_computed_style.h
[modify] https://crrev.com/f930f101b81d07cd66b0642d10c88edd1a93d2d7/third_party/blink/renderer/core/layout/layout_view_test.cc

📍 Job complete. See results below.
https://pinpoint-dot-chromeperf.appspot.com/job/1137180f940000

Sign in to add a comment