Closed Bug 1248864 Opened 4 years ago Closed 4 years ago

support multiple restyle manager backends

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla47
Tracking Status
firefox47 --- fixed

People

(Reporter: heycam, Assigned: heycam)

Details

Attachments

(3 files)

This is similar to bug 1244068 and bug 1244074 but for RestyleManager.
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".
Assignee: nobody → cam
Status: NEW → ASSIGNED
Attachment #8720239 - Flags: review?(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 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.)
Attachment #8720239 - Flags: review?(dholbert) → review+
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.
Attachment #8720240 - Flags: review?(dholbert) → review+
(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 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)?
Attachment #8720241 - Flags: review?(dholbert) → review+
You need to log in before you can comment on or make changes to this bug.