stylo: Convert :-moz-list-bullet and :-moz-list-number to (non-web-exposed) inheriting anonymous boxes and eliminate the lazy pseudo cache

RESOLVED WONTFIX

Status

()

enhancement
P4
normal
RESOLVED WONTFIX
2 years ago
2 years ago

People

(Reporter: bholley, Assigned: bholley)

Tracking

unspecified
Points:
---

Firefox Tracking Flags

(firefox57 wontfix, firefox58 fix-optional)

Details

Attachments

(2 attachments, 1 obsolete attachment)

See bug 1401317 comment 11. There is some webcompat risk here, but it would simplify various things and save memory.
Priority: -- → P4
Assignee: nobody → bobbyholley
Doing this on top of bug 1429126.
MozReview-Commit-ID: 8rR8tz6Rq7w
Attachment #8941274 - Flags: review?(emilio)
MozReview-Commit-ID: 8rR8tz6Rq7w
Attachment #8941284 - Flags: review?(emilio)
Attachment #8941274 - Attachment is obsolete: true
Attachment #8941274 - Flags: review?(emilio)
Comment on attachment 8941273 [details] [diff] [review]
Part 1 - Convert -moz-list-bullet and -moz-list-number to (non-web-exposed) inheriting anonymous boxes. v1

Review of attachment 8941273 [details] [diff] [review]:
-----------------------------------------------------------------

Code-wise looks ok, though I'm surprised to not see any test-changes. Also, could you send an intent to unship to dev-platform, and wait a bit for replies or concerns to come in in case there are any?
Comment on attachment 8941273 [details] [diff] [review]
Part 1 - Convert -moz-list-bullet and -moz-list-number to (non-web-exposed) inheriting anonymous boxes. v1

Review of attachment 8941273 [details] [diff] [review]:
-----------------------------------------------------------------

Also, thinking a bit more about this change, it's probably kinda weird how the restyling setup ends up... Because this is still restyling them from UpdatePseudoElementStyles, and AppendDirectlyOwnedAnonBoxes doesn't append it...

Boris, wdyt of the patch as-is? Maybe we should change them to actually make bullets use the anon box machinery instead...
Attachment #8941273 - Flags: feedback?(bzbarsky)
Comment on attachment 8941284 [details] [diff] [review]
Part 2 - Eliminate lazy pseudo style cache. v2

Review of attachment 8941284 [details] [diff] [review]:
-----------------------------------------------------------------

This is fine of course, once the previous patch is addressed.

Maybe worth mentioning in the commit message that the other lazy pseudos don't seem worth sharing.
Attachment #8941284 - Flags: review?(emilio) → review+
NI Jason, just as a heads-up that -moz-list-bullet and -moz-list-number will no longer be content-exposed and thus the fuzzers probably shouldn't bother generating them anymore once this lands.
Flags: needinfo?(jkratzer)
Noted - thanks.  Eventually, supported selectors will be detected as part of our CI process but for now, I'll go ahead and manually remove them.
Flags: needinfo?(jkratzer)
While drafting the intent-to-unship, I realized that the current implementation lines up pretty well with ::marker, which we may want to implement at some point. Emilio, wdyt?
(and Jason, maybe don't remove them just yet, since there's a chance we might not land this)
Flags: needinfo?(jkratzer)
Flags: needinfo?(emilio)
Flags: needinfo?(jkratzer)
Hm, I suppose now that we have the patches in bug 1429126, the reasoning for two separate linked lists (in bug 1368291 comment 5) no longer holds, and we could just merge the lazy pseudo cache with the anonymous box cache. I'll go ahead and do that.
(My patches for this bug, which I will probably abandon, are at https://github.com/bholley/gecko/commits/list_bullet_and_number_to_anon_box )
Comment on attachment 8941273 [details] [diff] [review]
Part 1 - Convert -moz-list-bullet and -moz-list-number to (non-web-exposed) inheriting anonymous boxes. v1

Worth checking whether people style these in the wild and why?

And yes, ::marker would likely have similar issues in terms of sharing, so those might be worth solving anyway.
Attachment #8941273 - Flags: feedback?(bzbarsky) → feedback+
Fwiw, a least github searches show very few hits for these pseudo-elements...
Blocks: 1429529
I'm going to try merging the caches in bug 1429529, and leaving moz-list-{bullet,number} alone.
No longer blocks: 1429529
If we want to implement ::marker I agree this problem needs to be solved regardless. But if we can make this problem not exist by merging the caches, that's even better, I agree :)
Flags: needinfo?(emilio)
Attachment #8941273 - Flags: review?(emilio)
OK, let's take the approach in the other bug then.

So Jason, the final word here is that :-moz-list-bullet and :-moz-list-number should not be removed from the fuzzer, since they'll still be valid (and fruitful for fuzz bugs). NI-ing just to be extra sure you see this. :-)
Status: NEW → RESOLVED
Closed: 2 years ago
Flags: needinfo?(jkratzer)
Resolution: --- → WONTFIX
Got it - thanks!
Flags: needinfo?(jkratzer)
You need to log in before you can comment on or make changes to this bug.