Closed
Bug 1248864
Opened 8 years ago
Closed 8 years ago
support multiple restyle manager backends
Categories
(Core :: CSS Parsing and Computation, defect)
Core
CSS Parsing and Computation
Tracking
()
RESOLVED
FIXED
mozilla47
Tracking | Status | |
---|---|---|
firefox47 | --- | fixed |
People
(Reporter: heycam, Assigned: heycam)
Details
Attachments
(3 files)
19.34 KB,
patch
|
dholbert
:
review+
|
Details | Diff | Splinter Review |
19.10 KB,
patch
|
dholbert
:
review+
|
Details | Diff | Splinter Review |
48.33 KB,
patch
|
dholbert
:
review+
|
Details | Diff | Splinter Review |
This is similar to bug 1244068 and bug 1244074 but for RestyleManager.
Assignee | ||
Comment 1•8 years ago
|
||
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 | ||
Comment 2•8 years ago
|
||
Attachment #8720240 -
Flags: review?(dholbert)
Assignee | ||
Comment 3•8 years ago
|
||
Attachment #8720241 -
Flags: review?(dholbert)
Assignee | ||
Comment 4•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5ab899bf4bfc
Comment 5•8 years ago
|
||
(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•8 years ago
|
||
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 7•8 years ago
|
||
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+
Comment 8•8 years ago
|
||
(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•8 years ago
|
||
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+
Comment 10•8 years ago
|
||
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•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/12e4449dd3d1 https://hg.mozilla.org/mozilla-central/rev/f3ab6216d197 https://hg.mozilla.org/mozilla-central/rev/06bc3102b900
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
You need to log in
before you can comment on or make changes to this bug.
Description
•