Closed Bug 1366721 Opened 7 years ago Closed 7 years ago

stylo: Restyle additional style contexts

Categories

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

Product:
Core
Component:
CSS Parsing and Computation
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla56
Tracking Flags:
Tracking Status
firefox56 --- fixed

People

(Reporter: emilio, Assigned: emilio)

References

Details

Attachments

(3 files)

Bug 1366721: Switch all the APIs in ServoStyleSet to use ServoStyleContext.
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1366721: Restyle additional style contexts in ServoRestyleManager.
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1366721: Update reftest expectations.
59 bytes, text/x-review-board-request
heycam
: review+
Details 
Looking over ::-moz-math-anonymous and similar stuff, it seems they're restyled using the {Get,Set}AdditionalStyleContext API.

We should presumably restyle those.
I think this goes with bz's anonymous box restyles stuff.
Assignee: nobody → bzbarsky
Priority: -- → P2
Once this is fixed, we should try the test case from bug 1376406 to ensure that its Servo-side fix in bug 1379865 had the desired effect.
We've received already two reports of this, and this isn't blocked on anything particular afaict.

Boris, should I just take this?
Flags: needinfo?(bzbarsky)
If you have the bandwidth for it, sure!
Flags: needinfo?(bzbarsky)
Assignee: bzbarsky → emilio+bugs
Might be worth asserting that anon boxes don't have additional style contexts.
(In reply to Boris Zbarsky [:bz] from comment #7)
> Might be worth asserting that anon boxes don't have additional style
> contexts.

Yeah, in the second commit message I mention it... I guess I'll do that.
Comment on attachment 8888390 [details]
Bug 1366721: Switch all the APIs in ServoStyleSet to use ServoStyleContext.

https://reviewboard.mozilla.org/r/159340/#review165092

::: layout/style/ServoStyleSet.h:133
(Diff revision 1)
> -  already_AddRefed<nsStyleContext>
> +  already_AddRefed<ServoStyleContext>
>    ResolveStyleFor(dom::Element* aElement,
>                    ServoStyleContext* aParentContext,
>                    LazyComputeBehavior aMayCompute);

Hooray for already_AddRef<BaseClass>'s ability to autoconvert from already_AddRef<DerivedClass>, in StyleSetHandle::Ptr::ResolveStyleFor. :-)

::: layout/style/ServoStyleSet.h:364
(Diff revision 1)
>  
>    /**
>     * Resolve style for the given element, and return it as a
> -   * ServoComputedValues, not an nsStyleContext.
> +   * ServoStyleContext.
> +   *
> +   * FIXME(emilio): Is there a point in this after bug 1367904?

Probably not!
Attachment #8888390 - Flags: review?(cam) → review+
Comment on attachment 8888391 [details]
Bug 1366721: Restyle additional style contexts in ServoRestyleManager.

https://reviewboard.mozilla.org/r/159342/#review165096

r=me with the appropriate thing done for anonymous boxes.

::: commit-message-b9cca:3
(Diff revision 2)
> +I was about to assert that other non-primary frames don't have additional style
> +contexts everywhere, but that wouldn't make much sense, given they don't
> +correspond to an element we could selector-match against.

Couldn't they in theory use the same element as the primary frame?  Though I don't think we have any cases of non-primary frames that use additional style contexts.  You could assert that somewhere under UpdateStyleOfOwnedAnonBoxes if you want.

::: layout/base/ServoRestyleManager.cpp:436
(Diff revision 2)
> +  RefPtr<ServoStyleContext> newContext =
> +    aRestyleState.StyleSet().ResolvePseudoElementStyle(

What about if the old style context was for an anonymous box?  Or does that not happen for the current GetAdditionalStyleContext implementations?  If so, maybe assert it up the top of the function.

(I notice that GeckoRestyleManager doesn't re-selector match (non-XUL) additional style contexts, but that doesn't seem to be an important optimization.)

::: layout/base/ServoRestyleManager.cpp:635
(Diff revision 2)
>        MOZ_ASSERT(!styleFrame);
>        displayContentsNode->mStyle = newContext;
>      }
>  
>      if (styleFrame) {
> +      UpdateAdditionalStyleContexts(styleFrame, aRestyleState);

What about continuations?  Maybe it doesn't matter in practice, since all of the frames that have additional style contexts can never be split?  Maybe assert this in UpdateAdditionalStyleContexts?
Attachment #8888391 - Flags: review?(cam) → review+
Comment on attachment 8888496 [details]
Bug 1366721: Update reftest expectations.

https://reviewboard.mozilla.org/r/159462/#review165102

Might be worth adding a reftest based on the button test case from bug 1382222?
Attachment #8888496 - Flags: review?(cam) → review+
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/6ce98d80d882
Switch all the APIs in ServoStyleSet to use ServoStyleContext. r=heycam
https://hg.mozilla.org/integration/autoland/rev/346f57883936
Restyle additional style contexts in ServoRestyleManager. r=heycam
https://hg.mozilla.org/integration/autoland/rev/e7c0f783e4fc
Update reftest expectations. r=heycam
(In reply to Cameron McCormack (:heycam) from comment #2)
> Once this is fixed, we should try the test case from bug 1376406 to ensure
> that its Servo-side fix in bug 1379865 had the desired effect.

Just checked, and that test still works.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: