Closed
Bug 1303605
Opened 8 years ago
Closed 7 years ago
Get rid of the undisplayed content and display: contents maps.
Categories
(Core :: CSS Parsing and Computation, defect, P3)
Core
CSS Parsing and Computation
Tracking
()
RESOLVED
FIXED
mozilla61
| Tracking | Status | |
|---|---|---|
| firefox61 | --- | fixed |
People
(Reporter: emilio, Assigned: emilio)
References
(Depends on 1 open bug, Blocks 1 open bug)
Details
Attachments
(2 files, 1 obsolete file)
This is pretty much a rough WIP, but I wanted to get some feedback on it.
I thought that it would be easy to get rid of the frame constructor |UndisplayedMap|s if we reuse the primary frame pointer to store a style context when there's no frame. It turns out is not as easy as I initially thought (surprise!), but to be a quick-ish experiment it seems to be working out decently.
There are still a few problems with the current patch (I spent quite a bit of effort to make the PresShell tear-down correct, but I still need to handle other stuff, specially rule tree reconstruction). Also, still unsure if I got all the restyle bits correctly (from browsing the web a bit with it applied it seems fine, but I'd have to reduce the orange in try to be sure).
In general, I think it's a nice simplification not having to reach the frame constructor every time.
I wanted to experiment with this model because I wondered how does it interact with the rule tree, and how easy are to integrate some of the Stylo restyle proposals that basically consist on hanging arena-allocated pointers off the nodes.
This, on one hand, saves the memory and runtime overhead of looking and adding the style of elements in the undisplayed content map, avoiding problems as bug 560616.
On the other hand, my main concern is that this may make the RestyleUndisplayedNodes calls a bit more costly (we don't get the list of undisplayed children in constant time, we have to iterate over them), though I don't have a clear intuition on how hot are these functions. Also, if they're really hot, this may be leveraged flagging the parent from the frame constructor when we mark a node as undisplayed, and only iterating over children of content with that flag.
I can try to finish this, but I'd sincerely want some opinions about whether it seems like a good idea before going all the way down, given the trade-offs I've exposed above.
There is, of course, a middle-ground approach which will make teardown and reconstruction cheaper, which is keeping a set of content node pointers we've marked as undisplayed, that the shutdown/reconstruction code would look at. I can also try to go that path.
In general, does it seem sensible to try do this? Are the trade-offs reasonable?
Will post the WIP patch shortly.
|
Assignee | |
Comment 1•8 years ago
|
||
Maybe (probably?) you are busy at TPAC, there's no rush on this since I'm probably going to be catching up with school during next week.
I was going to flag :dbaron too, but he's not accepting feedback requests. Anyone that is also familiar with this code or has any opinion please feel free to comment on it.
Thanks in advance! :)
Attachment #8792302 -
Flags: feedback?(cam)
|
||
Comment 2•8 years ago
|
||
Comment on attachment 8792302 [details] [diff] [review]
0001-Get-rid-of-the-undisplayed-content-map.patch
Not at TPAC this week (though others are). In principle I like the idea of getting rid of the undisplayed content map and instead hanging style contexts off the nodes themselves.
But it's not great that we would have to iterate over the children of a node to find the display:none and display:contents nodes during restyle. Even with a bit that records whether we have any undisplayed children, with a large, flat document like the HTML single-page spec we could end up iterating over many nodes just to find a single display:none element. (That document has >9000 child elements of the <body>.) I'm not sure it's worth having the half-way approach of storing a set of content node pointers to quickly find them.
With stylo, as you say, we may end up storing style contexts for display:none and display:contents nodes off the nodes anyway (although possibly in a different form than just an nsStyleContext pointer). But since stylo traverses using the DOM tree rather than the frame tree we don't have that same problem of needing to find the undisplayed children after having restyled the displayed children.
So my feeling is that it is not worth eliminating the undisplayed contents map right now. We will be able to remove it once we drop the current style system and move to stylo permanently. Until then we probably don't want to make searching for undisplayed children slower.
(If we do want to make changes to how we want the undisplayed contents map to work, I'd want bz and/or Mats to take a look.)
Attachment #8792302 -
Flags: feedback?(cam) → feedback-
|
||
Updated•7 years ago
|
Priority: -- → P3
|
|
Assignee | |
Updated•7 years ago
|
Depends on: stylo-everywhere
Summary: [WIP] Try to get rid of the undisplayed content maps. → Get rid of the undisplayed content and display: contents maps.
|
|
Assignee | |
Updated•7 years ago
|
Flags: needinfo?(emilio)
|
|
Assignee | |
Comment 3•7 years ago
|
||
|
|
Assignee | |
Comment 4•7 years ago
|
||
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
|
|
Assignee | |
Comment 7•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=49224491ace84aa0d54fbf96923f990b3f521f55 is a patch with effectively the first commit. I guess I can move the second one to another bug if needed. But I wanted to make those assertions hold :)
Flags: needinfo?(emilio)
|
|
Assignee | |
Updated•7 years ago
|
Assignee: nobody → emilio
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
|
|
Assignee | |
Updated•7 years ago
|
Attachment #8792302 -
Attachment is obsolete: true
| Comment hidden (mozreview-request) |
|
|
Assignee | |
Comment 11•7 years ago
|
||
Here's a try run, minus the test and expectation update added in the last request:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2cb1a6227139b8bf269b170ebbc0d696dc345431
The devtools property database stuff should be ignored, comes from a patch under those unprefixing ::-moz-selection.
|
|
Assignee | |
Comment 12•7 years ago
|
||
Try run with the updated patches: https://treeherder.mozilla.org/#/jobs?repo=try&revision=698ce5e614ba3781379ea2c9791e2638cbb37021
|
||
Comment 13•7 years ago
|
||
| mozreview-review | ||
Comment on attachment 8963547 [details]
Bug 1303605: Remove the undisplayed maps.
https://reviewboard.mozilla.org/r/232472/#review238052
The code changes looks fine to me (excluding glue.rs which I didn't look at).
I'm a bit concerned about the performance impact of the ContentRemoved
change though, so r- for now until that's clarified.
::: layout/base/nsCSSFrameConstructor.cpp:8046
(Diff revision 2)
> // If we're removing the root, then make sure to remove things starting at
> // the viewport's child instead of the primary frame (which might even be
> // null if the root had an XBL binding or display:none, even though the
> // frames above it got created). We do the adjustment after the childFrame
> - // check above, because we do want to clear any undisplayed content we might
> - // have for the root. Detecting removal of a root is a little exciting; in
> - // particular, having a null aContainer is necessary but NOT sufficient. Due
> - // to how we process reframes, the content node might not even be in our
> + // check above, because we do want.
> + //
> + // Detecting removal of a root is a little exciting; in particular, having a
> + // null aContainer is necessary but NOT sufficient.
> + //
> + // Due to how we process reframes, the content node might not even be in our
> // document by now. So explicitly check whether the viewport's first kid's
> // content node is aChild.
The "We do the adjustment after the childFrame check above, because we do want." doesn't make much sense. I think you can probably remove this sentence.
I don't think we should split this comment into three separate paragraphs, because all the text is about the "If we're removing the root" case (IIUC), so that context gets a bit lost for the 2nd/3rd paragraph now.
::: layout/base/nsCSSFrameConstructor.cpp:8084
(Diff revision 2)
> + auto CouldHaveBeenDisplayContext = [&](nsIContent* aContent) -> bool {
> + return aFlags == REMOVE_FOR_RECONSTRUCTION || IsDisplayContents(aContent);
> + };
Making this true for "aFlags == REMOVE_FOR_RECONSTRUCTION" seems like it might be bad for performance (iterating all children and calling ContentRemoved with REMOVE_FOR_RECONSTRUCTION).
REMOVE_FOR_RECONSTRUCTION is set when toggling display:none/block for example, right?
::: layout/generic/nsFrameSelection.cpp:1591
(Diff revision 2)
> +static bool
> +HasDisplayContents(const nsIContent* aContent)
nit: I'd prefer IsDisplayContents (not Has) to follow the naming used elsewhere.
::: layout/generic/nsPlaceholderFrame.cpp:219
(Diff revision 2)
> - nsIContent* parentContent = mContent ? mContent->GetFlattenedTreeParent() : nullptr;
> - if (parentContent) {
> - ComputedStyle* sc =
> + Element* parentElement = mContent
> + ? mContent->GetFlattenedTreeParentElement()
> + : nullptr;
nit: I think this code style is more readable when it fits (as it does here):
nsIContent* parentContent =
mContent ? mContent->GetFlattenedTreeParent() : nullptr;
or, in cases where highlighting the difference between the first/second
expression helps readability, this style:
auto x = SomeLongConditionalExpression() ? eMinContentMinSizing
: eMaxContentMinSizing;
Attachment #8963547 -
Flags: review?(mats) → review-
|
|
Assignee | |
Comment 14•7 years ago
|
||
| mozreview-review-reply | ||
Comment on attachment 8963547 [details]
Bug 1303605: Remove the undisplayed maps.
https://reviewboard.mozilla.org/r/232472/#review238052
> The "We do the adjustment after the childFrame check above, because we do want." doesn't make much sense. I think you can probably remove this sentence.
>
> I don't think we should split this comment into three separate paragraphs, because all the text is about the "If we're removing the root" case (IIUC), so that context gets a bit lost for the 2nd/3rd paragraph now.
Fixed, thanks.
> nit: I think this code style is more readable when it fits (as it does here):
> nsIContent* parentContent =
> mContent ? mContent->GetFlattenedTreeParent() : nullptr;
>
> or, in cases where highlighting the difference between the first/second
> expression helps readability, this style:
> auto x = SomeLongConditionalExpression() ? eMinContentMinSizing
> : eMaxContentMinSizing;
Agreed.
|
|
Assignee | |
Comment 15•7 years ago
|
||
| mozreview-review | ||
Comment on attachment 8963547 [details]
Bug 1303605: Remove the undisplayed maps.
https://reviewboard.mozilla.org/r/232472/#review238120
::: layout/base/nsCSSFrameConstructor.cpp:8084
(Diff revision 2)
> + auto CouldHaveBeenDisplayContext = [&](nsIContent* aContent) -> bool {
> + return aFlags == REMOVE_FOR_RECONSTRUCTION || IsDisplayContents(aContent);
> + };
Yes, it is, unfortunately.
I don't see a great way around it though, other than keeping a node bit around, but that seems fragile as long as we tear down the frame tree from the frame tree itself instead of the content tree, since the frame tree doesn't know about display: contents nodes at all and cannot clear the bit.
We don't have any other state that would allow us to track down whether this truly was a display: contents element.
In practice, I think this is not really that bad (traversing the flat tree is not really slow, and it's definitely faster when there's no primary frame). And in the case it actually wasn't `display: contents`, just the tree walk is not much work (full page html spec took less than 10ms on my machine last time I measured).
The cleanest thing here would be to tear down the frame tree from the style system, instead of here... But no way I try to call that code OMT :).
I can think a bit more about it, but I think this is an improvement overall...
I'll think a bit more about it and push a few talos runs. If it shows up somewhere I can add a "was display contents" bit to the node or something, but that sounds really fragile, but I'd rather keep the code simpler. I'll also profile a bit with something like:
```
let start = Date.now();
for (let i = 0; i < 1000; ++i) {
document.documentElement.style.display = "none";
document.body.offsetTop;
document.documentElement.style.display = "";
document.body.offsetTop;
}
console.log(Date.now() - start);
```
on the HTML spec or what not.
|
|
Assignee | |
Comment 16•7 years ago
|
||
Comment on attachment 8963547 [details]
Bug 1303605: Remove the undisplayed maps.
So with a Linux64 PGO build from:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=041044a7d96b79a8a5b9f46814d33c91b598e728
I did the following:
* Open http://html.spec.whatwg.org/
* Open devtools.
* In devtools, paste:
var start = Date.now();
for (var i = 0; i < 10; ++i) {
document.documentElement.style.display = "none";
getComputedStyle(document.body).display;
document.documentElement.style.display = "";
getComputedStyle(document.body).display;
}
console.log(Date.now() - start);
(Note how I used getComputedStyle instead of offsetTop because otherwise reflow takes all the time).
I couldn't differentiate noise from differences between that and current nightly on a clean profile. There was some variance in both directions (sometimes patched build was faster, sometimes slower, but roughly the same time).
I did the same with display = "flex" instead of display = "none" just to sanity check and ensure that wins from making frame construction faster weren't taken away from this, and same result.
Not sure if that's enough to change your mind about the patch, I'd rather err in the side of simplicity for now at least, unless profiling / talos / other performance analysis tells the opposite.
It's worth also considering that the undisplayed maps are a considerable source of useless memory usage right now.
Attachment #8963547 -
Flags: review- → review?(mats)
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
|
|
||
Comment 19•7 years ago
|
||
| mozreview-review | ||
Comment on attachment 8963547 [details]
Bug 1303605: Remove the undisplayed maps.
https://reviewboard.mozilla.org/r/232472/#review238194
> just the tree walk is not much work (full page html spec took
> less than 10ms on my machine last time I measured).
My worry is mostly about the performance impact on low-end CPUs with
small caches. And also the fact that toggling the 'display' value is
a fairly common use case on the web.
Anyway, I guess we can evaluate any regressions when/if they come up.
> It's worth also considering that the undisplayed maps are
> a considerable source of useless memory usage right now.
Meh, I don't think that's an issue. Getting rid of slow hashtable
operations is very nice though.
Your work on simplifying this rather messy code is great though
since it paves the way to future optimizations.
Attachment #8963547 -
Flags: review?(mats) → review+
|
|
Assignee | |
Comment 20•7 years ago
|
||
(In reply to Mats Palmgren (:mats) from comment #19)
> Comment on attachment 8963547 [details]
> Bug 1303605: Remove the undisplayed maps.
>
> https://reviewboard.mozilla.org/r/232472/#review238194
>
> > just the tree walk is not much work (full page html spec took
> > less than 10ms on my machine last time I measured).
>
> My worry is mostly about the performance impact on low-end CPUs with
> small caches. And also the fact that toggling the 'display' value is
> a fairly common use case on the web.
> Anyway, I guess we can evaluate any regressions when/if they come up.
Yeah, as promised I thought a bit more about it and I think I can make it work without much trouble. But requires a bit more refactoring that I'd rather do in a followup. I filed bug 1450366 with the details.
> > It's worth also considering that the undisplayed maps are
> > a considerable source of useless memory usage right now.
>
> Meh, I don't think that's an issue. Getting rid of slow hashtable
> operations is very nice though.
Well, note that they're holding the style of display: none elements, and in order to avoid the hashtable lookup in Servo we leave them there unchanged, which means that it keeps alive a large amount of style structs and such. See bug 1440221 and co.
> Your work on simplifying this rather messy code is great though
> since it paves the way to future optimizations.
Thanks!
|
||
Comment 21•7 years ago
|
||
| mozreview-review | ||
Comment on attachment 8963547 [details]
Bug 1303605: Remove the undisplayed maps.
https://reviewboard.mozilla.org/r/232472/#review238260
I'm sorry this took so long. I spent a while digging through possible problems, but failed to convince myself that they are not in fact problems.
I really like all the core removal, but I have some concerns; see below.
::: layout/base/PresShell.cpp:3005
(Diff revision 3)
> + AssertNoFramesInSubtree(shadowRoot);
> + }
> + if (auto* binding = c->GetXBLBinding()) {
> + if (auto* bindingWithContent = binding->GetBindingWithContent()) {
> + nsIContent* anonContent = bindingWithContent->GetAnonymousContent();
> + for (nsIContent* child = anonContent->GetFirstChild();
Why can't you just AssertNoFramesInSubtree(anonContent)?
::: layout/base/ServoRestyleManager.cpp:854
(Diff revision 3)
> // during the replacement. In practice it's not a huge deal, but better not
> // playing with dangling pointers if not needed.
> RefPtr<ComputedStyle> oldComputedStyle =
> styleFrame ? styleFrame->Style() : nullptr;
>
> - ComputedStyle* displayContentsStyle = nullptr;
> + const bool isDisplayContents = Servo_Element_IsDisplayContents(aElement);
Can we skip doing this when oldComputedStyle is already non-null?
::: layout/base/ServoRestyleManager.cpp:858
(Diff revision 3)
>
> - ComputedStyle* displayContentsStyle = nullptr;
> - // FIXME(emilio, bug 1303605): This can be simpler for Servo.
> - // Note that we intentionally don't check for display: none content.
> - if (!oldComputedStyle) {
> - displayContentsStyle =
> + const bool isDisplayContents = Servo_Element_IsDisplayContents(aElement);
> + if (isDisplayContents) {
> + // NOTE(emilio): This is kind of a lie, because the old style is lost
> + // already during restyling. But if we're here we know that `display` didn't
> + // change, which is the only thing we really care about.
Here's what we use oldComputedStyle for:
1. The ResolveSameStructsAs call in the wasRestyled case. This really does need the old computed style as far as I can tell. How is this supposed to work in the new world? That needs to be explained in these comments.
2. The upToDateContext in the !wasRestyled case. Probably ok here.
3. The ComputedData() not being same asserts in the wasRestyled case (which I see you removed, because they would now fail).
4. The SendA11yNotifications call. Why is passing the wrong thing for oldComputedStyle ok here? Again, needs to be clearly explained.
In general, naming something "oldComputedStyle" when it's not just seems like a recipe for confusion. It would be better to leave oldComputedStyle null and have a separate variable for the "old or new-if-display-contents" style. Or something. And then using the right things in the right places. For example, we could sanely set oldComputedStyle to the current style in the !wasRestyled case, right? That would address item 2 in the list above...
::: layout/base/nsCSSFrameConstructor.h
(Diff revision 3)
> }
> -
> - // Create the undisplayed entries for our mUndisplayedItems, if any, but
> - // only if we have tried constructing frames for this item list. If we
> - // haven't, then we're just throwing it away and will probably try again.
> - if (!mUndisplayedItems.IsEmpty() && mTriedConstructingFrames) {
This was the only consumer of mTriedConstructingFrames. Want to remove that too?
::: layout/base/nsCSSFrameConstructor.cpp:427
(Diff revision 3)
> + * True for display: contents elements.
> + */
> +static inline bool
> +IsDisplayContents(const Element* aElement)
> +{
> + return aElement->HasServoData() && Servo_Element_IsDisplayContents(aElement);
So this will return whether the style is display:contents right now.
Is that what GetDisplayContentsStyleFor was doing? Or could it return "stale" style for elements which had a pending reframe that hadn't been processed yet?
A concrete scenario I'm worried about is this. Say I have two siblings, both display:inline. Display on both is toggled to "contents". We queue up reframes for both siblings. We process the reframe on the first sibling, remove its frame, go to do ContentInserted on it. We end up digging around for siblings, find the one that _hasn't_ been reframed yet, and the IsDisplayContents will reflect the new not-updated-to state, right? Then we will end up using its first child as our sibling, then it will get reframed and our frame will get killed.
Or something along those lines, at least. Possibly similar for going from display:contents to not....
Or am I just totally misremembering how this stuff works?
::: layout/base/nsCSSFrameConstructor.cpp
(Diff revision 3)
> noPrimaryFrame = needsFrameBitSet = false;
> }
> if (!noPrimaryFrame && !content->GetPrimaryFrame()) {
> - ComputedStyle* sc = GetDisplayNoneStyleFor(content);
> + noPrimaryFrame = !IsDisplayContents(content);
> - noPrimaryFrame = !GetDisplayContentsStyleFor(content) &&
> - (sc && !sc->IsInDisplayNoneSubtree());
Why is dropping this !IsInDisplayNoneSubtree() check ok? What was it actually checking for? The commit message should mention this bit, ideally.
::: layout/base/nsCSSFrameConstructor.cpp:6911
(Diff revision 3)
> if (!parent) {
> // Not part of the flat tree, nothing to do.
> return true;
> }
>
> + if (Servo_Element_IsDisplayNone(parent)) {
Why this change? Is this the thing that handles the now-gone IsInDisplayNoneSubtree() check?
What will Servo_Element_IsDisplayNone() return here if it's the _parent_ of `parent` that is display:none?
::: layout/base/nsCSSFrameConstructor.cpp:7211
(Diff revision 3)
> "Parent frame should not be fieldset or details!");
>
> // Deal with possible :after generated content on the parent, or display:
> // contents.
> nsIFrame* nextSibling = nullptr;
> - if (GetDisplayContentsStyleFor(insertion.mContainer) ||
> + if (IsDisplayContents(insertion.mContainer) ||
Again, depending on the order our reframes are processed in, does it matter that we're now returning "updated" style which may not match the existing frame tree?
::: layout/base/nsCSSFrameConstructor.cpp:7616
(Diff revision 3)
>
> MOZ_ASSERT_IF(aContainer->IsShadowRoot(), !parentFrame);
>
> // Otherwise, we've got parent content. Find its frame.
> NS_ASSERTION(!parentFrame || parentFrame->GetContent() == aContainer ||
> - GetDisplayContentsStyleFor(aContainer), "New XBL code is possibly wrong!");
> + IsDisplayContents(aContainer),
I don't see why is right, if IsDisplayContents() gets current style that may not be reflected in the frame tree yet.
::: layout/base/nsCSSFrameConstructor.cpp:8081
(Diff revision 3)
> + //
> + // If we reconstruct A because its display changed to "none", we still need to
> + // cleanup the frame on B, but A's display is now "none", so we can't poke at
> + // the style of it.
> + auto CouldHaveBeenDisplayContents = [&](nsIContent* aContent) -> bool {
> + return aFlags == REMOVE_FOR_RECONSTRUCTION || IsDisplayContents(aContent);
So this condition is likely to be true a _lot_. People toggle display all the time. In many cases it's from none to non-none or back, but block-to-inline-and-back is not that rare either.
In any case, the "not-none-to-none" case is in fact the one that this is really trying to detect, right?
::: layout/base/nsCSSFrameConstructor.cpp:8093
(Diff revision 3)
> + StyleChildrenIterator iter(aChild);
> + for (nsIContent* c = iter.GetNextChild(); c; c = iter.GetNextChild()) {
> + if (c->GetPrimaryFrame() || CouldHaveBeenDisplayContents(aChild)) {
> + LAYOUT_PHASE_TEMP_EXIT();
> + bool didReconstruct =
> + ContentRemoved(aChild, c, nullptr, REMOVE_FOR_RECONSTRUCTION);
So if REMOVE_FOR_RECONSTRUCTION then we'll tear this frame tree down one frame at a time, effectively, in a depth-first traversal? That seems relly horrible performance-wise, but maybe we have data otherwise? Have you done any measurement of what happens when you toggle a largish subtree from block to none? Esp. when it has large subtrees of itself that were display:none to start with...
::: layout/base/nsStyleChangeList.cpp:35
(Diff revision 3)
> "must have content");
> // XXXbz we should make this take Element instead of nsIContent
> MOZ_ASSERT(!aContent || aContent->IsElement() ||
> // display:contents elements posts the changes for their children:
> - (aFrame && aContent->GetParent() &&
> - aFrame->PresContext()->FrameConstructor()->
> + (aFrame && aContent->GetFlattenedTreeParentElementForStyle() &&
> + Servo_Element_IsDisplayContents(
Again, this will check the "updated" style, which may not match the actual frametree right now, right?
::: layout/generic/nsFrame.cpp:9942
(Diff revision 3)
> - nsCSSFrameConstructor* fm = PresContext()->FrameConstructor();
> - ComputedStyle* sc = fm->GetDisplayContentsStyleFor(parentContent);
> - if (MOZ_UNLIKELY(sc)) {
> - return sc;
> + if (Servo_Element_IsDisplayContents(parentElement)) {
> + RefPtr<ComputedStyle> style =
> + PresShell()->StyleSet()->AsServo()->ResolveServoStyle(parentElement);
> + // NOTE(emilio): we return a weak reference because the element also
> + // holds the style context alive. This is a bit silly (we could've
> + // returned a weak ref directly), but it's probably not worth
I'm not sure what "could've returned a weak ref directly" means. We _are_ returning a weak ref. Is is that we're not doing it "directly"?
::: layout/generic/nsFrameSelection.cpp:1594
(Diff revision 3)
> }
>
> +static bool
> +IsDisplayContents(const nsIContent* aContent)
> +{
> + return aContent->IsElement() && aContent->AsElement()->HasServoData() &&
We have this in the frame constructor too. Can we just put it on nsIContent/Element?
I guess we need to somehow avoid it being called on unstyled elements if we do that....
::: layout/generic/nsFrameSelection.cpp:1610
(Diff revision 3)
> return nullptr;
>
> if (aOffset < 0)
> return nullptr;
>
> - if (!aNode->GetPrimaryFrame() &&
> + if (!aNode->GetPrimaryFrame() && !IsDisplayContents(aNode)) {
Again, this seems like it could end up with an IsDisplayContents() value that the frame tree has not updated to yet. Is that a problem?
::: layout/generic/nsPlaceholderFrame.cpp:221
(Diff revision 3)
> {
> NS_PRECONDITION(GetParent(), "How can we not have a parent here?");
>
> - nsIContent* parentContent = mContent ? mContent->GetFlattenedTreeParent() : nullptr;
> - if (parentContent) {
> - ComputedStyle* sc =
> + Element* parentElement =
> + mContent ? mContent->GetFlattenedTreeParentElement() : nullptr;
> + if (parentElement && Servo_Element_IsDisplayContents(parentElement)) {
And the usual worry here.... We're mixing the semantics, where non-display:contents returns last-time-we-processed-a-restyle style while display:contents returns current style.
Attachment #8963547 -
Flags: review?(bzbarsky) → review-
|
|
||
Comment 22•7 years ago
|
||
| mozreview-review | ||
Comment on attachment 8963548 [details]
Bug 1303605: Make LazyFC assertions actually hold.
https://reviewboard.mozilla.org/r/232488/#review240320
r=me
::: commit-message-3cbda:3
(Diff revision 4)
> +Bug 1303605: Make LazyFC assertions actually hold. r?bz
> +
> +So while removing that wallpaper I started hitting the !noPrimaryFrame
It's not clear what wallpaper. At least point to the changeset the wallpaper is in, or if it's here to where it is?
::: commit-message-3cbda:12
(Diff revision 4)
> +in the parent chain, but we don't bail out in the
> +!GetContentInsertionFrameFor(aContainer) in the case that it's a children
> +element, because they actually have no insertion frame, though their children
> +do.
> +
> +Move the LazyFC check after the insertion point check. That makes the previous
"previous" check being the LazyFC check or the insertion point check? Unclear.
::: commit-message-3cbda:16
(Diff revision 4)
> +
> +Move the LazyFC check after the insertion point check. That makes the previous
> +check work on the insertion point of the child, which makes it sound.
> +
> +This also fixes bug 1410020, and with it a Shadow DOM test-case that was failing
> +because the content other slot wasn't getting properly flagged, and thus the
"other slot"?
::: layout/base/nsCSSFrameConstructor.cpp:7602
(Diff revision 4)
> - // a parent. While its uncommon to change the structure of the default content itself, a label,
> - // for example, can be reframed by having its value attribute set or removed.
> - if (!parentFrame &&
> - !(aContainer->IsActiveChildrenElement() || aContainer->IsShadowRoot())) {
> + // See if we have an XBL insertion point. If so, then that's our
> + // real parent frame; if not, then the frame hasn't been built yet
> + // and we just bail.
> + insertion = GetInsertionPoint(aStartChild);
> + } else {
> + // Get our insertion point. If we need to issue single ContentInserted's
s/'//
Attachment #8963548 -
Flags: review?(bzbarsky) → review+
|
|
Assignee | |
Comment 23•7 years ago
|
||
| mozreview-review-reply | ||
Comment on attachment 8963547 [details]
Bug 1303605: Remove the undisplayed maps.
https://reviewboard.mozilla.org/r/232472/#review238260
> Why can't you just AssertNoFramesInSubtree(anonContent)?
Because the anon content children don't point to the anon content, and that confuses GetNextNode a lot.
> Can we skip doing this when oldComputedStyle is already non-null?
Yes, can fix.
> Here's what we use oldComputedStyle for:
>
> 1. The ResolveSameStructsAs call in the wasRestyled case. This really does need the old computed style as far as I can tell. How is this supposed to work in the new world? That needs to be explained in these comments.
> 2. The upToDateContext in the !wasRestyled case. Probably ok here.
> 3. The ComputedData() not being same asserts in the wasRestyled case (which I see you removed, because they would now fail).
> 4. The SendA11yNotifications call. Why is passing the wrong thing for oldComputedStyle ok here? Again, needs to be clearly explained.
>
> In general, naming something "oldComputedStyle" when it's not just seems like a recipe for confusion. It would be better to leave oldComputedStyle null and have a separate variable for the "old or new-if-display-contents" style. Or something. And then using the right things in the right places. For example, we could sanely set oldComputedStyle to the current style in the !wasRestyled case, right? That would address item 2 in the list above...
That's fair. I renamed it, since introducing extra refcount churn here is somewhat tricky (atomic refcounting already shows in profiles in this code).
> This was the only consumer of mTriedConstructingFrames. Want to remove that too?
Sure
> So this will return whether the style is display:contents right now.
>
> Is that what GetDisplayContentsStyleFor was doing? Or could it return "stale" style for elements which had a pending reframe that hadn't been processed yet?
>
> A concrete scenario I'm worried about is this. Say I have two siblings, both display:inline. Display on both is toggled to "contents". We queue up reframes for both siblings. We process the reframe on the first sibling, remove its frame, go to do ContentInserted on it. We end up digging around for siblings, find the one that _hasn't_ been reframed yet, and the IsDisplayContents will reflect the new not-updated-to state, right? Then we will end up using its first child as our sibling, then it will get reframed and our frame will get killed.
>
> Or something along those lines, at least. Possibly similar for going from display:contents to not....
>
> Or am I just totally misremembering how this stuff works?
Hmm, that's a good point, but I think we're safe here due to the order we do stuff (indeed the display-contents-acid test tests this situation a lot).
In particular, the code we use to get an insertion point checks first whether we have a primary frame, _then_ whether it's display: contents. So we'd find the frame properly. Will document on the commit message.
> Why is dropping this !IsInDisplayNoneSubtree() check ok? What was it actually checking for? The commit message should mention this bit, ideally.
That's mentioned in the next commit. This was really just a wallpaper, since we were actually putting dirty bits in display: none subtrees when we're actually not.
> Why this change? Is this the thing that handles the now-gone IsInDisplayNoneSubtree() check?
>
> What will Servo_Element_IsDisplayNone() return here if it's the _parent_ of `parent` that is display:none?
Yeah, kind of, next patch should make it clearer.
> Again, depending on the order our reframes are processed in, does it matter that we're now returning "updated" style which may not match the existing frame tree?
Generally it should always match our container style. Otherwise you could make the same argument about bogus frame tree insertions. We always process parent reframes before children's, at least initially.
|
|
Assignee | |
Comment 24•7 years ago
|
||
| mozreview-review | ||
Comment on attachment 8963547 [details]
Bug 1303605: Remove the undisplayed maps.
https://reviewboard.mozilla.org/r/232472/#review240378
::: layout/base/nsCSSFrameConstructor.cpp:7211
(Diff revision 3)
> "Parent frame should not be fieldset or details!");
>
> // Deal with possible :after generated content on the parent, or display:
> // contents.
> nsIFrame* nextSibling = nullptr;
> - if (GetDisplayContentsStyleFor(insertion.mContainer) ||
> + if (IsDisplayContents(insertion.mContainer) ||
The reframes for the container are always processed before the reframes for the children, so we should be fine here. The only tricky case here are siblings, and that's fine because we look at frames before display: contents.
::: layout/base/nsCSSFrameConstructor.cpp:7616
(Diff revision 3)
>
> MOZ_ASSERT_IF(aContainer->IsShadowRoot(), !parentFrame);
>
> // Otherwise, we've got parent content. Find its frame.
> NS_ASSERTION(!parentFrame || parentFrame->GetContent() == aContainer ||
> - GetDisplayContentsStyleFor(aContainer), "New XBL code is possibly wrong!");
> + IsDisplayContents(aContainer),
It should if we're looking up in the tree.
::: layout/base/nsCSSFrameConstructor.cpp:8081
(Diff revision 3)
> + //
> + // If we reconstruct A because its display changed to "none", we still need to
> + // cleanup the frame on B, but A's display is now "none", so we can't poke at
> + // the style of it.
> + auto CouldHaveBeenDisplayContents = [&](nsIContent* aContent) -> bool {
> + return aFlags == REMOVE_FOR_RECONSTRUCTION || IsDisplayContents(aContent);
The problematic case is none-to-other, where we forcibly traverse the subtree. Bug 1450366 will take care of that.
::: layout/base/nsCSSFrameConstructor.cpp:8093
(Diff revision 3)
> + StyleChildrenIterator iter(aChild);
> + for (nsIContent* c = iter.GetNextChild(); c; c = iter.GetNextChild()) {
> + if (c->GetPrimaryFrame() || CouldHaveBeenDisplayContents(aChild)) {
> + LAYOUT_PHASE_TEMP_EXIT();
> + bool didReconstruct =
> + ContentRemoved(aChild, c, nullptr, REMOVE_FOR_RECONSTRUCTION);
Block to none doesn't even enter this block of code due to the !childFrame case. I have some numbers on https://bugzilla.mozilla.org/show_bug.cgi?id=1303605#c16, and ways to improve on that in bug 1450366.
::: layout/base/nsStyleChangeList.cpp:35
(Diff revision 3)
> "must have content");
> // XXXbz we should make this take Element instead of nsIContent
> MOZ_ASSERT(!aContent || aContent->IsElement() ||
> // display:contents elements posts the changes for their children:
> - (aFrame && aContent->GetParent() &&
> - aFrame->PresContext()->FrameConstructor()->
> + (aFrame && aContent->GetFlattenedTreeParentElementForStyle() &&
> + Servo_Element_IsDisplayContents(
It'd better be if we're posting restyles for children, given reframes on the parent subsume all child hints.
Generally, looking for IsDisplayContents on an ancestor from restyling / content-inserted / etc. should be safe. Doing siblings is tricky, but I'll mention how it works on the commit message.
::: layout/generic/nsFrame.cpp:9942
(Diff revision 3)
> - nsCSSFrameConstructor* fm = PresContext()->FrameConstructor();
> - ComputedStyle* sc = fm->GetDisplayContentsStyleFor(parentContent);
> - if (MOZ_UNLIKELY(sc)) {
> - return sc;
> + if (Servo_Element_IsDisplayContents(parentElement)) {
> + RefPtr<ComputedStyle> style =
> + PresShell()->StyleSet()->AsServo()->ResolveServoStyle(parentElement);
> + // NOTE(emilio): we return a weak reference because the element also
> + // holds the style context alive. This is a bit silly (we could've
> + // returned a weak ref directly), but it's probably not worth
I meant that `ResolveServoStyle` doesn't really need to return an already_AddRefed, since it is always kept alive by the element.
::: layout/generic/nsFrame.cpp:9942
(Diff revision 3)
> - nsCSSFrameConstructor* fm = PresContext()->FrameConstructor();
> - ComputedStyle* sc = fm->GetDisplayContentsStyleFor(parentContent);
> - if (MOZ_UNLIKELY(sc)) {
> - return sc;
> + if (Servo_Element_IsDisplayContents(parentElement)) {
> + RefPtr<ComputedStyle> style =
> + PresShell()->StyleSet()->AsServo()->ResolveServoStyle(parentElement);
> + // NOTE(emilio): we return a weak reference because the element also
> + // holds the style context alive. This is a bit silly (we could've
> + // returned a weak ref directly), but it's probably not worth
I meant that `ResolveServoStyle` doesn't really need to return an already_AddRefed, since it is always kept alive by the element. Will clarify.
::: layout/generic/nsFrameSelection.cpp:1594
(Diff revision 3)
> }
>
> +static bool
> +IsDisplayContents(const nsIContent* aContent)
> +{
> + return aContent->IsElement() && aContent->AsElement()->HasServoData() &&
Yeah, taht's not a big issue. Since there were only a couple callers I didn't think it was worth it.
::: layout/generic/nsFrameSelection.cpp:1610
(Diff revision 3)
> return nullptr;
>
> if (aOffset < 0)
> return nullptr;
>
> - if (!aNode->GetPrimaryFrame() &&
> + if (!aNode->GetPrimaryFrame() && !IsDisplayContents(aNode)) {
These semantics are the same as the GetDisplayContentsStyleFor, since we shouldn't call into this code mid-reframe.
::: layout/generic/nsPlaceholderFrame.cpp:221
(Diff revision 3)
> {
> NS_PRECONDITION(GetParent(), "How can we not have a parent here?");
>
> - nsIContent* parentContent = mContent ? mContent->GetFlattenedTreeParent() : nullptr;
> - if (parentContent) {
> - ComputedStyle* sc =
> + Element* parentElement =
> + mContent ? mContent->GetFlattenedTreeParentElement() : nullptr;
> + if (parentElement && Servo_Element_IsDisplayContents(parentElement)) {
Same as above, not as long as we don't call this mid-reframe, which we don't. As noted in a comment in this same patch, this has only one caller now for the first-line stuff, which would be nice to nix, and that doesn't need to care of ancestors switching display dynamically at that point.
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
|
|
Assignee | |
Comment 30•7 years ago
|
||
So, to be clear, re. insertion order, the case where I see ourselves doing something different than before is something like the following:
<!doctype html>
<style>
div { height: 100px; background: blue }
.contents { display: contents }
</style>
<div id="a" style="display: none; background: green;">
</div>
<div id="b" style="display: contents">
<div id="b1" style="background: red;"></div>
</div>
<div id="c"></div>
<script>
document.body.offsetTop;
a.style.display = "block";
b.style.display = "none";
</script>
Before, when reframing "a", we'd find "b1" as the next sibling for the insertion point. Now we find "c" directly. However , we then reframe "b" and tear down "b1", so I think it's sound, it's just a matter of how the intermediate frame tree in-between reframes looks like.
|
|
||
Comment 31•7 years ago
|
||
| mozreview-review | ||
Comment on attachment 8963547 [details]
Bug 1303605: Remove the undisplayed maps.
https://reviewboard.mozilla.org/r/232472/#review240404
r=me
::: layout/base/PresShell.cpp:2995
(Diff revisions 3 - 5)
> + // because the children of the <content> element's parent isn't the
> + // <content> element, but the bound element, and that confuses
Would be clearer as:
"because the parent of the children of the <content> element isn't the <content> element, but the bound element ..."
Attachment #8963547 -
Flags: review?(bzbarsky) → review+
|
||
Comment 32•7 years ago
|
||
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/bdf546b6ba28
Remove the undisplayed maps. r=bz,mats
https://hg.mozilla.org/integration/autoland/rev/12a824f8d55a
Make LazyFC assertions actually hold. r=bz
|
||
Comment 33•7 years ago
|
||
| bugherder | ||
https://hg.mozilla.org/mozilla-central/rev/bdf546b6ba28
https://hg.mozilla.org/mozilla-central/rev/12a824f8d55a
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox61:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Created web-platform-tests PR https://github.com/w3c/web-platform-tests/pull/10370 for changes under testing/web-platform/tests
You need to log in
before you can comment on or make changes to this bug.
Description
•