bugzilla.mozilla.org has resumed normal operation. Attachments prior to 2014 will be unavailable for a few days. This is tracked in Bug 1475801.
Please report any other irregularities here.

stylo: Make initial style structs per-document

RESOLVED FIXED in Firefox 53

Status

()

Core
CSS Parsing and Computation
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: emilio, Assigned: bz)

Tracking

(Blocks: 1 bug)

unspecified
mozilla53
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox53 fixed)

Details

Attachments

(19 attachments)

3.35 KB, patch
bholley
: review+
Details | Diff | Splinter Review
22.46 KB, patch
bholley
: review+
manishearth
: feedback+
Details | Diff | Splinter Review
25.59 KB, patch
bholley
: review+
Details | Diff | Splinter Review
7.04 KB, patch
bholley
: review+
Details | Diff | Splinter Review
4.93 KB, patch
bholley
: review+
Details | Diff | Splinter Review
7.68 KB, patch
bholley
: review+
Details | Diff | Splinter Review
4.17 KB, patch
bholley
: review+
Details | Diff | Splinter Review
13.81 KB, patch
bholley
: review+
Details | Diff | Splinter Review
15.63 KB, patch
bholley
: review+
Details | Diff | Splinter Review
11.30 KB, patch
bholley
: review+
Details | Diff | Splinter Review
4.54 KB, patch
bholley
: review+
Details | Diff | Splinter Review
5.11 KB, patch
bholley
: review+
Details | Diff | Splinter Review
8.55 KB, patch
bholley
: review+
Details | Diff | Splinter Review
6.47 KB, patch
bholley
: review+
Details | Diff | Splinter Review
23.50 KB, patch
bholley
: review+
Details | Diff | Splinter Review
43.76 KB, patch
bholley
: review+
Details | Diff | Splinter Review
6.66 KB, patch
bholley
: review+
Details | Diff | Splinter Review
2.36 KB, patch
Details | Diff | Splinter Review
3.06 KB, patch
Details | Diff | Splinter Review
(Reporter)

Description

2 years ago
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
(Reporter)

Updated

2 years ago
Blocks: 1290228
(Reporter)

Updated

2 years ago
Blocks: 1243581
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?
Flags: needinfo?(dbaron)
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.
(Reporter)

Comment 3

2 years ago
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.
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.
Flags: needinfo?(dbaron)
Summary: stylo: Decide how we want to align initial computed values between Gecko and Servo. → stylo: Make initial style structs per-document
Boris is going to work on this.
Flags: needinfo?(bzbarsky)
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.
Created attachment 8822369 [details] [diff] [review]
part 1.  Make StyleStructContext work with a const nsPresContext
Attachment #8822369 - Flags: review?(bobbyholley)
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Created attachment 8822386 [details] [diff] [review]
part 2.  Pass through an nsPresContext to the PerDocumentStyleData constructor
Attachment #8822386 - Flags: review?(bobbyholley)
Created attachment 8822387 [details] [diff] [review]
part 3.  Add a default ComputedValues member to PerDocumentStyleData
Attachment #8822387 - Flags: review?(bobbyholley)
Created attachment 8822388 [details] [diff] [review]
part 4.  Recreate the default computed values for a document as needed
Attachment #8822388 - Flags: review?(bobbyholley)
Created attachment 8822390 [details] [diff] [review]
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
Attachment #8822390 - Flags: review?(bobbyholley)
Created attachment 8822391 [details] [diff] [review]
part 6.  Stop using initial_values when doing inheritance in Gecko glue code
Attachment #8822391 - Flags: review?(bobbyholley)
Created attachment 8822392 [details] [diff] [review]
part 7.  Stop using initial_values in general in Gecko glue code
Attachment #8822392 - Flags: review?(bobbyholley)
Created attachment 8822393 [details] [diff] [review]
part 8.  Pass a SharedStyleContext, not a Stylist, to Legalizer methods
Attachment #8822393 - Flags: review?(bobbyholley)
Created attachment 8822395 [details] [diff] [review]
part 9.  Pass through useful default styles to cascade()
Attachment #8822395 - Flags: review?(bobbyholley)
Created attachment 8822397 [details] [diff] [review]
part 10.  Pass through useful default styles to apply_declarations()
Attachment #8822397 - Flags: review?(bobbyholley)
Created attachment 8822398 [details] [diff] [review]
part 11.  Pass through useful default styles to CascadePropertyFn
Attachment #8822398 - Flags: review?(bobbyholley)
Created attachment 8822399 [details] [diff] [review]
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.
Attachment #8822399 - Flags: review?(bobbyholley)
Created attachment 8822400 [details] [diff] [review]
part 13.  Make sure Device has a ComputedValues for stylo

Again, a cleaner solution is pending media query stuff being sorted out
Attachment #8822400 - Flags: review?(bobbyholley)
Created attachment 8822401 [details] [diff] [review]
part 14.  Remove ComputedValues::initial_values for stylo
Attachment #8822401 - Flags: review?(bobbyholley)
Created attachment 8822402 [details] [diff] [review]
part 15.  Rip out the initial() methods on style structs in stylo
Attachment #8822402 - Flags: review?(bobbyholley)
Created attachment 8822403 [details] [diff] [review]
part 16.  Remove StyleStructContext
Attachment #8822403 - Flags: review?(bobbyholley)
> * 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.
Flags: needinfo?(bzbarsky) → needinfo?(bobbyholley)
(Reporter)

Comment 24

2 years ago
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.
(Reporter)

Comment 25

2 years ago
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?
(Reporter)

Comment 26

2 years ago
(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.
(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
Flags: needinfo?(bobbyholley) → needinfo?(bzbarsky)
Attachment #8822369 - Flags: review?(bobbyholley) → review+
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.
Attachment #8822386 - Flags: review?(bobbyholley)
Attachment #8822386 - Flags: review+
Attachment #8822386 - Flags: feedback?(manishearth)
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.
Attachment #8822387 - Flags: review?(bobbyholley) → review+
Attachment #8822386 - Flags: feedback?(manishearth) → feedback+
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?
Attachment #8822388 - Flags: review?(bobbyholley) → review+
Attachment #8822390 - Flags: review?(bobbyholley) → review+
Attachment #8822391 - Flags: review?(bobbyholley) → review+
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.
Attachment #8822392 - Flags: review?(bobbyholley) → review+
Attachment #8822393 - Flags: review?(bobbyholley) → review+
Attachment #8822395 - Flags: review?(bobbyholley) → review+
Attachment #8822398 - Flags: review?(bobbyholley) → review+
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.
Attachment #8822399 - Flags: review?(bobbyholley) → review+
Comment on attachment 8822401 [details] [diff] [review]
part 14.  Remove ComputedValues::initial_values for stylo

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

\o/
Attachment #8822401 - Flags: review?(bobbyholley) → review+
Attachment #8822402 - Flags: review?(bobbyholley) → review+
Attachment #8822403 - Flags: review?(bobbyholley) → review+
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.
Attachment #8822400 - Flags: review?(bobbyholley) → review+
Attachment #8822397 - Flags: review?(bobbyholley) → review+
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...
(Reporter)

Comment 36

2 years ago
(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?).
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.
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
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.
Flags: needinfo?(bobbyholley)
Created attachment 8823160 [details] [diff] [review]
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.
Flags: needinfo?(bobbyholley)
Attachment #8823160 - Flags: review?(bobbyholley)
> 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...
Flags: needinfo?(bzbarsky)
Flags: needinfo?(bzbarsky)
Flags: needinfo?(bzbarsky) → needinfo?(bobbyholley)
Flags: needinfo?(bzbarsky)
Oh, and for the separate-landing bits... where do I land the Gecko patches?  Inbound?  The incubator repo directly?  Something else?
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 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
Attachment #8823160 - Flags: review?(bobbyholley) → review+
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.
> from the default nsStyleFont ctor

nsStyleList, of course.
Created attachment 8823361 [details] [diff] [review]
Interdiff for part 3 to "fix" the crashtest asserts.

The real fix will be bug 1328319
Created attachment 8823362 [details] [diff] [review]
Interdiff for part 16 to keep avoiding the assert
Flags: needinfo?(bobbyholley)
> 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.
Flags: needinfo?(bzbarsky)

Comment 51

2 years ago
Pushed by bzbarsky@mozilla.com:
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
Blocks: 1328487

Comment 53

2 years ago
Pushed by bzbarsky@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f6c47ff600f7
followup whitespace fix to address a missed review comment.  DONTBUILD
You need to log in before you can comment on or make changes to this bug.