1244068 - support multiple style set backends

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Cameron McCormack (:heycam)]

In this bug I'm generalizing interactions with the style set so that we can support both nsStyleSets (the existing Gecko-implemented style set) and a new ServoStyleSet (which will use Servo's Stylist).  ServoStyleSet's method will initially just be a bunch of "not implemented" assertions.

Initially I tried introducing an interface and making most methods on nsStyleSet virtual but I ran into small performance regressions.  The approach I'm taking now is to replace most uses of nsStyleSet pointers with a new StyleSetHandle, which encapsulates a pointer to either an nsStyleSet or a ServoStyleSet and packing that into a single pointer value using the low-bit-set-or-not trick that we use in Declaration to store either an nsHTMLCSSStyleSheet or a Rule.  The ability to store a ServoStyleSet pointer in the handle is in an |#ifdef MOZ_STYLO| block, so when that's not set (as it isn't by default) this should all optimize down to effectively passing an nsStyleSet* around like we currently do.

In certain classes we can assume a StyleSetHandle's pointer is to an nsStyleSet, since the decision to use a Gecko-backed or Servo-backed style implementation will be for all of the style set, style sheets, restyle manager, etc.  For example in RestyleManager we just unconditionally call ->AsGecko() on the StyleSheetHandle, since we'll eventually have a different class playing RestyleManager's role when using the Servo-backed style system.

There are a bunch of cases where we call ->AsGecko() since we're either (a) not worrying about supporting certain features in the Servo-backed style system, or (b) I haven't added the right interfaces to StyleSetHandle/ServoStyleSet yet, and I've added XXXheycam comments pointing most of these out.

I had to split StyleSetHandle's forwarding methods into a separate StyleSetHandleInlines.h file due to mutual header inclusion issues.

A later bug will actually create ServoStyleSets in certain situations (in nsDocumentViewer::CreateStyleSet).


Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3b3b15c07fe5

I tried doing a Talos run but I'm not sure perfherder is picking up the right jobs -- the try run is Linux64 PGO, though due to the way you have to request that on try (through a mozconfig option) it doesn't show up as PGO jobs.  As a result the numbers look quite wrong:

https://treeherder.mozilla.org/perf.html#/compare?originalProject=mozilla-central&originalRevision=a1b94785b484&newProject=try&newRevision=3b3b15c07fe5&framework=1

Let me know if you know how to make that compare to the PGO jobs on mozilla-central.

Comment 1

[Cameron McCormack (:heycam)]

Attachment: Part 1: Add enum to represent the style system backend type.

Comment 2

[Cameron McCormack (:heycam)]

Attachment: Part 2: Add skeleton ServoStyleSet and a StyleSetHandle smart pointer.

Comment 3

[Cameron McCormack (:heycam)]

Attachment: Part 3: Factor out nsStyleSet getting in RestyleManager/ElementRestyler.

Comment 4

[Cameron McCormack (:heycam)]

Attachment: Part 4: Use StyleSetHandle instead of concrete style set class in most places.

Comment 5

[Cameron McCormack (:heycam)]

OK, here is a proper perfheder comparison:

https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=0c2e56525db5&newProject=try&newRevision=48c846cdccdc&framework=1

The memory usage regressions are something to look into, but the perf numbers look good.

Comment 6

[Cameron McCormack (:heycam)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6ff9f6bc37d6 shows there's a leak, which is probably caused by a CSSStyleSheet living too long.  I'll see if I get a chance to fix this over the coming week but hopefully it shouldn't prevent the patches being reviewed.

Comment 7

[Cameron McCormack (:heycam)]

New patch in bug 1244074 fixes that leak:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0c287a021eb8&group_state=expanded

Comment 8

[Daniel Holbert [:dholbert]]

Sorry for the wait here. Looking at this patch-stack now & tomorrow.

Comment 9

[Daniel Holbert [:dholbert]]

Comment on attachment 8713565 [details] [diff] [review]
Part 1: Add enum to represent the style system backend type.

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

r=me on part 1, with nit addressed:

::: layout/style/StyleImplementation.h
@@ +11,5 @@
> +
> +/**
> + * Enumeration that represents one of the two supported style system backends.
> + */
> +enum class StyleImplementation : int

I'd suggest renaming this enum class -- "StyleImplementation" seems a bit ambiguous to me. We have a lot of classes with names like "FooImpl", and these classes generally are concrete classes representing an implementation of some interface, rather than an enum representing a type of implementation.  (and "StyleImplementation" sounds like it's one of these)

Maybe "StyleBackendType" or "StyleImplType" would be clearer?

Comment 10

[Cameron McCormack (:heycam)]

StyleBackend sounds OK to me.

Comment 11

[Daniel Holbert [:dholbert]]

Comment on attachment 8713566 [details] [diff] [review]
Part 2: Add skeleton ServoStyleSet and a StyleSetHandle smart pointer.

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

::: layout/style/ServoStyleSet.cpp
@@ +137,5 @@
> +}
> +
> +CSSStyleSheet*
> +ServoStyleSet::StyleSheetAt(SheetType aType,
> +                         int32_t aIndex) const

reindent

::: layout/style/ServoStyleSet.h
@@ +7,5 @@
> +#ifndef mozilla_ServoStyleSet_h
> +#define mozilla_ServoStyleSet_h
> +
> +#include "mozilla/RefPtr.h"
> +#include "mozilla/ServoStyleSet.h"

I think this #include wants to go away, right? This seems to be ServoStyleSet.h trying to #include itself.

::: layout/style/StyleSetHandle.h
@@ +10,5 @@
> +#include "mozilla/RefPtr.h"
> +#include "mozilla/SheetType.h"
> +#include "mozilla/StyleImplementation.h"
> +#include "nsCSSPseudoElements.h"
> +#include "nsIAtom.h"

Can we just forward-declare nsIAtom, and get rid of this #include?  I think the only atom-related thing in this file is a nsIAtom* in a function-declaration.

@@ +33,5 @@
> +#ifdef MOZ_STYLO
> +#define SERVO_BIT_IF_ENABLED SERVO_BIT
> +#else
> +#define SERVO_BIT_IF_ENABLED 0x0
> +#endif

What do we gain from the SERVO_BIT_IF_ENABLED macro dance here?

It looks like SERVO_BIT_IF_ENABLED is only used in one spot -- the "IsServo()" impl just below this.  I'm guessing the goal is to help us return false more quickly there, because when this macro is 0, the compiler can see that mValue & 0 = false always and do some optimizing.

That does seem like a benefit, but this seems like a roundabout way of achieving that (and it creates confusion from having SERVO_BIT and SERVO_BIT_IF_ENABLED both in play, alongside each other, with both being used in different bits of code).  I'd prefer something simpler/clearer, like just implementing IsServo() with its own explicit #ifdef MOZ_STYLO and a "return false" in the #else clause.  Then SERVO_BIT_IF_ENABLED can just go away.  How does that sound? Am I misunderstanding the point of this macro?

@@ +42,5 @@
> + */
> +class StyleSetHandle
> +{
> +public:
> +  class Ptr

Please add a comment explaining what this "Ptr" helper-class exists (and how it differs from StyleSetHandle).  It's unclear (at this point in the code at least) what the division of labor is between StyleSetHandle & Ptr, & why we have two nested pointer-ish classes instead of one.

@@ +50,5 @@
> +
> +    bool IsGecko() const { return !IsServo(); }
> +    bool IsServo() const
> +    {
> +      MOZ_ASSERT(mValue);

Why the MOZ_ASSERT(mValue) here?  Can Ptr not represent a null pointer?

(Or, maybe you're conceptually trying to treat IsServo/IsGecko calls on a null pointer kind of like a null-deref, at least in debug builds? (hence the fatal assertion)  That makes some sense -- but in that case, please add an assertion message here, to make that clearer and to make it easier to diagnose/debug what's wrong if & when this assertion fails.)

@@ +55,5 @@
> +      return mValue & SERVO_BIT_IF_ENABLED;
> +    }
> +
> +    StyleImplementation Implementation() const
> +    {

Similar to comment 9 -- this "Implementation()" method probably needs renaming. Right now it sounds like it returns an actual implementation of something (e.g. on nsCSSParser, I'd expect a method called Implementation() to return its mImpl pointer).

This method probably should to be renamed to ImplType() or BackendType() or similar, depending on your thoughts on potential renames in comment 9.

@@ +83,5 @@
> +
> +    const ServoStyleSet* AsServo() const
> +    {
> +      MOZ_ASSERT(IsServo());
> +      return reinterpret_cast<const ServoStyleSet*>(mValue);

Seems like this should just return...
  const_cast<Ptr*>(this)->AsServo();
...to match the pattern in your const-flavored AsGecko() method directly above this.

(No need to assert here either, in that case; yo ucan just rely on the call to the other AsServo() method to do the assertion for you.)

@@ +147,5 @@
> +
> +  private:
> +    // Stores a pointer to an nsStyleSet or a ServoStyleSet.  The least
> +    // significant bit is 0 for the former, 1 for the latter.
> +    uintptr_t mValue;

Please add a bit of an explanation to the code comment, to justify why it's safe to co-opt the least significant bit here.  (This seems pretty hand-wavy/magic right now; I suppose you're copying this trick from 'Declaration', which doesn't have much of an explanation. It probably should add an explanation, too; but for now we should at least explain this more clearly in new code.)

Also: please add assertions to verify that our co-opting is valid -- in particular, assert that this bit is *never* set in any value that we're given to store.  So e.g. both of the operator= implementations, just below this, should probably assert that (aStyleSet | SERVO_BIT) == 0, with a helpful assertion-message, before they do anything else.

@@ +176,5 @@
> +
> +  // Make StyleSetHandle usable in boolean contexts.
> +#ifdef MOZ_HAVE_REF_QUALIFIERS
> +  operator void*() const & { return reinterpret_cast<void*>(mPtr.mValue); }
> +  operator void*() const && = delete;

Why do we need "void*" cast operators for this type? (I understand why we want the bool cast operators that are defined below this. But I don't see why we want void* cast operators.  That seems to be exposing our internals in a way that's a bit sketchy.)

Note that the void* returned by this operator does include our SERVO_BIT, since it's just directly taking mPtr.mValue. Depending on how the void* is used, that could be bad.

And why are the bool operators (just below this) conditioned on MOZ_HAVE_REF_QUALIFIERS?  Can't we have those unconditionally?

::: layout/style/StyleSetHandleInlines.h
@@ +9,5 @@
> +
> +#include "mozilla/ServoStyleSet.h"
> +#include "nsStyleSet.h"
> +
> +using namespace mozilla;

This should be "namespace mozilla {", and should go after the #define FORWARD.

At least, that's what we do in these two other mozilla-namespaced "Inlines" header-files:
http://mxr.mozilla.org/mozilla-central/source/dom/base/ElementInlines.h
http://mxr.mozilla.org/mozilla-central/source/dom/base/NodeInfoInlines.h

...and the Mozilla coding style guide says: No "using" statements are allowed in header files, except inside class definitions or functions.
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Namespaces

@@ +79,5 @@
> +
> +already_AddRefed<nsStyleContext>
> +StyleSetHandle::Ptr::ResolveStyleFor(dom::Element* aElement,
> +                nsStyleContext* aParentContext,
> +                TreeMatchContext& aTreeMatchContext)

Reindent the parameters here, to put them inside the "(".

@@ +94,5 @@
> +already_AddRefed<nsStyleContext>
> +StyleSetHandle::Ptr::ResolvePseudoElementStyle(dom::Element* aParentElement,
> +                          nsCSSPseudoElements::Type aType,
> +                          nsStyleContext* aParentContext,
> +                          dom::Element* aPseudoElement)

Indentation seems odd/arbitrary here.  You can indent all the args to be to the right of the open-paren, without going over 80 characters (barely).

@@ +102,5 @@
> +}
> +
> +// aFlags is an nsStyleSet flags bitfield
> +already_AddRefed<nsStyleContext>
> +StyleSetHandle::Ptr::ResolveAnonymousBoxStyle(nsIAtom* aPseudoTag, nsStyleContext* aParentContext,

This line is too long -- add a newline before nsStyleContext.

@@ +103,5 @@
> +
> +// aFlags is an nsStyleSet flags bitfield
> +already_AddRefed<nsStyleContext>
> +StyleSetHandle::Ptr::ResolveAnonymousBoxStyle(nsIAtom* aPseudoTag, nsStyleContext* aParentContext,
> +                         uint32_t aFlags)

And reindent this line to be inside the "(".

@@ +137,5 @@
> +
> +nsresult
> +StyleSetHandle::Ptr::InsertStyleSheetBefore(SheetType aType,
> +                                CSSStyleSheet* aNewSheet,
> +                                CSSStyleSheet* aReferenceSheet)

Reindent params.

@@ +170,5 @@
> +// check whether there is ::before/::after style for an element
> +already_AddRefed<nsStyleContext>
> +StyleSetHandle::Ptr::ProbePseudoElementStyle(dom::Element* aParentElement,
> +                        nsCSSPseudoElements::Type aType,
> +                        nsStyleContext* aParentContext)

Reindent params.

@@ +180,5 @@
> +StyleSetHandle::Ptr::ProbePseudoElementStyle(dom::Element* aParentElement,
> +                        nsCSSPseudoElements::Type aType,
> +                        nsStyleContext* aParentContext,
> +                        TreeMatchContext& aTreeMatchContext,
> +                        dom::Element* aPseudoElement)

Maybe reindent params (aTreeMatchContext goes just over 80 characters but the rest of the lines are OK).

Comment 12

[Daniel Holbert [:dholbert]]

Comment on attachment 8713567 [details] [diff] [review]
Part 3: Factor out nsStyleSet getting in RestyleManager/ElementRestyler.

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

r=me on part 3; just one nit:

::: layout/base/RestyleManager.cpp
@@ +44,5 @@
>  #include "nsSMILAnimationController.h"
>  #include "nsCSSRuleProcessor.h"
>  #include "ChildIterator.h"
>  #include "Layers.h"
> +#include "nsStyleSet.h"

This #include list isn't really in order, but there's probably a more logical place to insert this, in semi-sorted order -- maybe just before #include "nsStyleUtil.h" (which also happens to be pretty close to the one other nsStyleXYZ include)

Comment 13

[Daniel Holbert [:dholbert]]

(In reply to Cameron McCormack (:heycam) (busy Jan 30 – Feb 6) (away Feb 8–9) from comment #10)
> StyleBackend sounds OK to me.

Why not StyleBackendType? :D

I could live with StyleBackend (I prefer it to StyleImplementation), but I do think having something "enum-flavored" in the name makes things a little clearer.

Also -- if we end up having myStyleSetHandle->Backend() as an accessor (instead of Implementation()), I think that'll still be pretty vague & easy to misinterpret as "give me a handle to the actual backend". I think BackendType() would be a much clearer accessor-name, and for naming consistency, seems like StyleBackendType should be the typename...

But anyway, if you don't find this compelling, I can live with StyleBackend as the typename. :)  (though I'd still push for the accessor to be named BackendType or something like that.)

Comment 14

[Cameron McCormack (:heycam)]

(In reply to Daniel Holbert [:dholbert] from comment #13)
> Why not StyleBackendType? :D
> 
> I could live with StyleBackend (I prefer it to StyleImplementation), but I
> do think having something "enum-flavored" in the name makes things a little
> clearer.

I feel like StyleBackendType would describe a class of backend types, while in reality we're only going to have two, specifically named backends.

> Also -- if we end up having myStyleSetHandle->Backend() as an accessor
> (instead of Implementation()), I think that'll still be pretty vague & easy
> to misinterpret as "give me a handle to the actual backend". I think
> BackendType() would be a much clearer accessor-name, and for naming
> consistency, seems like StyleBackendType should be the typename...

OK, I guess so. :-)

> But anyway, if you don't find this compelling, I can live with StyleBackend
> as the typename. :)  (though I'd still push for the accessor to be named
> BackendType or something like that.)

I'll go for StyleBackendType then!  Thanks for the reviews so far.

Comment 15

[Daniel Holbert [:dholbert]]

Comment on attachment 8713568 [details] [diff] [review]
Part 4: Use StyleSetHandle instead of concrete style set class in most places.

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

Initial notes on part 4 (going to lunch, figured I'd post what I've got so far):

::: dom/base/nsIDocument.h
@@ +81,5 @@
>  class nsScriptLoader;
>  class nsSMILAnimationController;
> +namespace mozilla {
> +class StyleSetHandle;
> +} // namespace mozilla

No need to create a new "namespace mozilla {...}" block here just for this one forward-decl.  This should join the already-existing "namespace mozilla {" chunk, about 15 lines down.

::: dom/canvas/CanvasRenderingContext2D.cpp
@@ +2217,5 @@
> +    // XXXheycam ServoStyleSets do not support resolving style from a list of
> +    // rules yet.
> +    error.Throw(NS_ERROR_FAILURE);
> +    return nullptr;
> +  }

I don't think we need this special early-return, really.

Can't we just let servo's ResolveStyleForRules() method return nullptr (to indicate failure/non-support), which will then make us take the already-existing failure case further down:
 if (!result) {
   error.Throw(NS_ERROR_FAILURE);
   return nullptr;
 }

@@ +2260,5 @@
> +    // XXXheycam ServoStyleSets do not support resolving style from a list of
> +    // rules yet.
> +    error.Throw(NS_ERROR_FAILURE);
> +    return nullptr;
> +  }

Above question applies here, too.

Though here, it looks like we don't null-check the result of the ResolveStyleForRules() call (the function that you're trying to avoid calling here). Perhaps we should null-check that? [and that's how Servo would trigger failure here for the moment])

Note that even Gecko's ResolveStyleForRules implementation can return nullptr (during shutdown at least), so that does seem like something that'd make sense to allow for here, even in the absence of a stub Servo implementation.

SO: Maybe this error-handling block that I'm quoting should morph into a null-check on the result of ResolveStyleForRules?

@@ +2336,5 @@
> +    // XXXheycam ServoStyleSets do not support resolving style from a list of
> +    // rules yet.
> +    error.Throw(NS_ERROR_FAILURE);
> +    return nullptr;
> +  }

Same here. (Probably worth morphing this into a null-check on the result of ResolveStyleForRules?)

Comment 16

[Daniel Holbert [:dholbert]]

Comment on attachment 8713568 [details] [diff] [review]
Part 4: Use StyleSetHandle instead of concrete style set class in most places.

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

r- on part 4 for now, mainly because there are a lot of unwarranted or unjustified AsGecko() invocations (see below), and it's worth another round of review to sanity-check whatever fallback code and/or assertions you'd like to add to address these.

::: dom/html/ImageDocument.cpp
@@ +26,5 @@
>  #include "nsIPresShell.h"
>  #include "nsPresContext.h"
>  #include "nsStyleContext.h"
>  #include "nsAutoPtr.h"
> +#include "mozilla/StyleSetHandle.h"

I'll bet this #include should just be removed (instead of being upgraded from nsStyleSet.h).

At least, this #include is the only instance of the string "StyleSet" that I can find in this file.  So, I'll bet the old #include is vestigial, and just wants to be removed instead of being ported over to the new StyleSetHandle.

::: gfx/layers/apz/util/ActiveElementManager.cpp
@@ +166,5 @@
> +  if (!styleSet) {
> +    // XXXheycam ServoStyleSets don't support exposing whether we have
> +    // :active rules yet.
> +    return false;
> +  }

Similar to comment 15: I'm wondering if we can get rid of this special case. Seems like we should just make the servo HasStateDependentStyle() implementation immediately return false (and assert or warn, perhaps).

That would have the same effect as what you're doing here, I think, and it'd let us keep this code clean & abstract.

::: layout/base/RestyleManager.cpp
@@ +4977,5 @@
>  
>  nsStyleSet*
>  RestyleManager::StyleSet() const
>  {
> +  return mPresContext->StyleSet()->AsGecko();

This code seems to be promising to return non-null in two different ways -- its name (StyleSet) and the fact that it's using "AsGecko" instead of GetAsGecko (which implies that it knows that the StyleSetHandle is gecko-flavored).

But it's non-obvious to me how we know that the StyleSetHandle is gecko-flavored here.  (Maybe we only use RestyleManager with gecko-flavored StyleSets? not sure.)

If this gecko-flavoredness assumption is in fact valid, could you assert that mPresContext->StyleSet()->IsGecko() here, with an explanation for why we expect this to be true?

(The assertion itself will be semi-redundant, because we'll already be asserting inside of the AsGecko function.  But that assertion doesn't give us any sort of explanation for why we feel safe calling AsGecko -- and I'm hoping that the assertion you add here will have that sort of explanation in its assertion-message.)

@@ +4983,5 @@
>  
>  nsStyleSet*
>  ElementRestyler::StyleSet() const
>  {
> +  return mPresContext->StyleSet()->AsGecko();

Same here.

::: layout/base/nsDocumentViewer.cpp
@@ +2143,5 @@
>  }
>  
>  nsresult
>  nsDocumentViewer::CreateStyleSet(nsIDocument* aDocument,
> +                                   StyleSetHandle* aStyleSet)

Fix indentation (delete 2 spaces), while you're here.

(Also: it looks like this method always returns NS_OK, so its return value is useless.  I wonder if we can promote this StyleSetHandle outparam to be the direct return value, maybe in a followup?)

::: layout/base/nsPresShell.cpp
@@ +9705,5 @@
>    cx->SetVisibleArea(r);
>  
>    // Create a new presentation shell to view the document. Use the
>    // exact same style information that this document has.
> +  nsAutoPtr<nsStyleSet> newSet(CloneStyleSet(mStyleSet->AsGecko()));

The "AsGecko()" call here assumes that mStyleSet is gecko-flavored (calling the infallible AsGecko function).  Do we really have that guarantee?  The next chunk after this one uses the fallible "Get" version, so I'm guessing we should probably be using that here, too.

I'd expect we could just "return false" here for servo, if there's nothing better we can do at the moment. (We seem to return false on failure below this, at least, via a NS_ENSURE_TRUE call.)  And we'd probably want an XXXheycam comment on making us do something better than returning false eventually.

Or alternately, if you really are sure we're guaranteed to have a gecko-flavored styleset here, please add an assertion to that effect with an assertion-message stating why we're sure.

::: layout/base/nsPresShell.h
@@ +28,5 @@
>  #include "nsWeakReference.h"
>  #include "nsCRT.h"
>  #include "nsAutoPtr.h"
>  #include "nsIWidget.h"
> +#include "mozilla/StyleSetHandle.h"

Nit: ideally, this should be grouped with all the other mozilla/Foo.h #includes, further down in this #include list.

(This applies to pretty much all of the #include changes in this patch, really... Some of them are less of an issue because the modified #include list is already super disorganized. :) But in cases where it's currently relatively organized, it'd be nice to keep it organized & not add a mozilla/ include in the middle of a bunch of non-mozilla/ includes.)

If you'd like to do this reordering in a followup patch (so this patch stays as mostly a drop-in replacement), that's fine by me. But I think it does need fixing one way or another, for a good number of the files here, to avoid moving from organized --> disorganized.)

::: layout/printing/nsPrintEngine.cpp
@@ +126,5 @@
>  #include "nsIChannel.h"
>  #include "xpcpublic.h"
>  #include "nsVariant.h"
> +#include "nsStyleSet.h"
> +#include "mozilla/ServoStyleSet.h"

I don't think we need the ServoStyleSet include here.

We also don't need nsStyleSet.h, because StyleSetHandleInlines.h already includes it.

(We should be fine with just StyleSetHandle.h and its inlines file -- the #includes you're adding everywhere else.)

::: layout/printing/nsPrintObject.h
@@ +9,5 @@
>  
>  // Interfaces
>  #include "nsCOMPtr.h"
>  #include "nsIPresShell.h"
> +#include "mozilla/StyleSetHandle.h"

Pretty sure we can remove this include. There are no other mentions of "StyleSet" in this file.

::: layout/style/CSSRuleList.h
@@ +5,5 @@
>  
>  #ifndef mozilla_dom_CSSRuleList_h
>  #define mozilla_dom_CSSRuleList_h
>  
> +#include "mozilla/CSSStyleSheet.h"

I'm not clear why this new #include is needed -- this file only has one mention of "CSSStylesheet*" (as a return value). Maybe this #include really belongs in a .cpp file?  Or, if we do really need it here, then you can presumably drop the "class CSSStyleSheet;" forward-decl that's 6 lines below this #include.

::: layout/style/CounterStyleManager.cpp
@@ +2015,4 @@
>    nsCSSCounterStyleRule* rule =
> +    mPresContext->StyleSet()->IsGecko() ?
> +      mPresContext->StyleSet()->AsGecko()->CounterStyleRuleForName(aName) :
> +      nullptr;

The AsGecko line is a bit too long. Maybe shorten (and remove some repeated boilerplate) with a helper-var, like:
  StyleSetHandle styleSet =  mPresContext->StyleSet();
  nsCSSCounterStyleRule* rule = styleSet->IsGecko() ?
    styleSet->AsGecko()->...

@@ +2059,3 @@
>      nsCSSCounterStyleRule* newRule =
> +      mPresContext->StyleSet()->IsGecko() ?
> +        mPresContext->StyleSet()->AsGecko()->CounterStyleRuleForName(iter.Key()) :

same here.

::: layout/style/StyleAnimationValue.cpp
@@ +2624,5 @@
>    RefPtr<nsStyleContext> styleContext = LookupStyleContext(aTargetElement);
>    if (!styleContext) {
>      return false;
>    }
> +  nsStyleSet* styleSet = styleContext->PresContext()->StyleSet()->AsGecko();

Are we really guaranteed that the styleset IsGecko() here? (and that this AsGecko() call will succeed)

If so, please add an assertion. If not, this probably wants some sort of fallback servo codepath (maybe just returning false, with an XXXheycam for doing something better.)

::: layout/style/nsAnimationManager.cpp
@@ +555,5 @@
>                 "Keyframe rule has !important data");
>  
>      nsCOMArray<nsIStyleRule> rules;
>      rules.AppendObject(aKeyframeDeclaration);
> +    RefPtr<nsStyleContext> resultStrong = aPresContext->StyleSet()->AsGecko()->

My above AsGecko() concerns apply throughout this file as well. (4 chunks which use AsGecko, un-checked)

::: layout/style/nsComputedDOMStyle.cpp
@@ +504,5 @@
>  
>    if (aStyleType == eDefaultOnly) {
> +    if (styleSet->IsServo()) {
> +      NS_WARNING("ServoStyleSet cannot produce eDefaultOnly computed "
> +                 "style objects");

Perhaps this should be NS_NOTREACHED (nonfatal but counts as an assertion-failure for test runs), or MOZ_ASSERT_UNREACHABLE (fatal)?

This sounds like a never-expected-to-happen codepath, which means we probably should yell if it happens.  NS_WARNING seems better for stuff that's unimplemented-but-will-eventually-be-implemented.

::: layout/style/nsRuleNode.cpp
@@ +1533,5 @@
>       point.  In any case, we don't want to treat the root rulenode as
>       unused.  */
>    if (!IsRoot()) {
>      mParent->AddRef();
> +    aContext->StyleSet()->AsGecko()->RuleNodeUnused();

Again, if this AsGecko() usage is valid, it should come with a MOZ_ASSERT(IsGecko(), "justification for why we know we must be gecko");

That, or we should have a servo fallback codepath (even if it's just an XXX comment and a NS_WARNING or NS_NOTYETIMPLEMENTED).

(I'm guessing the AsGecko assumption is *not* valid here, since we have a case in the chunk below this one where we treat IsServo as a possibility.)

@@ +9800,5 @@
>    // wants to hold onto the root node and not worry about re-creating a
>    // rule walker if the root node is deleted.
>    if (!(mDependentBits & NS_RULE_NODE_GC_MARK) &&
>        // Skip this only if we're the *current* root and not an old one.
> +      !(IsRoot() && mPresContext->StyleSet()->AsGecko()->GetRuleTree() == this)) {

As above, this AsGecko() usage needs an assertion to justify it, or it needs to be generified.

::: layout/style/nsStyleContext.cpp
@@ +131,5 @@
>  {
>    NS_ASSERTION((nullptr == mChild) && (nullptr == mEmptyChild), "destructing context with children");
>  
>    nsPresContext *presContext = mRuleNode->PresContext();
> +  nsStyleSet* styleSet = presContext->PresShell()->StyleSet()->AsGecko();

As above, this AsGecko() needs to be removed, or justified with an assertion (if we're really sure it'll hold always), or softened to IsGecko or GetAsGecko with appropriate code in case of failure.

::: layout/style/nsStyleSet.h
@@ +533,5 @@
>  inline
>  void nsRuleNode::AddRef()
>  {
>    if (mRefCnt++ == 0 && !IsRoot()) {
> +    mPresContext->StyleSet()->AsGecko()->RuleNodeInUse();

As above, add an assertion to justify AsGecko usage (and in Release() below this, too).

::: layout/style/nsTransitionManager.cpp
@@ +315,5 @@
>    // but still inheriting from data that contains transitions that are
>    // not stopping or starting right now.
>    RefPtr<nsStyleContext> afterChangeStyle;
>    if (collection) {
> +    nsStyleSet* styleSet = mPresContext->StyleSet()->AsGecko();

As above, add an assertion to justify AsGecko usage.

::: layout/xul/tree/nsTreeStyleCache.cpp
@@ +78,5 @@
>    }
>    if (!result) {
>      // We missed the cache. Resolve this pseudo-style.
> +    // XXXheycam ServoStyleSets do not support XUL tree styles.
> +    RefPtr<nsStyleContext> newResult = aPresContext->StyleSet()->AsGecko()->

As above, this AsGecko usage needs an assertion to justify why it's a safe assumption.

(I'm unclear if the XXXheycam comment is saying "we can never get here with a servo backend", vs. "if we get here with a servo backend, we'll break but it's not a big deal. Please clarify, perhaps in the form of an assertion message if we really can assert here.)

Comment 17

[Daniel Holbert [:dholbert]]

Comment on attachment 8713566 [details] [diff] [review]
Part 2: Add skeleton ServoStyleSet and a StyleSetHandle smart pointer.

[setting r- on part 2, since there's enough that needs changing & that I've got questions about (in comment 11) that it definitely merits another round of review.]

Comment 18

[Cameron McCormack (:heycam)]

Thanks for all the reviews, Daniel.

(In reply to Daniel Holbert [:dholbert] from comment #11)
> @@ +33,5 @@
> > +#ifdef MOZ_STYLO
> > +#define SERVO_BIT_IF_ENABLED SERVO_BIT
> > +#else
> > +#define SERVO_BIT_IF_ENABLED 0x0
> > +#endif
> 
> What do we gain from the SERVO_BIT_IF_ENABLED macro dance here?
> 
> It looks like SERVO_BIT_IF_ENABLED is only used in one spot -- the
> "IsServo()" impl just below this.  I'm guessing the goal is to help us
> return false more quickly there, because when this macro is 0, the compiler
> can see that mValue & 0 = false always and do some optimizing.

Yes, I want the compiler to optimize out the check, and to limit the #ifdefs throughout the code to make it easier to read.

> That does seem like a benefit, but this seems like a roundabout way of
> achieving that (and it creates confusion from having SERVO_BIT and
> SERVO_BIT_IF_ENABLED both in play, alongside each other, with both being
> used in different bits of code).  I'd prefer something simpler/clearer, like
> just implementing IsServo() with its own explicit #ifdef MOZ_STYLO and a
> "return false" in the #else clause.  Then SERVO_BIT_IF_ENABLED can just go
> away.  How does that sound? Am I misunderstanding the point of this macro?

OK, that's fine.  You are not misunderstanding.

> @@ +42,5 @@
> > + */
> > +class StyleSetHandle
> > +{
> > +public:
> > +  class Ptr
> 
> Please add a comment explaining what this "Ptr" helper-class exists (and how
> it differs from StyleSetHandle).  It's unclear (at this point in the code at
> least) what the division of labor is between StyleSetHandle & Ptr, & why we
> have two nested pointer-ish classes instead of one.

You probably worked it out, but the reason it exists is so that StyleSetHandle can behave like a smart pointer, and have an operator-> that returns an object with these methods on it.  With the methods directly on StyleSetHandle, there wouldn't be a way to do myStyleSetHandle->ResolveStyleFor(...) and instead you would need to write myStyleSetHandle.ResolveStyleFor(...).  I felt that it was a smaller change (to the rest of the codebase) to have StyleSetHandles be more pointer-like than wrapper-object-like.

> @@ +50,5 @@
> > +
> > +    bool IsGecko() const { return !IsServo(); }
> > +    bool IsServo() const
> > +    {
> > +      MOZ_ASSERT(mValue);
> 
> Why the MOZ_ASSERT(mValue) here?  Can Ptr not represent a null pointer?
>
> (Or, maybe you're conceptually trying to treat IsServo/IsGecko calls on a
> null pointer kind of like a null-deref, at least in debug builds? (hence the
> fatal assertion)  That makes some sense -- but in that case, please add an
> assertion message here, to make that clearer and to make it easier to
> diagnose/debug what's wrong if & when this assertion fails.)

Yes, that's it.  I'll add a message to the assertion to say that it's effectively a null pointer dereference.

> @@ +176,5 @@
> > +
> > +  // Make StyleSetHandle usable in boolean contexts.
> > +#ifdef MOZ_HAVE_REF_QUALIFIERS
> > +  operator void*() const & { return reinterpret_cast<void*>(mPtr.mValue); }
> > +  operator void*() const && = delete;
> 
> Why do we need "void*" cast operators for this type? (I understand why we
> want the bool cast operators that are defined below this. But I don't see
> why we want void* cast operators.  That seems to be exposing our internals
> in a way that's a bit sketchy.)

I think you're right we can get rid of this one.  The one in the non-MOZ_HAVE_REF_QUALIFIERS one is still needed though, following the code in mfbt/RefPtr.h.  (Or so I thought...)

> And why are the bool operators (just below this) conditioned on
> MOZ_HAVE_REF_QUALIFIERS?  Can't we have those unconditionally?

So I am just following the pattern in RefPtr.h, and I don't know why the operator bool/operator! can't be just defined unconditionally.  I suspect that they are unnecessary in the non-MOZ_HAVE_REF_QUALIFIERS case in RefPtr, since the compiler will choose to first convert to T* then to bool.  But your comment above about not needing the operator void*s makes me think that we could take these operator bool/operator!s out of the #ifdef and that would let us to remove them.

Comment 19

[Cameron McCormack (:heycam)]

(In reply to Daniel Holbert [:dholbert] from comment #16)
> ::: gfx/layers/apz/util/ActiveElementManager.cpp
> @@ +166,5 @@
> > +  if (!styleSet) {
> > +    // XXXheycam ServoStyleSets don't support exposing whether we have
> > +    // :active rules yet.
> > +    return false;
> > +  }
> 
> Similar to comment 15: I'm wondering if we can get rid of this special case.
> Seems like we should just make the servo HasStateDependentStyle()
> implementation immediately return false (and assert or warn, perhaps).

OK, I can add HasStateDependentStyle to the StyleSetHandle "interface".

> ::: layout/base/RestyleManager.cpp
> @@ +4977,5 @@
> >  
> >  nsStyleSet*
> >  RestyleManager::StyleSet() const
> >  {
> > +  return mPresContext->StyleSet()->AsGecko();
> 
> This code seems to be promising to return non-null in two different ways --
> its name (StyleSet) and the fact that it's using "AsGecko" instead of
> GetAsGecko (which implies that it knows that the StyleSetHandle is
> gecko-flavored).
> 
> But it's non-obvious to me how we know that the StyleSetHandle is
> gecko-flavored here.  (Maybe we only use RestyleManager with gecko-flavored
> StyleSets? not sure.)

Yes, that's exactly right.  I'm sorry I didn't explain clearly before, but the current plan is to use this set of Gecko-flavoured objects all together: nsStyleSet, CSSStyleSheet, nsRuleNode, RestyleManager/ElementRestyler.  So they will always be able to ->AsGecko() any handles.  Similarly for the Servo-flavoured ones once we implement them.

> If this gecko-flavoredness assumption is in fact valid, could you assert
> that mPresContext->StyleSet()->IsGecko() here, with an explanation for why
> we expect this to be true?
> 
> (The assertion itself will be semi-redundant, because we'll already be
> asserting inside of the AsGecko function.  But that assertion doesn't give
> us any sort of explanation for why we feel safe calling AsGecko -- and I'm
> hoping that the assertion you add here will have that sort of explanation in
> its assertion-message.)

OK, I can add an additional assertion with a more useful message.

> ::: layout/base/nsPresShell.cpp
> @@ +9705,5 @@
> >    cx->SetVisibleArea(r);
> >  
> >    // Create a new presentation shell to view the document. Use the
> >    // exact same style information that this document has.
> > +  nsAutoPtr<nsStyleSet> newSet(CloneStyleSet(mStyleSet->AsGecko()));
> 
> The "AsGecko()" call here assumes that mStyleSet is gecko-flavored (calling
> the infallible AsGecko function).  Do we really have that guarantee?  The
> next chunk after this one uses the fallible "Get" version, so I'm guessing
> we should probably be using that here, too.
> 
> I'd expect we could just "return false" here for servo, if there's nothing
> better we can do at the moment. (We seem to return false on failure below
> this, at least, via a NS_ENSURE_TRUE call.)  And we'd probably want an
> XXXheycam comment on making us do something better than returning false
> eventually.

OK.  I'll add a check and a failing early return here until we work out how to implement CloneStyleSet more generally.  (This should only affect printing, FWIW.)

> ::: layout/style/StyleAnimationValue.cpp
> @@ +2624,5 @@
> >    RefPtr<nsStyleContext> styleContext = LookupStyleContext(aTargetElement);
> >    if (!styleContext) {
> >      return false;
> >    }
> > +  nsStyleSet* styleSet = styleContext->PresContext()->StyleSet()->AsGecko();
> 
> Are we really guaranteed that the styleset IsGecko() here? (and that this
> AsGecko() call will succeed)
> 
> If so, please add an assertion. If not, this probably wants some sort of
> fallback servo codepath (maybe just returning false, with an XXXheycam for
> doing something better.)

OK, I'll be more explicit about failing here.  (I haven't yet worked out how Gecko's animation support classes, like StyleAnimationValue, will work in conjunction with Servo's restyle code, if at all.)

> ::: layout/style/nsAnimationManager.cpp
> @@ +555,5 @@
> >                 "Keyframe rule has !important data");
> >  
> >      nsCOMArray<nsIStyleRule> rules;
> >      rules.AppendObject(aKeyframeDeclaration);
> > +    RefPtr<nsStyleContext> resultStrong = aPresContext->StyleSet()->AsGecko()->
> 
> My above AsGecko() concerns apply throughout this file as well. (4 chunks
> which use AsGecko, un-checked)

OK.  (I don't think we should get in here with a Servo-flavoured style backend, but I'll add explicit checks with more useful messages.)

> ::: layout/style/nsComputedDOMStyle.cpp
> @@ +504,5 @@
> >  
> >    if (aStyleType == eDefaultOnly) {
> > +    if (styleSet->IsServo()) {
> > +      NS_WARNING("ServoStyleSet cannot produce eDefaultOnly computed "
> > +                 "style objects");
> 
> Perhaps this should be NS_NOTREACHED (nonfatal but counts as an
> assertion-failure for test runs), or MOZ_ASSERT_UNREACHABLE (fatal)?
> 
> This sounds like a never-expected-to-happen codepath, which means we
> probably should yell if it happens.  NS_WARNING seems better for stuff
> that's unimplemented-but-will-eventually-be-implemented.

It's something that we'll need to implement eventually, so that devtools can continue to expose UA-sheet-only styles, but I'm not sure how we'll do that yet.  But a MOZ_ASSERT_UNREACHABLE sounds fine to me, as a reminder to fix this.

> ::: layout/style/nsRuleNode.cpp
> @@ +1533,5 @@
> >       point.  In any case, we don't want to treat the root rulenode as
> >       unused.  */
> >    if (!IsRoot()) {
> >      mParent->AddRef();
> > +    aContext->StyleSet()->AsGecko()->RuleNodeUnused();
> 
> Again, if this AsGecko() usage is valid, it should come with a
> MOZ_ASSERT(IsGecko(), "justification for why we know we must be gecko");

OK.  (Yes, nsRuleNode will never be used with the Servo-flavoured backend.)

> That, or we should have a servo fallback codepath (even if it's just an XXX
> comment and a NS_WARNING or NS_NOTYETIMPLEMENTED).
> 
> (I'm guessing the AsGecko assumption is *not* valid here, since we have a
> case in the chunk below this one where we treat IsServo as a possibility.)

Oh, good point.  That should change to assuming IsGecko.

> ::: layout/style/nsStyleContext.cpp
> @@ +131,5 @@
> >  {
> >    NS_ASSERTION((nullptr == mChild) && (nullptr == mEmptyChild), "destructing context with children");
> >  
> >    nsPresContext *presContext = mRuleNode->PresContext();
> > +  nsStyleSet* styleSet = presContext->PresShell()->StyleSet()->AsGecko();
> 
> As above, this AsGecko() needs to be removed, or justified with an assertion
> (if we're really sure it'll hold always), or softened to IsGecko or
> GetAsGecko with appropriate code in case of failure.

Yes, this one should be softened.

> ::: layout/style/nsStyleSet.h
> @@ +533,5 @@
> >  inline
> >  void nsRuleNode::AddRef()
> >  {
> >    if (mRefCnt++ == 0 && !IsRoot()) {
> > +    mPresContext->StyleSet()->AsGecko()->RuleNodeInUse();
> 
> As above, add an assertion to justify AsGecko usage (and in Release() below
> this, too).

I'll add justification for this one (due to this being in nsRuleNode).

> ::: layout/xul/tree/nsTreeStyleCache.cpp
> @@ +78,5 @@
> >    }
> >    if (!result) {
> >      // We missed the cache. Resolve this pseudo-style.
> > +    // XXXheycam ServoStyleSets do not support XUL tree styles.
> > +    RefPtr<nsStyleContext> newResult = aPresContext->StyleSet()->AsGecko()->
> 
> As above, this AsGecko usage needs an assertion to justify why it's a safe
> assumption.
> 
> (I'm unclear if the XXXheycam comment is saying "we can never get here with
> a servo backend", vs. "if we get here with a servo backend, we'll break but
> it's not a big deal. Please clarify, perhaps in the form of an assertion
> message if we really can assert here.)

I'll convert the comment to an assertion.  The current plan is to never be able to get in here with a Servo-flavoured backend.

Comment 20

[Cameron McCormack (:heycam)]

Skipped over this batch of comments:

(In reply to Daniel Holbert [:dholbert] from comment #15)
> ::: dom/canvas/CanvasRenderingContext2D.cpp
> @@ +2217,5 @@
> > +    // XXXheycam ServoStyleSets do not support resolving style from a list of
> > +    // rules yet.
> > +    error.Throw(NS_ERROR_FAILURE);
> > +    return nullptr;
> > +  }
> 
> I don't think we need this special early-return, really.
> 
> Can't we just let servo's ResolveStyleForRules() method return nullptr (to
> indicate failure/non-support), which will then make us take the
> already-existing failure case further down:
>  if (!result) {
>    error.Throw(NS_ERROR_FAILURE);
>    return nullptr;
>  }

I don't have a ResolveStyleForRules on StyleSetHandle, since it deals with StyleRule objects that we don't have access to from the ServoStyleSheet, and we're going to need something different to handle this.  I think for now we should keep this check.

> @@ +2260,5 @@
> > +    // XXXheycam ServoStyleSets do not support resolving style from a list of
> > +    // rules yet.
> > +    error.Throw(NS_ERROR_FAILURE);
> > +    return nullptr;
> > +  }
> 
> Above question applies here, too.
> 
> Though here, it looks like we don't null-check the result of the
> ResolveStyleForRules() call (the function that you're trying to avoid
> calling here). Perhaps we should null-check that? [and that's how Servo
> would trigger failure here for the moment])
> 
> Note that even Gecko's ResolveStyleForRules implementation can return
> nullptr (during shutdown at least), so that does seem like something that'd
> make sense to allow for here, even in the absence of a stub Servo
> implementation.
> 
> SO: Maybe this error-handling block that I'm quoting should morph into a
> null-check on the result of ResolveStyleForRules?

I feel like we shouldn't be able to get into these canvas methods if we've already shut down the style set, since that would require us to have dropped all strong references to the document.  And with the above comment, I will leave this as is for now.

Comment 21

[Cameron McCormack (:heycam)]

Attachment: Part 4: Use StyleSetHandle instead of concrete style set class in most places. (v2)

I'm going to leave the #include sorting until later.

Comment 22

[Daniel Holbert [:dholbert]]

(In reply to Cameron McCormack (:heycam) from comment #20)
> (In reply to Daniel Holbert [:dholbert] from comment #15)
> > ::: dom/canvas/CanvasRenderingContext2D.cpp
> > @@ +2217,5 @@
> > > +    // XXXheycam ServoStyleSets do not support resolving style from a list of
> > > +    // rules yet.
> > > +    error.Throw(NS_ERROR_FAILURE);
> > > +    return nullptr;
> I don't have a ResolveStyleForRules on StyleSetHandle, since it deals with
> StyleRule objects that we don't have access to from the ServoStyleSheet, and
> we're going to need something different to handle this.  I think for now we
> should keep this check.

OK, sounds good. (Maybe these merit a NS_ERROR [nonfatal assertion] mentioning "stylo", for easy grepping later, per bug 1244074 comment 30 / bug 1244074 comment 33? Or a mention of "stylo:" in the comment before we call Throw, if that's the key greppable string.)

(To be clear: this is for the two "Throw" calls added to CanvasRenderingContext2D.cpp in part 4 here.)

Comment 23

[Daniel Holbert [:dholbert]]

Comment on attachment 8717782 [details] [diff] [review]
Part 4: Use StyleSetHandle instead of concrete style set class in most places. (v2)

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

r=me on part 4, with nits below addressed as you see fit:

::: gfx/layers/apz/util/APZCCallbackHelper.cpp
@@ +14,5 @@
>  #include "mozilla/IntegerPrintfMacros.h"
>  #include "mozilla/layers/LayerTransactionChild.h"
>  #include "mozilla/layers/ShadowLayers.h"
> +#include "mozilla/StyleSetHandle.h"
> +#include "mozilla/StyleSetHandleInlines.h"

(This is new in this patch.  It's also the only change that this patch makes to this file.)

Do we really need these? I don't see "styleset" mentioned anywhere in this file, in mozilla-central.  (And there are only 2 instances of the word "style" in this file, both of which are in "aFrame->GetScrollbarStyles()" calls. So, I don't think this file needs to care about StyleSetHandle/StyleSetHandleInlines...)

::: layout/base/RestyleManager.cpp
@@ +4987,5 @@
>  nsStyleSet*
>  ElementRestyler::StyleSet() const
>  {
> +  MOZ_ASSERT(mPresContext->StyleSet()->IsGecko(),
> +             "RestyleManager should only be used with a Gecko-flavored "

s/RestyleManager/ElementRestyler/ perhaps? (this is an ElementRestyler method)

::: layout/style/nsRuleNode.cpp
@@ +9968,5 @@
>    // wants to hold onto the root node and not worry about re-creating a
>    // rule walker if the root node is deleted.
>    if (!(mDependentBits & NS_RULE_NODE_GC_MARK) &&
>        // Skip this only if we're the *current* root and not an old one.
> +      !(IsRoot() && mPresContext->StyleSet()->AsGecko()->GetRuleTree() == this)) {

This needs a
  MOZ_ASSERT(mPresContext->StyleSet()->IsGecko(),
	     "ServoStyleSets should not have rule nodes");
to justify the AsGecko usage here. (The other spots in this file are all good now; only this one is unguarded).

Comment 24

[Cameron McCormack (:heycam)]

(In reply to Daniel Holbert [:dholbert] from comment #23)
> ::: gfx/layers/apz/util/APZCCallbackHelper.cpp
> @@ +14,5 @@
> >  #include "mozilla/IntegerPrintfMacros.h"
> >  #include "mozilla/layers/LayerTransactionChild.h"
> >  #include "mozilla/layers/ShadowLayers.h"
> > +#include "mozilla/StyleSetHandle.h"
> > +#include "mozilla/StyleSetHandleInlines.h"
> 
> (This is new in this patch.  It's also the only change that this patch makes
> to this file.)

Oh, that should have gone in gfx/layers/apz/util/ActiveElementManager.cpp.  The error messages gcc gives me for not including StyleSetHandleInlines.h when unified compilation is being used don't make it easy to see which file was missing the include:

 0:09.19 In file included from /z/moz2/c/obj/dist/include/nsIPresShell.h:27:0,
 0:09.19                  from /z/moz2/c/obj/dist/include/nsPresContext.h:16,
 0:09.19                  from /z/moz2/c/obj/dist/include/nsRuleNode.h:19,
 0:09.19                  from /z/moz2/c/obj/dist/include/nsStyleContext.h:13,
 0:09.19                  from /z/moz2/c/obj/dist/include/mozilla/WritingModes.h:10,
 0:09.19                  from /z/moz2/c/obj/dist/include/nsIFrame.h:29,
 0:09.19                  from /z/moz2/c/obj/dist/include/ContentHelper.h:9,
 0:09.19                  from /z/moz2/c/gfx/layers/apz/util/APZCCallbackHelper.cpp:8,
 0:09.19                  from /z/moz2/c/obj/gfx/layers/Unified_cpp_gfx_layers2.cpp:11:
 0:09.19 /z/moz2/c/obj/dist/include/mozilla/StyleSetHandle.h:153:26: error: inline function ‘nsRestyleHint mozilla::StyleSetHandle::Ptr::HasStateDependentStyle(mozilla::dom::Element*, mozilla::EventStates)’ used but never defined [-Werror]
 0:09.19      inline nsRestyleHint HasStateDependentStyle(dom::Element* aElement,

as it's showing the place where the StyleSetHandle inline function declaration was first included.

Comment 25

[Daniel Holbert [:dholbert]]

Ah, that makes sense. Thanks for clearing that up!

Comment 26

[Daniel Holbert [:dholbert]]

(I'd still like to take a look at the final "part 2" here before this all lands, too, per comment 17. I assume you're still finishing that one up; just wanted to be sure that comment doesn't get lost amidst the other review comments here. Thanks!)

Comment 27

[Cameron McCormack (:heycam)]

Attachment: Part 2: Add skeleton ServoStyleSet and a StyleSetHandle smart pointer. (v2)

I did overlook attaching this for some reason...

Comment 28

[Cameron McCormack (:heycam)]

FWIW I discovered an obvious problem in testing with MOZ_STYLO enabled, which I've fixed locally -- the StyleSetHandle::AsServo method needs to mask off SERVO_BIT.  (Similarly for StyleSheetHandle::AsServo in bug 1244074.)

Comment 29

[Daniel Holbert [:dholbert]]

Comment on attachment 8718122 [details] [diff] [review]
Part 2: Add skeleton ServoStyleSet and a StyleSetHandle smart pointer. (v2)

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

Part 2 looks good! Just a few minor textual nits.

(Please also be sure that the style-sheet smart pointer class [in the other bug] benefits from all these same improvements you've made here, too.)

::: layout/style/StyleSetHandle.h
@@ +40,5 @@
> +class StyleSetHandle
> +{
> +public:
> +  // We define this Ptr class with a StyleSet API that forwards on to the
> +  // wrapper pointer, rather than putting these methods on StyleSetHandle

s/wrapper/wrapped/, I think?

(If I were to declare things as being "wrapper" vs. "wrapped" in this structure, I'd say the Ptr and StyleSetHandle classes are "wrappers", and the underlying styleset is "wrapped". So the underlying styleset pointer would be the "wrapped pointer", I'd think.)

@@ +171,5 @@
> +  StyleSetHandle(ServoStyleSet* aSet) { *this = aSet; }
> +
> +  StyleSetHandle& operator=(nsStyleSet* aStyleSet)
> +  {
> +    MOZ_ASSERT(!(reinterpret_cast<uintptr_t>(aStyleSet) & SERVO_BIT));

Thanks for adding this assertion! If I hadn't suggested it, I'd have to stare at it a bit to figure out what we're asserting / why we're we're asserting it, though. (particularly in this setter, which doesn't directly involve SERVO_BIT at all)

So: please add an explanatory assertion message to make this clearer -- something like:
  "least significant bit shouldn't be set; we use it for state"

(I'm pretty sure that's short enough to fit here as a one-liner message.)

@@ +179,5 @@
> +
> +  StyleSetHandle& operator=(ServoStyleSet* aStyleSet)
> +  {
> +#ifdef MOZ_STYLO
> +    MOZ_ASSERT(!(reinterpret_cast<uintptr_t>(aStyleSet) & SERVO_BIT));

Please add the same assertion message here.

Comment 30

[Daniel Holbert [:dholbert]]

(In reply to Cameron McCormack (:heycam) from comment #28)
> The StyleSetHandle::AsServo method needs to mask off SERVO_BIT.

Right! Good catch. :)

Comment 31

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9ac2b2c34c7c
https://hg.mozilla.org/integration/mozilla-inbound/rev/64552d361e5e
https://hg.mozilla.org/integration/mozilla-inbound/rev/af6b36331895
https://hg.mozilla.org/integration/mozilla-inbound/rev/a2a7c4cc94dc

Comment 32

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9ac2b2c34c7c
https://hg.mozilla.org/mozilla-central/rev/64552d361e5e
https://hg.mozilla.org/mozilla-central/rev/af6b36331895
https://hg.mozilla.org/mozilla-central/rev/a2a7c4cc94dc