Closed Bug 1369903 Opened 3 years ago Closed 3 years ago

stylo: Audit cycle-collector handling

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: smaug, Assigned: bradwerth)

References

Details

Attachments

(3 files)

bradwerth was debugging a stylo leak which happened when one added an expando to a stylesheet, and a worrisome place in code is
http://searchfox.org/mozilla-central/rev/3efd1caff252061946f0773df68bdd50de5eb756/layout/style/Loader.cpp#2656-2658

This hints that CC handling in Stylo could use some auditing.
Summary: Audit Stylo's CC handling → stylo: Audit cycle-collector handling
(note that the stylo: prefix is what gets things on the radar, moreso than bug dependencies).
We should audit all the IsGecko/IsServo/GetAsGecko/GetAsServo calls, pretty much, see http://searchfox.org/mozilla-central/rev/3b4f95ac5946b639827358d93b5258d837a5ddde/dom/base/nsDocument.cpp#10113, for an example that could/should be fixed now too.

At a glance there aren't that many, which is good.
I'll go through the IsGecko/IsServo/GetAsGecko/GetAsServo calls and remove them where possible.
Assignee: nobody → bwerth
Priority: -- → P2
Attachment #8874488 - Flags: review?(emilio+bugs)
Attachment #8874489 - Flags: review?(emilio+bugs)
Attachment #8874490 - Flags: review?(emilio+bugs)
Comment on attachment 8874489 [details]
Bug 1369903 Part 2: Allow HTMLEditor to enable/disable any type of stylesheet.

https://reviewboard.mozilla.org/r/145860/#review150052

This is fine, but we should make sure that disabled sheets work properly in stylo. I think they do because how they affect the StyleSet, but we do have a boolean in servo's stylesheet that we definitely check. I think that may be false always right now, but worth documenting it, getting rid of it.
Attachment #8874489 - Flags: review?(emilio+bugs) → review+
Comment on attachment 8874490 [details]
Bug 1369903 Part 3: Ensure that Loader enables all cloned stylesheets, and checks modified state of all stylesheets, regardless of backend.

https://reviewboard.mozilla.org/r/145862/#review150054
Attachment #8874490 - Flags: review?(emilio+bugs) → review+
Comment on attachment 8874488 [details]
Bug 1369903 Part 1: Remove over-specified stylesheet calls in nsDocument.

https://reviewboard.mozilla.org/r/145858/#review150056

So, these ones are for printing, aren't there any tests that fail without this?
Attachment #8874488 - Flags: review?(emilio+bugs) → review+
Pushed by bwerth@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ea5ac39685cd
Part 1: Remove over-specified stylesheet calls in nsDocument. r=emilio
https://hg.mozilla.org/integration/autoland/rev/66a0bce9a299
Part 2: Allow HTMLEditor to enable/disable any type of stylesheet. r=emilio
https://hg.mozilla.org/integration/autoland/rev/42669f4e317b
Part 3: Ensure that Loader enables all cloned stylesheets, and checks modified state of all stylesheets, regardless of backend. r=emilio
You need to log in before you can comment on or make changes to this bug.