1447827 - Unify RestyleManager and ServoRestyleManager.

Status: RESOLVED FIXED

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

Description

[Emilio Cobos Álvarez (:emilio)]

We can do it now that GeckoRestyleManager is gone.

Comment 1

[Jet Villegas (inactive)]

[Triage 2018/03/23 - P3]

Comment 2

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1447827: Move members from ServoRestyleManager to RestyleManager.

This leaves ServoRestyleManager as just a typedef.

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

Comment 3

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1447827: Move ServoRestyleManager.cpp into RestyleManager.cpp.

Only moving implementation files, and s/ServoRestyleManager/RestyleManager/,
plus some indentation fixes.

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

Comment 4

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1447827: Remove ServoRestyleManager.

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

Comment 5

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1447827: Remove unused hover generation.

Not worth to keep the dead code, even if we may end up having to do something
about it in bug 1376082.

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

Comment 6

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1447827: Rename the reparenting stuff.

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

Comment 7

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8965526 [details]
Bug 1447827: Move members from ServoRestyleManager to RestyleManager.

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

Comment 8

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8965527 [details]
Bug 1447827: Move ServoRestyleManager.cpp into RestyleManager.cpp.

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

Comment 9

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8965528 [details]
Bug 1447827: Remove ServoRestyleManager.

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

Comment 10

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8965526 [details]
Bug 1447827: Move members from ServoRestyleManager to RestyleManager.

https://reviewboard.mozilla.org/r/234318/#review239904

r=me with comments addressed.

::: layout/base/RestyleManager.h:29
(Diff revision 2)
> +class nsStyleChangeList;
>  class nsStyleChangeList;
>  
>  namespace mozilla {
>  
>  class EventStates;

You probably should still add forward declaration for `ServoStyleSet` (that you are removing from ServoRestyleManager). It doesn't seem to me this is defined in any headers this includes.

::: layout/base/RestyleManager.h:346
(Diff revision 2)
> -  inline void ProcessPendingRestyles();
> -  inline void ContentStateChanged(nsIContent* aContent,
> -                                  EventStates aStateMask);
> -  inline void AttributeWillChange(dom::Element* aElement,
> +
> +  void ProcessPendingRestyles();
> +  void ProcessAllPendingAttributeAndStateInvalidations();
> +
> +  void ContentStateChanged(nsIContent* aContent, EventStates aStateMask);
> +  void AttributeWillChange(Element* aElement,

`dom::`?

::: layout/base/RestyleManager.h:457
(Diff revision 2)
>      MOZ_ASSERT(!mAnimationsWithDestroyedFrame,
>                 "leaving dangling pointers from AnimationsWithDestroyedFrame");

The destructor of the old `ServoRestyleManager` asserts `!mReentrantChanges`, maybe you should add it here?

Comment 11

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8965527 [details]
Bug 1447827: Move ServoRestyleManager.cpp into RestyleManager.cpp.

https://reviewboard.mozilla.org/r/234320/#review239906

Comment 12

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8965528 [details]
Bug 1447827: Remove ServoRestyleManager.

https://reviewboard.mozilla.org/r/234322/#review239908

Comment 13

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8965526 [details]
Bug 1447827: Move members from ServoRestyleManager to RestyleManager.

https://reviewboard.mozilla.org/r/234318/#review239904

> You probably should still add forward declaration for `ServoStyleSet` (that you are removing from ServoRestyleManager). It doesn't seem to me this is defined in any headers this includes.

Good point, will fix.

> `dom::`?

There's a `typedef mozilla::dom::Element Element;` at the beginning of RestyleManager.

> The destructor of the old `ServoRestyleManager` asserts `!mReentrantChanges`, maybe you should add it here?

Yup, indeed.

Comment 14

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8965531 [details]
Bug 1447827: Remove unused hover generation.

https://reviewboard.mozilla.org/r/234328/#review239916

Comment 15

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8965532 [details]
Bug 1447827: Rename the reparenting stuff.

https://reviewboard.mozilla.org/r/234330/#review239918

::: layout/base/RestyleManager.cpp:3424
(Diff revision 1)
>      return;
>    }
>  
>    bool isElement = aFrame->GetContent()->IsElement();
>  
> -  // We probably don't want to initiate transitions from
> +  // We probably don't want to initiate transitions from ReparentComputedStyle,

ReparentComputedStyleForFirstLetter?

::: layout/base/RestyleManager.cpp:3473
(Diff revision 1)
>      }
>    }
>    ComputedStyle* layoutParent = providerFrame->Style();
>  
>    RefPtr<ComputedStyle> newStyle =
>      aStyleSet.ReparentComputedStyle(oldStyle,

You may also want to rename `ServoStyleSet::ReparentComputedStyle`?

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/750575e7a835
Move members from ServoRestyleManager to RestyleManager. r=xidorn
https://hg.mozilla.org/integration/autoland/rev/03d83fe60bb9
Move ServoRestyleManager.cpp into RestyleManager.cpp. r=xidorn
https://hg.mozilla.org/integration/autoland/rev/a32957ee77a6
Remove ServoRestyleManager. r=xidorn
https://hg.mozilla.org/integration/autoland/rev/533fdeb7d3bb
Remove unused hover generation. r=xidorn
https://hg.mozilla.org/integration/autoland/rev/99569a0d505e
Rename the reparenting stuff. r=xidorn

Comment 17

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/750575e7a835
https://hg.mozilla.org/mozilla-central/rev/03d83fe60bb9
https://hg.mozilla.org/mozilla-central/rev/a32957ee77a6
https://hg.mozilla.org/mozilla-central/rev/533fdeb7d3bb
https://hg.mozilla.org/mozilla-central/rev/99569a0d505e