1369187 - stylo: Implement the basic building blocks for ::first-line and ::first-letter.

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, enhancement)

Description

[Emilio Cobos Álvarez (:emilio)]

So Bobby recently asked for this, and I had this patch lying around.

I guess it's better to land them in this state so Boris and I can work on the remaining pieces in parallel.

This is just the building blocks, so there's some (a lot) of stuff that's missing. In particular:

 * The "only some properties affect first-line/first-letter" bits.
 * The restyle handling for them is terribly inefficient (we just reframe, which is unacceptable for shipping). More on this below.
 * The "this element will never generate a first-line/first-letter pseudo" is also not done (the API is there though).

I think it's worth landing, because there are a few open questions re. the rest. In particular:

# CalcStyleDifference

Right now for ::before and ::after we find the nsStyleContext of the relevant frame on the fly, which is ok. It's not clear to me though whether we want to do frame tree traversal in order to get the ::first-line/::first-letter pseudo-element.

In particular, given we now diff all the structs unconditionally, and the property set that applies to them is reduced, the win of the "has this struct been used? If not, avoid adding change hints due to it" may not be as huge.

So my proposal for it is allowing to call CalcStyleDifference on a pair of ComputedValues. We're going to need something like that for ::visited anyway.

# Frame traversal

I expect Boris to be able to just answer/implement most of the bits related to this. What is a good API we could expose to avoid adding a perf hit for normal restyles, while making these correct?

Thoughts on those?

Comment 1

[Emilio Cobos Álvarez (:emilio)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=29c46696d748203d67e59bd218a31397b5006d3e

Comment 2

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1369187: Don't return an old style context for ::first-line and ::first-letter, but not crash either.

This will make us reframe until fixed. It's not hard to fix, just requires to
discuss how :)

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

Comment 3

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1369187: style: Add an API to fast-reject eager pseudos.

Also, do nothing for now (we'll hook the Gecko pieces here when the time comes).

Let me know if you want to hold-off landing this.

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

Comment 4

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1369187: style: Add first-line and first-letter to the set of eager pseudo styles.

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

Comment 5

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1369187: style: Use ArrayVec for the pseudo-elements we need to restyle.

Just a drive-by, easy optimization.

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

Comment 6

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1369187: style: Assert that if we find an eager, element-backed pseudo, it's ::before or ::after.

This holds because ::first-line and ::first-letter are not element-backed.

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

Comment 7

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #0)
> So my proposal for it is allowing to call CalcStyleDifference on a pair of
> ComputedValues. We're going to need something like that for ::visited anyway.

Yeah, for :visited, we could either compute the nsStyleContext for visited and then compare those like Gecko does[1], or we could directly compare the ServoComputedValues like you are suggesting.  (For :visited, we may want to take the same optimization as Gecko and only compare visited-dependent properties.)

[1]: http://searchfox.org/mozilla-central/rev/1a0d9545b9805f50a70de703a3c04fc0d22e3839/layout/style/nsStyleContext.cpp#1146-1201

Comment 8

[Emilio Cobos Álvarez (:emilio)]

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #7)
> (In reply to Emilio Cobos Álvarez [:emilio] from comment #0)
> > So my proposal for it is allowing to call CalcStyleDifference on a pair of
> > ComputedValues. We're going to need something like that for ::visited anyway.
> 
> Yeah, for :visited, we could either compute the nsStyleContext for visited
> and then compare those like Gecko does[1], or we could directly compare the
> ServoComputedValues like you are suggesting.  (For :visited, we may want to
> take the same optimization as Gecko and only compare visited-dependent
> properties.)

Right, that's also true to some extent for ::first-letter and ::first-line, so that's why I plan to share the infrastructure here with that :)

I think we should try to not compute the nsStyleContext for visited, since it's only used in a few places, and nsStyleContext's are quite fat right now, so they take up a bit of memory.

Comment 9

[Bobby Holley (:bholley)]

Comment on attachment 8873208 [details]
Bug 1369187: Don't return an old style context for ::first-line and ::first-letter, but not crash either.

https://reviewboard.mozilla.org/r/144656/#review148888

Comment 10

[Bobby Holley (:bholley)]

Comment on attachment 8873209 [details]
Bug 1369187: style: Add an API to fast-reject eager pseudos.

https://reviewboard.mozilla.org/r/144658/#review148890

Comment 11

[Bobby Holley (:bholley)]

Comment on attachment 8873210 [details]
Bug 1369187: style: Add first-line and first-letter to the set of eager pseudo styles.

https://reviewboard.mozilla.org/r/144660/#review148892

Comment 12

[Bobby Holley (:bholley)]

Comment on attachment 8873211 [details]
Bug 1369187: style: Use ArrayVec for the pseudo-elements we need to restyle.

https://reviewboard.mozilla.org/r/144662/#review148894

Comment 13

[Bobby Holley (:bholley)]

Comment on attachment 8873212 [details]
Bug 1369187: style: Assert that if we find an eager, element-backed pseudo, it's ::before or ::after.

https://reviewboard.mozilla.org/r/144664/#review148896

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/cf869515f9be
Don't return an old style context for ::first-line and ::first-letter, but not crash either. r=bholley
https://hg.mozilla.org/integration/autoland/rev/2fca98f65d11
Update expectations. r=emilio

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/fec3a4b50acd
More expectation updates. r=emilio

Comment 16

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

https://hg.mozilla.org/mozilla-central/rev/cf869515f9be
https://hg.mozilla.org/mozilla-central/rev/2fca98f65d11
https://hg.mozilla.org/mozilla-central/rev/fec3a4b50acd