Bobby Holley (:bholley) Assignee
	Description • 8 years ago

This came up during an IRC conversation between me and Emilio.

Servo currently traverses and computes style for text nodes. It starts with a copy of the parent style, and then layout occasionally tweaks it (I believe).

For stylo, it's not clear to me that we need this, and I'm wondering if we can get away with just sharing the style context with the containing element. This would let us avoid attaching ServoNodeData to non-Element nodes (and thus save memory), and simplify change hint processing.

	Bobby Holley (:bholley) Assignee
	Comment 1 • 8 years ago

Cameron, any thoughts here?

I'm also a bit confused about the :-moz-text anonymous box that we use to resolve style for text frames. It looks like we don't have any rules matching it, and the comment even says as much. Does that mean we can just use the parent style? If so, what's the purpose of it? Just to give us a different style context for some reason?

	David Baron :dbaron:
	Comment 2 • 8 years ago

So it's not the same style as the parent because all the non-inherited properties are their initial values rather than inherited values.  And we actually depend on that, so we couldn't just go ahead and use the same style context.

We also hard-code (ResolveStyleForText / ResolveStyleForOtherNonElement) the fact that it doesn't match anything and don't even try running matching on that pseudo.

(I don't recall why we introduced the distinction between :-moz-text and :-moz-other-non-element, but I suspect it has to do with SVG text and heycam can answer.)

	Bobby Holley (:bholley) Assignee
	Comment 3 • 8 years ago

[19:01:28]  <bholley>	dbaron: oh, ok - I totally had that wrong then
[19:01:59]  <bholley>	dbaron: but we never tweak the style in any more interesting way than that?
[19:02:32]  <bholley>	dbaron: i.e. it seems like we could still avoid cascading in servo, and give them their own style context with an inherited version of the parent's computed values (which we could compute on the fly)
[19:03:29] bholley	has to run
[19:03:37]  <bholley>	dbaron: thanks for the help!
[19:03:56]  <emilio>	bholley: we sort of do that already, the text node's style is only the inherited version from the parent. But yeah, computing it lazily from gecko should be doable, I guess

	Bobby Holley (:bholley) Assignee
	Comment 4 • 8 years ago

I guess it boils down to the question of whether the work to create an inherited version of the parent style is nontrivial enough to be worth doing in parallel at the expense of the memory cost on text nodes.

	Emilio Cobos Álvarez (:emilio)
	Comment 5 • 8 years ago

In servo inheriting style structs is as easy as bumping the refcount in the appropriate structs, either from the initial values or the parent style, so my guess is that it's sort of trivial and we'll have to measure whether the memory cost is worth it.

I'm more concerned about the fact that right now we have to compute change hints for text nodes, and stick them in the parent element's change hint, which is sort lame :/.

Ideally we should find a solution that addresses both problems.

	Cameron McCormack (:heycam)
	Comment 6 • 8 years ago

(In reply to David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) from comment #2)
> (I don't recall why we introduced the distinction between :-moz-text and
> :-moz-other-non-element, but I suspect it has to do with SVG text and heycam
> can answer.)

We needed to be able to apply some special rules to munge writing-mode when using text-combine-upright in ruby text, but previously we had a single pseudo :-moz-non-element used for both text and for a couple of other things (like the nsFirstLineFrame that contained the things outside the ::first-line).

I think Emilio is right that the way we produce ComputedValues for text nodes is relatively cheap.

	Bobby Holley (:bholley) Assignee
	Comment 7 • 8 years ago

(In reply to Cameron McCormack (:heycam) from comment #6)
> (In reply to David Baron :dbaron: ⌚️UTC-7 (review requests must explain
> patch) from comment #2)
> > (I don't recall why we introduced the distinction between :-moz-text and
> > :-moz-other-non-element, but I suspect it has to do with SVG text and heycam
> > can answer.)
> 
> We needed to be able to apply some special rules

Do you mean via having a selector that matches them, or some other mechanism? I understood from dbaron's comment and from grepping that we don't have any selectors that match :-moz-text in the UA sheet (or in the tree).

Does that also mean that we need to munge the text style data in some sort of interesting way (i.e. that just inheriting values from the parent is not sufficient)?

	Cameron McCormack (:heycam)
	Comment 8 • 8 years ago

Sorry, I meant rules in a more generic way.  We munge writing-mode in nsStyleContext::ApplyStyleFixups for text nodes when we're in a text-combine-upright:all, and we didn't want to do that for other non-element style contexts.  So yes, just inheriting is not sufficient in this case.  I think it's the only time Gecko doesn't just inherit the inherited structs for a text node.

	Bobby Holley (:bholley) Assignee
	Comment 9 • 8 years ago

(In reply to Cameron McCormack (:heycam) from comment #8)
> Sorry, I meant rules in a more generic way.  We munge writing-mode in
> nsStyleContext::ApplyStyleFixups for text nodes when we're in a
> text-combine-upright:all, and we didn't want to do that for other
> non-element style contexts.  So yes, just inheriting is not sufficient in
> this case.  I think it's the only time Gecko doesn't just inherit the
> inherited structs for a text node.

I see. I think we can/should probably move that logic into servo, and have a special accessor that allows us to get the style for text nodes (which is identical to Servo_InheritComputedValues, modulo that tweak).

Then the only question is whether we should do that computation eagerly (at the expense of the memory overhead of having a ServoNodeData pointer on non-element nodes). I guess that's something we can decide later when we're looking at memory overhead.

	Bobby Holley (:bholley) Assignee
	Comment 10 • 8 years ago

Note: I'm adding some traversal code that will need to change if we do this. Grep the source for this bug number.

	Bobby Holley (:bholley) Assignee
	Comment 11 • 8 years ago

(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #10)
> Note: I'm adding some traversal code that will need to change if we do this.
> Grep the source for this bug number.

Scratch that - I decided to just return them and let the servo traversal code handle them so that we can keep all that logic in one place.

	Bobby Holley (:bholley) Assignee
	Comment 12 • 8 years ago

MozReview-Commit-ID: CroFtWpuIrO

	Bobby Holley (:bholley) Assignee
	Comment 13 • 8 years ago

This enables us to stop traversing text nodes in servo. I'll have a PR up for that shortly.

	Bobby Holley (:bholley) Assignee
	Comment 14 • 8 years ago

MozReview-Commit-ID: CroFtWpuIrO

	Pulsebot
	Comment 15 • 8 years ago

Pushed by bholley@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1c446e42a269
Style text nodes on the main thread. r=heycam

	Carsten Book [:Tomcat]
	Comment 16 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/1c446e42a269