Closed Bug 1292930 Opened 5 years ago Closed 5 years ago

stylo: More proper handling of change hints, plus don't restyle display: none content.

Categories

(Core :: CSS Parsing and Computation, defect)

Product:
Core
Component:
CSS Parsing and Computation
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla51

People

(Reporter: emilio, Assigned: emilio)

References

(
URL
)

Details

Attachments

(2 files, 6 obsolete files)

Servo PR
41 bytes, text/x-github-pull-request
Details | Review
Bug 1292930: Handle the case of having unresolved URIs in nsStyleDisplay::mBinding.
58 bytes, text/x-review-board-request
heycam
: review+
Details
Attached file Servo PR — 
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.
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)
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/
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/
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/
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 on attachment 8778611 [details]
Bug 1292930: Hoist ChangeHintToString to RestyleManagerBase

https://reviewboard.mozilla.org/r/69832/#review67028
Attachment #8778611 - Flags: review?(cam) → 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 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 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 on attachment 8778615 [details]
Bug 1292930: stylo: Remove expected warning.

https://reviewboard.mozilla.org/r/69840/#review67048
Attachment #8778615 - Flags: review?(cam) → 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+
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.
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
Blocks: 1293478
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.)
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
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
Keywords: leave-open
Attachment #8778611 - Attachment is obsolete: true
Attachment #8778612 - Attachment is obsolete: true
Attachment #8778613 - Attachment is obsolete: true
Attachment #8778614 - Attachment is obsolete: true
Attachment #8778615 - Attachment is obsolete: true
Attachment #8778616 - Attachment is obsolete: true
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+
(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.)
(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?
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 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 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.
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
Status: NEW → RESOLVED
Closed: 5 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.