1344914 - stylo: Why does ServoStyleSet::ResolveAnonymousBoxStyle not call GetContext?

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, enhancement, P5)

Description

[Boris Zbarsky [:bzbarsky]]

It's directly calling NS_NewStyleContext, which skips some work GetContext does...

Presumably this has to do with the broken fixup handling in GetContext; if so we might want to just fix that.

Comment 1

[Cameron McCormack (:heycam)]

I don't know if it was trying to avoid calling into broken fixup handling deliberately.  The fixup skipping stuff in nsStyleContext's constructor doesn't actually have any effect in stylo -- anything like that should be handled on the Servo side, to influence the ServoComputedValues object we get before sticking it in an nsStyleContext.  So probably we could just call GetContext here.

Comment 2

[Boris Zbarsky [:bzbarsky]]

> anything like that should be handled on the Servo side, to influence the
> ServoComputedValues object we get before sticking it in an nsStyleContext

Do we actually do that in this case?  I don't see a mechanism for it....

Comment 3

[Cameron McCormack (:heycam)]

No, I don't think we do.  So I guess it's likely some form controls etc. are incorrectly blockifying their anonymous box children when display:grid/flex are specified on them.

Comment 4

[Cameron McCormack (:heycam)]

Sorry, that's completely wrong.  We do skip fixups for NAC roots, using the TElement::skip_root_and_item_based_display_fixup function to check whether to do it, rather than relying on the flags that are passed in.

Comment 5

[Boris Zbarsky [:bzbarsky]]

> We do skip fixups for NAC roots

We actually skip it for all NAC, looks like.  That doesn't seem right to me.  We should be skipping it for NAC roots, and pseudo-elements, but probably not other NAC...

Comment 6

[Bobby Holley (:bholley)]

I'll defer to heycam on questions surrounding style fixup, which I don't know much about. However, I generally agree that this work should happen during the servo cacade and shouldn't live on nsStyleContext.

Comment 7

[Cameron McCormack (:heycam)]

Attachment: Bug 1344914 - stylo: Stop pretending to handle style fixups on the C++ side.

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

Comment 8

[Cameron McCormack (:heycam)]

(In reply to Boris Zbarsky [:bz] (still a bit busy) (if a patch has no decent message, automatic r-) from comment #5)
> We actually skip it for all NAC, looks like.  That doesn't seem right to me.
> We should be skipping it for NAC roots, and pseudo-elements, but probably
> not other NAC...

I split that issue out into bug 1359303.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8861286 [details]
Bug 1344914 - stylo: Stop pretending to handle style fixups on the C++ side.

https://reviewboard.mozilla.org/r/133246/#review136338

r=me with the nits below fixed.

::: layout/style/nsStyleContext.cpp:130
(Diff revision 1)
>      PresContext()->PresShell()->StyleSet()->RootStyleContextAdded();
>    }
>  
>    mSource.AsGeckoRuleNode()->SetUsedDirectly(); // before ApplyStyleFixups()!
> -  FinishConstruction(aSkipParentDisplayBasedStyleFixup);
> +  FinishConstruction();
> +  ApplyStyleFixups(aSkipParentDisplayBasedStyleFixup);

Header definition of ApplyStyleFixups should document that it's only called for Gecko style contexts.

And the impl should assert that.

Comment 10

[Cameron McCormack (:heycam)]

Comment on attachment 8861286 [details]
Bug 1344914 - stylo: Stop pretending to handle style fixups on the C++ side.

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

Comment 11

[Cameron McCormack (:heycam)]

Comment on attachment 8861286 [details]
Bug 1344914 - stylo: Stop pretending to handle style fixups on the C++ side.

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

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0ec24df56cce
stylo: Stop pretending to handle style fixups on the C++ side. r=bz

Comment 13

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/0ec24df56cce