Closed
Bug 1292930
Opened 6 years ago
Closed 6 years ago
stylo: More proper handling of change hints, plus don't restyle display: none content.
Categories
(Core :: CSS Parsing and Computation, defect)
Core
CSS Parsing and Computation
Tracking
()
RESOLVED
FIXED
mozilla51
People
(Reporter: emilio, Assigned: emilio)
References
()
Details
Attachments
(2 files, 6 obsolete files)
Before https://github.com/servo/servo/pull/12757, we had a hack in tree that effectively made us return nsChangeHint_ReconstructFrame always. This explains why we didn't have to bother about quite a few things in the stylo side, but also means that we were always doing a vast amount of work (much more than what was needed in fact). With that hack gone, this patches rework the handling of change hints, in order to fix all the uncovered bugs.
Assignee | ||
Comment 1•6 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/69830/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/69830/
Attachment #8778610 -
Flags: review?(cam)
Attachment #8778611 -
Flags: review?(cam)
Attachment #8778612 -
Flags: review?(cam)
Attachment #8778613 -
Flags: review?(cam)
Attachment #8778614 -
Flags: review?(cam)
Attachment #8778615 -
Flags: review?(cam)
Attachment #8778616 -
Flags: review?(cam)
Assignee | ||
Comment 2•6 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/69832/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/69832/
Assignee | ||
Comment 3•6 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/69834/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/69834/
Assignee | ||
Comment 4•6 years ago
|
||
This is used by the frame construction to know if it can skip recreating a frame based on the -moz-binding value. Other approach would be doing string comparison for each of the unresolved members or similar. Review commit: https://reviewboard.mozilla.org/r/69836/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/69836/
Assignee | ||
Comment 5•6 years ago
|
||
Otherwise, the parent style context doesn't have some inherited structs in the cache, so it avoids comparing them entirely. For example, the following example: <p>Hey</p> p { color: blue } p:hover { color: red } Wouldn't work as intended, because when calculating the change hint the nsStyleColor struct in the element hasn't been accessed by layout (only the child text frame's has), so it will report a change hint of 0 when hovering over the paragraph, and we would ignore the nsChangeHint_ReconstructFrame generated by the text node. Review commit: https://reviewboard.mozilla.org/r/69838/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/69838/
Assignee | ||
Comment 6•6 years ago
|
||
This should actually have been a followup for bug 1290335. Review commit: https://reviewboard.mozilla.org/r/69840/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/69840/
Assignee | ||
Comment 7•6 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/69842/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/69842/
Comment 8•6 years ago
|
||
Comment on attachment 8778610 [details] Bug 1292930: Handle the case of having unresolved URIs in nsStyleDisplay::mBinding. https://reviewboard.mozilla.org/r/69830/#review67026
Attachment #8778610 -
Flags: review?(cam) → review+
Comment 9•6 years ago
|
||
mozreview-review |
Comment on attachment 8778611 [details] Bug 1292930: Hoist ChangeHintToString to RestyleManagerBase https://reviewboard.mozilla.org/r/69832/#review67028
Attachment #8778611 -
Flags: review?(cam) → review+
Comment 10•6 years ago
|
||
mozreview-review |
Comment on attachment 8778612 [details] Bug 1292930: stylo: Fix handling of eRestyle_LaterSiblings when not combined with other change hints. https://reviewboard.mozilla.org/r/69834/#review67030
Attachment #8778612 -
Flags: review?(cam) → review+
Comment 11•6 years ago
|
||
mozreview-review |
Comment on attachment 8778613 [details] Bug 1292930: Allow comparing unresolved URLValueDatas. https://reviewboard.mozilla.org/r/69836/#review67032 ::: layout/style/nsCSSValue.cpp:2577 (Diff revision 1) > - MOZ_ASSERT(mURIResolved && aOther.mURIResolved, > - "How do you know the URIs aren't null?"); > + if (MOZ_UNLIKELY(!mURIResolved || !aOther.mURIResolved)) { > + NS_WARNING("Comparing unresolved URIs"); > + return false; > + } Why do we trigger this assertion now? Can we just resolve the URIs in here rather than return false? Returning false seems OK for -moz-binding (it might result in us doing more work than necessary, although it's probably rare to change -moz-binding values between two different URLs) but I think I would prefer URIEquals to return an accurate result. So how about we change this URIEquals to call GetURI() on both URLValueData objects, and do the comparison based on those, and then update the comment in the method body (it seems to be out of date already) and the comment above the declaration in nsCSSSValue.h.
Attachment #8778613 -
Flags: review?(cam) → review-
Comment 12•6 years ago
|
||
mozreview-review |
Comment on attachment 8778614 [details] Bug 1292930: stylo: Store the change hint generated by non-elements in their parent element. https://reviewboard.mozilla.org/r/69838/#review67044 ::: layout/style/ServoBindings.cpp:232 (Diff revision 1) > MOZ_ASSERT(aNode->IsContent()); > + MOZ_ASSERT(aNode->IsDirtyForServo(), > + "Change hint stored in a not-dirty node"); > > - nsIContent* aContent = aNode->AsContent(); > - nsIFrame* primaryFrame = aContent->GetPrimaryFrame(); > + // For elements, we need to store the change hint in the proper style context. > + // For nodes, we want to store the change hint in the parent element, since s/For nodes/For text nodes/ ::: layout/style/ServoBindings.cpp:237 (Diff revision 1) > + // For Gecko this is not a problem, presumably because they access the > + // inherited structs from the parent style context to inherit them? Yes, looks like that's the reason. When an inherited struct is requested from the style context for a text node, since there are never any rules that match a text node, we fall into the case in nsRuleNode::WalkRuleTree where we decide we can just inherit the struct from the parent. This is done by calling StyleData() on the parent, which causes it to cache that struct too.
Attachment #8778614 -
Flags: review?(cam) → review+
Comment 13•6 years ago
|
||
mozreview-review |
Comment on attachment 8778615 [details] Bug 1292930: stylo: Remove expected warning. https://reviewboard.mozilla.org/r/69840/#review67048
Attachment #8778615 -
Flags: review?(cam) → review+
Comment 14•6 years ago
|
||
mozreview-review |
Comment on attachment 8778616 [details] Bug 1292930: stylo: Make change hint processing more straight-forward. https://reviewboard.mozilla.org/r/69842/#review67050 ::: layout/base/RestyleManagerBase.cpp (Diff revision 1) > // option here would be a dedicated boolean to track whether we need > // to do so (set here and unset in ProcessPendingRestyles). > presShell->GetDocument()->SetNeedStyleFlush(); > } > > - Nit: If you can be bothered, avoid adding this blank line in the first place in the earlier patch. ::: layout/base/ServoRestyleManager.cpp:94 (Diff revision 1) > + // Add the stored change hint if there's a frame. If there isn't a frame, > + // generate a ReconstructFrame change hint except if the new display value > + // (that we can look at in the content node) is not none. Maybe reword that parenthetical to be a bit clearer -- does it mean "which we can get from the ComputedValues stored on the node"? Also, s/except //?
Attachment #8778616 -
Flags: review?(cam) → review+
Assignee | ||
Comment 15•6 years ago
|
||
mozreview-review |
Comment on attachment 8778613 [details] Bug 1292930: Allow comparing unresolved URLValueDatas. https://reviewboard.mozilla.org/r/69836/#review67230 ::: layout/style/nsCSSValue.cpp:2577 (Diff revision 1) > - MOZ_ASSERT(mURIResolved && aOther.mURIResolved, > - "How do you know the URIs aren't null?"); > + if (MOZ_UNLIKELY(!mURIResolved || !aOther.mURIResolved)) { > + NS_WARNING("Comparing unresolved URIs"); > + return false; > + } We don't trigger it on "normal" pages, I just noticed this when opening the build after a crash, in the about:sessionresore page. I *think* I've only ever hit this in the frame constructor, but as far as I know, this also can get called from CalcStyleDifference, so it's possible for it to be called off main thread. That's why I didn't went the "resolve it" approach.
Assignee | ||
Comment 16•6 years ago
|
||
mozreview-review-reply |
Comment on attachment 8778616 [details] Bug 1292930: stylo: Make change hint processing more straight-forward. https://reviewboard.mozilla.org/r/69842/#review67050 > Nit: If you can be bothered, avoid adding this blank line in the first place in the earlier patch. Yeah, missed in the fixup commit, will fix it. > Maybe reword that parenthetical to be a bit clearer -- does it mean "which we can get from the ComputedValues stored on the node"? > > Also, s/except //? Wow, I don't know how to English, I'll rephrase :P
Comment 17•6 years ago
|
||
mozreview-review-reply |
Comment on attachment 8778613 [details] Bug 1292930: Allow comparing unresolved URLValueDatas. https://reviewboard.mozilla.org/r/69836/#review67230 > We don't trigger it on "normal" pages, I just noticed this when opening the build after a crash, in the about:sessionresore page. > > I *think* I've only ever hit this in the frame constructor, but as far as I know, this also can get called from CalcStyleDifference, so it's possible for it to be called off main thread. That's why I didn't went the "resolve it" approach. OK. I think we still shouldn't warn, if it is legitimate that we get in here from the frame constructor. I wonder why we can get in here without having resolved the mBinding URL. Maybe it is something to do with restyling display:none elements? In any case, I think maybe your initial approach of considering unresolved URIs as unequal is OK, but let's do that check outside of URLValueData::URIEquals (and leave the assertion in there) so that we can catch legitimate problems with comparing unresolved URLs from e.g. CalcStyleDifference. Either add an accessor for mIsResolved that we can check from the EqualURIs function in nsCSSFrameConstructor.cpp, or add a new method on URLValueData that return false if either is unresolved. (And maybe rename the EqualURIs function since it will now operate a bit differently from the identically named one in nsStyleStruct.cpp.)
Comment 18•6 years ago
|
||
Pushed by ecoal95@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/2d5a7f342256 stylo: Reuse RestyleManager::PostRestyleEventInternal. r=heycam https://hg.mozilla.org/integration/mozilla-inbound/rev/294df63717f1 Hoist ChangeHintToString to RestyleManagerBase. r=heycam https://hg.mozilla.org/integration/mozilla-inbound/rev/0890cda8c432 stylo: Fix handling of eRestyle_LaterSiblings when not combined with other change hints. r=heycam https://hg.mozilla.org/integration/mozilla-inbound/rev/7be5dfc0e9c3 stylo: Store the change hint generated by non-elements in their parent element. r=heycam https://hg.mozilla.org/integration/mozilla-inbound/rev/9c18533ec3fa stylo: Remove expected warning. r=heycam https://hg.mozilla.org/integration/mozilla-inbound/rev/c45412486f68 stylo: Make change hint processing more straight-forward. r=heycam
Assignee | ||
Comment 19•6 years ago
|
||
I pushed all the reviewed changes except the URL one, since the related changes in servo had already landed and this sort of blocks bobby. Marking as leave-open
Assignee | ||
Updated•6 years ago
|
Keywords: leave-open
Comment hidden (mozreview-request) |
Assignee | ||
Updated•6 years ago
|
Attachment #8778611 -
Attachment is obsolete: true
Assignee | ||
Updated•6 years ago
|
Attachment #8778612 -
Attachment is obsolete: true
Assignee | ||
Updated•6 years ago
|
Attachment #8778613 -
Attachment is obsolete: true
Assignee | ||
Updated•6 years ago
|
Attachment #8778614 -
Attachment is obsolete: true
Assignee | ||
Updated•6 years ago
|
Attachment #8778615 -
Attachment is obsolete: true
Assignee | ||
Updated•6 years ago
|
Attachment #8778616 -
Attachment is obsolete: true
Comment 21•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/2d5a7f342256 https://hg.mozilla.org/mozilla-central/rev/294df63717f1 https://hg.mozilla.org/mozilla-central/rev/0890cda8c432 https://hg.mozilla.org/mozilla-central/rev/7be5dfc0e9c3 https://hg.mozilla.org/mozilla-central/rev/9c18533ec3fa https://hg.mozilla.org/mozilla-central/rev/c45412486f68
Comment 22•6 years ago
|
||
mozreview-review |
Comment on attachment 8778610 [details] Bug 1292930: Handle the case of having unresolved URIs in nsStyleDisplay::mBinding. https://reviewboard.mozilla.org/r/69830/#review68988 (FYI: MozReview tells me I've reviewed this patch when I haven't. Maybe you are re-using MozReview-Commit-IDs in the commit messages, so MozReview thinks this patch is logically a new version of a previous patch that I r+ed? Also, working in a leave-open bug and pushing this new patch without the ones that already landed marked those as obsolete.) ::: layout/style/nsStyleStruct.cpp:63 (Diff revision 2) > NS_SUCCEEDED(aURI1->Equals(aURI2, &eq)) && // not equal on fail > eq); > } > > static bool > EqualURIs(mozilla::css::URLValue *aURI1, mozilla::css::URLValue *aURI2) Let's rename this function since it's operating a bit differently from the other EqualURIs. Why don't we need to do this in nsCSSFrameConstructor.cpp's comaprison of mBinding too?
Attachment #8778610 -
Flags: review+
Comment 23•6 years ago
|
||
(Or it might not be re-used MozReview-Commit-IDs but just the fact we have a new patch in the same position in the patch queue, after the other ones were removed.)
Comment 24•6 years ago
|
||
(In reply to Cameron McCormack (:heycam) from comment #23) > (Or it might not be re-used MozReview-Commit-IDs but just the fact we have a > new patch in the same position in the patch queue, after the other ones were > removed.) I'm pretty sure that they use commit ids - that's the whole point of them, right?
Assignee | ||
Comment 25•6 years ago
|
||
mozreview-review-reply |
Comment on attachment 8778610 [details] Bug 1292930: Handle the case of having unresolved URIs in nsStyleDisplay::mBinding. https://reviewboard.mozilla.org/r/69830/#review68988 > Let's rename this function since it's operating a bit differently from the other EqualURIs. > > Why don't we need to do this in nsCSSFrameConstructor.cpp's comaprison of mBinding too? I was not hitting that anymore, presumably because with this patch we don't generate ReconstructFrame hints that much.
Comment 26•6 years ago
|
||
mozreview-review |
Comment on attachment 8778610 [details] Bug 1292930: Handle the case of having unresolved URIs in nsStyleDisplay::mBinding. https://reviewboard.mozilla.org/r/69830/#review69292 OK. r=me with EqualsURI renamed.
Attachment #8778610 -
Flags: review+
Comment 27•6 years ago
|
||
mozreview-review-reply |
Comment on attachment 8778610 [details] Bug 1292930: Handle the case of having unresolved URIs in nsStyleDisplay::mBinding. https://reviewboard.mozilla.org/r/69830/#review69292 EqualURIs rather.
Comment 28•6 years ago
|
||
Pushed by ecoal95@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/2d422399bc48 Handle the case of having unresolved URIs in nsStyleDisplay::mBinding. r=heycam
Comment 29•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/2d422399bc48
Updated•6 years ago
|
Status: NEW → RESOLVED
Closed: 6 years ago
Keywords: leave-open
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
You need to log in
before you can comment on or make changes to this bug.
Description
•