Closed Bug 1250379 Opened 4 years ago Closed 4 years ago

create css::Loaders for specific style backend types

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla47
Tracking Status
firefox47 --- fixed

People

(Reporter: heycam, Assigned: heycam)

References

Details

Attachments

(1 file, 1 obsolete file)

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.
Attached patch patch (obsolete) — Splinter Review
Assignee: nobody → cam
Status: NEW → ASSIGNED
Attachment #8722320 - Flags: review?(dholbert)
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?)
Attachment #8722320 - Flags: review?(dholbert) → review-
(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.
Attached patch patch v2Splinter Review
Attachment #8722320 - Attachment is obsolete: true
Attachment #8722332 - Flags: review?(dholbert)
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.)
Attachment #8722332 - Flags: review?(dholbert) → review+
(Also, be sure to drop the "v2" from your commit message when landing, too.)
Blocks: 1250767
https://hg.mozilla.org/mozilla-central/rev/f1ee875a249c
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
You need to log in before you can comment on or make changes to this bug.