1298588 - stylo: Make initial style structs per-document

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Emilio Cobos Álvarez (:emilio)]

In Servo the initial computed values for all the properties is completely static. In Gecko, most style struct constructors take a value that is presshell dependent.

This allows handling, for example, the initial font size depending on zooming[1], and other similar, document-dependent stuff.

We need to align this behavior between both, but I think it's hard if not impossible handling the amount of things Gecko handles there without per-document style data.

[1]: http://searchfox.org/mozilla-central/source/layout/style/nsStyleStruct.cpp#224

Comment 1

[Bobby Holley (:bholley)]

Yeah, I was hoping that we could keep the initial computed values totally static and share them across documents (that's why I introduced StyleStructContext). However, last I talked to dbaron he was skeptical about this approach (in particular with respect to text zooming, IIRC).

David, could you have a look at http://searchfox.org/mozilla-central/rev/f80822840bc5f3d2d3cae3ece621ddbce72e7f54/layout/style/StyleStructContext.h#15 (both the comment and API surface) and let us know whether you think this is a feasible approach, or whether we should just bite the bullet and make style structs per-document?

Comment 2

[Bobby Holley (:bholley)]

I guess the basic alternative I was imagining would be that consumers (which are document-aware) would post-process the values when they're used. So I guess the question is whether there are transformations that are not commutative with respect to the cascade.

Also, the only time that Gecko calls these constructors is for the initial computed values (everything else is copy-on-write modified from those). Does that match Gecko, and is there any reason that would cause problems if we started making the style structs document-dependent? We'd presumably need to throw them away and regenerate them if text zoom changes, but I wonder if there's anything else.

Comment 3

[Emilio Cobos Álvarez (:emilio)]

I guess that's not really feasible, given the conversion from relative units to computed values depend on this too. Once you exit the style system you don't have rem units, for example, you should have already converted them to computed values. And to do so you need the font-size of the root.

Worth waiting for David's input though.

Comment 4

[Bobby Holley (:bholley)]

From https://public.etherpad-mozilla.org/p/stylo-taipei :

It seems that the initial style structs depend on a few different external values (Content-Language for 'direction', device pixel ratio, ...) so we should just have per-document initial style structs rather than sharing them across all documents.

Comment 5

[Bobby Holley (:bholley)]

Boris is going to work on this.

Comment 6

[Bobby Holley (:bholley)]

Some other items that would be good to handle here or in followups:
* Set things up so that we regenerate the initial style structs in whatever situations cause them to be invalidated (presumably some subset of the things that trigger whole-document restyle).
* Fix things up so that we call Servo_Init()/Servo_Shutdown() properly for the content process in e10s mode.
* See if things run without --disable-e10s. If they do, file a bug to remove that from our stylo mozconfig file. If they don't, file followups for the remaining issues.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Attachment: part 1. Make StyleStructContext work with a const nsPresContext

Comment 8

[Boris Zbarsky [:bzbarsky]]

Attachment: part 2. Pass through an nsPresContext to the PerDocumentStyleData constructor

Comment 9

[Boris Zbarsky [:bzbarsky]]

Attachment: part 3. Add a default ComputedValues member to PerDocumentStyleData

Comment 10

[Boris Zbarsky [:bzbarsky]]

Attachment: part 4. Recreate the default computed values for a document as needed

Comment 11

[Boris Zbarsky [:bzbarsky]]

Attachment: part 5. Store a ComputedValues on SharedStyleContext

The servo side of this is pretty much being left to https://github.com/servo/servo/issues/14773

Comment 12

[Boris Zbarsky [:bzbarsky]]

Attachment: part 6. Stop using initial_values when doing inheritance in Gecko glue code

Comment 13

[Boris Zbarsky [:bzbarsky]]

Attachment: part 7. Stop using initial_values in general in Gecko glue code

Comment 14

[Boris Zbarsky [:bzbarsky]]

Attachment: part 8. Pass a SharedStyleContext, not a Stylist, to Legalizer methods

Comment 15

[Boris Zbarsky [:bzbarsky]]

Attachment: part 9. Pass through useful default styles to cascade()

Comment 16

[Boris Zbarsky [:bzbarsky]]

Attachment: part 10. Pass through useful default styles to apply_declarations()

Comment 17

[Boris Zbarsky [:bzbarsky]]

Attachment: part 11. Pass through useful default styles to CascadePropertyFn

Comment 18

[Boris Zbarsky [:bzbarsky]]

Attachment: part 12. Compile some bits that call ComputedValues::initial_values only for servo, not stylo

I'm not super-happy with this, but a better solution is best done after we decide how we're doing media queries in stylo.

Comment 19

[Boris Zbarsky [:bzbarsky]]

Attachment: part 13. Make sure Device has a ComputedValues for stylo

Again, a cleaner solution is pending media query stuff being sorted out

Comment 20

[Boris Zbarsky [:bzbarsky]]

Attachment: part 14. Remove ComputedValues::initial_values for stylo

Comment 21

[Boris Zbarsky [:bzbarsky]]

Attachment: part 15. Rip out the initial() methods on style structs in stylo

Comment 22

[Boris Zbarsky [:bzbarsky]]

Attachment: part 16. Remove StyleStructContext

Comment 23

[Boris Zbarsky [:bzbarsky]]

> * Set things up so that we regenerate the initial style structs in whatever
> situations cause them to be invalidated (presumably some subset of the
> things that trigger whole-document restyle).

I believe this is more or less done in the attached patches, except for the Device bits.

> * Fix things up so that we call Servo_Init()/Servo_Shutdown() properly for
> the content process in e10s mode.

As far as I can tell this already happens.

> * See if things run without --disable-e10s. If they do, file a bug to remove
> that from our stylo mozconfig file.

Things seem to run fine that way (or as fine as without these patches, at least).  That said, I don't see --disable-e10s in browser/config/mozconfigs/linux64/stylo or browser/config/mozconfigs/linux64/stylo-debug.  So not sure where things need removing from.

Comment 24

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8822400 [details] [diff] [review]
part 13.  Make sure Device has a ComputedValues for stylo

Review of attachment 8822400 [details] [diff] [review]:
-----------------------------------------------------------------

::: servo/components/style/gecko/data.rs
@@ +67,5 @@
> +
> +        // FIXME(bz): We're going to need to either update the computed values
> +        // in the Stylist's Device or give the Stylist a new Device when our
> +        // default_computed_values changes.
> +        let device = Device::new(MediaType::Screen, window_size, &default_computed_values);

I wouldn't worry too much about the `Device` right now because I probably need to rewrite it in Stylo for bug 1325878. So it's probably acceptable to leave this as-is in the meantime.

Comment 25

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8822399 [details] [diff] [review]
part 12.  Compile some bits that call ComputedValues::initial_values only for servo, not stylo

Review of attachment 8822399 [details] [diff] [review]:
-----------------------------------------------------------------

Can we compile out ComputedValues::initial_values too?

Comment 26

[Emilio Cobos Álvarez (:emilio)]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #25)
> Comment on attachment 8822399 [details] [diff] [review]
> part 12.  Compile some bits that call ComputedValues::initial_values only
> for servo, not stylo
> 
> Review of attachment 8822399 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Can we compile out ComputedValues::initial_values too?

NVM, saw the next patch.

Comment 27

[Bobby Holley (:bholley)]

(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #23)
> > * Set things up so that we regenerate the initial style structs in whatever
> > situations cause them to be invalidated (presumably some subset of the
> > things that trigger whole-document restyle).
> 
> I believe this is more or less done in the attached patches, except for the
> Device bits.

\o/

> 
> > * Fix things up so that we call Servo_Init()/Servo_Shutdown() properly for
> > the content process in e10s mode.
> 
> As far as I can tell this already happens.

Are you sure? The existing failure mode without --disable-e10s is that the initial computed values are null, which I always assumed was because we call Servo_Init in nsAppRunner rather than nsLayoutStatics [1]. The reason for this, which is explained above [1], is obsoleted by your patches. So I think we just need another patch to move the calls back to nsLayoutStatics.

> > * See if things run without --disable-e10s. If they do, file a bug to remove
> > that from our stylo mozconfig file.
> 
> Things seem to run fine that way (or as fine as without these patches, at
> least).  That said, I don't see --disable-e10s in
> browser/config/mozconfigs/linux64/stylo or
> browser/config/mozconfigs/linux64/stylo-debug.  So not sure where things
> need removing from.

Yeah, I realized that we're currently building both e10s and non-e10s, so we're actually good there.

NI on the Servo_Init patch. I'll see if I can review the rest of the patches now.

[1] http://searchfox.org/mozilla-central/rev/51aa673e28802fe6ea89f4793721fc55334a6ac8/toolkit/xre/nsAppRunner.cpp#4390

Comment 28

[Bobby Holley (:bholley)]

Comment on attachment 8822386 [details] [diff] [review]
part 2.  Pass through an nsPresContext to the PerDocumentStyleData constructor

Review of attachment 8822386 [details] [diff] [review]:
-----------------------------------------------------------------

lgtm. Flagging Manish for a quick look at the FFI glue if he gets the chance. No need to block on it to land.

Comment 29

[Bobby Holley (:bholley)]

Comment on attachment 8822387 [details] [diff] [review]
part 3.  Add a default ComputedValues member to PerDocumentStyleData

Review of attachment 8822387 [details] [diff] [review]:
-----------------------------------------------------------------

::: servo/components/style/properties/gecko.mako.rs
@@ +132,5 @@
> +    pub fn default_values(pres_context: RawGeckoPresContextBorrowed) -> Arc<Self> {
> +        Arc::new(ComputedValues {
> +            custom_properties: None,
> +            shareable: true,
> +            writing_mode: WritingMode::empty(), // This seems dubious

Nit: The convention in Servo is to do:

FIXME(bz): This seems dubious

Same below.

Comment 30

[Bobby Holley (:bholley)]

Comment on attachment 8822388 [details] [diff] [review]
part 4.  Recreate the default computed values for a document as needed

Review of attachment 8822388 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/base/ServoRestyleManager.cpp
@@ +96,5 @@
> +  // We probably need to do some actual restyling here too, though.  And figure
> +  // out whether it actually matters that we may be recomputing the default
> +  // styles in too many cases.  For one thing, we do a bunch of eager work here,
> +  // whereas we should really just set a bit that says to recompute the default
> +  // computed styles before the next time we restyle anything!

Yes, I think that would be better. Can you file a blocker against stylo-incremental to implement this method correctly?

Comment 31

[Manish Goregaokar [:manishearth]]

Comment on attachment 8822386 [details] [diff] [review]
part 2.  Pass through an nsPresContext to the PerDocumentStyleData constructor

Review of attachment 8822386 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/style/ServoStyleSet.cpp
@@ +28,5 @@
>  void
>  ServoStyleSet::Init(nsPresContext* aPresContext)
>  {
>    mPresContext = aPresContext;
> +  mRawSet.reset(Servo_StyleSet_Init(aPresContext));

I assume this is never null, since we later pass it unchecked to appendstylesheet.

Comment 32

[Bobby Holley (:bholley)]

Comment on attachment 8822399 [details] [diff] [review]
part 12.  Compile some bits that call ComputedValues::initial_values only for servo, not stylo

Review of attachment 8822399 [details] [diff] [review]:
-----------------------------------------------------------------

Please add some comments explaining why we're compiling these out.

Comment 33

[Bobby Holley (:bholley)]

Comment on attachment 8822401 [details] [diff] [review]
part 14.  Remove ComputedValues::initial_values for stylo

Review of attachment 8822401 [details] [diff] [review]:
-----------------------------------------------------------------

\o/

Comment 34

[Bobby Holley (:bholley)]

Comment on attachment 8822400 [details] [diff] [review]
part 13.  Make sure Device has a ComputedValues for stylo

Review of attachment 8822400 [details] [diff] [review]:
-----------------------------------------------------------------

Probably worth filing a bug depending on bug 1290228 and blocking the stylo metabug to make sure that we end up updating these values.

Comment 35

[Boris Zbarsky [:bzbarsky]]

I'll update the patches and whatnot tomorrow, and answer needinfos.  I did have one question: what is the landing procedure for this?  Ideally one that preserves bisectability on both Gecko and Servo side...

Comment 36

[Emilio Cobos Álvarez (:emilio)]

(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #35)
> I'll update the patches and whatnot tomorrow, and answer needinfos.  I did
> have one question: what is the landing procedure for this?  Ideally one that
> preserves bisectability on both Gecko and Servo side...

That's hard to do right now, the only properly bisectable commits are the syncs, which are "known good" builds.

You should land the Servo part independently doing a PR to servo/servo (to export your patches you can use `git format-patch <revision> --relative=servo`), and then land the gecko patches independently (I usually do `git format-patch --relative=layout`, but maybe there's a better way?).

Comment 37

[Boris Zbarsky [:bzbarsky]]

OK, I guess I just don't understand what the current integration testing story is.  On the _gecko_ side we will certainly want all commits to be bisectable; I guess I should build just the Gecko parts one by one without the servo parts and see if they actually compile when not doing --enable-stylo...

The git commands are not relevant to me, because I'm not using git here to start with.  ;)  I know how to produce patches; the question is what patches to produce.

Comment 38

[Bobby Holley (:bholley)]

You should also push your patches to try with the target platform linux64-stylo and see how the results compare to https://treeherder.mozilla.org/#/jobs?repo=stylo&revision=68a742b057d75c060fc7e37b8ab89bbffbbb9e2e

Comment 39

[Boris Zbarsky [:bzbarsky]]

Fwiw, the changes in bug 1324627 bitrotted part 6 of this bug.  I'm going to back out some of those changes, so I keep having a PerDocumentStyleData in ResolveStyle.  Please let me know whether you want to re-review the resulting part 6.

Comment 40

[Boris Zbarsky [:bzbarsky]]

Attachment: Interdiff for part 6

OK, fixing things turned out to be a big more complicated than a simple backout.  Here's an interdiff of the actual changes needed to make part 6 work again.

Comment 41

[Boris Zbarsky [:bzbarsky]]

> see how the results compare to https://treeherder.mozilla.org/#/jobs?repo=stylo&revision=68a742b057d75c060fc7e37b8ab89bbffbbb9e2e

Apart from "both are totally orange"?

The reftest failures seem about the same.

For crashtest, I see a ton more assertions in my try push, coming from CounterStyleManager::NotifyRuleChanged called from the mPresContext->FlushCounterStyles() call in PresShell::FlushPendingNotifications.  But I'm not sure why this wouldn't get hit without my patches; I would expect this assert to be hit all the time as things stand...

Comment 42

[Boris Zbarsky [:bzbarsky]]

Oh, and for the separate-landing bits... where do I land the Gecko patches?  Inbound?  The incubator repo directly?  Something else?

Comment 43

[Manish Goregaokar [:manishearth]]

Inbound. Rebase them onto tip and remove the servo/ changes. https://public.etherpad-mozilla.org/p/stylo#lineNumber=52 has a git-filter-branch invocation that helps if you're using cinnabar.

Comment 44

[Bobby Holley (:bholley)]

Comment on attachment 8823160 [details] [diff] [review]
Interdiff for part 6

Review of attachment 8823160 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/style/ServoStyleSet.cpp
@@ +558,5 @@
> +already_AddRefed<ServoComputedValues>
> +ServoStyleSet::ResolveServoStyle(Element* aElement,
> +                                 ConsumeStyleBehavior aConsume)
> +{
> +  return Servo_ResolveStyle(aElement, mRawSet.get(), aConsume).Consume();  

nit: whitespace

Comment 45

[Boris Zbarsky [:bzbarsky]]

OK, the crashtest bit was because we started calling CounterStyleManager::BuildCounterStyle() from the default nsStyleFont ctor.  I'm going to just hack around it to keep not doing that for now for the servo case.

Comment 46

[Boris Zbarsky [:bzbarsky]]

> from the default nsStyleFont ctor

nsStyleList, of course.

Comment 47

[Boris Zbarsky [:bzbarsky]]

Attachment: Interdiff for part 3 to "fix" the crashtest asserts.

The real fix will be bug 1328319

Comment 48

[Boris Zbarsky [:bzbarsky]]

Attachment: Interdiff for part 16 to keep avoiding the assert

Comment 49

[Boris Zbarsky [:bzbarsky]]

> Are you sure?

No.  I saw nothing that would prevent XREMain::XRE_mainRun from running in the child process... but I guess the point is that we just fork _after_ that or something.  Though I wonder how we bring up XPCOM in the child process in that case...

In any case, I filed bug 1328651 on this; will fix.

> FIXME(bz): This seems dubious

Fixed.

> Can you file a blocker against stylo-incremental to implement this method correctly?

Filed bug 1328652.

> I assume this is never null, since we later pass it unchecked to appendstylesheet.

Correct.

> Please add some comments explaining why we're compiling these out.

Done.

> Probably worth filing a bug depending on bug 1290228 and blocking the stylo metabug

Bug 1328655 filed.

Comment 50

[Boris Zbarsky [:bzbarsky]]

https://github.com/servo/servo/pull/14848

Comment 51

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/82ae3ed4c30c
part 1.  Make StyleStructContext work with a const nsPresContext.  r=bholley
https://hg.mozilla.org/integration/mozilla-inbound/rev/f230c3fb62f5
part 2, gecko piece.  Pass through an nsPresContext to the PerDocumentStyleData constructor.  r=bholley
https://hg.mozilla.org/integration/mozilla-inbound/rev/94daa42ee46f
part 3, gecko piece.  Add a default ComputedValues member to PerDocumentStyleData.  r=bholley
https://hg.mozilla.org/integration/mozilla-inbound/rev/6d4f963d3655
part 4, gecko piece.  Recreate the default computed values for a document as needed.  r=bholley
https://hg.mozilla.org/integration/mozilla-inbound/rev/47a58ef26a92
part 6, gecko piece.  Stop using initial_values when doing inheritance in Gecko glue code.  r=bholley
https://hg.mozilla.org/integration/mozilla-inbound/rev/ac84ae5fa2da
part 7, gecko piece.  Stop using initial_values in general in Gecko glue code.  r=bholley
https://hg.mozilla.org/integration/mozilla-inbound/rev/4ccc8126ea20
part 9, gecko piece.  Pass through useful default styles to cascade().  r=bholley
https://hg.mozilla.org/integration/mozilla-inbound/rev/6994a692b48c
part 10, gecko piece.  Pass through useful default styles to apply_declarations().  r=bholley
https://hg.mozilla.org/integration/mozilla-inbound/rev/0978881c4e10
part 15, gecko piece.  Rip out the initial() methods on style structs in stylo.  r=bholley
https://hg.mozilla.org/integration/mozilla-inbound/rev/e743dff9dca1
part 16.  Remove StyleStructContext.  r=bholley

Comment 52

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/82ae3ed4c30c
https://hg.mozilla.org/mozilla-central/rev/f230c3fb62f5
https://hg.mozilla.org/mozilla-central/rev/94daa42ee46f
https://hg.mozilla.org/mozilla-central/rev/6d4f963d3655
https://hg.mozilla.org/mozilla-central/rev/47a58ef26a92
https://hg.mozilla.org/mozilla-central/rev/ac84ae5fa2da
https://hg.mozilla.org/mozilla-central/rev/4ccc8126ea20
https://hg.mozilla.org/mozilla-central/rev/6994a692b48c
https://hg.mozilla.org/mozilla-central/rev/0978881c4e10
https://hg.mozilla.org/mozilla-central/rev/e743dff9dca1

Comment 53

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f6c47ff600f7
followup whitespace fix to address a missed review comment.  DONTBUILD

Comment 54

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

https://hg.mozilla.org/mozilla-central/rev/f6c47ff600f7