Closed Bug 1367996 Opened 7 years ago Closed 7 years ago

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

Categories

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

Product:
Core
Component:
CSS Parsing and Computation
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla56
Tracking Flags:
Tracking Status
firefox56 --- fixed

People

(Reporter: heycam, Assigned: bradwerth)

References

Details

Attachments

(2 files)

Bug 1367996 Part 1: Make ServoStyleSheet::ReparseSheet call nsDocument::StyleRuleAdded and StyleRuleRemoved methods.
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1367996 Part 2: Add a test of the StyleRuleAdded and StyleRuleRemoved events.
59 bytes, text/x-review-board-request
heycam
: review+
Details 
We don't call any of the nsIDocument::StyleRule{Changed,Added,Removed} notifications methods for Servo style sheets.
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
Attachment #8879351 - Flags: review?(cam)
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.
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+
Writing a test using inDOMUtils.parseSheet or whatever the API is should be fine.
Attachment #8880503 - Flags: review?(cam)
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.
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+
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.
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
Blocks: 1371453
https://hg.mozilla.org/mozilla-central/rev/6fe1a6cf55a6
https://hg.mozilla.org/mozilla-central/rev/03bcfbadd559
Status: NEW → RESOLVED
Closed: 7 years 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.

Attachment

General

Creator:
Created:
Updated:
Size: