1362991 - stylo: Compute a single text style context for all the text children of the same element

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, enhancement)

Description

[Emilio Cobos Álvarez (:emilio)]

We can do it quite easily, and it seems it's a pretty much free win.

Comment 1

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1362991: Pass StyleSet by reference in ServoRestyleManager.

UpdateStyleOfOwnedAnonBoxes already does this, and I think it's cleaner.

Happy to revert if wanted, but I'd like us to do this more in general... :/

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

Comment 2

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1362991: Compute at most one text style context per element.

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

Comment 3

[Cameron McCormack (:heycam)]

Comment on attachment 8865429 [details]
Bug 1362991: Pass StyleSet by reference in ServoRestyleManager.

https://reviewboard.mozilla.org/r/137078/#review140380

I don't think we should do this, as it goes against the prevailing style of the codebase.  We tend to use references just for outparams (and inout), and even that goes against an existing coding style rule.  If we do want to move towards a codebase that uses references instead of pointers, it deserves a discussion on dev-platform resulting in something explicit in the style guide.

Comment 4

[Cameron McCormack (:heycam)]

Comment on attachment 8865430 [details]
Bug 1362991: Compute at most one text style context per element.

https://reviewboard.mozilla.org/r/137080/#review140392

::: layout/base/ServoRestyleManager.cpp:142
(Diff revision 1)
>   * This is currently used to properly compute change hints when the parent
>   * element of this node is a display: contents node.

Please update this comment to say that we also use it to avoid performing duplicate on sibling text nodes.

Comment 5

[Emilio Cobos Álvarez (:emilio)]

(In reply to Cameron McCormack (:heycam) from comment #3)
> Comment on attachment 8865429 [details]
> Bug 1362991: Pass StyleSet by reference in ServoRestyleManager.
> 
> https://reviewboard.mozilla.org/r/137078/#review140380
> 
> I don't think we should do this, as it goes against the prevailing style of
> the codebase.  We tend to use references just for outparams (and inout), and
> even that goes against an existing coding style rule.  If we do want to move
> towards a codebase that uses references instead of pointers, it deserves a
> discussion on dev-platform resulting in something explicit in the style
> guide.

I sent https://groups.google.com/forum/#!topic/mozilla.dev.platform/_jfnwDvcvN4

Will revert this for now :)

Comment 6

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8865430 [details]
Bug 1362991: Compute at most one text style context per element.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/137080/diff/1-2/

Comment 7

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8865430 [details]
Bug 1362991: Compute at most one text style context per element.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/137080/diff/2-3/

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/42e6c5b52136
Compute at most one text style context per element. r=heycam

Comment 9

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

https://hg.mozilla.org/mozilla-central/rev/42e6c5b52136