1248864 - support multiple restyle manager backends

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Cameron McCormack (:heycam)]

This is similar to bug 1244068 and bug 1244074 but for RestyleManager.

Comment 1

[Cameron McCormack (:heycam)]

Attachment: Part 1: Move RestyleManager.h to EXPORTS.mozilla.

RestyleManager is used outside of layout/ so I'm not sure why it's not in EXPORTS.mozilla.  Let's put it in there so that we can #include it as  "mozilla/RestyleManager.h".

Comment 2

[Cameron McCormack (:heycam)]

Attachment: Part 2: Add skeleton ServoRestyleManager and a RestyleManagerHandle smart pointer.

Comment 3

[Cameron McCormack (:heycam)]

Attachment: Part 3: Use RestyleManagerHandle instead of concrete restyle manager class.

Comment 4

[Cameron McCormack (:heycam)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5ab899bf4bfc

Comment 5

[Daniel Holbert [:dholbert]]

(Heads up: I won't be able to get to this today, and I'm on PTO for the rest of this week, but I'll take a look at this first thing next week.)

Comment 6

[Daniel Holbert [:dholbert]]

Comment on attachment 8720239 [details] [diff] [review]
Part 1: Move RestyleManager.h to EXPORTS.mozilla.

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

::: layout/base/RestyleManager.cpp
@@ +12,2 @@
>  #include "mozilla/EventStates.h"
> +#include "mozilla/RestyleManager.h"

[you're moving the RestyleManager.h include down a bit here.]

By convention, we try to ensure that Foo.cpp includes Foo.h as its very first include, as a way of ensuring that Foo.h actually includes or forward-declares everything that it needs. (This convention doesn't 100% ensure this, thanks to unified builds, but it helps.)

This convention is relevant to this patch-chunk, because here you're moving RestyleManager.cpp's include for RestyleManager.h.

So: please don't move the RestyleManager.h include down here -- if anything, please move it *up* to put it above <algorithm>. (Or you can leave it in its original spot if you prefer -- e.g. if you're worried about bustage risk from moving it up.)

Comment 7

[Daniel Holbert [:dholbert]]

Comment on attachment 8720240 [details] [diff] [review]
Part 2: Add skeleton ServoRestyleManager and a RestyleManagerHandle smart pointer.

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

::: layout/base/RestyleManagerHandle.h
@@ +95,5 @@
> +    const ServoRestyleManager* GetAsServo() const { return IsServo() ? AsServo() : nullptr; }
> +
> +    // These inline methods are defined in RestyleManagerHandleInlines.h.
> +    inline MozExternalRefCountType AddRef();
> +    inline MozExternalRefCountType Release();

Do these AddRef/Release methods actually get exercised anywhere? If not, consider removing them (and the refcounting macro in ServoRestyleManager) until/unless they're needed...

Unlike bug 1244074, we don't have HandleRefPtr here, so I'm not sure what code (if any code) is expected to use these AddRef/Release methods on this handle class.

@@ +144,5 @@
> +    uintptr_t mValue;
> +  };
> +
> +  RestyleManagerHandle(decltype(nullptr) = nullptr) { mPtr.mValue = 0; }
> +  RestyleManagerHandle(const RestyleManagerHandle& aOth) { mPtr.mValue = aOth.mPtr.mValue; }

This line's too long (92 characters) -- please rewrap.

@@ +146,5 @@
> +
> +  RestyleManagerHandle(decltype(nullptr) = nullptr) { mPtr.mValue = 0; }
> +  RestyleManagerHandle(const RestyleManagerHandle& aOth) { mPtr.mValue = aOth.mPtr.mValue; }
> +  RestyleManagerHandle(RestyleManager* aSet) { *this = aSet; }
> +  RestyleManagerHandle(ServoRestyleManager* aSet) { *this = aSet; }

"aSet" seems likely copypaste error from StyleSet code.

I think this wants to be "aManager" or something like that?

Same goes for a bunch of code below this, which has e.g. "RestyleManager* aStyleSet".  Please search through this file for "Set" and "StyleSet" to be sure you've caught all of these.

@@ +148,5 @@
> +  RestyleManagerHandle(const RestyleManagerHandle& aOth) { mPtr.mValue = aOth.mPtr.mValue; }
> +  RestyleManagerHandle(RestyleManager* aSet) { *this = aSet; }
> +  RestyleManagerHandle(ServoRestyleManager* aSet) { *this = aSet; }
> +
> +  RestyleManagerHandle& operator=(decltype(nullptr)) { mPtr.mValue = 0; return *this; }

This line's too long (87 characters) -- please rewrap.

::: layout/base/RestyleManagerHandleInlines.h
@@ +41,5 @@
> +
> +void
> +RestyleManagerHandle::Ptr::PostRestyleEvent(dom::Element* aElement,
> +                                       nsRestyleHint aRestyleHint,
> +                                       nsChangeHint aMinChangeHint)

Indentation is off here, as well as every other multi-line function signature in this file. Please adjust.

Comment 8

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #7)
> Do these AddRef/Release methods actually get exercised [...]
> Unlike bug 1244074, we don't have HandleRefPtr here

Sorry, disregard this comment -- I missed the HandleRefPtr usage, but I see it now.

Comment 9

[Daniel Holbert [:dholbert]]

Comment on attachment 8720241 [details] [diff] [review]
Part 3: Use RestyleManagerHandle instead of concrete restyle manager class.

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

r=me with nits addressed as you see fit:

::: layout/base/nsCSSFrameConstructor.cpp
@@ +1554,5 @@
>  
> +  // stylo: ServoRestyleManager does not need to be notified of frames being
> +  // destroyed.
> +  if (RestyleManager()->IsGecko()) {
> +    RestyleManager()->AsGecko()->NotifyDestroyingFrame(aFrame);

This RestyleManager() call (on nsCSSFrameConstructor) is cheap, but it's not free -- it's a multi-step lookup.  So, probably better to avoid back-to-back calls to it here.

Consider rewriting the above code as something like:
  RestyleManager* geckoRM = RestyleManager()->GetAsGecko();
  if (geckoRM) {
    geckoRM->NotifyDestroyingFrame(aFrame);

@@ +1819,5 @@
> +  // does it probably won't need to track reframed style contexts to start
> +  // transitions correctly.
> +  if (RestyleManager()->IsGecko()) {
> +    RestyleManager::ReframingStyleContexts* rsc =
> +      RestyleManager()->AsGecko()->GetReframingStyleContexts();

As above, this code ends up with two back-to-back RestyleManager() calls -- consider collapsing with a single call to RestyleManager()->GetAsGecko()

@@ +4937,5 @@
> +  // it probably won't need to track reframed style contexts to start
> +  // transitions correctly.
> +  if (RestyleManager()->IsGecko()) {
> +    RestyleManager::ReframingStyleContexts* rsc =
> +      RestyleManager()->AsGecko()->GetReframingStyleContexts();

Same here.

::: layout/base/nsPresShell.cpp
@@ +9267,5 @@
>            ++mChangeNestCount;
> +          RestyleManagerHandle restyleManager = mPresContext->RestyleManager();
> +          if (restyleManager->IsServo()) {
> +            MOZ_CRASH("stylo: PresShell::RecreateFramesFor not implemented for "
> +                      "Servo-backed style system");

Copypaste error in MOZ_CRASH text here, I think -- we're in PresShell::Observe, not RecreateFramesFor.

 So that's probably what you want to be mentioning here (Observe)?

Comment 10

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/12e4449dd3d1
https://hg.mozilla.org/integration/mozilla-inbound/rev/f3ab6216d197
https://hg.mozilla.org/integration/mozilla-inbound/rev/06bc3102b900

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/12e4449dd3d1
https://hg.mozilla.org/mozilla-central/rev/f3ab6216d197
https://hg.mozilla.org/mozilla-central/rev/06bc3102b900