1345950 - stylo: slow selector flag handling is broken

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect, P1)

Description

[Bobby Holley (:bholley)]

See bug 1338982 comment 9.

Comment 1

[Bobby Holley (:bholley)]

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?

Comment 2

[Emilio Cobos Álvarez (:emilio)]

(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.

Comment 3

[Bobby Holley (:bholley)]

(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! ;-)

Comment 4

[Bobby Holley (:bholley)]

Putting this on Emilio's plate for now, I can grab it if he doesn't get to it.

Comment 5

[Emilio Cobos Álvarez (:emilio)]

Sounds good, I'll look at it tomorrow.

Comment 6

[Emilio Cobos Álvarez (:emilio)]

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

Comment 7

[Emilio Cobos Álvarez (:emilio)]

(so I don't forget)

Comment 8

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1345950: stylo: Fix slow selector flags.

Review commit: https://reviewboard.mozilla.org/r/122330/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/122330/

Comment 9

[Bobby Holley (:bholley)]

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.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/069fe1ab5515
Fixup test expectations. r=bholley

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/069fe1ab5515