stylo: Decide style system backend type for documents earlier

RESOLVED FIXED

Status

()

Core
CSS Parsing and Computation
RESOLVED FIXED
a year ago
7 months ago

People

(Reporter: heycam, Assigned: heycam)

Tracking

(Blocks: 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

10.44 KB, patch
Bobby Holley (parental leave - send mail for anything urgent)
: review+
Details | Diff | Splinter Review
Created attachment 8764145 [details] [diff] [review]
patch

Currently we decide whether to use a Servo- or a Gecko-backed style system at the time we create the nsDocumentViewer.  This assumes that the document is being presented (which turns out not to be true for all documents that get Loaders, style sets, etc., which need to know the backend type), resulting in many warnings on startup.  This patch makes the document the ultimate decider of the backend type, rather than the nsDocumentViewer, but still delays that decision until the first GetStyleBackendType call on it.  This still allows us to choose, for now, to use Servo for documents that will be in content docshells without having the docshell set on the document yet, since we can call doc->IsContentDocument() to get the same information.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=62051343b69d
Attachment #8764145 - Flags: review?(bobbyholley)
Comment on attachment 8764145 [details] [diff] [review]
patch

Test failures to look into.
Attachment #8764145 - Flags: review?(bobbyholley)
Created attachment 8764242 [details] [diff] [review]
patch v1.1

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d5816dc3bb05
Assignee: nobody → cam
Attachment #8764145 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8764242 - Flags: review?(bobbyholley)
Comment on attachment 8764242 [details] [diff] [review]
patch v1.1

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

r=me with that.

::: dom/base/nsIDocument.h
@@ +1071,5 @@
>      return mCSSLoader;
>    }
>  
> +  mozilla::StyleBackendType GetStyleBackendType() const {
> +    if (mStyleBackendType == mozilla::StyleBackendType(0)) {

This mechanism feels too hacky to me. How about just storing a Maybe<StyleBackendType> and checking whether it's been initialized? Then we can nix the "= 1" in the enum class.

@@ +1077,5 @@
> +    }
> +    MOZ_ASSERT(mStyleBackendType != mozilla::StyleBackendType(0));
> +    return mStyleBackendType;
> +  }
> +  void UpdateStyleBackendType();

Let's call this ComputeStyleBackendType. And add a comment explaining why we need to do this lazily?

::: layout/style/StyleBackendType.h
@@ +13,5 @@
>   * Enumeration that represents one of the two supported style system backends.
>   */
>  enum class StyleBackendType : int
>  {
> +  Gecko = 1,

Per above let's revert this.
Attachment #8764242 - Flags: review?(bobbyholley) → review+
ISTR this landing?
Flags: needinfo?(cam)
Summary: decide style system backend type for documents earlier → stylo: Decide style system backend type for documents earlier
This landed with a wrong bug number (bug 1285474).
Status: ASSIGNED → RESOLVED
Last Resolved: 7 months ago
Flags: needinfo?(cam)
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.