1472065 - simplify management of style sheet and parent rule pointers in css::Rule

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, enhancement, P3)

Description

[Cameron McCormack (:heycam)]

The style sheet and parent rule should be known at the time the Rule object is constructed, so I think it would be better to pass those in to the constructor rather than wait for SetStyleSheet / SetParentRule calls later.

Comment 1

[Cameron McCormack (:heycam)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=016c928630ece25ed4f0fbca2fdd039ad1032769

Comment 2

[Cameron McCormack (:heycam)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=31d0e9232dbf3131a37dcbc14dae8468271c03b5

Comment 3

[Cameron McCormack (:heycam)]

Attachment: Bug 1472065 - Initialize mSheet and mParentRule in css::Rule's constructor.

Review commit: https://reviewboard.mozilla.org/r/254092/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/254092/

Comment 4

[Cameron McCormack (:heycam)]

Two things to note:

1. I moved the check we do when clearing out mSheet in group rules to avoid quadratic time behavior when unlinking to inside the function that would drop the reference (ServoCSSRuleList::DropSheetReference, renamed from SetStyleSheet).

2. I removed the mention of the direct style sheet owner of ServoCSSRuleList, since previously we would end up calling SetStyleSheet on it for non-top-level group rules anyway.

Comment 5

[Cameron McCormack (:heycam)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=913f595b99d7e890169e5624fbb1657007c16986

Comment 6

[Xidorn Quan [:xidorn] UTC+11]

Comment on attachment 8988973 [details]
Bug 1472065 - Initialize mSheet and mParentRule in css::Rule's constructor.

https://reviewboard.mozilla.org/r/254092/#review260884

::: layout/style/Rule.h:132
(Diff revision 1)
>    bool IsKnownLive() const;
>  
> -  // This is sometimes null (e.g., for style attributes).
> -  StyleSheet* mSheet;
> -  // When the parent GroupRule is destroyed, it will call SetParentRule(nullptr)
> -  // on this object. (Through SetParentRuleReference);
> +  // mSheet should only ever be null when we create a synthetic CSSFontFaceRule
> +  // for an InspectorFontFace.
> +  //
> +  // mSheet and mParentRule will be cleared when the parent object is destroyed.

Probably better "when they are detached from the parent object, either because the rule is removed or the parent is destroyed."

Comment 7

[Cameron McCormack (:heycam)]

Comment on attachment 8988973 [details]
Bug 1472065 - Initialize mSheet and mParentRule in css::Rule's constructor.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/254092/diff/1-2/

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8fb94a730b6c
Initialize mSheet and mParentRule in css::Rule's constructor. r=xidorn

Comment 9

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/8fb94a730b6c