Note: There are a few cases of duplicates in user autocompletion which are being worked on.

stylo: slow selector flag handling is broken

RESOLVED FIXED in Firefox 55

Status

()

Core
CSS Parsing and Computation
P1
normal
RESOLVED FIXED
5 months ago
4 months ago

People

(Reporter: bholley, Assigned: emilio)

Tracking

(Blocks: 2 bugs)

unspecified
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

MozReview Requests

()

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

Attachments

(1 attachment)

See bug 1338982 comment 9.
In bug 1338982 comment 9, emilio writes:

> The best idea I have right now is using an outparam Vec<(Element, Flags)>,
> but that sounds quite terrible :(.

I think a better approach might be to pass an FnMut callback through the selector matching machinery, which would allow the code to add the SequentialTask directly, or just immediately set the flags, depending on the callsite.

WDYT?
Flags: needinfo?(emilio+bugs)
Blocks: 1338982
(Assignee)

Comment 2

5 months ago
(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #1)
> In bug 1338982 comment 9, emilio writes:
> 
> > The best idea I have right now is using an outparam Vec<(Element, Flags)>,
> > but that sounds quite terrible :(.
> 
> I think a better approach might be to pass an FnMut callback through the
> selector matching machinery, which would allow the code to add the
> SequentialTask directly, or just immediately set the flags, depending on the
> callsite.
> 
> WDYT?

WFM, though it'd be somewhat inconvenient to use from outside of Servo I guess... Not that I have a better idea.
Flags: needinfo?(emilio+bugs)
(In reply to Emilio Cobos Álvarez [:emilio] from comment #2)
> (In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #1)
> > In bug 1338982 comment 9, emilio writes:
> > 
> > > The best idea I have right now is using an outparam Vec<(Element, Flags)>,
> > > but that sounds quite terrible :(.
> > 
> > I think a better approach might be to pass an FnMut callback through the
> > selector matching machinery, which would allow the code to add the
> > SequentialTask directly, or just immediately set the flags, depending on the
> > callsite.
> > 
> > WDYT?
> 
> WFM, though it'd be somewhat inconvenient to use from outside of Servo I
> guess... Not that I have a better idea.

I don't think so - most of those consumers probably wouldn't set the flags anyway (and can pass |_, _| {}), and consumers that don't need the SequentialTask stuff can just pass |e, f| e.set_flags(f). And maybe even eta-reduce it! ;-)
Putting this on Emilio's plate for now, I can grab it if he doesn't get to it.
Assignee: nobody → emilio+bugs
(Assignee)

Comment 5

5 months ago
Sounds good, I'll look at it tomorrow.
(Assignee)

Comment 6

5 months ago
I plan to actually implement this once https://github.com/servo/servo/pull/15890 lands, given they're heavily in conflict.
(Assignee)

Comment 7

5 months ago
(so I don't forget)
Flags: needinfo?(emilio+bugs)
Blocks: 1348030
Blocks: 1349100
Comment hidden (mozreview-request)
(Reporter)

Comment 9

4 months ago
mozreview-review
Comment on attachment 8849548 [details]
Bug 1345950: stylo: Fix slow selector flags.

https://reviewboard.mozilla.org/r/122330/#review124670

Stealing from Cameron. lgtm, thanks for doing this.
Attachment #8849548 - Flags: review+
Attachment #8849548 - Flags: review?(cam)

Comment 10

4 months ago
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/069fe1ab5515
Fixup test expectations. r=bholley
Duplicate of this bug: 1330885
(Assignee)

Updated

4 months ago
Duplicate of this bug: 1338982

Comment 13

4 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/069fe1ab5515
Status: NEW → RESOLVED
Last Resolved: 4 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
(Assignee)

Updated

4 months ago
Flags: needinfo?(emilio+bugs)
You need to log in before you can comment on or make changes to this bug.