stylo: call document notification methods for style rule additions and removals

RESOLVED FIXED in Firefox 56

Status

()

Core
CSS Parsing and Computation
P2
normal
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: heycam, Assigned: bradwerth)

Tracking

(Blocks: 1 bug)

unspecified
mozilla56
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox56 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments)

(Reporter)

Description

a year ago
We don't call any of the nsIDocument::StyleRule{Changed,Added,Removed} notifications methods for Servo style sheets.
(Assignee)

Comment 1

a year ago
I'm willing to take this on, if I can get some help spotting the needed callsites. When looking at the layout/style callsites for StyleRuleChanged, they seem to me to already have Servo counterparts:

1) The StyleRule SetCSSDeclaration call has a counterpart in ServoStyleRule.
2) The nsCSSKeyframesRule calls seem to have counterparts in ServoKeyframeRule.
3) The nsCSSCounterStyleRule calls don't need Servo counterparts(?) because according to ServoCSSRuleList::GetRule those Servo rules are just RefPtrs to Gecko rules.

For StyleRuleAdded and StyleRuleDeleted:

1) CSSStyleSheet::InsertRuleInternal has a counterpart in ServoStyleSheet.
2) CSSStyleSheet::StyleSheetLoaded has a counterpart in ServoStyleSheet.
3) CSSStyleSheet::ReparseSheet DOES NOT not have a counterpart in ServoStyleSheet, but do we reparse ServoStyleSheets? http://searchfox.org/mozilla-central/source/layout/style/nsLayoutStylesheetCache.cpp#994 seems to indicate that we don't.

So what's missing? Is there a test that fails?
Flags: needinfo?(cam)
Hi Brad, I think Cameron means the patch I'll land in Bug 1358993. In my patch, the document notifications are missing in the ServoStyleSheet::ReparseSheet().
Flags: needinfo?(cam)
Assignee: nobody → bwerth
Priority: -- → P2
Comment hidden (mozreview-request)
(Assignee)

Updated

a year ago
Attachment #8879351 - Flags: review?(cam)
(Assignee)

Comment 4

a year ago
I'd like to add a test to ensure this patch solves the whole problem. I'm not sure how to test this with a web API.
(Reporter)

Comment 5

a year ago
mozreview-review
Comment on attachment 8879351 [details]
Bug 1367996 Part 1: Make ServoStyleSheet::ReparseSheet call nsDocument::StyleRuleAdded and StyleRuleRemoved methods.

https://reviewboard.mozilla.org/r/150640/#review155518

::: layout/style/ServoStyleSheet.cpp:240
(Diff revision 1)
> +  if (mDocument && mRuleList) {
> +    uint32_t ruleCount = mRuleList->Length();
> +    for (uint32_t i = 0; i < ruleCount; ++i) {
> +      css::Rule* rule = mRuleList->GetRule(i);
> +      MOZ_ASSERT(rule);
> +      mDocument->StyleRuleRemoved(this, rule);

I wonder if it's possible for some StyleRuleRemoved notification listener to cause the document to be destroyed?  CSSStyleSheet::ReparseSheet null checks it every time.  I'm not too sure, but let's just follow what CSSStyleSheet is doing.

::: layout/style/ServoStyleSheet.cpp:249
(Diff revision 1)
> +  // Notify mDocument that all our new rules are added.
> +  if (mDocument && mRuleList) {
> +    uint32_t ruleCount = mRuleList->Length();
> +    for (uint32_t i = 0; i < ruleCount; ++i) {
> +      css::Rule* rule = mRuleList->GetRule(i);
> +      MOZ_ASSERT(rule);
> +      mDocument->StyleRuleAdded(this, rule);
> +    }
> +  }

In CSSStyleSheet::ReparseSheet, we only call StyleRuleAdded on an @import rule if it has finished loading.  Should we do the same here too?
Attachment #8879351 - Flags: review?(cam) → review+
(Reporter)

Comment 6

a year ago
Writing a test using inDOMUtils.parseSheet or whatever the API is should be fine.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

a year ago
Attachment #8880503 - Flags: review?(cam)
(Assignee)

Comment 9

a year ago
mozreview-review-reply
Comment on attachment 8879351 [details]
Bug 1367996 Part 1: Make ServoStyleSheet::ReparseSheet call nsDocument::StyleRuleAdded and StyleRuleRemoved methods.

https://reviewboard.mozilla.org/r/150640/#review155518

> I wonder if it's possible for some StyleRuleRemoved notification listener to cause the document to be destroyed?  CSSStyleSheet::ReparseSheet null checks it every time.  I'm not too sure, but let's just follow what CSSStyleSheet is doing.

I have not figured out a way to test a JS observer that detaches the sheet from the document, since both StyleRuleAdded and StyleRuleRemoved JS events will be received after this function has completed execution. I'm not familiar with the XPCOM observers and how they might respond to the event.

> In CSSStyleSheet::ReparseSheet, we only call StyleRuleAdded on an @import rule if it has finished loading.  Should we do the same here too?

The new test confirms that style rule notifications arrive before import rule notifications due to this deferred load.
(Reporter)

Comment 10

a year ago
mozreview-review
Comment on attachment 8880503 [details]
Bug 1367996 Part 2: Add a test of the StyleRuleAdded and StyleRuleRemoved events.

https://reviewboard.mozilla.org/r/151832/#review157448

::: layout/inspector/tests/chrome/test_parseStyleSheetObservers.html:83
(Diff revision 1)
> +  info("test2Result: called");
> +  is(foundStyle, true, "Test 2: Got the style rule.");
> +
> +  document.removeEventListener("StyleRuleAdded", styleFirstAddListener, listenerOptions);

After this, if we wait until the imported sheet is loaded, we will get a StyleRuleAdded event dispatched for the @import rule, right?  Can you test for that too?

::: layout/style/ServoStyleSheet.cpp:213
(Diff revision 1)
> +  mozAutoDocUpdate updateBatch(mDocument, UPDATE_STYLE, true);
> +

This change and the comment probably belong in the previous patch.

Also, I notice that CSSStyleSheet::ReparseSheet calls WillDirty / DidDirty.  Should we be doing that here too?  (DidDirty doesn't do anything for ServoStyleSheets, but WillDirty should do an EnsureUniqueInner.)
Attachment #8880503 - Flags: review?(cam) → review+
(Assignee)

Comment 11

a year ago
mozreview-review-reply
Comment on attachment 8880503 [details]
Bug 1367996 Part 2: Add a test of the StyleRuleAdded and StyleRuleRemoved events.

https://reviewboard.mozilla.org/r/151832/#review157448

> After this, if we wait until the imported sheet is loaded, we will get a StyleRuleAdded event dispatched for the @import rule, right?  Can you test for that too?

I would like to, but I'm just not sure how to do it without adding a timeout or switching to an async function approach. I don't want to add a test that creates intermittent oranges on try. I'll try something that doesn't add an arbitrary timeout.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 14

a year ago
Pushed by bwerth@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/6fe1a6cf55a6
Part 1: Make ServoStyleSheet::ReparseSheet call nsDocument::StyleRuleAdded and StyleRuleRemoved methods. r=heycam
https://hg.mozilla.org/integration/autoland/rev/03bcfbadd559
Part 2: Add a test of the StyleRuleAdded and StyleRuleRemoved events. r=heycam
(Assignee)

Updated

a year ago
Blocks: 1371453

Comment 15

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/6fe1a6cf55a6
https://hg.mozilla.org/mozilla-central/rev/03bcfbadd559
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.