In this bug we tell css::Loader objects what StyleBackendType to use when creating style sheets. For nsLayoutStylesheetCache, we have separate Loader objects for the two style backends.

Comment on attachment 8722320 [details] [diff] [review] patch Review of attachment 8722320 [details] [diff] [review]: ----------------------------------------------------------------- r- for now, because I think mStyleBackendTypeSet wants to be either eliminated (per my first comment below) or folded into a Maybe<> variable (per my second comment), and either way, the code here would change enough to merit a second look.. ::: layout/style/Loader.cpp @@ +2638,5 @@ > + MOZ_ASSERT(mStyleBackendTypeSet || mDocument); > + if (mStyleBackendTypeSet) { > + return mStyleBackendType; > + } else { > + return mDocument->GetStyleBackendType(); Can we query this from the document eagerly, up-front in the Loader(nsIDocument*) constructor? (Or, is the document's type not established yet at that point? Or can the document's type change?) If we can do this eagerly in the constructor, that seems simpler than maintaining the "is this set or not" state and having these branches here. ::: layout/style/Loader.h @@ +590,5 @@ > > bool mEnabled; // is enabled to load new styles > > + StyleBackendType mStyleBackendType; > + bool mStyleBackendTypeSet; This sort of pattern is really asking to be collapsed into: Maybe<StyleBackendType> mStyleBackendType; ::: layout/style/nsLayoutStylesheetCache.h @@ +91,5 @@ > > static mozilla::StaticRefPtr<nsLayoutStylesheetCache> gStyleCache_Gecko; > static mozilla::StaticRefPtr<nsLayoutStylesheetCache> gStyleCache_Servo; > + static mozilla::css::Loader* gCSSLoader_Gecko; > + static mozilla::css::Loader* gCSSLoader_Servo; These should probably just be mozilla::StaticRefPt, like the gStyleCache values above them... Then we won't have to manually addref/release them as carefully. (If you'd rather not address that as part of this bug, could you file a helper bug on that conversion?)

(In reply to Daniel Holbert [:dholbert] from comment #4) > Can we query this from the document eagerly, up-front in the > Loader(nsIDocument*) constructor? (Or, is the document's type not > established yet at that point? Or can the document's type change?) Yeah, we can't call this eagerly. The document returns the backend type of its pres shell's style set, and at the point we create the Loader we haven't set the pres shell on the document. > This sort of pattern is really asking to be collapsed into: > Maybe<StyleBackendType> mStyleBackendType; OK. > ::: layout/style/nsLayoutStylesheetCache.h > @@ +91,5 @@ > > > > static mozilla::StaticRefPtr<nsLayoutStylesheetCache> gStyleCache_Gecko; > > static mozilla::StaticRefPtr<nsLayoutStylesheetCache> gStyleCache_Servo; > > + static mozilla::css::Loader* gCSSLoader_Gecko; > > + static mozilla::css::Loader* gCSSLoader_Servo; > > These should probably just be mozilla::StaticRefPt, like the gStyleCache > values above them... Then we won't have to manually addref/release them as > carefully. Good point, I'll do that.

Comment on attachment 8722332 [details] [diff] [review] patch v2 Review of attachment 8722332 [details] [diff] [review]: ----------------------------------------------------------------- Looks good. Nits below: ::: layout/style/Loader.cpp @@ +2631,5 @@ > > +StyleBackendType > +Loader::GetStyleBackendType() const > +{ > + MOZ_ASSERT(mStyleBackendType.isSome() || mDocument); Probably drop the .isSome() (see explanation in my next comment), and add an assertion-message. (RE the message -- it's not actually 100% obvious to me why this assertion is justified. I think it's implicitly assuming that the Loader(nsIDocument*) constructor *must* have been called with a non-null arg -- but we don't actually assert or depend on that assumption inside of that constructor. If your assertion here depends on that assumption, maybe add an assertion in the Loader(nsIDocuemnt*) constructor, to check that its arg is non-null?) @@ +2633,5 @@ > +Loader::GetStyleBackendType() const > +{ > + MOZ_ASSERT(mStyleBackendType.isSome() || mDocument); > + if (mStyleBackendType.isSome()) { > + return mStyleBackendType.value(); Drop the ".isSome()", and convert .value to just use "*" as a dereference-operator. e.g.: if (mStyleBackendType) { return *mStyleBackendType; (Maybe<> has the same behavior that a pointer/smart-pointer has, w.r.t. boolean conversion and operator* and operator->. The verbose accessors exist, but from talking to Seth, I believe we should prefer operators, just like we do on e.g. RefPtr.) @@ +2635,5 @@ > + MOZ_ASSERT(mStyleBackendType.isSome() || mDocument); > + if (mStyleBackendType.isSome()) { > + return mStyleBackendType.value(); > + } else { > + return mDocument->GetStyleBackendType(); No else-after-return -- just drop the "else" and de-indent the return statement here. ::: layout/style/Loader.h @@ +589,5 @@ > nsString mPreferredSheet; // title of preferred sheet > > bool mEnabled; // is enabled to load new styles > > + mozilla::Maybe<StyleBackendType> mStyleBackendType; Move this up to be before |mEnabled|, so that boolean member-vars at the end can pack nicely. (Make sure to reorder the constructor init-list in Loader.cpp accordingly, too.)

(Also, be sure to drop the "v2" from your commit message when landing, too.)